Skip to content

Conversation

manzil-infinity180
Copy link
Contributor

@manzil-infinity180 manzil-infinity180 commented Sep 17, 2025

What this PR does / why we need it

Description

Added the new command for attaching the attestation

witness attach attestation \
--attestation <first_attestation.json> \
<image_uri>

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:

  1. Add test ( in-progress)
  2. Add demo for ECR registry (done here - https://github.com/manzil-infinity180/witness-kms-demo/blob/main/docs/witness-attach.md#using-aws-ecr)
  3. Add docs (done - 83275b1 but i am kind of not satified with it will figure out)

Related to Issue #602

Acceptance Criteria Met

  • Docs changes if needed
  • Testing changes if needed
  • All workflow checks passing (automatically enforced)
  • All review conversations resolved (automatically enforced)
  • DCO Sign-off

Special notes for your reviewer:

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>
Copy link

netlify bot commented Sep 17, 2025

Deploy Preview for witness-project ready!

Name Link
🔨 Latest commit cefe249
🔍 Latest deploy log https://app.netlify.com/projects/witness-project/deploys/68cc7655efa60d0008a53557
😎 Deploy Preview https://deploy-preview-661--witness-project.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
@manzil-infinity180
Copy link
Contributor Author

cc: @colek42

Signed-off-by: Rahul Vishwakarma <rahulvs2809@gmail.com>
@colek42
Copy link
Member

colek42 commented Sep 18, 2025

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

@colek42
Copy link
Member

colek42 commented Sep 18, 2025

  1. Would love to see an in-memory registry test (httptest + ggcr options) that runs AttachAttestationToEntity followed by WriteAttestations. Right now we don’t have any signal that the new attestation layer actually lands in the signatures index or that duplicates are handled the way we expect.

  2. Right now every method on staticLayer (Payload, Signature, Digest, Size, Annotations, etc.) just calls panic("unimplemented"), so the type can’t actually be used. Once you replace those stubs with real logic, returning the in-memory bytes, computing the digest/size, surfacing annotations, we should immediately add unit tests that cover each method. A table-driven test (the usual Go pattern where you iterate over a slice of test cases) lets us feed in a known payload/annotation set and assert that all of those methods return exactly what we expect. That way, when ggcr later reads the layer during a push/pull, we know it’ll see the same bytes we constructed, and any future regression (wrong digest, wrong size, missing annotation) will be caught by the test.

  3. AttestationCmd could really use unit tests that feed a valid DSSE, a payload with the wrong payloadType, and one without signatures. That would lock in the success/error paths and ensure we don’t silently accept malformed envelopes.

  4. Cosign checks that every DSSE envelope you’re about to attach has a subject entry whose digest matches the exact image digest you resolved (unless you ask it not to). Without that guardrail you can accidentally attach an attestation describing some other artifact, which undermines the trust model. So when we hook that logic up in Witness, we should immediately add tests that simulate the three realistic scenarios:

  • subject digest matches the resolved image digest → command succeeds,
  • subject digest mismatches → command fails with the expected error,
  • subject digest mismatches but SkipVerification (or whatever flag we expose) is set → command skips the check and succeeds.
  • we also need to think about backrefs. if the attestation is connected via a backref we should also allow attaching. (maybe we want a command that attaches all related attestations?)

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>
Copy link

@kriscoleman kriscoleman Sep 18, 2025

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>

Copy link
Contributor Author

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.

Copy link
Member

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>

Copy link

@kriscoleman kriscoleman Sep 22, 2025

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:

  1. Make the final command simple, clear, and human-readable
  2. Allow unlimited attestations as positional arguments
  3. Provide a cleaner CLI experience without the need for repeated --attestation flags
  4. 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>
@jkjell
Copy link
Member

jkjell commented Sep 22, 2025

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:

  1. Use an external tool to upload attestations to a registry (there are many dedicated tools for this purpose: regclient, oras, crane, cosign, etc)
  2. If we put this functionality directly into Witness, upload the attestations at the same time we would when using --enable-archivista, using the context of the attestation creation (i.e. upload to the image associated with the docker attestation, which is also the likely process space with credentialed access to push the image).

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 go-containerregistry. oras-go or regclient may be more suitable.

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.

@colek42
Copy link
Member

colek42 commented Sep 22, 2025

@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

@manzil-infinity180
Copy link
Contributor Author

@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 attach command approach has some cons. It doesn’t ensure the attestation is signed before being uploaded. And there’s no built-in way to fetch it back and verify it (completeness issue). Cosign handles this with a dedicated attest command, which Witness doesn’t currently replicate.

I would like to explore doing this within the witness run flow instead.

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:

  1. How do we use the attestations that are stored in Archivista?
  2. Do we sign them before storing them in Archivista?
  3. Since this will be a bit similar to what cosign attest does, I guess we need another flag to enable it (a --enable-* CLI flag). Or can I get how the command looks like?

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