fix: Fix missing checks on product include/exclude glob for attestation. #66
Conversation
|
Hi @matglas, thanks for the contribution! I should explain my thought process behind this decision so we can determine the best course of action. Long story short, I intended to still include files in the product and material attestors so we could still use them for integrity checking between steps. The thought is that it would be less than desirable if I could fool a policy to sneak files into a compilation process by just including a CLI argument to ignore those files. Or mess with something in an ignored folder to compromise a build (ignoring npm_modules, and then injecting some code into a module for example). The decision to have this apply to just the in-toto statement's subjects was because we probably don't need to consider every file in node_modules a subject, but we may still care about the file integrity of that folder. However, I can still imagine use cases where we may want to exclude files from the materials and product attestors. And perhaps an implementation of in-toto's artifact rules would help mitigate my concerns. I know from experience that it's painful to create an attestation about an |
|
Thank for the response. I completely get it and understand its would be best for policy checking. When I implemented it I already doubted a little. But here is my case. In my case we are building a mono repo. So thats already a lot of file to index and checksum. Then the output of our build tool, https://please.build, has intermediate output of different targets which is there inside a plz-out folder but it also has all the intermediate stuff that is needed to build the final artifacts. That is not one project with node modules but multiple. If I don't use the glob like I implemented my output file is over a 1gb in size and take almost ~10 minutes to create. |
|
To add. The final thing that I want to verify is the a package of all the things so I would not need the intermediate files. They are in the package. But with the current setup I have for executing build in the pipeline I do not have the option yet to remove the intermediates after I packaged it. I might be able to add a wrapper on the build command and only keep what I need. |
|
I did notice my logic in the code is not fully working. If you knly specify the exclude it still includes everything because the include is a |
matglas
force-pushed
the
fix-product-glob
branch
4 times, most recently
from
713f5d6
to
3e451ed
Compare
|
I implemented the logic to fix the default glob combined with an exclude. And I added configuration output so a policy can reason about if it wants to accept this configuration. Because of the output change in the attestation I bumped the version number too. |
jkjell
force-pushed
the
fix-product-glob
branch
from
3e451ed
to
21f60ba
Compare
|
Hi @jkjell I see you pushed changes. But because I'm not too familiar with github and this process yet I can't really understand what happened. Can you explain what you did? I just pushed some more changes. And merged main back to my branch if I'm correct. |
|
Also I noticed that the test jobs for product attestor fail. I need to fix those. |
matglas
force-pushed
the
fix-product-glob
branch
3 times, most recently
from
5227e45
to
a622435
Compare
|
Did an rebase to sign-off all commits. |
| ) | ||
|
|
||
| const ( | ||
| Name = "material" | ||
| Type = "https://witness.dev/attestations/material/v0.1" | ||
| Type = "https://witness.dev/attestations/material/v0.2" |
There was a problem hiding this comment.
@jkjell I was just thinking if we might need to mention somewhere that the version is bumped. Like a changelog of some sorts? This will break current policies that are in place. Same goes for the product.
There was a problem hiding this comment.
I added CHANGELOG-ATTESTORS.md.
matglas
force-pushed
the
fix-product-glob
branch
4 times, most recently
from
bf3026d
to
c6cebd3
Compare
Signed-off-by: Matthias Glastra <[email protected]>
When not adding a include it would always catch every file and the exclude would be of no benefit. Signed-off-by: Matthias Glastra <[email protected]>
Signed-off-by: Matthias Glastra <[email protected]>
Signed-off-by: Matthias Glastra <[email protected]>
Signed-off-by: Matthias Glastra <[email protected]>
Signed-off-by: John Kjell <[email protected]>
Co-authored-by: John Kjell <[email protected]> Signed-off-by: Matthias Glastra <[email protected]>
Signed-off-by: Matthias Glastra <[email protected]>
jkjell
force-pushed
the
fix-product-glob
branch
from
c6cebd3
to
fed0ca9
Compare
Signed-off-by: Matthias Glastra <[email protected]>
jkjell
force-pushed
the
fix-product-glob
branch
from
fed0ca9
to
762dc49
Compare
| } | ||
| } | ||
|
|
||
| return json.Marshal(output) | ||
| } | ||
|
|
||
| func (a *Attestor) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
We'll need to make this backwards compatible. I think we can check a.Type() for the version and unmarshal into the new struct format.
|
@matglas I brought everything up-to-date and resolved the conflicts. I added one more comment about backwards compatibility. I think if we can get that and testing to ensure it works, we should be good. On resolving the conflicts, there was some overlap with a tracing PR that @mikhailswift reviewed. He's back tomorrow so, it'll be good to have him take a look and make sure I didn't break that either. |
This change addresses a problem with
--attestor-product-exclude-globand--attestor-product-include-globthey only worked for the Subject collection, but not for the Product attestation. Also it did not allow for include overwriting the exclude on sub elements.Changes:
RecordArtifacts. These allow you to pass glob statements the same way they are created in Product attestor for the arguments.nilwhere the do not have a implementation. This is a breaking change for any API contract for those using this as a library. So maybe I need to change the commit tofix!:for breaking change depending on the way we want to maintain contracts.Product.Subjects()andFile.shouldRecord()command now allow for include to take precedence over the exclude.Fixes #65