Skip to content
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

Adding ability to list attestors #384

Merged
merged 10 commits into from
Mar 27, 2024

Conversation

ChaosInTheCRD
Copy link
Collaborator

@ChaosInTheCRD ChaosInTheCRD commented Feb 12, 2024

What this PR does / why we need it

#372 highlights the lack of ability for users to list the attestors that are available for calling when running witness run.

This PR adds a witness attestors subcommand, which allows users to get a list of the attestors available back. In the future we should consider adding descriptions into the attestors in the https://github.com/in-toto/go-witness library and exposing those descriptions here.

Executing the command gives an output like:

➜ go run . attestors           
+-----------------------+---------------------------------------------------+-------------+
|         NAME          |                       TYPE                        |   RUNTYPE   |
+-----------------------+---------------------------------------------------+-------------+
| material (always run) | https://witness.dev/attestations/material/v0.1    | material    |
| gcp-iit               | https://witness.dev/attestations/gcp-iit/v0.1     | prematerial |
| github                | https://witness.dev/attestations/github/v0.1      | prematerial |
| gitlab                | https://witness.dev/attestations/gitlab/v0.1      | prematerial |
| git (default)         | https://witness.dev/attestations/git/v0.1         | prematerial |
| jwt                   | https://witness.dev/attestations/jwt/v0.1         | prematerial |
| oci                   | https://witness.dev/attestations/oci/v0.1         | postproduct |
| environment (default) | https://witness.dev/attestations/environment/v0.1 | prematerial |
| maven                 | https://witness.dev/attestations/maven/v0.1       | prematerial |
| aws                   | https://witness.dev/attestations/aws/v0.1         | prematerial |
| sarif                 | https://witness.dev/attestations/sarif/v0.1       | postproduct |
| product (always run)  | https://witness.dev/attestations/product/v0.1     | product     |
+-----------------------+---------------------------------------------------+-------------+

@ChaosInTheCRD ChaosInTheCRD marked this pull request as ready for review February 12, 2024 15:31
@mikhailswift
Copy link
Member

Definitely a very welcome addition and will become even more necessary as more attestors get added. I do wonder if it's worth giving the command-run attestor the same treatment as the product and material attestors.

Need to run make docgen to update the docs

@ChaosInTheCRD
Copy link
Collaborator Author

Definitely a very welcome addition and will become even more necessary as more attestors get added. I do wonder if it's worth giving the command-run attestor the same treatment as the product and material attestors.

Need to run make docgen to update the docs

So @mikhailswift, you have probably noticed that I deliberately removed command-run from the list of "available attestors". This is for the simple fact that it's a slightly confusing user experience to have one attestor that simply isn't designed to be supplied with the -a flag. This is highlighted by #372 and @Strakeln's comments.

Maybe command-run simply should be listed here, and instead we should better handle the case where someone calls -a command-run. Possibly we should warn the user (like we already do in the case of e.g., witness run -a product,product, to inform them that the command-run attestor is invoked by supplying arguments to the witness command? I just think we need to put some thought into the user experience around this to make it more obvious.

@jkjell
Copy link
Member

jkjell commented Feb 13, 2024

Maybe command-run simply should be listed here

This is maybe a useless philosophical perspective for end users but, for me, command-run is implicit in the invocation of witness run. I think adding it as "always run" is a reasonable start to get feedback on.

@mikhailswift
Copy link
Member

I see both sides and am good either way. I do think adding some logic to handle the case where -a "command-run" gets passed in and handled gracefully would be a very welcome QOL change, regardless of whether or not we add it to the attestor list here.

Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for witness-project canceled.

Name Link
🔨 Latest commit 77c1e39
🔍 Latest deploy log https://app.netlify.com/sites/witness-project/deploys/66041322893af20008a18d39

@ChaosInTheCRD ChaosInTheCRD merged commit 6f7d4a8 into in-toto:main Mar 27, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants