Skip to content

Commit

Permalink
feat: refactor error messages for cosign verification
Browse files Browse the repository at this point in the history
  • Loading branch information
binbin-li committed Aug 28, 2024
1 parent aaba189 commit 360d80b
Show file tree
Hide file tree
Showing 7 changed files with 262 additions and 74 deletions.
4 changes: 2 additions & 2 deletions pkg/referrerstore/oras/cosign.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func getCosignReferences(ctx context.Context, subjectReference common.Reference,
return nil, nil
}
evictOnError(ctx, err, subjectReference.Original)
return nil, re.ErrorCodeRepositoryOperationFailure.WithError(err).WithComponentType(re.ReferrerStore)
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to resolve Cosign signature reference to descriptor").WithError(err)
}

references = append(references, ocispecs.ReferenceDescriptor{
Expand All @@ -64,7 +64,7 @@ func getCosignReferences(ctx context.Context, subjectReference common.Reference,
func attachedImageTag(subjectReference common.Reference, tagSuffix string) (string, error) {
// sha256:d34db33f -> sha256-d34db33f.suffix
if subjectReference.Digest.String() == "" {
return "", re.ErrorCodeReferenceInvalid.WithComponentType(re.ReferrerStore).WithDetail("Cosign subject digest is empty")
return "", re.ErrorCodeReferenceInvalid.WithDetail("Cosign subject digest is empty").WithRemediation("Ensure the Cosign signature was attached correctly.")
}
tagStr := strings.ReplaceAll(subjectReference.Digest.String(), ":", "-") + tagSuffix
return fmt.Sprintf("%s:%s", subjectReference.Path, tagStr), nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/referrerstore/oras/oras.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func (store *orasStore) GetBlobContent(ctx context.Context, subjectReference com
var err error
repository, err := store.createRepository(ctx, store, subjectReference)
if err != nil {
return nil, err
return nil, re.ErrorCodeGetBlobContentFailure.WithDetail("Failed to create client to remote registry").WithError(err)

Check warning on line 261 in pkg/referrerstore/oras/oras.go

View check run for this annotation

Codecov / codecov/patch

pkg/referrerstore/oras/oras.go#L261

Added line #L261 was not covered by tests
}

// create a dummy Descriptor to check the local store cache
Expand Down
38 changes: 19 additions & 19 deletions pkg/verifier/cosign/cosign.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,17 @@ func (f *cosignVerifierFactory) Create(_ string, verifierConfig config.VerifierC
logger.GetLogger(context.Background(), logOpt).Debugf("creating cosign verifier with config %v, namespace '%v'", verifierConfig, namespace)
verifierName, hasName := verifierConfig[types.Name].(string)
if !hasName {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName).WithDetail("missing name in verifier config")
return nil, re.ErrorCodeConfigInvalid.WithDetail("Missing name in Cosign verifier config")
}

config, err := parseVerifierConfig(verifierConfig)
if err != nil {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName)
return nil, re.ErrorCodeConfigInvalid.WithDetail("Failed to create Cosign verifier").WithError(err)
}

// if key or rekorURL is provided, trustPolicies should not be provided
if (config.KeyRef != "" || config.RekorURL != "") && len(config.TrustPolicies) > 0 {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName).WithDetail("'key' and 'rekorURL' are part of cosign legacy configuration and cannot be used with `trustPolicies`")
return nil, re.ErrorCodeConfigInvalid.WithDetail("'key' and 'rekorURL' are part of cosign legacy configuration and cannot be used with `trustPolicies`")
}

var trustPolicies *TrustPolicies
Expand All @@ -163,7 +163,7 @@ func (f *cosignVerifierFactory) Create(_ string, verifierConfig config.VerifierC
logger.GetLogger(context.Background(), logOpt).Debugf("legacy cosign verifier configuration not found, creating trust policies")
trustPolicies, err = CreateTrustPolicies(config.TrustPolicies, verifierName)
if err != nil {
return nil, err
return nil, re.ErrorCodePluginInitFailure.WithDetail("Failed to create Cosign verifier").WithError(err)
}
legacy = false
}
Expand Down Expand Up @@ -224,18 +224,18 @@ func (v *cosignVerifier) verifyInternal(ctx context.Context, subjectReference co
// get the reference manifest (cosign oci image)
referenceManifest, err := referrerStore.GetReferenceManifest(ctx, subjectReference, referenceDescriptor)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to get reference manifest: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to get reference manifest").WithError(err)), nil
}

// manifest must be an OCI Image
if referenceManifest.MediaType != imgspec.MediaTypeImageManifest {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("reference manifest is not an image")), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Reference manifest is not an oci image")), nil
}

// get the subject image descriptor
subjectDesc, err := referrerStore.GetSubjectDescriptor(ctx, subjectReference)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to create subject hash: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeGetSubjectDescriptorFailure.WithDetail(fmt.Sprintf("Failed to get the descriptor for subject: %s", subjectReference.Digest)).WithError(err)), nil
}

// create the hash of the subject image descriptor (used as the hashed payload)
Expand All @@ -255,23 +255,23 @@ func (v *cosignVerifier) verifyInternal(ctx context.Context, subjectReference co
// fetch the blob content of the signature from the referrer store
blobBytes, err := referrerStore.GetBlobContent(ctx, subjectReference, blob.Digest)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to get blob content: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeGetBlobContentFailure.WithDetail(fmt.Sprintf("Failed to get blob content for %s", blob.Digest)).WithError(err)), nil
}
// convert the blob to a static signature
staticOpts, err := staticLayerOpts(blob)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to parse static signature opts: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to parse static signature options").WithError(err)), nil
}
sig, err := static.NewSignature(blobBytes, blob.Annotations[static.SignatureAnnotationKey], staticOpts...)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to generate static signature: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to generate static signature").WithError(err)), nil

Check warning on line 267 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L267

Added line #L267 was not covered by tests
}
if len(keysMap) > 0 {
// if keys are found, perform verification with keys
var verifications []cosignExtension
verifications, hasValidSignature, err = verifyWithKeys(ctx, keysMap, sig, blob.Annotations[static.SignatureAnnotationKey], blobBytes, staticOpts, &cosignOpts, subjectDescHash)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to verify with keys: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to verify Cosign signature with keys").WithError(err)), nil
}
extensionListEntry.Verifications = append(extensionListEntry.Verifications, verifications...)
} else {
Expand All @@ -295,7 +295,7 @@ func (v *cosignVerifier) verifyInternal(ctx context.Context, subjectReference co
), nil
}

errorResult := errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("no valid signatures found"))
errorResult := errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("No valid Cosign signatures found"))

Check warning on line 298 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L298

Added line #L298 was not covered by tests
errorResult.Extensions = Extension{SignatureExtension: sigExtensions, TrustPolicy: trustPolicy.GetName()}
return errorResult, nil
}
Expand Down Expand Up @@ -485,7 +485,7 @@ func staticLayerOpts(desc imgspec.Descriptor) ([]static.Option, error) {

// ErrorToVerifyResult returns a verifier result with the error message and isSuccess set to false
func errorToVerifyResult(name string, verifierType string, err error) verifier.VerifierResult {
verifierErr := re.ErrorCodeVerifyReferenceFailure.WithDetail("Verification failed").WithError(err)
verifierErr := re.ErrorCodeVerifyReferenceFailure.WithDetail("Cosign signature verification failed").WithError(err)
return verifier.NewVerifierResult(
"",
name,
Expand Down Expand Up @@ -540,14 +540,14 @@ func verifyWithKeys(ctx context.Context, keysMap map[PKKey]keymanagementprovider
if pubKey.ProviderType == azurekeyvault.ProviderName {
hashType, sig, err = processAKVSignature(sigEncoded, sig, pubKey.Key, payload, staticOpts)
if err != nil {
return verifications, false, fmt.Errorf("failed to process AKV signature: %w", err)
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to process AKV signature").WithError(err)
}
}

// return the correct verifier based on public key type and bytes
verifier, err := signature.LoadVerifier(pubKey.Key, hashType)
if err != nil {
return verifications, false, fmt.Errorf("failed to load public key from provider [%s] name [%s] version [%s]: %w", mapKey.Provider, mapKey.Name, mapKey.Version, err)
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("failed to load public key from provider [%s] name [%s] version [%s]", mapKey.Provider, mapKey.Name, mapKey.Version)).WithError(err)

Check warning on line 550 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L550

Added line #L550 was not covered by tests
}
cosignOpts.SigVerifier = verifier
// verify signature with cosign options + perform bundle verification
Expand Down Expand Up @@ -627,17 +627,17 @@ func processAKVSignature(sigEncoded string, staticSig oci.Signature, publicKey c
// EC verifiers in cosign have built in ASN.1 decoding, but RSA verifiers do not
base64DecodedBytes, err := base64.StdEncoding.DecodeString(sigEncoded)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to decode base64 signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to decode base64 signature").WithError(err)
}
// decode ASN.1 signature to raw signature if it is ASN.1 encoded
decodedSigBytes, err := decodeASN1Signature(base64DecodedBytes)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to decode ASN.1 signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to decode ASN.1 signature").WithError(err)

Check warning on line 635 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L635

Added line #L635 was not covered by tests
}
encodedBase64SigBytes := base64.StdEncoding.EncodeToString(decodedSigBytes)
staticSig, err = static.NewSignature(payloadBytes, encodedBase64SigBytes, staticOpts...)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to generate static signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to generate static signature").WithError(err)

Check warning on line 640 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L640

Added line #L640 was not covered by tests
}
case *ecdsa.PublicKey:
switch keyType.Curve {
Expand All @@ -651,7 +651,7 @@ func processAKVSignature(sigEncoded string, staticSig oci.Signature, publicKey c
return crypto.SHA256, nil, fmt.Errorf("ECDSA key check: unsupported key curve: %s", keyType.Params().Name)
}
default:
return crypto.SHA256, nil, fmt.Errorf("unsupported public key type: %T", publicKey)
return crypto.SHA256, nil, fmt.Errorf("Unsupported public key type: %T", publicKey)
}
return hashType, staticSig, nil
}
Expand Down
138 changes: 119 additions & 19 deletions pkg/verifier/cosign/cosign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"crypto/elliptic"
"crypto/rand"
"crypto/rsa"
"encoding/base64"
"fmt"
"io"
"slices"
Expand Down Expand Up @@ -114,6 +115,26 @@ func TestCreate(t *testing.T) {
},
wantErr: false,
},
{
name: "duplicate trust policies in config",
config: config.VerifierConfig{
"name": "test",
"artifactTypes": "testtype",
"trustPolicies": []TrustPolicyConfig{
{
Name: "test",
Keyless: KeylessConfig{CertificateIdentity: testIdentity, CertificateOIDCIssuer: testIssuer},
Scopes: []string{"*"},
},
{
Name: "test",
Keyless: KeylessConfig{CertificateIdentity: testIdentity, CertificateOIDCIssuer: testIssuer},
Scopes: []string{"*"},
},
},
},
wantErr: true,
},
{
name: "invalid config with legacy and trust policies",
config: config.VerifierConfig{
Expand Down Expand Up @@ -407,8 +428,8 @@ func TestErrorToVerifyResult(t *testing.T) {
if verifierResult.Type != "cosign" {
t.Errorf("errorToVerifyResult() = %v, want %v", verifierResult.Type, "cosign")
}
if verifierResult.Message != "Verification failed" {
t.Errorf("errorToVerifyResult() = %v, want %v", verifierResult.Message, "Verification failed")
if verifierResult.Message != "Cosign signature verification failed" {
t.Errorf("errorToVerifyResult() = %v, want %v", verifierResult.Message, "Cosign signature verification failed")
}
if verifierResult.ErrorReason != "test error" {
t.Errorf("errorToVerifyResult() = %v, want %v", verifierResult.ErrorReason, "test error")
Expand Down Expand Up @@ -573,16 +594,16 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
keys: map[PKKey]keymanagementprovider.PublicKey{},
getKeysError: true,
store: &mocks.MemoryTestStore{},
expectedResultMessagePrefix: "Verification failed",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "error",
},
{
name: "manifest fetch error",
keys: map[PKKey]keymanagementprovider.PublicKey{},
getKeysError: false,
store: &mocks.MemoryTestStore{},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to get reference manifest: manifest not found",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "manifest not found",
},
{
name: "incorrect reference manifest media type error",
Expand All @@ -595,8 +616,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
},
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "reference manifest is not an image",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "Reference manifest is not an oci image",
},
{
name: "failed subject descriptor fetch",
Expand All @@ -609,8 +630,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
},
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to create subject hash: subject not found for sha256:5678",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "subject not found for sha256:5678",
},
{
name: "failed to fetch blob",
Expand All @@ -636,8 +657,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
},
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to get blob content: blob not found",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "blob not found",
},
{
name: "invalid key type for AKV",
Expand Down Expand Up @@ -668,8 +689,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
blobDigest: validSignatureBlob,
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to verify with keys: failed to process AKV signature: unsupported public key type: *ecdh.PublicKey",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "Unsupported public key type: *ecdh.PublicKey",
},
{
name: "invalid RSA key size for AKV",
Expand Down Expand Up @@ -700,8 +721,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
blobDigest: validSignatureBlob,
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to verify with keys: failed to process AKV signature: RSA key check: unsupported key size: 128",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "RSA key check: unsupported key size: 128",
},
{
name: "invalid ECDSA curve type for AKV",
Expand Down Expand Up @@ -732,8 +753,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
blobDigest: validSignatureBlob,
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to verify with keys: failed to process AKV signature: ECDSA key check: unsupported key curve: P-224",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "ECDSA key check: unsupported key curve: P-224",
},
{
name: "valid hash: 256 keysize: 2048 RSA key AKV",
Expand Down Expand Up @@ -965,8 +986,8 @@ mmBwUAwwW0Uc+Nt3bDOCiB1nUsICv1ry
"sha256:d1226e36bc8502978324cb2cb2116c6aa48edb2ea8f15b1c6f6f256ed43388f6": []byte(`{"critical":{"identity":{"docker-reference":"wabbitnetworks.azurecr.io/test/cosign-image"},"image":{"docker-manifest-digest":"sha256:623621b56649b5e0c2c7cf3ffd987932f8f9a5a01036e00d6f3ae9480087621c"},"type":"cosign container image signature"},"optional":null}`),
},
},
expectedResultMessagePrefix: "Verification failed",
expectedErrorReason: "failed to parse static signature opts: failed to unmarshal bundle from blob payload: illegal base64 data at input byte 91",
expectedResultMessagePrefix: "Cosign signature verification failed",
expectedErrorReason: "failed to unmarshal bundle from blob payload: illegal base64 data at input byte 91",
},
}

Expand Down Expand Up @@ -1051,3 +1072,82 @@ func TestVerificationMessage(t *testing.T) {
})
}
}

func TestProcessAKVSignature_RSAKey(t *testing.T) {
tests := []struct {
name string
keySize int
encodedSig string
expectedErr bool
expectedHashType crypto.Hash
expectedSigOut bool
}{
{
name: "RSA 2048 bits",
keySize: 256,
expectedErr: false,
expectedHashType: crypto.SHA256,
expectedSigOut: true,
},
{
name: "RSA 3072 bits",
keySize: 384,
expectedErr: false,
expectedHashType: crypto.SHA384,
expectedSigOut: true,
},
{
name: "RSA 4096 bits",
keySize: 512,
expectedErr: false,
expectedHashType: crypto.SHA512,
expectedSigOut: true,
},
{
name: "Unsupported key size",
keySize: 128,
expectedErr: true,
},
{
name: "Invalid base64 encoded signature",
keySize: 256,
encodedSig: "ThisIsNot@ValidBase64%String!",
expectedErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create a mock RSA public key
privateKey, err := rsa.GenerateKey(rand.Reader, tt.keySize*8)
if err != nil {
t.Fatalf("Failed to generate RSA key: %v", err)
}
rsaPublicKey := &privateKey.PublicKey

// Mock the signature as base64 encoded string
sig := "dummy_signature"
encodedSig := base64.StdEncoding.EncodeToString([]byte(sig))
if tt.encodedSig != "" {
encodedSig = tt.encodedSig
}

// Process the signature
hashType, sigOut, err := processAKVSignature(encodedSig, nil, rsaPublicKey, []byte("test payload"), []static.Option{})

if tt.expectedErr {
if err == nil {
t.Fatalf("Expected error but got nil")
}
} else {
if hashType != tt.expectedHashType {
t.Fatalf("Expected hash type %v but got %v", tt.expectedHashType, hashType)
}
if tt.expectedSigOut != (sigOut != nil) {
t.Fatalf("Expected signature output to be %v but got %v", tt.expectedSigOut, sigOut)
}
}

})

Check failure on line 1151 in pkg/verifier/cosign/cosign_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary trailing newline (whitespace)
}
}
Loading

0 comments on commit 360d80b

Please sign in to comment.