Merge pull request #631 from smallstep/mariano/mackms-tags

Allow to retry with no tags when loading privatekeys

Author: maraino

kms/mackms/mackms.go

@@ -52,12 +52,28 @@ type keyAttributes struct {
 	label            string
 	tag              string
 	hash             []byte
+	retry            bool
 	useSecureEnclave bool
 	useBiometrics    bool
 	sigAlgorithm     apiv1.SignatureAlgorithm
 	keySize          int
 }
 
+// retryAttributes returns the original URI attributes used to get a private
+// key, but only if they are different that the ones set. It will return nil, if
+// they are the same. The only attribute that can change is the tag. This method
+// would return the tag empty if it was set using the default value.
+func (k *keyAttributes) retryAttributes() *keyAttributes {
+	if !k.retry {
+		return nil
+	}
+	return &keyAttributes{
+		label: k.label,
+		hash:  k.hash,
+		retry: false,
+	}
+}
+
 type keySearchAttributes struct {
 	label            string
 	tag              string
@@ -715,6 +731,12 @@ func getPrivateKey(u *keyAttributes) (*security.SecKeyRef, error) {
 
 	var key cf.TypeRef
 	if err := security.SecItemCopyMatching(query, &key); err != nil {
+		// If not found retry without the tag if it wasn't set.
+		if errors.Is(err, security.ErrNotFound) {
+			if ru := u.retryAttributes(); ru != nil {
+				return getPrivateKey(ru)
+			}
+		}
 		return nil, fmt.Errorf("macOS SecItemCopyMatching failed: %w", err)
 	}
 	return security.NewSecKeyRef(key), nil
@@ -990,6 +1012,7 @@ func parseURI(rawuri string) (*keyAttributes, error) {
 		return &keyAttributes{
 			label: rawuri,
 			tag:   DefaultTag,
+			retry: true,
 		}, nil
 	}
 
@@ -1006,6 +1029,7 @@ func parseURI(rawuri string) (*keyAttributes, error) {
 				return &keyAttributes{
 					label: k,
 					tag:   DefaultTag,
+					retry: true,
 				}, nil
 			}
 		}
@@ -1025,6 +1049,7 @@ func parseURI(rawuri string) (*keyAttributes, error) {
 		label:            label,
 		tag:              tag,
 		hash:             u.GetEncoded("hash"),
+		retry:            !u.Has("tag"),
 		useSecureEnclave: u.GetBool("se"),
 		useBiometrics:    u.GetBool("bio"),
 	}, nil

kms/mackms/mackms_test.go

@@ -307,7 +307,7 @@ func TestMacKMS_GetPublicKey(t *testing.T) {
 
 	// Create private keys only
 	r2 := createPrivateKeyOnly(t, "mackms:label=test-ecdsa", apiv1.ECDSAWithSHA256)
-	r3 := createPrivateKeyOnly(t, "mackms:label=test-rsa", apiv1.SHA256WithRSA)
+	r3 := createPrivateKeyOnly(t, "mackms:label=test-rsa;tag=", apiv1.SHA256WithRSA)
 
 	t.Cleanup(func() {
 		assert.NoError(t, kms.DeleteKey(&apiv1.DeleteKeyRequest{
@@ -334,7 +334,8 @@ func TestMacKMS_GetPublicKey(t *testing.T) {
 		{"ok", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: r1.Name}}, r1.PublicKey, assert.NoError},
 		{"ok no tag", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-p256;tag="}}, r1.PublicKey, assert.NoError},
 		{"ok private only ECDSA ", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-ecdsa"}}, r2.PublicKey, assert.NoError},
-		{"ok private only RSA ", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: r3.Name}}, r3.PublicKey, assert.NoError},
+		{"ok private only RSA", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: r3.Name}}, r3.PublicKey, assert.NoError},
+		{"ok private only RSA with retry", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-rsa"}}, r3.PublicKey, assert.NoError},
 		{"ok no uri", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "test-p256"}}, r1.PublicKey, assert.NoError},
 		{"ok uri simple", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:test-p256"}}, r1.PublicKey, assert.NoError},
 		{"ok uri label", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-p256"}}, r1.PublicKey, assert.NoError},
@@ -541,11 +542,12 @@ func Test_parseURI(t *testing.T) {
 		assertion assert.ErrorAssertionFunc
 	}{
 		{"ok", args{"mackms:label=the-label;tag=the-tag;hash=0102abcd"}, &keyAttributes{label: "the-label", tag: "the-tag", hash: []byte{1, 2, 171, 205}}, assert.NoError},
-		{"ok label", args{"the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag}, assert.NoError},
-		{"ok label uri", args{"mackms:label=the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag}, assert.NoError},
+		{"ok label", args{"the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
+		{"ok label uri", args{"mackms:label=the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
+		{"ok label uri simple", args{"mackms:the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
 		{"ok label empty tag", args{"mackms:label=the-label;tag="}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
 		{"ok label empty tag no equal", args{"mackms:label=the-label;tag"}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
-		{"fail parse", args{"mackms:::label=the-label"}, nil, assert.Error},
+		{"fail parse", args{"mackms:%label=the-label"}, nil, assert.Error},
 		{"fail missing label", args{"mackms:hash=0102abcd"}, nil, assert.Error},
 	}
 	for _, tt := range tests {
@@ -1306,3 +1308,58 @@ func TestMacKMS_SearchKeys(t *testing.T) {
 
 	assert.Equal(t, expectedHashes, hashes)
 }
+
+func Test_keyAttributes_retryAttributes(t *testing.T) {
+	type fields struct {
+		label string
+		tag   string
+		hash  []byte
+		retry bool
+	}
+
+	mustFields := func(s string) fields {
+		t.Helper()
+		u, err := parseURI(s)
+		require.NoError(t, err)
+		return fields{
+			label: u.label,
+			tag:   u.tag,
+			hash:  u.hash,
+			retry: u.retry,
+		}
+	}
+
+	tests := []struct {
+		name   string
+		fields fields
+		want   *keyAttributes
+	}{
+		{"with tag", mustFields("mackms:label=label;tag=tag"), nil},
+		{"with tag and hash", mustFields("mackms:label=label;hash=FF00;tag=tag"), nil},
+		{"with empty tag", mustFields("mackms:label=label;tag="), nil},
+		{"with no tag", mustFields("mackms:label=label;hash=FF00"), &keyAttributes{
+			label: "label",
+			hash:  []byte{0xFF, 0x00},
+		}},
+		{"legacy name only", mustFields("label"), &keyAttributes{
+			label: "label",
+		}},
+		{"legacy with schema", mustFields("mackms:label"), &keyAttributes{
+			label: "label",
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			k := &keyAttributes{
+				label: tt.fields.label,
+				tag:   tt.fields.tag,
+				hash:  tt.fields.hash,
+				retry: tt.fields.retry,
+			}
+			if tt.name == "with no tag" {
+				t.Log("foo")
+			}
+			assert.Equal(t, tt.want, k.retryAttributes())
+		})
+	}
+}