-
Notifications
You must be signed in to change notification settings - Fork 105
RTDEV-58178 - Collect GitHub attestation into JFrog Artifactory at the end of the workflow #287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
d57b3f5 to
e297e7f
Compare
e297e7f to
a9a4c65
Compare
a9a4c65 to
f7579b0
Compare
f7579b0 to
7554b72
Compare
7554b72 to
649c963
Compare
src/evidence-collection.ts
Outdated
| } | ||
|
|
||
| if (!accessToken) { | ||
| throw new Error('No access token available for authentication'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Basic Auth is still supported (JF_USER and JF_PASSWORD), I would throw a different error.
Additionally, Won't an error fail the step and pipeline? I don't think we want to do it in case of Basic Auth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support to basic auth too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant GitHub action supports basic auth (legacy) while evidence doesn't (API will fail) but we need to make it transparent at the actions level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the code to check if authentication is using user-password at the beginning and just issue a log message and skip evidence collection
| accessToken = yield OidcUtils.exchangeOidcToken(credentials); | ||
| } | ||
| if (!accessToken) { | ||
| throw new Error('No access token available for authentication'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are supporting Basic Auth with JF_USER and JF_PASSWORD we should give a meaningful error in this case.
| for (const filePath of filePaths) { | ||
| try { | ||
| const fileStats = yield fs_1.promises.stat(filePath); | ||
| const fileSizeMB = fileStats.size / (1024 * 1024); // Convert bytes to MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in config API.
If we will use it in Azure for example I don't think we want to convert each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think converting makes sense. It leaves the API readable to humans
| continue; | ||
| } | ||
| core.info(`Creating evidence for: ${filePath}`); | ||
| const output = yield utils_1.Utils.runCliAndGetOutput(['evd', 'create', '--sigstore-bundle', filePath]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the case JF_PROJECT is defined? don't we want to use --project here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that we pass project in the jf cli:
func NewCreateEvidenceCustom(serverDetails *config.ServerDetails, predicateFilePath, predicateType, markdownFilePath, key, keyId, subjectRepoPath,
subjectSha256, sigstoreBundlePath, providerId string) evidence.Command {
return &createEvidenceCustom{
createEvidenceBase: createEvidenceBase{
serverDetails: serverDetails,
predicateFilePath: predicateFilePath,
predicateType: predicateType,
providerId: providerId,
markdownFilePath: markdownFilePath,
key: key,
keyId: keyId,
},
subjectRepoPath: subjectRepoPath,
subjectSha256: subjectSha256,
sigstoreBundlePath: sigstoreBundlePath,
}
}
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to change it but good for now
src/evidence-collection.ts
Outdated
| // Check if evidence collection is supported by the server | ||
| const evidenceConfig = await getEvidenceConfiguration(); | ||
| if (!evidenceConfig.external_evidence_collection_supported) { | ||
| core.info('Evidence collection is not supported by this Artifactory server. Skipping evidence collection.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe more specific message is needed as to why we are not supporting it (License, Basic_Auth etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only the license that don't allow it. Changed the log message
649c963 to
d3e62c7
Compare
d3e62c7 to
a941c11
Compare
a941c11 to
1eb762b
Compare
1eb762b to
30b8fff
Compare
30b8fff to
2a440a0
Compare
| // Check if evidence collection is supported by the server | ||
| const evidenceConfig = await getEvidenceConfiguration(); | ||
| if (!evidenceConfig.external_evidence_collection_supported) { | ||
| core.info("Evidence collection is not supported by Artifactory's license type. Skipping evidence collection."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "Evidence collection is supported only with Enterprise Plus license. Skipping evidence collection".
It avoids future questions and also similar to other messages of this type across platform.
2a440a0 to
32b7228
Compare
32b7228 to
e191da4
Compare
e191da4 to
2eb28a7
Compare
2eb28a7 to
76fee48
Compare
…e end of the workflow
76fee48 to
c182aab
Compare

npm run formatfor formatting the code before submitting the pull request.