-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add provenance metadata support #5
Conversation
@SantiagoTorres This is the work so far, can you take a look? :) |
setProvenanceMaterials(InTotoWrapper.collectArtifacts(this.cwd)); | ||
|
||
//setting up Invocation | ||
this.invocation = new BuildInvocation(); |
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.
Why can't this be locally scoped?
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'm guessing it's because of setProvenanceMaterials
etc but I suspect it can be cleaner.
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.
Are you referring to the BuildInvocation
object?
I think there is no need as such for it to be global, we can make it local as well.
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.
Yep, I think I agree. The same probably goes for the metadata
object as well?
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.
Yes, I'll do that
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.
There are some minor nits that I'd love to address :)
setBuilder(initialEnvironment); | ||
|
||
//setting up materials | ||
setProvenanceMaterials(InTotoWrapper.collectArtifacts(this.cwd)); |
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.
You probably want to reuse the retval on 210 so you don't make two consecutive calls to the same
invocation.setEnvironments(initialEnvironment); | ||
try { | ||
invocation.setConfigSource(initialEnvironment); | ||
} catch (NoSuchAlgorithmException e) { |
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.
Not entirely sure why setConfigSource throws a NSAE. isn't this for bouncycastle/dsse signature algorithms?
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.
Ah it's my bad, I used bouncycastle to get the sha256 hash of the git commit and that threw NSAE.
But yes, now the try-catch block is not needed anymore. Will update it...
return environment; | ||
} | ||
|
||
public void setConfigSource(EnvVars initialEnvironment) throws NoSuchAlgorithmException{ |
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 above. Who is throwing this? uriDigest?
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 does not, will remove throws NoSuchAlgorithmException
private void setProvenanceMaterials(HashMap<String, ArtifactHash> results) { | ||
|
||
HashMap<String, ArtifactHash> materials =new HashMap<String, ArtifactHash>(); | ||
materials=link.excludeArtifactsByPattern(results,null); |
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.
Hmm, this logic is somewhat convoluted here. You call collectArtifacts twice, in one you implicitly use it here, the other you pass as a parameter. Am I understanding this right? I think we could simplify this code by:
- reusing the link as we are doing
- not passing the results parameter.
- Iterate over th elements on the link itself (not sure why are we filtering)
- Form the arraylist using the iterator
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.
Yes, you are right, this seems more straightforward. Will implement this...
Is it a good idea to remove the import statements that are not used? |
@lakshya8066 is this ready for a review again? |
@adityasaky Yes, the updates are ready for review. |
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 this is mostly in good shape with some TODOs we should ticketize separately. We should open a ticket for Rekor support and another for Archivist support.
src/main/java/io/jenkins/plugins/intotorecorder/InTotoWrapper.java
Outdated
Show resolved
Hide resolved
@adityasaky By user controlled, do you mean the user running the plugin through Jenkinsfile? |
There is a ticked opened for Rekor already, so opened a ticked for Archivist only. |
Yes but on second thought, probably okay to open a ticket for a feature request. Probably a good first issue. |
I am working on this, but yeah I think let's get this PR merged and I can push a patch for this afterward. Is that okay? |
Sounds good, can we still open a ticket for it? You can probably submit that patch directly to jenkinsci as well :) |
Sure! |
@Override | ||
public String invoke(File f, VirtualChannel channel) { | ||
Gson gson = new Gson(); | ||
System.out.println(this.ProvenanceData); |
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.
System.out.println(this.ProvenanceData); |
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 we can merge this with in-toto/in-toto-java#40 after the print statement nit.
a70bb26
to
fd18d7f
Compare
@adityasaky This should be ready to merge right? |
We want to cut a release of -java and bump up the version here, yes? |
Ah yes, correct. Will update the version here once -java is released... |
fd18d7f
to
87bbdd7
Compare
Thanks for your work, @lakshya8066! |
Add provenance metadata support
This PR adds support for generating Provenance v0.2 metadata for each step of the Jenkins build.