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

Accept URI for Sigstore Signed Images #2235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukewarmtemp
Copy link

@lukewarmtemp lukewarmtemp commented Jan 2, 2024

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 fulcioTrustRoot struct and associated functions were modifed/created. Logic to handle either having a URI or email address as the SAN was also implemented.

Testing:

  1. Copy and store fulcio_v1.crt.pem to /etc/pki/containers/fulcio.sigstore.dev.pub and rekor.pub to /etc/pki/containers/rekor.sigstore.dev.pub, both of which can be found at: https://github.com/sigstore/root-signing/blob/main/README.md

  2. Configure /etc/containers/policy.json

{
  "default": [
    {
      "type": "reject"
    }
  ],
  "transports": {
    "docker": {
      "registry.access.redhat.com": [
        {
          "type": "signedBy",
          "keyType": "GPGKeys",
          "keyPath": "/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release"
        }
      ],
      "registry.redhat.io": [
        {
          "type": "signedBy",
          "keyType": "GPGKeys",
          "keyPath": "/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release"
        }
      ],
      "ghcr.io/jmpolom": [
        {
          "type": "sigstoreSigned",
          "fulcio": {
            "caPath": "/etc/pki/containers/fulcio.sigstore.dev.pub",
            "oidcIssuer": "https://token.actions.githubusercontent.com",
            "URI": "https://github.com/jmpolom/fedora-ostree-ws-container/.github/workflows/build.yml@refs/heads/main"
          },
          "rekorPublicKeyPath": "/etc/pki/containers/rekor.sigstore.dev.pub",
          "signedIdentity": {
            "type": "matchRepository"
          }
        }
      ]
    },
    "docker-daemon": {
      "": [
        {
          "type": "insecureAcceptAnything"
        }
      ]
    }
  }
}
  1. Create /etc/containers/registries.d/ghcr.io.yaml
docker:
     ghcr.io/jmpolom:
         use-sigstore-attachments: true
  1. Build the skopeo binary and run the following
$ ./bin/skopeo copy docker://ghcr.io/jmpolom/fedora-silverblue-ws:38-main dir:/some/local/directory

Comment on lines +179 to +192
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))
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ⚠️ 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@jmpolom jmpolom Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +346 to +351
if gotSubjectEmail && !gotURI {
opts = append(opts, PRSigstoreSignedFulcioWithSubjectEmail(tmp.SubjectEmail))
}
if !gotSubjectEmail && gotURI {
opts = append(opts, PRSigstoreSignedFulcioWithURI(tmp.URI))
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is silently ignoring the options if both are specified?!

@lukewarmtemp
Copy link
Author

Thanks!

  • Yes, this needs to be a PR against containers/image, not against Skopeo . Please file one and we’ll continue there.
  • I’m not immediately sure the URI field is the one we should target. Can you post the full openssl x509 -text of the generated certificate, please? https://github.com/sigstore/fulcio/blob/main/docs/oid-info.md#directory documents various more specific fields, maybe this should actually target one (or more?) of them. OTOH just matching on SubjectAltName URIs might be more general. I don’t have a strong opinion so far.
  • For c/image/signature, it would also need unit tests with close-to-perfect coverage (both of the policy parsing and implementation/enforcement).

@lukewarmtemp lukewarmtemp changed the title Accept fulcio uri Accept URI for Sigstore Signed Images Jan 2, 2024
@lukewarmtemp
Copy link
Author

lukewarmtemp commented Jan 2, 2024

Can you post the full openssl x509 -text of the generated certificate, please?

@mtrmac The following is what I get from running the commands stated in coreos/rpm-ostree#4272 (comment)

$ skopeo inspect docker://ghcr.io/jmpolom/fedora-silverblue-ws@sha256:48bb3ee03f4687f86ac0c4d095e096dfa547003321f2519f2d547a2a7dd81ab0 | jq -r .LayersData[0].Annotations.\"dev.sigstore.cosign/certificate\" > cert
$openssl x509 -text -noout -in cert
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            57:14:84:bb:73:33:70:42:10:5c:0b:62:5b:97:38:67:f1:33:4a:4e
        Signature Algorithm: ecdsa-with-SHA384
        Issuer: O = sigstore.dev, CN = sigstore-intermediate
        Validity
            Not Before: Aug 16 12:35:48 2023 GMT
            Not After : Aug 16 12:45:48 2023 GMT
        Subject: 
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:7c:e1:10:2f:f2:af:7a:c2:c8:63:2e:eb:c5:f2:
                    ac:dd:d2:8b:bc:69:6c:4b:b2:c0:84:60:50:1c:dc:
                    e0:fa:92:5d:23:1b:b8:5e:21:c0:77:10:79:b1:2f:
                    3e:82:10:37:8c:55:0e:99:c4:dd:30:e2:ac:59:27:
                    46:26:6e:0a:30
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        X509v3 extensions:
            X509v3 Key Usage: critical
                Digital Signature
            X509v3 Extended Key Usage: 
                Code Signing
            X509v3 Subject Key Identifier: 
                94:5C:C3:7F:60:48:47:17:17:E5:80:65:E6:00:3E:02:B1:0E:0B:AC
            X509v3 Authority Key Identifier: 
                DF:D3:E9:CF:56:24:11:96:F9:A8:D8:E9:28:55:A2:C6:2E:18:64:3F
            X509v3 Subject Alternative Name: critical
                URI:https://github.com/jmpolom/fedora-ostree-ws-container/.github/workflows/build.yml@refs/heads/main
            1.3.6.1.4.1.57264.1.1: 
                https://token.actions.githubusercontent.com
            1.3.6.1.4.1.57264.1.2: 
                push
            1.3.6.1.4.1.57264.1.3: 
                f64127443e51de5aebe1b675310dc9f793cd15ff
            1.3.6.1.4.1.57264.1.4: 
                Build and push ostree containers
            1.3.6.1.4.1.57264.1.5: 
                jmpolom/fedora-ostree-ws-container
            1.3.6.1.4.1.57264.1.6: 
                refs/heads/main
            1.3.6.1.4.1.57264.1.8: 
                .+https://token.actions.githubusercontent.com
            1.3.6.1.4.1.57264.1.9: 
                .ahttps://github.com/jmpolom/fedora-ostree-ws-container/.github/workflows/build.yml@refs/heads/main
            1.3.6.1.4.1.57264.1.10: 
                .(f64127443e51de5aebe1b675310dc9f793cd15ff
            1.3.6.1.4.1.57264.1.11: 
github-hosted   .
            1.3.6.1.4.1.57264.1.12: 
                .5https://github.com/jmpolom/fedora-ostree-ws-container
            1.3.6.1.4.1.57264.1.13: 
                .(f64127443e51de5aebe1b675310dc9f793cd15ff
            1.3.6.1.4.1.57264.1.14: 
                ..refs/heads/main
            1.3.6.1.4.1.57264.1.15: 
                ..640089852
            1.3.6.1.4.1.57264.1.16: 
                ..https://github.com/jmpolom
            1.3.6.1.4.1.57264.1.17: 
                ..399581
            1.3.6.1.4.1.57264.1.18: 
                .ahttps://github.com/jmpolom/fedora-ostree-ws-container/.github/workflows/build.yml@refs/heads/main
            1.3.6.1.4.1.57264.1.19: 
                .(f64127443e51de5aebe1b675310dc9f793cd15ff
            1.3.6.1.4.1.57264.1.20: 
                ..push
            1.3.6.1.4.1.57264.1.21: 
                .Xhttps://github.com/jmpolom/fedora-ostree-ws-container/actions/runs/5878667266/attempts/1
            1.3.6.1.4.1.57264.1.22: 
                ..public
            CT Precertificate SCTs: 
                Signed Certificate Timestamp:
                    Version   : v1 (0x0)
                    Log ID    : DD:3D:30:6A:C6:C7:11:32:63:19:1E:1C:99:67:37:02:
                                A2:4A:5E:B8:DE:3C:AD:FF:87:8A:72:80:2F:29:EE:8E
                    Timestamp : Aug 16 12:35:49.021 2023 GMT
                    Extensions: none
                    Signature : ecdsa-with-SHA256
                                30:45:02:20:7D:E6:AE:44:F5:A0:23:18:BD:48:34:D4:
                                7F:F1:8E:86:CD:0B:78:2C:FE:40:1F:BD:6A:A7:DD:F3:
                                0B:B5:50:02:02:21:00:A3:E8:7C:78:AC:34:41:29:2C:
                                41:D5:B6:49:EE:E8:93:7B:A0:BF:DC:23:95:97:A3:FA:
                                21:A2:77:D9:87:D0:33
    Signature Algorithm: ecdsa-with-SHA384
    Signature Value:
        30:65:02:31:00:98:45:d6:90:f2:9c:34:10:fc:ca:14:57:3c:
        da:49:5d:97:80:ab:6c:2c:7d:fa:33:ec:0f:1f:86:b0:d2:cb:
        3d:fd:84:b9:c6:f7:84:d3:7d:b7:82:5e:eb:9a:9f:00:7a:02:
        30:01:6e:d1:b8:91:6f:14:b0:18:9a:db:86:7e:19:52:d3:db:
        a0:0e:6d:7a:e6:c8:6d:1f:a2:a8:e2:ae:41:74:1d:08:87:82:
        8e:db:11:eb:a2:14:a1:d2:22:7c:58:4d:e0

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be something like subjectURI (if we decide to add the URI field, and not something else).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 2, 2024

Can you post the full openssl x509 -text of the generated certificate, please?

@mtrmac The following is what I get from running the commands stated in coreos/rpm-ostree#4272 (comment)

Thanks. So, wouldn’t one or more of

  • 1.3.6.1.4.1.57264.1.12 | Source Repository URI
  • 1.3.6.1.4.1.57264.1.14 | Source Repository Ref
  • 1.3.6.1.4.1.57264.1.9 | Build Signer URI
  • 1.3.6.1.4.1.57264.1.18 | Build Config URI

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?

  • We could have a policy that requires a specific git commit to be the source of the accepted build, but that’s clearly unmaintainable … and pointless, we could just as well only accept specified image digests without having a signature policy.
  • A policy that requires “was run from refs/heads/main sometime in the past” is, at least, saying “this code was merged into the main branch based on the project’s policies” and built by CI, I guess that’s something for projects with a rapid CI/CD workflow … but wouldn’t those projects want some kind of limit on “only accept builds from the last 2 months, not 3 years old”?
  • For third-party produced software shipped to an external customer, that customer probably doesn’t care about many of the fields (and in particular, that customer doesn’t want to update their policy.json just because the project decided go rename files in .github/workflows). I suppose that customer might want the repository to match… and the version to (somehow) match the tag of the image (which is a completely different kind of feature, sure).

I guess where I’m going here is:

  • It’s not very clear to me that the Fulcio approach with a ton of metadata in a certificate is all that valuable, compared to the software author just holding a private key. That might well be my lack of knowledge.
  • It seems to me that allowing matches on the 1.12 value, and perhaps on the 1.14 value, would be generalizable to several other use cases; a full match on 1.9/1.18 is only truly relevant to consumers strongly integrated with the repository producing the signature.

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.

@jmpolom
Copy link

jmpolom commented Jan 2, 2024

Unfortunately @lukewarmtemp is the one working on this from an issue I created against rpm-ostree. I am not the one doing the implementation work.

Thanks. So, wouldn’t one or more of

1.3.6.1.4.1.57264.1.12 | Source Repository URI~~

I actually would like this one much better personally. I think this one is much more user friendly and intuitive. Using the URI to the CI workflow is more likely to break if CI configurations are revised and will require matching updates to the policy. The location of the repo though, however, is probably much less likely to change with development. As you said you'd need tight integration with the upstream project's development to get any value (instead of annoyance) from the URI to the CI config.

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 cosign verify for a keyless signed container you specify the option --certificate-identity= which will look in the SAN field for either an email (if it was an interactive keyless sign) or a URI (if non-interactive in CI). I've not seen the DNS name or IP address variants in practice which are listed as the other options.

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.

I guess where I’m going here is:

  • It’s not very clear to me that the Fulcio approach with a ton of metadata in a certificate is all that valuable, compared to the software author just holding a private key. That might well be my lack of knowledge.

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.

  • It seems to me that allowing matches on the 1.12 value, and perhaps on the 1.14 value, would be generalizable to several other use cases; a full match on 1.9/1.18 is only truly relevant to consumers strongly integrated with the repository producing the signature.

I'd agree that those OIDs are probably better choices than the SAN. 1.14 probably does not make sense without 1.12 though but 1.12 should be usable with 1.14.

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.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 3, 2024

I don't think we need to second guess the upstream ratione as to what the best identity is to tie to the certificate.

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.

I think the URI SAN needs to be supported. When you do cosign verify for a keyless signed container you specify the option --certificate-identity= which will look in the SAN field for either an email (…) or a URI (…).

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 subjectEmail, not (in effect) anySubjectAltNameString.

  • It’s not very clear to me that the Fulcio approach with a ton of metadata in a certificate is all that valuable, compared to the software author just holding a private key. That might well be my lack of knowledge.

The advantage is not having to lifecycle keys manually which just feels so antiquated these days.

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.

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.

(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?)

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.

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 oidcIssuer — only if that issuer is sufficiently narrowly scoped).

@jmpolom
Copy link

jmpolom commented Jan 3, 2024

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 oidcIssuer — only if that issuer is sufficiently narrowly scoped).

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.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 3, 2024

Are you proposing then that the SAN URI not be supported?

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 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.

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 .github/workflows/build.yml@refs/heads/main in my repository? (It’s not that I can construct an attack right now, it’s just one more risk to keep track of.)

Ambiguity is a risk. Contra UNIX, text is a bad format for representing composite values.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 3, 2024

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 oidcIssuer with that email address, and the owner of that OIDC account has logged in to approve this signature”. And consumers can see “The image was declared to be the one appropriate for repo:tag by the owner of that OIDC account” (e.g. “@mtrmac has declared that this is the official image for Skopeo 1.20.0”)

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.

@jmpolom
Copy link

jmpolom commented Jan 3, 2024

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.

For example, what if I create a file named .github/workflows/build.yml@refs/heads/main in my repository? (It’s not that I can construct an attack right now, it’s just one more risk to keep track of.)

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:

  • the signature was made via an automated process
  • the process had permission to authenticate non-interactively to the controlling service
  • the Fulcio CA signed the cert after the successful auth

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.

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.

A generic match on SAN would be an improvement over the current single choice of email.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 3, 2024

(Reordered)

This makes the support for keyless sigstore signatures incomplete within the containers ecosystem.

Sure, and I do think that extending the supported use cases is a worthy goal.


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.

“I want this command to pass” is not a security objective. What is the information that you want to ascertain by validating the signature?

  • the signature was made via an automated process
  • the process had permission to authenticate non-interactively to the controlling service
  • the Fulcio CA signed the cert after the successful auth

This is a possible objective but it could equally apply to unmerged PRs.


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.

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 .


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.

A generic match on SAN would be an improvement over the current single choice of email.

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.

@jmpolom
Copy link

jmpolom commented Jan 4, 2024

“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 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.

This is a possible objective but it could equally apply to unmerged PRs.

Not entirely sure I understand what you're referring to here. Please elaborate if you can.

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 .

I am aware of how Fulcio adds the 'https://github.com/' to the job_workflow_ref in the GH actions OIDC token. I find this a pretty minor manipulation on their part to achieve an FQDN for the URI. I really do not see this as a major problem. The most meaningful bit is straight from the OIDC token.

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.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 4, 2024

“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 seems like an unfair reductionist view

I think that’s a fair complaint — I do want to direct the conversation towards the specific goals, so let’s discuss those.

The three bullet points I listed explain my validation needs.

This is a possible objective but it could equally apply to unmerged PRs.

Not entirely sure I understand what you're referring to here. Please elaborate if you can.

AFAICS a workflow triggered by an incoming PR (coming from an untrusted party) could satisfy all three quoted criteria:

  • the signature was made via an automated process

check

the process had permission to authenticate non-interactively to the controlling service

check? (not sure about this part)

  • the Fulcio CA signed the cert after the successful auth

check


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 .

I am aware of how Fulcio adds the 'https://github.com/' to the job_workflow_ref in the GH actions OIDC token. I find this a pretty minor manipulation on their part to achieve an FQDN for the URI. I really do not see this as a major problem.

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.

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.

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.

@jmpolom
Copy link

jmpolom commented Jan 5, 2024

AFAICS a workflow triggered by an incoming PR (coming from an untrusted party) could satisfy all three quoted criteria

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).

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.

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.

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.

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.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 5, 2024

AFAICS a workflow triggered by an incoming PR (coming from an untrusted party) could satisfy all three quoted criteria

This is a project/user use case and policy issue to decide.

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?


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.

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”.

  • In this example, it’s the same value as the SAN URI, so for users who claim not to care about these details, it would make no difference whether we match on one or the other.
  • The “top level workflow” is the part that is more likely to differentiate between PR builds and release builds; and we can document that?
    • The workflow must only be used for publishing “the same kind of builds” (e.g. release builds only, or testing builds only, or PR builds only)
    • The workflow is responsible for singing the right repo:tag value, matching the built contents.
  • (I’d be happier with a separate repo and workflow fields, but that’s not something Fulcio generates today.)

@jmpolom
Copy link

jmpolom commented Jan 5, 2024

Sure, we both can imagine case where that might be desired. Is it usually? Do users think carefully enough about that?

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.

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.

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.

In this example, it’s the same value as the SAN URI, so for users who claim not to care about these details, it would make no difference whether we match on one or the other.

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 mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jan 8, 2024
j1mc added a commit to j1mc/ansible-silverblue-oci that referenced this pull request Jan 28, 2024
@jmpolom
Copy link

jmpolom commented Feb 8, 2024

@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?

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 8, 2024

@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.

@jmpolom
Copy link

jmpolom commented Feb 8, 2024

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?

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 8, 2024

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

@mtrmac Yup, just updated the commit. Feel free to let me know if it's good or not.

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 8, 2024

@lukewarmtemp Thanks, that helps a lot.

@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

@lukewarmtemp @mtrmac anything going on with this PR?

@sallyom
Copy link

sallyom commented May 16, 2024

@lukewarmtemp please toggle this from Draft to PR 🙏

@lukewarmtemp lukewarmtemp marked this pull request as ready for review May 16, 2024 15:49
@mtrmac
Copy link
Collaborator

mtrmac commented May 16, 2024

@lukewarmtemp @mtrmac anything going on with this PR?

Design discussions happening in Slack, sadly not visible here.

@lukewarmtemp
Copy link
Author

@mtrmac Here are five things we can potentially verify against:

1.3.6.1.4.1.57264.1.8 | Issuer (V2)
1.3.6.1.4.1.57264.1.12 | Source Repository URI
1.3.6.1.4.1.57264.1.14 | Source Repository Ref
1.3.6.1.4.1.57264.1.9 | Build Signer URI
1.3.6.1.4.1.57264.1.18 | Build Config URI

Option 1
1.3.6.1.4.1.57264.1.8 AND 1.3.6.1.4.1.57264.1.9
OR
1.3.6.1.4.1.57264.1.8 AND 1.3.6.1.4.1.57264.1.18

This is the strictest combination, requiring verification against workflow name and ref used

Option 2
1.3.6.1.4.1.57264.1.8 AND 1.3.6.1.4.1.57264.1.12 (AND optionally, 1.3.6.1.4.1.57264.1.14 )

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 1.3.6.1.4.1.57264.1.14 can allow for branch name verification as well if we decide that it's important.

What are your thoughts on these options?

@jmpolom
Copy link

jmpolom commented May 17, 2024

Option 1
1.3.6.1.4.1.57264.1.8 AND 1.3.6.1.4.1.57264.1.9

How is this any different than just supporting the URI SAN? The definition in the Fulcio source for OID 1.3.6.1.4.1.57264.1.9 and the URI are the exact same. Refer to the link to the source above.

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.

@sallyom @rhatdan

@rhatdan
Copy link
Member

rhatdan commented May 28, 2024

@lukewarmtemp Needs a rebase?

Signed-off-by: Luke Yang <[email protected]>
@lukewarmtemp
Copy link
Author

@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 rpm-ostree. It seems as though there are many others who are more experienced in this field. Thus, if anyone is willing to take over this PR or wants to create a new PR to rewrite or re-work the commits here as needed, they have my full support to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image signature not getting verified for container-native builds signed with Cosign
5 participants