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.

mtrmac · 2024-09-20T20:34:31Z

signature/policy_types.go

@@ -167,6 +170,22 @@ type prSigstoreSignedFulcio struct {
 	SubjectEmail string `json:"subjectEmail,omitempty"`
 }

+// prSigstoreSignedPKI contains non-fulcio certificate PKI configuration options for prSigstoreSigned
+type prSigstoreSignedPKI struct {


There should be some public type callers can use to refer to the value. Following the other examples here, probably an interface with a private method, and public functions to construct it (NewPRSigstoreSignedPKI etc.) using the “functional option” pattern.

mtrmac · 2024-09-20T20:35:42Z

signature/policy_types.go

+	CAIntermediatesPath string `json:"caIntermediatesPath"`
+	// CAIntermediatesData contains accepted CA intermediate certificates in PEM format, all of that base64-encoded. Only one of CAIntermediatesPath or CAIntermediatesData can be specified, not both.
+	CAIntermediatesData []byte `json:"caIntermediatesData"`
+	// SubjectEmail specifies the expected email address imposed on the subject to which the certificate was issued. Exactly one of SubjectEmail and Hostname must be specified.


(Nit: A blank line above would separate the two kinds of fields.)

mtrmac · 2024-09-20T20:36:08Z

signature/policy_types.go

+	// SubjectEmail specifies the expected email address imposed on the subject to which the certificate was issued. Exactly one of SubjectEmail and Hostname must be specified.
+	SubjectEmail string `json:"subjectEmail"`
+	// Hostname specifies the expected hostname imposed on the subject to which the certificate was issued. Exactly one of SubjectEmail and Hostname must be specified.
+	Hostname string `json:"hostname"`


Suggested change

Hostname string `json:"hostname"`

SubjectHostname string `json:"subjectHostname"`

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

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.

mtrmac · 2024-10-03T17:06:43Z

signature/pki_cert.go

+	untrustedIntermediatePool := x509.NewCertPool()
+	if len(untrustedIntermediateChainBytes) > 0 {
+		untrustedIntermediateChain, err := cryptoutils.UnmarshalCertificatesFromPEM(untrustedIntermediateChainBytes)
+		if err != nil {
+			return nil, internal.NewInvalidSignatureError(fmt.Sprintf("loading certificate chain: %v", err))
+		}
+		if len(untrustedIntermediateChain) > 1 {
+			for _, untrustedIntermediateCert := range untrustedIntermediateChain {
+				if isIntermediateCA(untrustedIntermediateCert) {
+					untrustedIntermediatePool.AddCert(untrustedIntermediateCert)
+				}
+			}
+		}
+	}
+
+	if len(pkiTrustRoot.caIntermediateCertificates) > 0 {
+		trustedIntermediates, err := cryptoutils.UnmarshalCertificatesFromPEM(pkiTrustRoot.caIntermediateCertificates)
+		if err != nil {
+			return nil, internal.NewInvalidSignatureError(fmt.Sprintf("loading trusted intermediate certificates: %v", err))
+		}
+		for _, trustedIntermediateCert := range trustedIntermediates {
+			untrustedIntermediatePool.AddCert(trustedIntermediateCert)
+		}
+	}


I think this can all happen in prSigstoreSignedPKI.prepareTrustRoot:

With incorrect configurations we don’t even start parsing the high-risk signature itself.

It’s a long-term goal to store the trust root data in PolicyContext instead of doing that work on every single signature verification. (That’s certainly not this PR, just that structuring the code to make that possible would be nice.)

mtrmac · 2024-10-03T17:08:27Z

signature/pki_cert.go

+}
+
+// isIntermediateCA checks if the certificate is an intermediate CA
+func isIntermediateCA(cert *x509.Certificate) bool {


It’s not immediately clear to me that this is necessary, I’d expect the crypto/x509 code to validate the intermediates are usable. Is there a specific reason to filter them this way? If so, it would be nice to document that.

mtrmac · 2024-10-03T17:10:40Z

signature/policy_eval_sigstore.go

 	case trustRoot.publicKeys != nil && trustRoot.fulcio != nil: // newPRSigstoreSigned rejects such combinations.
 		return sarRejected, errors.New("Internal inconsistency: Both a public key and Fulcio CA specified")
-	case trustRoot.publicKeys == nil && trustRoot.fulcio == nil: // newPRSigstoreSigned rejects such combinations.
-		return sarRejected, errors.New("Internal inconsistency: Neither a public key nor a Fulcio CA specified")
+	case trustRoot.publicKeys != nil && trustRoot.pki != nil: // newPRSigstoreSigned rejects such combinations.
+		return sarRejected, errors.New("Internal inconsistency: Both a public key and PKI specified")
+	case trustRoot.fulcio != nil && trustRoot.pki != nil: // newPRSigstoreSigned rejects such combinations.
+		return sarRejected, errors.New("Internal inconsistency: Both Fulcio CA and PKI specified")
+	case trustRoot.publicKeys == nil && trustRoot.fulcio == nil && trustRoot.pki == nil: // newPRSigstoreSigned rejects such combinations.
+		return sarRejected, errors.New("Internal inconsistency: A public key, Fulcio, or PKI must be specified.")


This O(N^2) item list is getting large — maybe switching this to check counting keySources == 1 would be easier to follow.

mtrmac · 2024-10-03T17:12:08Z

signature/policy_eval_sigstore.go

+		return nil, err
+	}
+	if caIntermediatesCertPEMs != nil {
+		pki.caIntermediateCertificates = caIntermediatesCertPEMs[0]


A sanity check that len() == 1 would be nice here.

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

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 from d558162 to b3a0606 Compare October 2, 2024 23:07

policy.json BYOPKI signature verification API

f8585cd

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

QiWang19 force-pushed the byo-pki-verify branch 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

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

mtrmac Sep 20, 2024

mtrmac Sep 20, 2024

mtrmac Sep 20, 2024

QiWang19 commented Oct 3, 2024

mtrmac left a comment

mtrmac Oct 3, 2024

mtrmac Oct 3, 2024

mtrmac Oct 3, 2024 •

edited

Loading

mtrmac Oct 3, 2024

mtrmac commented Oct 3, 2024

	Hostname string `json:"hostname"`
	SubjectHostname string `json:"subjectHostname"`

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

mtrmac Sep 20, 2024

Choose a reason for hiding this comment

mtrmac Sep 20, 2024

Choose a reason for hiding this comment

mtrmac Sep 20, 2024

Choose a reason for hiding this comment

QiWang19 commented Oct 3, 2024

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Oct 3, 2024

Choose a reason for hiding this comment

mtrmac Oct 3, 2024

Choose a reason for hiding this comment

mtrmac Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

mtrmac Oct 3, 2024

Choose a reason for hiding this comment

mtrmac commented Oct 3, 2024

QiWang19 commented Sep 20, 2024 •

edited

Loading

mtrmac Oct 3, 2024 •

edited

Loading