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

Verify certificate against job_workflow_ref string #1313

Closed
asraa opened this issue Jan 13, 2022 · 15 comments
Closed

Verify certificate against job_workflow_ref string #1313

asraa opened this issue Jan 13, 2022 · 15 comments
Labels
bug Something isn't working

Comments

@asraa
Copy link
Contributor

asraa commented Jan 13, 2022

Description

Right now, --cert-email only verifies against the cert.EmailAddresses field. If the image was signed by a github actions, there is no way to supply a URI (possibly a regex?) to match against the job_workflow_ref included in the cert URI's field. I'd be interested in doing this, to verify, for e.g. that a container was signed by the correct repository and/or workflow, so that we could use this to pin trust against particular workflows that were meant to generate signatures.

cosign verify --cert-uri "username/reponame/.github/workflows/token.yml@refs/heads/master" $IMG

or a regex might be more appropriate

cosign verify --cert-uri-regex "username\/reponame\/.*" $IMG

@asraa asraa added the bug Something isn't working label Jan 13, 2022
@asraa
Copy link
Contributor Author

asraa commented Jan 13, 2022

TBH I think unifying the email/uri field like CertSubject does is better...

cosign verify --cert-subject <either URI or email> $IMG

Can I add this flag? It would do the same as --cert-email, but IMO is more generic and probably could supersede/deprecate email

@haydentherapper
Copy link
Contributor

+1 to a single flag, --cert-subject, with an eventual deprecation of --cert-email. Regex support can be added without a separate flag, with documentation noting what is accepted. For documentation, we also need to note that this will accept SPIFFE IDs.

@dekkagaijin You were on the original ticket for this feature, and you had noted you wanted to think through the UX for a subject flag. Did you have any thoughts about this?

@dekkagaijin
Copy link
Member

We're currently in the process of thinking through what the policy API should look like, with this being a core concern. The industry standard method of kicking the can down the road is to have users specify such assertions in a language like CUE, but the UX is understandably awful and I do think that we have a mandate to offer users something better than a prickly footgun.

IMHO, for cosign itself, we should decide on the minimum viable set of values that are required to be able to assert confidence in the origin of a signature and go from there. I can't think of a good reason why we'd want to be surfacing the structure of an x509 cert as part of the standard validation path.

In this specific case, I'd be OK with deprecating --cert-email but don't believe that consolidating things into a single flag like --cert-subject would be a step in the right direction. Fundamentally, we want to assert that a sig was created by a trusted actor, without relying on pinning certs or public keys.

Currently, "trusted actor" just means a user ID (i.e. email), but the next major step will be to tie that user ID to a trusted identity provider.

... all that being said, I'm not going to deny that there are valid use cases like the one that Asra brings up. All that I'd like to see is that we have a UX-first approach to such design questions, only surfacing such implementation details as a last resort.

@dlorenc
Copy link
Member

dlorenc commented Jan 14, 2022

I like cert-subject but might be missing something. What's the concern with that @dekkagaijin ?

@dekkagaijin
Copy link
Member

dekkagaijin commented Jan 14, 2022

My concern is that what constitutes a valid x509 cert Subject is as unfriendly as it is flexible. I don't believe it's possible to create a sane, powerful, transparent, and user-friendly implementation of a unified --cert-subject, but we can pick some of those things. It wouldn't be unreasonable to offer a way for users to input CUE or some other language to express assertions on the fields and sub-(sub-)*fields of a raw cert, but that certainly wouldn't constitute a "happy path" and IMHO shouldn't be required in a canonically secure base case

@haydentherapper
Copy link
Contributor

haydentherapper commented Jan 14, 2022

For users that want simple identity pinning, a combination of a subject flag plus an OIDC issuer flag (which I'm adding on #1308) seems sufficient. I agree that it's not very robust, but for simple use cases like where you trust just one identity issuer+subject, this seems like an easy approach. We'll just need to document that cert-subject means "Check the subject alt name of a certificate only".

For more complex cases like trusting multiple subjects, trusting all SPIFFE workloads in a trust domain, etc, then policy would be what we should recommend for users. Maybe let's avoid adding regex support to cert-subject (for now, unless there's an ask for it in the very near future), and regex matching can be a part of a policy? I could also see somewhere down the line providing more customization of the issued certs, and verification of that should probably stay in a policy. Does this seem like a good approach?

@dlorenc
Copy link
Member

dlorenc commented Jan 14, 2022

We'll just need to document that cert-subject means "Check the subject alt name of a certificate only".

This is what I was imagining too. One concern might be that we have different ways expressing subjects, some of which expose multiple x509 extensions (mostly GitHub).

In the case I can start to see the concern with trying to squeeze it all into one flag. The next obvious approach is something like --cert-subject key=value (with multiple kbps) where key represents an entry in the subject alt name field (making sure to include our custom OIDs).

@haydentherapper
Copy link
Contributor

I was thinking cert-subject only handles these four cases. The additional information added to the cert for Github values would be out of scope for this flag. However, like you said, for just Github I don't know if just checking the job workflow URI would be very useful or UX-friendly.

@dekkagaijin
Copy link
Member

dekkagaijin commented Jan 14, 2022

The next obvious approach is something like --cert-subject key=value (with multiple kbps) where key represents an entry in the subject alt name field (making sure to include our custom OIDs).

I was thinking something like that, too, if only to constrain the test to one of (semantically aware) equality. It would also avoid the issue of ordering by just treating it as the dict that it is. The only issue is that it's a half measure; exposes some implementation details but doesn't allow full flexibility

@dekkagaijin
Copy link
Member

dekkagaijin commented Jan 14, 2022

The only issue

Any by that I mean "trade off"

I think everyone can admit that this is a hard, unsolved usability problem, and it'd be totally reasonable to propose some kind of middle ground experience

@asraa
Copy link
Contributor Author

asraa commented Jan 14, 2022

Yeah, the job workflow ref is pretty unfriendly to use from a client/cosign side. On the other hand though, my main problem is that running cosign veirfy $IMG to validate that something was signed in a github actions is meaningless without extra policy -- that image could have been signed in anyone else's github workflow. Maybe it's just worth calling out to workflow identity use-cases: like, using policy is super important here to pin on the expected repo/org/identity.

@ribbybibby
Copy link
Contributor

What would the key=value approach look like, more concretely?

Something like this?

--cert-subject email=example@example.com \
--cert-subject uri=https://github.com/example/example/.github/workflows/release-master.yaml@refs/tags/v0.0.1

@znewman01
Copy link
Contributor

Can we close this in favor of #1964 ? (I know that issue is a duplicate of this one so it's not totally fair, but most recent discussion has taken place over there.)

@haydentherapper
Copy link
Contributor

This also was fixed as of #1626 i believe

@haydentherapper
Copy link
Contributor

Marking as complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants