From 05f8a4b1d497c35b0c3ac6bb782431bca9ad70a2 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Nov 2024 16:16:45 -0800 Subject: [PATCH 1/2] Allow to retry with no tags when loading privatekeys This function allows to retry the retrieval of a key without tags if the original uri didn't have one. This is used only in GetPublicKey and CreateSigner. It is not used in SearchKeys as we will need to remove duplicated keys. --- kms/mackms/mackms.go | 27 ++++++++++++++++ kms/mackms/mackms_test.go | 67 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 022e9b84..22243d77 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -52,12 +52,30 @@ 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 { + fmt.Println("no retry") + return nil + } + fmt.Println("retry") + return &keyAttributes{ + label: k.label, + hash: k.hash, + retry: false, + } +} + type keySearchAttributes struct { label string tag string @@ -715,6 +733,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 +1014,7 @@ func parseURI(rawuri string) (*keyAttributes, error) { return &keyAttributes{ label: rawuri, tag: DefaultTag, + retry: true, }, nil } @@ -1006,6 +1031,7 @@ func parseURI(rawuri string) (*keyAttributes, error) { return &keyAttributes{ label: k, tag: DefaultTag, + retry: true, }, nil } } @@ -1025,6 +1051,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 diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index 9d3a23c6..e6c59a05 100644 --- a/kms/mackms/mackms_test.go +++ b/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()) + }) + } +} From d01bccd66e8a6790a440abbb57abb71959580959 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Nov 2024 18:52:23 -0800 Subject: [PATCH 2/2] Remove debug comments --- kms/mackms/mackms.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 22243d77..5d0e6ede 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -65,10 +65,8 @@ type keyAttributes struct { // would return the tag empty if it was set using the default value. func (k *keyAttributes) retryAttributes() *keyAttributes { if !k.retry { - fmt.Println("no retry") return nil } - fmt.Println("retry") return &keyAttributes{ label: k.label, hash: k.hash,