fix: Fix missing checks on product include/exclude glob for attestation.

[matglas]

This change addresses a problem with --attestor-product-exclude-glob and --attestor-product-include-glob they 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:

• It add the two glob arguments to the RecordArtifacts. These allow you to pass glob statements the same way they are created in Product attestor for the arguments.
• All references are updated to pass nil where 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 to fix!: for breaking change depending on the way we want to maintain contracts.
• The Product.Subjects() and File.shouldRecord() command now allow for include to take precedence over the exclude.

Fixes #65

[mikhailswift]

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 npm install and get a huge attestation output due to the sheer number of files included in the product attestor. Not sure of the use case you had in mind, but that was a driving reason for this feature initially.

[matglas]

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.

[matglas]

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.

[matglas]

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 * by default.

[matglas]

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.

[matglas]

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.

[matglas]

Also I noticed that the test jobs for product attestor fail. I need to fix those.

[matglas]

Did an rebase to sign-off all commits.

[jkjell]

I think this is just about ready! Just a minor suggestion to allow the Configuration to be omitted if no globs are used.

[matglas]

@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.

[matglas]

I added CHANGELOG-ATTESTORS.md.

[jkjell]

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.

[jkjell]

@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. ☺️