Skip to content

Commit

Permalink
Merge pull request #631 from smallstep/mariano/mackms-tags
Browse files Browse the repository at this point in the history 
Allow to retry with no tags when loading privatekeys
  • Loading branch information
@maraino
maraino authored Nov 14, 2024
2 parents 3a8464f + d01bccd commit 74dea0b
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 5 deletions.
25 changes: 25 additions & 0 deletions kms/mackms/mackms.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -990,6 +1012,7 @@ func parseURI(rawuri string) (*keyAttributes, error) {
return &keyAttributes{
label: rawuri,
tag: DefaultTag,
retry: true,
}, nil
}

Expand All @@ -1006,6 +1029,7 @@ func parseURI(rawuri string) (*keyAttributes, error) {
return &keyAttributes{
label: k,
tag: DefaultTag,
retry: true,
}, nil
}
}
Expand All @@ -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
Expand Down
67 changes: 62 additions & 5 deletions kms/mackms/mackms_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
})
}
}

0 comments on commit 74dea0b

Please sign in to comment.