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 #2182

Closed
wants to merge 1 commit into from

Conversation

lukewarmtemp
Copy link

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

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.

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

Hello @mtrmac, this is a small patch I've made to the sigstore verification process in order to address the issue linked in the PR message. I was wondering if you could both review my PR and offer some insight on some next steps/ways to generalize the patch so that we can catch more bugs related to sigstore from coming up down the line.

@cgwalters
Copy link
Contributor

Thanks for working on this!

Without digging into the details, I think (unless I'm misunderstanding something here) this should be a PR against https://github.com/containers/image - any changes to the vendored sources here would be lost when updating.

BTW a common thing when developing Go is to use the replace directive so that you can build a skopeo binary with your changes to the vendored library for integration testing.

Copy link
Contributor

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

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

Comment on lines +179 to +189
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
Contributor

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

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

If the policy requires both an email address and an URI, this must not accept signatures which only have one of them

I believe this would be an invalid policy with cosign. One or the other. A signed container won't have all the OIDs from my experience it will have a subset based on whether it was signed via the interactive cosign workflow or keyless method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then such an invalid policy must be rejected at parsing/initialization time, not processed in an unexpected way.

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
Contributor

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

Moved to containers/image#2235

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
4 participants