policy.json BYOPKI signature verification API #2579

QiWang19 · 2024-09-20T20:01:04Z

QiWang19 · 2024-09-20T20:09:20Z

Hi @mtrmac , I’ve created this draft PR for the policy.json API change, could you take a look and provide your initial thoughts on the structure of the API update?
I just want to make sure I’m on the right path before proceeding further. My plan is to merge both the API and code support in a single PR. What do you think?

mtrmac

Thanks! Looks good overall.

Earlier discussion openshift/enhancements#1658 .

Doing both the API and implementation in one PR works for me, having the API available but broken is not really helping anything.

signature/policy_types.go

QiWang19 · 2024-10-03T02:37:01Z

@mtrmac Could you please take a look? It's ready for review. I am unsure about how I’m handling the intermediate certificates.

mtrmac

Just a few-minute first pass — this looks very good. I didn’t read this carefully, and I skipped the tests completely, for now.

Combining the policy-provided and signature-provided intermediate certificates makes sense to me, but I’ll at least check what Cosign does.

signature/pki_cert.go

signature/policy_eval_sigstore.go

mtrmac · 2024-10-03T20:36:31Z

Cc: @Honny1 as well — c/image/signature is a somewhat separate part of the codebase, and as security-critical with a lot of paranoia. Worth understanding the structure and idioms.

QiWang19 · 2024-10-09T15:20:17Z

@mtrmac Do you think the PKI code needs a file like fulcio_cert_stub.go and should include build tags? I'm not fully clear on the purpose of this compile configuration.

mtrmac · 2024-10-09T21:47:21Z

The signature/*_stub.go files were added in #2180 , to avoid dependencies on sigstore Go modules not packaged (at the time??) in Debian.

Here, the only similar dependency seems to be github.com/sigstore/sigstore/pkg/cryptoutils, which is also required by no-stubbed policy_eval_sigstore.go; so this shouldn’t need a stub.

QiWang19 · 2024-10-25T02:08:20Z

Combining the policy-provided and signature-provided intermediate certificates makes sense to me, but I’ll at least check what Cosign does.

I checked some cosign code, maybe I got lost in the code path, I did see it process the signature-provided intermediate certificates 🤔

QiWang19 · 2024-10-25T02:08:45Z

@mtrmac could you review?

signature/policy_eval_sigstore.go

signature/pki_cert.go

saschagrunert

Just two nits, otherwise LGTM

docs/containers-policy.json.5.md

saschagrunert · 2024-11-19T11:06:10Z

@containers/image-maintainers can we get this merged?

QiWang19 · 2025-01-07T15:43:20Z

@mtrmac Could you review this PR?

QiWang19 · 2025-01-08T16:36:13Z

Rebased with the base branch.

dmitris · 2025-01-27T08:20:34Z

OCPNODE-2338

now tracked in RUN-2436

dmitris · 2025-01-27T08:29:21Z

Rebased with the base branch.

@QiWang19 could you rebase it again? (Maybe this would help to get this merged 😄 )

Signed-off-by: Qi Wang <[email protected]>

mtrmac

Looks very good overall, ~1.5 comment on the substance of the code; almost all are about testing or documentation.

mtrmac · 2025-02-07T22:31:56Z

docs/containers-policy.json.5.md

@@ -350,6 +358,11 @@ Both `oidcIssuer` and `subjectEmail` are mandatory,
 exactly specifying the expected identity provider,
 and the identity of the user obtaining the Fulcio certificate.

+If `pki` is present, the signature must be based on a non-Fulcio X.509 certificate.
+One of `caRootsPath` and `caRootsData` must be specified, containing the public key of the CA.
+Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing the public key of the intermediate CA.


Isn’t this

Suggested change

Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing the public key of the intermediate CA.

Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing certificates of intermediate CAs.

?

mtrmac · 2025-02-07T22:33:25Z

docs/containers-policy.json.5.md

+If `pki` is present, the signature must be based on a non-Fulcio X.509 certificate.
+One of `caRootsPath` and `caRootsData` must be specified, containing the public key of the CA.
+Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing the public key of the intermediate CA.
+One of `subjectEmail` and `subjectHostname` must be specified, exactly specifying the expected identity provider, and the identity of the user obtaining the certificate.


Suggested change

One of `subjectEmail` and `subjectHostname` must be specified, exactly specifying the expected identity provider, and the identity of the user obtaining the certificate.

One of `subjectEmail` and `subjectHostname` must be specified, exactly specifying the expected identity of the user obtaining the certificate.

and the latter part perhaps “the expected subject of the certificate”, or “expected identity to which the certificate was issued”.

mtrmac · 2025-02-07T22:36:29Z

docs/containers-policy.json.5.md

@@ -407,6 +420,18 @@ selectively allow individual transports and scopes as desired.
                    "rekorPublicKeyPath": "/path/to/rekor.pub",
                }
            ],
+            /* A Sigstore-signed repository using a certificate generated by the Bring Your Own Public Key Infrastructure (BYOPKI).*/


Something like

Suggested change

/* A Sigstore-signed repository using a certificate generated by the Bring Your Own Public Key Infrastructure (BYOPKI).*/

/* A Sigstore-signed repository using a certificate generated by a custom public-key infrastructure. */

? I’m not sure that BYOPKI is a widely-used term, and by its nature there is no “the” BYOPKI, every user has its own.

mtrmac · 2025-02-07T22:38:33Z

signature/policy_types.go

@@ -120,7 +120,7 @@ type prSigstoreSigned struct {
 	// KeyDatas is a set of trusted keys, base64-encoded. Exactly one of KeyPath, KeyPaths, KeyData, KeyDatas and Fulcio must be specified.
 	KeyDatas [][]byte `json:"keyDatas,omitempty"`

-	// Fulcio specifies which Fulcio-generated certificates are accepted. Exactly one of KeyPath, KeyPaths, KeyData, KeyDatas and Fulcio must be specified.
+	// Fulcio specifies which Fulcio-generated certificates are accepted. Exactly one of KeyPath, KeyPaths, KeyData, KeyDatas, Fulcio, and PKI must be specified.


Also the comments for KeyPath, KeyPaths, KeyData, KeyDatas need to be updated.

mtrmac · 2025-02-07T22:38:50Z

signature/policy_types.go

@@ -141,6 +141,9 @@ type prSigstoreSigned struct {
 	// otherwise it is optional (and Rekor inclusion is not required if a Rekor public key is not specified).
 	RekorPublicKeyDatas [][]byte `json:"rekorPublicKeyDatas,omitempty"`

+	// PKI specifies which PKI-generated certificates are accepted. Exactly one of KeyPath, KeyData, Fulcio, PKI must be specified.


Also KeyPaths, KeyDatas.

mtrmac · 2025-02-08T01:11:25Z

signature/pki_cert.go

+	untrustedLeafCerts, err := cryptoutils.UnmarshalCertificatesFromPEM(untrustedCertificateBytes)
+	if err != nil {
+		return nil, internal.NewInvalidSignatureError(fmt.Sprintf("parsing leaf certificate: %v", err))
+	}
+	switch len(untrustedLeafCerts) {
+	case 0:
+		return nil, internal.NewInvalidSignatureError("no certificate found in signature certificate data")
+	case 1:
+		break // OK
+	default:
+		return nil, internal.NewInvalidSignatureError("unexpected multiple certificates present in signature certificate data")
+	}
+	untrustedCertificate := untrustedLeafCerts[0]


The failure cases should have tests, similar to the ones in fulcio_cert.go.

Alternatively (preferably?) extract the code+test in fulcio_cert.go into a separate function, and share the code.

mtrmac · 2025-02-08T01:12:14Z

signature/pki_cert.go

+	if pkiTrustRoot.subjectEmail != "" {
+		if !slices.Contains(untrustedCertificate.EmailAddresses, pkiTrustRoot.subjectEmail) {
+			return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %q not found (got %q)",
+				pkiTrustRoot.subjectEmail,
+				untrustedCertificate.EmailAddresses))
+		}
+	}
+
+	if pkiTrustRoot.subjectHostname != "" {
+		if err = untrustedCertificate.VerifyHostname(pkiTrustRoot.subjectHostname); err != nil {
+			return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Unexpected subject hostname: %v", err))
+		}
+	}
+


We should establish trust in the certificate contents (that it is correctly signed) first, only then it makes sense to read any of the data fields.

So, (roots are done); intermediates first; individual certificate next; validate certificate against root+intermediates; then read contents.

mtrmac · 2025-02-08T01:13:56Z

signature/pki_cert.go

+		}
+	}
+
+	var trustedAndUntrustedIntermediatePool *x509.CertPool


See elsewhere about “trusted intermediates”.

Anyway, trusted+untrusted = untrusted, so naming this untrustedIntermediatePool would more accurately convey the risk.

mtrmac · 2025-02-08T01:16:49Z

signature/pki_cert.go

+			return nil, internal.NewInvalidSignatureError(fmt.Sprintf("loading certificate chain: %v", err))
+		}
+		if len(untrustedIntermediateChain) > 1 {
+			for _, untrustedIntermediateCert := range untrustedIntermediateChain {


The Fulcio code only processes [:len(untrustedIntermediateChain)-1] (because the last one is supposed to be the root which we already have), doesn’t it make sense to parse the same annotation the same way here?

Otherwise the len()>1 could cause us to ignore single-element chains for no clear reason.

mtrmac · 2025-02-08T01:25:34Z

signature/pki_cert_test.go

+	}, certBytes, chainBytes)
+	require.NoError(t, err)
+	assertPublicKeyMatchesCert(t, certBytes, pk)
+


This should test more failures, similar to the existing Fulcio code:

Failures to parse intermediates

Failure with intermediates with neither signature nor config, success with intermediates in signature only / config only

Failures to extract the leaf certificate (or perhaps extract that into a helper, see elsewhere)

For both email and subject: missing SAN entry; multiple entries, one matches; one entry, mismatch; multiple entries, all mismatches

mtrmac · 2025-02-08T02:09:31Z

Combining the policy-provided and signature-provided intermediate certificates makes sense to me, but I’ll at least check what Cosign does.

I checked some cosign code, maybe I got lost in the code path, I did see it process the signature-provided intermediate certificates 🤔

https://github.com/sigstore/cosign/blob/ad478088320a3c04a96b3c183bbde2205fff7bbb/cmd/cosign/cli/verify/verify.go#L248

I think this is for cosign verify with an explicit --certificate option. Users usually don’t do that.

I think the relevant code path for reading signatures from the image is https://github.com/sigstore/cosign/blob/737c83c71cd42ba8db843f84beadcd59b3aa6e0a/pkg/cosign/verify.go#L822-L841 .

It is convoluted, but, effectively, if the configuration contains intermediates, the ones from the signature are ignored.

I think that behavior is unintuitive — it means that adding more configuration (possibly 100% redundant configuration) can turn accepted signatures into rejected signatures.

That code comes from sigstore/cosign#1804 , which effectively proposes to use TUF to stop accepting intermediate certificates without revoking them in any way. I don’t understand the use of intermediate certificates in this manner; it seems to me that shipping these intermediates “as trusted roots” would work well, then TUF could be used to ship a configuration update where the compromised certificate is simply dropped.

For comparison, Go’s TLS implementation takes certificates only from the protocol data, it does not allow users to provide intermediates at all.

For BYOPKI, I wouldn’t expect TUF to be used. If I’m not missing something subtle about this, let’s leave the behavior as is.

QiWang19 changed the title ~~policy.json BYOPKI signature verification API~~ WIP: policy.json BYOPKI signature verification API Sep 20, 2024

mtrmac reviewed Sep 20, 2024

View reviewed changes

signature/policy_types.go Show resolved Hide resolved

signature/policy_types.go Outdated Show resolved Hide resolved

signature/policy_types.go Outdated Show resolved Hide resolved

mtrmac mentioned this pull request Sep 24, 2024

Request for Sigstore signature verification enhancement and flexibility in cosign verify. #2027

Open

QiWang19 force-pushed the byo-pki-verify branch 2 times, most recently from b3a0606 to f8585cd Compare October 2, 2024 23:08

QiWang19 changed the title ~~WIP: policy.json BYOPKI signature verification API~~ policy.json BYOPKI signature verification API Oct 3, 2024

QiWang19 marked this pull request as ready for review October 3, 2024 02:12

mtrmac reviewed Oct 3, 2024

View reviewed changes

signature/pki_cert.go Outdated Show resolved Hide resolved

signature/pki_cert.go Outdated Show resolved Hide resolved

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the byo-pki-verify branch from f8585cd to f70f17a Compare October 8, 2024 18:32

mtrmac reviewed Nov 4, 2024

View reviewed changes

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

signature/pki_cert.go Outdated Show resolved Hide resolved

signature/pki_cert.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the byo-pki-verify branch from f70f17a to 897b425 Compare November 5, 2024 01:59

saschagrunert approved these changes Nov 5, 2024

View reviewed changes

docs/containers-policy.json.5.md Outdated Show resolved Hide resolved

docs/containers-policy.json.5.md Outdated Show resolved Hide resolved

mtrmac reviewed Nov 5, 2024

View reviewed changes

docs/containers-policy.json.5.md Outdated Show resolved Hide resolved

QiWang19 force-pushed the byo-pki-verify branch from 897b425 to f020a87 Compare November 6, 2024 21:54

QiWang19 mentioned this pull request Nov 20, 2024

OCPNODE-2339: Add PKI field to (cluster)imagepolicy openshift/api#2088

Open

QiWang19 force-pushed the byo-pki-verify branch from f020a87 to c9ea5b9 Compare January 8, 2025 16:35

policy.json BYOPKI signature verification API

5e5e968

Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the byo-pki-verify branch from c9ea5b9 to 5e5e968 Compare January 27, 2025 15:55

mtrmac reviewed Feb 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy.json BYOPKI signature verification API #2579

policy.json BYOPKI signature verification API #2579

QiWang19 commented Sep 20, 2024 •

edited

Loading

QiWang19 commented Sep 20, 2024

mtrmac left a comment

QiWang19 commented Oct 3, 2024

mtrmac left a comment

mtrmac commented Oct 3, 2024

QiWang19 commented Oct 9, 2024

mtrmac commented Oct 9, 2024

QiWang19 commented Oct 25, 2024

QiWang19 commented Oct 25, 2024

saschagrunert left a comment

saschagrunert commented Nov 19, 2024

QiWang19 commented Jan 7, 2025

QiWang19 commented Jan 8, 2025

dmitris commented Jan 27, 2025

dmitris commented Jan 27, 2025

mtrmac left a comment

mtrmac Feb 7, 2025

mtrmac Feb 7, 2025

mtrmac Feb 7, 2025

mtrmac Feb 7, 2025

mtrmac Feb 7, 2025

mtrmac Feb 8, 2025

mtrmac Feb 8, 2025

mtrmac Feb 8, 2025

mtrmac Feb 8, 2025

mtrmac Feb 8, 2025

mtrmac commented Feb 8, 2025

	Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing the public key of the intermediate CA.
	Only one of `caIntermediatesPath` and `caIntermediatesData` can be present, containing certificates of intermediate CAs.

	One of `subjectEmail` and `subjectHostname` must be specified, exactly specifying the expected identity provider, and the identity of the user obtaining the certificate.
	One of `subjectEmail` and `subjectHostname` must be specified, exactly specifying the expected identity of the user obtaining the certificate.

	/* A Sigstore-signed repository using a certificate generated by the Bring Your Own Public Key Infrastructure (BYOPKI).*/
	/* A Sigstore-signed repository using a certificate generated by a custom public-key infrastructure. */

policy.json BYOPKI signature verification API #2579

Are you sure you want to change the base?

policy.json BYOPKI signature verification API #2579

Conversation

QiWang19 commented Sep 20, 2024 • edited Loading

QiWang19 commented Sep 20, 2024

mtrmac left a comment

Choose a reason for hiding this comment

QiWang19 commented Oct 3, 2024

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Oct 3, 2024

QiWang19 commented Oct 9, 2024

mtrmac commented Oct 9, 2024

QiWang19 commented Oct 25, 2024

QiWang19 commented Oct 25, 2024

saschagrunert left a comment

Choose a reason for hiding this comment

saschagrunert commented Nov 19, 2024

QiWang19 commented Jan 7, 2025

QiWang19 commented Jan 8, 2025

dmitris commented Jan 27, 2025

dmitris commented Jan 27, 2025

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Feb 8, 2025

QiWang19 commented Sep 20, 2024 •

edited

Loading