Skip to content

Commit

Permalink
UNFINISHED: Multiple Rekor public keys
Browse files Browse the repository at this point in the history 
Completely missing config handling and tests, just a vague sketch
of how loadBytesFromConfigSources helps.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
  • Loading branch information
@mtrmac
mtrmac committed Aug 17, 2024
1 parent 3b81ab4 commit fc34eb4
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 39 deletions.
4 changes: 2 additions & 2 deletions signature/fulcio_cert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,10 @@ func (f *fulcioTrustRoot) verifyFulcioCertificateAtTime(relevantTime time.Time,
return untrustedCertificate.PublicKey, nil
}

func verifyRekorFulcio(rekorPublicKey *ecdsa.PublicKey, fulcioTrustRoot *fulcioTrustRoot, untrustedRekorSET []byte,
func verifyRekorFulcio(rekorPublicKeys []*ecdsa.PublicKey, fulcioTrustRoot *fulcioTrustRoot, untrustedRekorSET []byte,
untrustedCertificateBytes []byte, untrustedIntermediateChainBytes []byte, untrustedBase64Signature string,
untrustedPayloadBytes []byte) (crypto.PublicKey, error) {
rekorSETTime, err := internal.VerifyRekorSET(rekorPublicKey, untrustedRekorSET, untrustedCertificateBytes,
rekorSETTime, err := internal.VerifyRekorSET(rekorPublicKeys, untrustedRekorSET, untrustedCertificateBytes,
untrustedBase64Signature, untrustedPayloadBytes)
if err != nil {
return nil, err
Expand Down
7 changes: 4 additions & 3 deletions signature/fulcio_cert_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ func TestVerifyRekorFulcio(t *testing.T) {
require.NoError(t, err)
rekorKeyECDSA, ok := rekorKey.(*ecdsa.PublicKey)
require.True(t, ok)
rekorKeysECDSA := []*ecdsa.PublicKey{rekorKeyECDSA}
setBytes, err := os.ReadFile("fixtures/rekor-set")
require.NoError(t, err)
sigBase64, err := os.ReadFile("fixtures/rekor-sig")
Expand All @@ -450,7 +451,7 @@ func TestVerifyRekorFulcio(t *testing.T) {
require.NoError(t, err)

// Success
pk, err := verifyRekorFulcio(rekorKeyECDSA, &fulcioTrustRoot{
pk, err := verifyRekorFulcio(rekorKeysECDSA, &fulcioTrustRoot{
caCertificates: caCertificates,
oidcIssuer: "https://github.com/login/oauth",
subjectEmail: "mitr@redhat.com",
Expand All @@ -459,7 +460,7 @@ func TestVerifyRekorFulcio(t *testing.T) {
assertPublicKeyMatchesCert(t, certBytes, pk)

// Rekor failure
pk, err = verifyRekorFulcio(rekorKeyECDSA, &fulcioTrustRoot{
pk, err = verifyRekorFulcio(rekorKeysECDSA, &fulcioTrustRoot{
caCertificates: caCertificates,
oidcIssuer: "https://github.com/login/oauth",
subjectEmail: "mitr@redhat.com",
Expand All @@ -468,7 +469,7 @@ func TestVerifyRekorFulcio(t *testing.T) {
assert.Nil(t, pk)

// Fulcio failure
pk, err = verifyRekorFulcio(rekorKeyECDSA, &fulcioTrustRoot{
pk, err = verifyRekorFulcio(rekorKeysECDSA, &fulcioTrustRoot{
caCertificates: caCertificates,
oidcIssuer: "https://github.com/login/oauth",
subjectEmail: "this-does-not-match@example.com",
Expand Down
10 changes: 8 additions & 2 deletions signature/internal/rekor_set.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (p UntrustedRekorPayload) MarshalJSON() ([]byte, error) {

// VerifyRekorSET verifies that unverifiedRekorSET is correctly signed by publicKey and matches the rest of the data.
// Returns bundle upload time on success.
func VerifyRekorSET(publicKey *ecdsa.PublicKey, unverifiedRekorSET []byte, unverifiedKeyOrCertBytes []byte, unverifiedBase64Signature string, unverifiedPayloadBytes []byte) (time.Time, error) {
func VerifyRekorSET(publicKeys []*ecdsa.PublicKey, unverifiedRekorSET []byte, unverifiedKeyOrCertBytes []byte, unverifiedBase64Signature string, unverifiedPayloadBytes []byte) (time.Time, error) {
// FIXME: Should the publicKey parameter hard-code ecdsa?

// == Parse SET bytes
Expand All @@ -130,7 +130,13 @@ func VerifyRekorSET(publicKey *ecdsa.PublicKey, unverifiedRekorSET []byte, unver
return time.Time{}, NewInvalidSignatureError(fmt.Sprintf("canonicalizing Rekor SET JSON: %v", err))
}
untrustedSETPayloadHash := sha256.Sum256(untrustedSETPayloadCanonicalBytes)
if !ecdsa.VerifyASN1(publicKey, untrustedSETPayloadHash[:], untrustedSET.UntrustedSignedEntryTimestamp) {
publicKeymatched := false
for _, pk := range publicKeys {
if ecdsa.VerifyASN1(pk, untrustedSETPayloadHash[:], untrustedSET.UntrustedSignedEntryTimestamp) {
publicKeymatched = true
}
}
if !publicKeymatched {
return time.Time{}, NewInvalidSignatureError("cryptographic signature verification of Rekor SET failed")
}

Expand Down
20 changes: 11 additions & 9 deletions signature/internal/rekor_set_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ func TestVerifyRekorSET(t *testing.T) {
require.NoError(t, err)
cosignRekorKeyECDSA, ok := cosignRekorKey.(*ecdsa.PublicKey)
require.True(t, ok)
cosignRekorKeysECDSA := []*ecdsa.PublicKey{cosignRekorKeyECDSA}
cosignSETBytes, err := os.ReadFile("testdata/rekor-set")
require.NoError(t, err)
cosignCertBytes, err := os.ReadFile("testdata/rekor-cert")
Expand All @@ -195,23 +196,24 @@ func TestVerifyRekorSET(t *testing.T) {
require.NoError(t, err)

// Successful verification
tm, err := VerifyRekorSET(cosignRekorKeyECDSA, cosignSETBytes, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err := VerifyRekorSET(cosignRekorKeysECDSA, cosignSETBytes, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
require.NoError(t, err)
assert.Equal(t, time.Unix(1670870899, 0), tm)

// For extra paranoia, test that we return a zero time on error.

// A completely invalid SET.
tm, err = VerifyRekorSET(cosignRekorKeyECDSA, []byte{}, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(cosignRekorKeysECDSA, []byte{}, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)

tm, err = VerifyRekorSET(cosignRekorKeyECDSA, []byte("invalid signature"), cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(cosignRekorKeysECDSA, []byte("invalid signature"), cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)

testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
require.NoError(t, err)
testPublicKeys := []*ecdsa.PublicKey{&testKey.PublicKey}
testSigner, err := sigstoreSignature.LoadECDSASigner(testKey, crypto.SHA256)
require.NoError(t, err)

Expand All @@ -227,12 +229,12 @@ func TestVerifyRekorSET(t *testing.T) {
UntrustedPayload: json.RawMessage(invalidPayload),
})
require.NoError(t, err)
tm, err = VerifyRekorSET(&testKey.PublicKey, invalidSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(testPublicKeys, invalidSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)

// Cryptographic verification fails (a mismatched public key)
tm, err = VerifyRekorSET(&testKey.PublicKey, cosignSETBytes, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(testPublicKeys, cosignSETBytes, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)

Expand All @@ -245,7 +247,7 @@ func TestVerifyRekorSET(t *testing.T) {
UntrustedPayload: json.RawMessage(invalidPayload),
})
require.NoError(t, err)
tm, err = VerifyRekorSET(&testKey.PublicKey, invalidSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(testPublicKeys, invalidSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)

Expand Down Expand Up @@ -379,15 +381,15 @@ func TestVerifyRekorSET(t *testing.T) {
UntrustedPayload: json.RawMessage(testPayload),
})
require.NoError(t, err)
tm, err = VerifyRekorSET(&testKey.PublicKey, testSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
tm, err = VerifyRekorSET(testPublicKeys, testSET, cosignCertBytes, string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)
}

// Invalid unverifiedBase64Signature parameter
truncatedBase64 := cosignSigBase64
truncatedBase64 = truncatedBase64[:len(truncatedBase64)-1]
tm, err = VerifyRekorSET(cosignRekorKeyECDSA, cosignSETBytes, cosignCertBytes,
tm, err = VerifyRekorSET(cosignRekorKeysECDSA, cosignSETBytes, cosignCertBytes,
string(truncatedBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)
Expand All @@ -399,7 +401,7 @@ func TestVerifyRekorSET(t *testing.T) {
[]byte("this is not PEM"),
bytes.Repeat(cosignCertBytes, 2),
} {
tm, err = VerifyRekorSET(cosignRekorKeyECDSA, cosignSETBytes, c,
tm, err = VerifyRekorSET(cosignRekorKeysECDSA, cosignSETBytes, c,
string(cosignSigBase64), cosignPayloadBytes)
assert.Error(t, err)
assert.Zero(t, tm)
Expand Down
44 changes: 24 additions & 20 deletions signature/policy_eval_sigstore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ func (f *prSigstoreSignedFulcio) prepareTrustRoot() (*fulcioTrustRoot, error) {

// sigstoreSignedTrustRoot contains an already parsed version of the prSigstoreSigned policy
type sigstoreSignedTrustRoot struct {
publicKeys []crypto.PublicKey
fulcio *fulcioTrustRoot
rekorPublicKey *ecdsa.PublicKey
publicKeys []crypto.PublicKey
fulcio *fulcioTrustRoot
rekorPublicKeys []*ecdsa.PublicKey
}

func (pr *prSigstoreSigned) prepareTrustRoot() (*sigstoreSignedTrustRoot, error) {
Expand All @@ -118,10 +118,10 @@ func (pr *prSigstoreSigned) prepareTrustRoot() (*sigstoreSignedTrustRoot, error)
return nil, err
}
if publicKeyPEMs != nil {
for _, keyData := range publicKeyPEMs {
for index, keyData := range publicKeyPEMs {
pk, err := cryptoutils.UnmarshalPEMToPublicKey(keyData)
if err != nil {
return nil, fmt.Errorf("parsing public key: %w", err)
return nil, fmt.Errorf("parsing public key %d: %w", index+1, err)
}
res.publicKeys = append(res.publicKeys, pk)
}
Expand All @@ -141,25 +141,29 @@ func (pr *prSigstoreSigned) prepareTrustRoot() (*sigstoreSignedTrustRoot, error)
rekorPublicKeyPEMs, err := loadBytesFromConfigSources(configBytesSources{
inconsistencyErrorMessage: `Internal inconsistency: both "rekorPublicKeyPath" and "rekorPublicKeyData" specified`,
path: pr.RekorPublicKeyPath,
paths: pr.RekorPublicKeyPaths,
data: pr.RekorPublicKeyData,
datas: pr.RekorPublicKeyDatas,

Check failure on line 146 in signature/policy_eval_sigstore.go

View workflow job for this annotation

GitHub Actions / Check for spelling errors 

datas ==> data
})
if err != nil {
return nil, err
}
if rekorPublicKeyPEMs != nil {
if len(rekorPublicKeyPEMs) != 1 { // Coverage: We only provide single-element sources to loadBytesFromConfigSources, and at most one is allowed.
return nil, errors.New(`Internal inconsistency: got more than one element in "rekorPublicKeyPath" and "rekorPublicKeyData"`)
}
pk, err := cryptoutils.UnmarshalPEMToPublicKey(rekorPublicKeyPEMs[0])
if err != nil {
return nil, fmt.Errorf("parsing Rekor public key: %w", err)
}
pkECDSA, ok := pk.(*ecdsa.PublicKey)
if !ok {
return nil, fmt.Errorf("Rekor public key is not using ECDSA")
for index, pem := range rekorPublicKeyPEMs {
pk, err := cryptoutils.UnmarshalPEMToPublicKey(pem)
if err != nil {
return nil, fmt.Errorf("parsing Rekor public key %d: %w", index+1, err)
}
pkECDSA, ok := pk.(*ecdsa.PublicKey)
if !ok {
return nil, fmt.Errorf("Rekor public key %d is not using ECDSA", index+1)

}
res.rekorPublicKeys = append(res.rekorPublicKeys, pkECDSA)
}
if len(res.rekorPublicKeys) == 0 {
return nil, errors.New("FIXME: UNIMPLEMENTED")
}
res.rekorPublicKey = pkECDSA
}

return &res, nil
Expand Down Expand Up @@ -193,7 +197,7 @@ func (pr *prSigstoreSigned) isSignatureAccepted(ctx context.Context, image priva
return sarRejected, errors.New("Internal inconsistency: Neither a public key nor a Fulcio CA specified")

case trustRoot.publicKeys != nil:
if trustRoot.rekorPublicKey != nil {
if trustRoot.rekorPublicKeys != nil {
untrustedSET, ok := untrustedAnnotations[signature.SigstoreSETAnnotationKey]
if !ok { // For user convenience; passing an empty []byte to VerifyRekorSet should work.
return sarRejected, fmt.Errorf("missing %s annotation", signature.SigstoreSETAnnotationKey)
Expand All @@ -210,7 +214,7 @@ func (pr *prSigstoreSigned) isSignatureAccepted(ctx context.Context, image priva
return sarRejected, fmt.Errorf("re-marshaling public key to PEM: %w", err)
}
// We don’t care about the Rekor timestamp, just about log presence.
_, err = internal.VerifyRekorSET(trustRoot.rekorPublicKey, []byte(untrustedSET), recreatedPublicKeyPEM, untrustedBase64Signature, untrustedPayload)
_, err = internal.VerifyRekorSET(trustRoot.rekorPublicKeys, []byte(untrustedSET), recreatedPublicKeyPEM, untrustedBase64Signature, untrustedPayload)
if err == nil {
publicKeys = append(publicKeys, candidatePublicKey)
break // The SET can only accept one public key entry, so if we found one, the rest either doesn’t match or is a duplicate
Expand All @@ -229,7 +233,7 @@ func (pr *prSigstoreSigned) isSignatureAccepted(ctx context.Context, image priva
}

case trustRoot.fulcio != nil:
if trustRoot.rekorPublicKey == nil { // newPRSigstoreSigned rejects such combinations.
if trustRoot.rekorPublicKeys == nil { // newPRSigstoreSigned rejects such combinations.
return sarRejected, errors.New("Internal inconsistency: Fulcio CA specified without a Rekor public key")
}
untrustedSET, ok := untrustedAnnotations[signature.SigstoreSETAnnotationKey]
Expand All @@ -244,7 +248,7 @@ func (pr *prSigstoreSigned) isSignatureAccepted(ctx context.Context, image priva
if untrustedIntermediateChain, ok := untrustedAnnotations[signature.SigstoreIntermediateCertificateChainAnnotationKey]; ok {
untrustedIntermediateChainBytes = []byte(untrustedIntermediateChain)
}
pk, err := verifyRekorFulcio(trustRoot.rekorPublicKey, trustRoot.fulcio,
pk, err := verifyRekorFulcio(trustRoot.rekorPublicKeys, trustRoot.fulcio,
[]byte(untrustedSET), []byte(untrustedCert), untrustedIntermediateChainBytes, untrustedBase64Signature, untrustedPayload)
if err != nil {
return sarRejected, err
Expand Down
6 changes: 3 additions & 3 deletions signature/policy_eval_sigstore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func TestPRSigstoreSignedPrepareTrustRoot(t *testing.T) {
assert.NotNil(t, res.publicKeys)
assert.Len(t, res.publicKeys, c.numKeys)
assert.Nil(t, res.fulcio)
assert.Nil(t, res.rekorPublicKey)
assert.Nil(t, res.rekorPublicKeys)
}
// Success with Fulcio
pr, err := newPRSigstoreSigned(
Expand All @@ -136,7 +136,7 @@ func TestPRSigstoreSignedPrepareTrustRoot(t *testing.T) {
require.NoError(t, err)
assert.Nil(t, res.publicKeys)
assert.NotNil(t, res.fulcio)
assert.NotNil(t, res.rekorPublicKey)
assert.Len(t, res.rekorPublicKeys, 1)
// Success with Rekor public key
for _, c := range [][]PRSigstoreSignedOption{
{
Expand Down Expand Up @@ -166,7 +166,7 @@ func TestPRSigstoreSignedPrepareTrustRoot(t *testing.T) {
require.NoError(t, err)
assert.NotNil(t, res.publicKeys)
assert.Nil(t, res.fulcio)
assert.NotNil(t, res.rekorPublicKey)
assert.Len(t, res.rekorPublicKeys, 1)
}

// Failure
Expand Down
4 changes: 4 additions & 0 deletions signature/policy_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,14 @@ type prSigstoreSigned struct {
// If Fulcio is used, one of RekorPublicKeyPath or RekorPublicKeyData must be specified as well; otherwise it is optional
// (and Rekor inclusion is not required if a Rekor public key is not specified).
RekorPublicKeyPath string `json:"rekorPublicKeyPath,omitempty"`
// FIXME: UNFINISHED
RekorPublicKeyPaths []string `json:"rekorPublicKeyPaths,omitempty"`
// RekorPublicKeyPath contain a base64-encoded public key of a Rekor server which must record acceptable signatures.
// If Fulcio is used, one of RekorPublicKeyPath or RekorPublicKeyData must be specified as well; otherwise it is optional
// (and Rekor inclusion is not required if a Rekor public key is not specified).
RekorPublicKeyData []byte `json:"rekorPublicKeyData,omitempty"`
// FIXME: UNFINISHED
RekorPublicKeyDatas [][]byte `json:"rekorPublicKeyDatas,omitempty"`

// SignedIdentity specifies what image identity the signature must be claiming about the image.
// Defaults to "matchRepoDigestOrExact" if not specified.
Expand Down

0 comments on commit fc34eb4

Please sign in to comment.