Accept URI for Sigstore Signed Images #2235
Conversation
| if !slices.Contains(untrustedCertificate.EmailAddresses, f.subjectEmail) && !strings.Contains(untrustedCertificate.URIs[0].String(), f.URI) { | ||
| if len(untrustedCertificate.EmailAddresses) > 0 { | ||
| return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %s not found (got %#v)", | ||
| f.subjectEmail, | ||
| untrustedCertificate.EmailAddresses)) | ||
| } | ||
| if len(untrustedCertificate.URIs) > 0 { | ||
| return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required URI %s not found (got %#v)", | ||
| f.URI, | ||
| untrustedCertificate.URIs)) | ||
| } |
There was a problem hiding this comment.
⚠️ If the policy requires both an email address and an URI, this must not accept signatures which only have one of them- This is invoking slices.Contains(…, "") if the field is not set. That’s at the very least unclean.
- I suspect the error reporting logic should also be tightened. It must primarily decide based on the policy, not on attacker-set untrusted* fields.
There was a problem hiding this comment.
@mtrmac Do you mind elaborating on:
I suspect the error reporting logic should also be tightened. It must primarily decide based on the policy, not on attacker-set untrusted* fields.
There was a problem hiding this comment.
E.g. if the policy requires an URI, this code could see len(untrustedCertificate.EmailAddresses) > 0 { and report something about a “required email”, when no email is required.
In general, treat all of the data in the signature as false and potentially malicious until it is explicitly validated against user-configured policy. A specific manifestation of that is that control flow should primarily be driven by that user-configured policy, and not by the signature data, if at all possible.
There was a problem hiding this comment.
In general, treat all of the data in the signature as false and potentially malicious until it is explicitly validated against user-configured policy. A specific manifestation of that is that control flow should primarily be driven by that user-configured policy, and not by the signature data, if at all possible.
Agreed. The logic seems backwards. If I specify in the policy to check for a signature with a URI in the SAN and do not specify an email, I do not care if the signature cert also has an email in the SAN at all. There is a question in that case though whether the presence of an email should mean the certificate is rejected (possibly malicious?) outright for having extra fields? I think you do not reject it for having too many SAN fields. If anything is done based only on the presence or absence of a field it should be configurable in the policy though.
But yes, only check for what the policy specifies and right now this seems to check for the presence of things based on what is defined in the cert (backwards part of the logic). Changing the logic to look based on what's defined in the policy would also probably allow you to eliminate both the slices.Contains and strings.Contains since you could directly compare those fields in the structs.
I would also agree that it's probably bad practice at a minimum to use something from golang.org/x/exp in anything destined to get merged into a main branch. Unfortunately this project seems to use go 1.19 and slices did not make it out of exp until 1.21.
There was a problem hiding this comment.
In c/image we do prefer x/exp/{slices, maps} over open-coded loops or copy&pasted helpers. These subpackages exist in Go 1.21, so we have a clear path to migrating to the standard library versions, and in the meantime, using shared well-tested code and idiomatic utilities is already better than single-use code repetition.
| if gotSubjectEmail && !gotURI { | ||
| opts = append(opts, PRSigstoreSignedFulcioWithSubjectEmail(tmp.SubjectEmail)) | ||
| } | ||
| if !gotSubjectEmail && gotURI { | ||
| opts = append(opts, PRSigstoreSignedFulcioWithURI(tmp.URI)) | ||
| } |
There was a problem hiding this comment.
This is silently ignoring the options if both are specified?!
|
@mtrmac The following is what I get from running the commands stated in coreos/rpm-ostree#4272 (comment) |
There was a problem hiding this comment.
Note also https://github.com/containers/image/blob/main/CONTRIBUTING.md#sign-your-prs ; we can’t accept code with unclear copyright status.
| return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required URI %s not found (got %#v)", | ||
| f.URI, | ||
| untrustedCertificate.URIs)) | ||
| } | ||
| } | ||
| // FIXME: Match more subject types? Cosign does: |
There was a problem hiding this comment.
This FIXME comment might eventually also need updating.
| @@ -154,6 +154,8 @@ type prSigstoreSignedFulcio struct { | |||
| OIDCIssuer string `json:"oidcIssuer,omitempty"` | |||
| // SubjectEmail specifies the expected email address of the authenticated OIDC identity, recorded by Fulcio into the generated certificates. | |||
| SubjectEmail string `json:"subjectEmail,omitempty"` | |||
| // URI specifies the expected URI of the authenticated OIDC identity, recorded by Fulcio into the generated certificates. | |||
| URI string `json:"URI,omitempty"` | |||
There was a problem hiding this comment.
This should probably be something like subjectURI (if we decide to add the URI field, and not something else).
There was a problem hiding this comment.
This should probably be something like subjectURI (if we decide to add the URI field, and not something else).
Agreed for consistency. I will also add that adding URI support is required for compatibility with keyless cosign signatures which as far as I'm concerned are the only way anyone should be using cosign to sign container images. The test workflow that @lukewarmtemp used was flawed and did not sign the container using the github actions token.
Thanks. So, wouldn’t one or more of
be a better fit? In particular the difference between 1.9 and 1.18 seems important to understand. Also, what exactly is the signature saying = what facts about the world is the policy trying to enforce?
I guess where I’m going here is:
Ultimately, in this PR the security goal that matters most is the one you care about — but at this point it’s not clear to me what that is. I understand the top-level description to be “accept signatures signed using a github workflow”, but that’s an implementation detail, not a security property. |
|
Unfortunately @lukewarmtemp is the one working on this from an issue I created against
It looks like this issue has been contemplated upstream by the sigstore devs already: https://github.com/sigstore/fulcio/blob/main/docs/oidc.md#how-to-pick-a-san I don't think we need to second guess the upstream ratione as to what the best identity is to tie to the certificate. I think the URI SAN needs to be supported. When you do There is a hint in the Fulcio OIDC docs that this could be instance specific and I'm basing my read of things by using the public Fulcio instance.
The advantage is not having to lifecycle keys manually which just feels so antiquated these days. The Fulcio CA creates ephemeral keys using an auth process done at signature time (via the ephemeral token created for the CI run). No long lived secrets to manually manage this route. To me this is a huge and distinct advantage. I have low opinions of gpg (gpg is long overdue for a modern replacement) because of how clunky and tedious key management is. Long lived manually managed keys only add a bit to security in 2023 when long lived secrets can be readily stolen in most cases. Especially if they are stored in pretty accessible CI variables. If I am faced with choosing to sign manually with static keys I would likely just opt to not sign at all. There are holes in that plan that could clearly lead to fraudulent signatures if your private key is compromised. By some definitions of compromise it's virtually guaranteed by virtue of the fact that the private key has to get from the github/gitlab to the runner. Static private keys moving around that then get used to sign things should be cause for concern. Better to issue the key at point of use and discard when done. Long lived private keys are risky and the best way to avoid that risk is to simply not have them but we do need keys to sign. Fulcio seems to solve this issue pretty eloquently. But your point about copious questionably usable metadata definitely resonates with me. I'd really like to be able to simply define an arbitrary identifier that gets added into a field on the cert that I can match off.
There are some reasonable reasons raised in the Fulcio OIDC docs about why the SAN should be used as is because it is scoped to neither be too broad nor too narrow. |
The rationale from that link is relevant; it doesn’t quite imply we should only ever consider checking the SAN value — otherwise all the other OIDs would not really need to be present in the certificate.
Cosign’s UI is a relevant precedent, but not necessarily a direct template to follow. E.g. in this example, it unnecessarily introduces a risk to only let users express the policy using an option which can match various fields with different semantics. Hence we have
That’s not my point, the existence of Fulcio is (I guess) a given. It’s “why have 20 fields in the certificate and leave it to consumers to decide which ones to trust”. To an extent, I think it’s better to the designers of the signing system to design a policy which can be widely understood and accepted.
(The recovery strategy from someone temporarily being able to invoke Fulcio to obtain a single certificate is not all that better. Pragmatically, only “abandon the OIDC identity and start signing using a new one” is well-supported by tools. That’s… perhaps even worse than keeping the project name but abandoning a public key?)
I read those reasons as equally arguing for the use of 1.9 / 1.18; using the SAN URI value is, if anything, strictly worse, because on the expected certificates the value is the same, and on unexpected certificates there is a slight risk of semantic collisions (probably mitigated by requiring a specific |
Are you proposing then that the SAN URI not be supported? The subject email SAN is already supported so that would not make sense to continue supporting one form but not another form that is in use. |
Yes, basically; because I think users (and/or us designing the policy) should be thinking harder about what the value means. The difference between 1.18 and 1.9 is an example of that ambiguity.
The “email” value has a reasonably specific semantics (pointing at a specific human … well, and/or a noreply@ address, yes). The URIs we have been discussing here are all constructed from some more specific and better-typed values; so I think of using a string match against a derived value (especially one derived by concatenation and embedded separators) is as a less preferable choice. For example, what if I create a file named Ambiguity is a risk. Contra UNIX, text is a bad format for representing composite values. |
|
We might end up just supporting a match on an SubjectAltName — either exclusively, or as one of the options. I don’t know. There is certainly a balance to be struck between clarity, convenience and risks. Where I would recommend centering the design is to answer the question “What does the signature mean” and/or “what information does the signature provide to consumers”? With a Fulcio certificate issued to an email address, the signature means “there is an account at the When a GitHub workflow is signing an image, as in here, what is the meaning of the signature, and what meaning do the users want? Maybe something similar: “This image is the one appropriate for repo:tag, and an official release build for that tag”. So we need to capture “authoritative for repo” (a GitHub repository or something like that) and “an official release build” — either by the workflow name, or by the project somehow promising to only sign/tag images of tagged releases. |
|
The original issue that generated this PR specifically addressed that keyless sigstore signatures CANNOT be verified via rpm-ostree if they're made non-interactively using the GH actions token. The reason is because the policy cannot be configured to check for a URI SAN (only email). This makes the support for keyless sigstore signatures incomplete within the containers ecosystem. As the user who submitted the issue against rpm-ostree, what I want is to be able to define a policy that matches the SAN as specified as a URI. It would be nice to support all the other OIDs available however that is well beyond the scope of my immediate needs and the original issue. For my needs the SAN URI will work fine.
This would require some error on the part of the OIDC token issuer (github perhaps) as the field that Fulcio places into the SAN URI is from the issued token. I'm not really sure there is as much difference between an email and a URI as is being made out here. In my opinion determining identity of a thing based on an email address is no better than a URI. In this case the URI communicates that:
While the current SAN URI scheme is not perfect I think it captures the important bits of info needed to identify an image signed in an automated build process.
A generic match on SAN would be an improvement over the current single choice of email. |
|
(Reordered)
Sure, and I do think that extending the supported use cases is a worthy goal.
“I want this command to pass” is not a security objective. What is the information that you want to ascertain by validating the signature?
This is a possible objective but it could equally apply to unmerged PRs.
Not quite. it is a combination of two values, constructed by Fulcio: https://github.com/sigstore/fulcio/blob/b202f9cf2797ef30146c75fad86c1e564ba9514e/pkg/identity/github/principal.go#L196C36-L196C36 .
OTOH the time to carefully choose the semantics is now. As soon as generic URI matches become possible, everyone will be much less motivated to deal with the possible alternatives. |
This seems like an unfair reductionist view of me as a user explaining the genesis of this PR and the preceding report in the issue. I believe my needs to be genuine, valid and pretty well thought out. The three bullet points I listed explain my validation needs.
Not entirely sure I understand what you're referring to here. Please elaborate if you can.
I am aware of how Fulcio adds the 'https://github.com/' to the If you examine how the other OIDs are constructed (further down in the source you linked) they are doing the exact same for the other URI fields. Even for some of the OIDs you have suggested are more applicable here. In the case of 1.9 the OID is exactly the same as what ends up in the SAN. Given that the other OIDs are all custom org specific extensions I really do not think those are better options over the SAN which is a standard extension. I believe the upstream project has already contemplated this issue of cert subject identity enough and decided what semantics are most reasonable. |
I think that’s a fair complaint — I do want to direct the conversation towards the specific goals, so let’s discuss those.
AFAICS a workflow triggered by an incoming PR (coming from an untrusted party) could satisfy all three quoted criteria:
check
check? (not sure about this part)
check
You’re right it’s probably not a major problem, especially given this one specific OIDC issuer, but I am paranoid enough to want the details to be precise.
There’s nothing standard in X.509 about the URI referring to a specific GitHub workflow on a specific branch (and the semantics being something like that seems be necessary to achieve your security objective). We are relying on semantics specific to the issuer + Fulcio in either case. |
This is a project/user use case and policy issue to decide. If the project owner wants to sign PR build artifacts by all means they should be allowed to do that and use those signatures to test (signature validation), if they want. This project should abstain from making overly opinionated unilateral decisions based on unfounded/presumptuous assumptions about what users may or may not need to do with software dependent on this library. There would be absolutely nothing to stop this from happening with static keys by the way! Can sign anything with static key files. I could see cases where signing a PR build artifact would be desirable for testing. I actually wanted to do this for testing but recall being unable to do so due to limitations with GH actions. Bottom line I do not see any problem whatsoever with the possibility that PR artifacts could get signed. I can readily see use cases where this would be desired activity. Perhaps you want to offer official test builds? We do not need to prevent that and the provided URI is capable of discriminating things that don't live on main (and the user can configure their policy to taste).
What details? I think there is sufficient detail here to understand what the use case and required feature is. The user needs to be empowered to decide what level of paranoia they are interested in adopting and tolerating. The tool should facilitate setting as restrictive or permissive of a policy as the user desires which would include trusting certs identified with a URI. The choice needs to be up to the user here, not this project.
I do not agree. Quickly reviewing RFC 5280 (X509 certs), leading to RFC 3986 (URIs) I think the specified URI is completely in conformance with the applicable Internet standards. If you believe otherwise please identify the requirements in the standard that are not satisfied. |
I agree with that. The thing to consider is “when you expressed these three bullet points as a security objective, did you intend to include untrusted PRs”? Sure, we both can imagine case where that might be desired. Is it usually? Do users think carefully enough about that?
I strongly disagree. Ever since c/image has started supporting signatures, back in 2016, I’ve been arguing with people who want me to add a knob (always the same knob) which ignores a critical security control, because they “just want to verify signatures and this is too complex”. And doing so would expose them to unwanted risks they didn’t think about. Whether we like it or not, it falls to us to make sure that users who don’t pay that much attention get, by default, “the right policy”. Because no-one else will ensure that, it seems. Sure, we can then add more options / alternatives for experts. The JSON format can accept an arbitrary number of extra option fields. But someone new to signing, following a tutorial from some blog written by someone also new to signing, should, if at all possible, not end up with a policy that accepts builds from third-party unreviewed PR builds, or any other clearly undesirable policy. Just to move things forward, I am proposing we should match on 1.18 “build config URI”.
|
I sure hope so! The URI requires specification of a git ref which would certainly be different for a PR. Whether a user does in fact understand all that, I can only hope. But, to be honest, how carefully a user is thinking about the consequences of their actions is not the concern of myself nor the concern of this project. If the user wishes to make bad decisions they should be fully enabled to do so at their own risk.
Preventing the user from making an oversight or mistake by opting outright not to support certain options is a pretty lousy solution to a user skills deficiency issue. Perhaps consider some policy validation tooling to provide this kind of feedback. If a configuration permutation is technically invalid that is one thing but if it comes down to security policy which can be quite arbitrary the user needs to have all the controls at their disposal. If the user doesn't gain the proper awareness needed to operate a tool safely that is completely on them. This project should just do its best to ensure the user can make the most educated configuration choices possible (documentation) and that those choices are actually available to them.
At this point I am finding it quite unreasonable that the SAN URI is not adopted as it appears to be the upstream choice for identifying these types of signatures. I cannot find any technical fault with the SAN URI other than that it doesn't resolve (this is a github problem) to the workflow source. However these are constructed nearly identically in the Fulcio source now so I do not care as either gets me what I'm after. |
|
@mtrmac Do you intend to make any of the changes discussed here to this PR? Does a separate issue need to be filed? Are you looking for outside contributions to add the desired feature? |
|
@jmpolom Right now this is a PR, and I have described a design that makes sense to me. So I was expecting for this to continue to be a PR where the implementation (including tests) is eventually finalized by the original author. If you want to track this as a RFE issue, separately from the fate of this PR, that’s of course fine. Taking over this PR by someone else would typically also be an option, but here with a missing DCO sign-off it’s not quite that easy. Prioritization of my time to implement new features is primarily decided by my employer. |
This is probably what I was mostly getting at. Not on your (immediate) horizon? If I sent changes to this PR could they be accepted without the previous author providing a DCO? Would it be better to open a new PR? |
lukewarmtemp
force-pushed
the
accept-fulcio-uri
branch
from
b300964 to
92699bd
Compare
|
My “immediate” horizon is booked pretty solid; I can’t really predict the priorities for next month. I think by far the easiest way to resolve the current situation would be for @lukewarmtemp to upload a version with a DCO (even if there were no other changes). @lukewarmtemp , could you do that, please? Without that, the situation for anyone who has seen this PR is… nonobvious? |
lukewarmtemp
force-pushed
the
accept-fulcio-uri
branch
from
92699bd to
afab406
Compare
|
@mtrmac Yup, just updated the commit. Feel free to let me know if it's good or not. |
|
@lukewarmtemp Thanks, that helps a lot. |
|
@lukewarmtemp @mtrmac anything going on with this PR? |
|
@lukewarmtemp please toggle this from Draft to PR 🙏 |
Design discussions happening in Slack, sadly not visible here. |
|
@mtrmac Here are five things we can potentially verify against: Option 1 This is the strictest combination, requiring verification against workflow name and ref used Option 2 This is a slightly looser combination since it does not verify against workflow name and is more suited for general use in which workflow is an implementation detail. Adding What are your thoughts on these options? |
How is this any different than just supporting the URI SAN? The definition in the Fulcio source for OID Options would be nice that allow configuring the specificity of the match within some bounds. Supporting all of the mentioned OID combinations seems like a good idea. |
|
@lukewarmtemp Needs a rebase? |
Signed-off-by: Luke Yang <[email protected]>
lukewarmtemp
force-pushed
the
accept-fulcio-uri
branch
from
afab406 to
7cfeed6
Compare
|
@rhatdan @sallyom, I’ve updated the PR with a DCO #2235 (comment), toggled from draft to PR, and rebased the code as requested. However, there still are unresolved implementation details preventing this PR from merging. To summarize the discussion to the best of my understanding, there is debate regarding which fields for signature verification should be supported #2235 (comment). There is a need to broaden the fields that policies can verify against and is the very reason this PR was created coreos/rpm-ostree#4272 (comment). However, allowing too many fields can result in greater risks for users since they might end up with a policy that accepts builds from third-party unreviewed PR builds. Therefore, to move forwards, we need to agree on what fields we want to verify in the policy. I’ve outlined two potential options based on previous discussion that I determined could be a suitable way forwards #2235 (comment). I am waiting on @mtrmac for his opinion on these options before making any more changes to this PR. I can implement changes as soon as we have a clear direction for how to move forwards. However, I am not an expert in this code base as it is outside the scope of repositories I usually work in. I took on this PR since I wanted to offer a meaningful contribution to this repository which is related to |
Fixes coreos/rpm-ostree#4272 (comment)
Allows for container images signatures with a URI instead of a email
address as the SAN. This is needed in certain cases such as using a github workflow to sign a container using the github actions OIDC token.
The URI field was added to the
fulcioTrustRootstruct and associated functions were modifed/created. Logic to handle either having a URI or email address as the SAN was also implemented.Testing:
Copy and store
fulcio_v1.crt.pemto/etc/pki/containers/fulcio.sigstore.dev.pubandrekor.pubto/etc/pki/containers/rekor.sigstore.dev.pub, both of which can be found at: https://github.com/sigstore/root-signing/blob/main/README.mdConfigure
/etc/containers/policy.json/etc/containers/registries.d/ghcr.io.yaml