-
Notifications
You must be signed in to change notification settings - Fork 70
add attach attestation command #661
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
base: main
Are you sure you want to change the base?
add attach attestation command #661
Conversation
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
✅ Deploy Preview for witness-project ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
cc: @colek42 |
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
This is looking good. I'd like to see some unit and e2e tests before we merge this in. @jkjell would love your thoughts as well |
Covering those cases keeps our behavior aligned with cosign’s safety story and makes sure a future refactor doesn’t accidentally drop or weaken the verification step. |
cmd := &cobra.Command{ | ||
Use: "attestation", | ||
Short: "Attach attestation to the supplied container image", | ||
Example: ` witness attach attestation --attestation <attestation file path> <image uri> |
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.
if the sub commands are attach attestation
I think the main arg should be the attestation, not the image uri.
the things the attestation are beign attached to should likely be the flags.
example: witness attach attestation --image-uri <image-uri> <attestation-file-path>
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.
Thanks for the suggestion! One of the reasons I went with the --attestation
flag instead of making the attestation file a positional argument is to support attaching multiple attestations at once.
witness attach attestation \
--attestation <first_attestation.json> \
--attestation <second_attestation.json> \
<image_uri>
This way, the image-uri remains the primary subject (as a positional argument), and we can flexibly pass one or more attestations with repeatable flags.
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 a good pattern -- I can't think of anything thing other than an OCI image we could attach to.
witness attach attestation
--attestation <first_attestation.json>
--attestation <second_attestation.json>
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.
@manzil-infinity180 Thanks for explaining your reasoning! I appreciate the thought you've put into supporting multiple attestations with the repeatable flag pattern.
I see your point, and I agree that the flexibility is important. However, I think we could still achieve the same flexibility with attestations as positional arguments. Most modern CLI frameworks (including cobra) support variadic positional arguments that can accept either a single value or an array of values. This would give us a cleaner syntax while maintaining the ability to attach multiple attestations.
Here are some example CLI commands showing how this could work:
Single attestation (positional):
witness attach attestation attestation.json --image-uri my-image:latest
Multiple attestations (positional array):
witness attach attestation attestation1.json attestation2.json attestation3.json --image-uri my-image:latest
And here's a pseudo-code example of how the Go CLI function definition could look using cobra:
cmd := &cobra.Command{
Use: "attestation [attestation-files]...",
Short: "Attach attestation(s) to the supplied container image",
Args: cobra.MinimumNArgs(1), // At least one attestation file
Example: ` # Attach a single attestation
witness attach attestation attestation.json --image-uri my-image:latest
# Attach multiple attestations
witness attach attestation att1.json att2.json att3.json --image-uri my-image:latest`,
RunE: func(cmd *cobra.Command, args []string) error {
imageURI, _ := cmd.Flags().GetString("image-uri")
attestationFiles := args // All positional args are attestation files
// Validate image URI was provided
if imageURI == "" {
return fmt.Errorf("--image-uri flag is required")
}
// Process each attestation file
for _, attestationFile := range attestationFiles {
// Attach attestation logic here
}
return nil
},
}
// Add the image-uri flag
cmd.Flags().StringP("image-uri", "i", "", "Container image URI to attach attestations to (required)")
cmd.MarkFlagRequired("image-uri")
This approach would:
- Make the final command simple, clear, and human-readable
- Allow unlimited attestations as positional arguments
- Provide a cleaner CLI experience without the need for repeated
--attestation
flags - Maintain all the flexibility you were aiming for
I think the --image-uri
flag does provide good syntax clarity about what the target is, while making the attestations (the things being attached) the primary positional arguments. What do you think?
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
I'll reshare my questions from the original issue here: #602 (comment). I'm not strongly in favor of embedding this type of functionality directly into Witness. My order of preference to enable this type of behavior would be:
With respect to implementing this in Witness, I didn't look deeply at the implementation but, I'd prefer to use a higher level library then If we did move forward with this, I'd also like to see a proposal for how Witness would consume attestations it has stored in a registry. That could be a follow on PR but, if we feel strongly enough this functionality belongs in Witness, it makes sense to enable both storing and retrieving attestations. |
@jkjell all good points, maybe this belongs in archivistactl. Then we can use the witness-run-action to orchestrate it all together. Thoughts @manzil-infinity180 |
I agree with @jkjell comment — the I would like to explore doing this within the But before writing a proposal, I want approval from the maintainers to confirm whether we actually need this or not. I’d like to hear from other maintainers and know their thoughts on it. If it’s agreed, then I will propose a way for uploading and retrieving attestations, and will explore how we can achieve this using either oras or regclient. A few questions:
|
What this PR does / why we need it
Description
Added the new command for attaching the attestation
Here is the demo/test which I did on Dockerhub & GitHub registry(ghcr) - https://github.com/manzil-infinity180/witness-kms-demo/blob/main/docs/witness-attach.md
todo:
Related to Issue #602
Acceptance Criteria Met
Special notes for your reviewer: