From 1e71f99b15df71ec542c15414438d445450b941c Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 25 Sep 2024 16:54:51 -0700 Subject: [PATCH] Add support for using mackms keys with no tags This commit allows to create, get, sign, and other operations using keys without a tag. By default, the default tag used by the mackms package is com.smallstep.crypto, this tag can be changed using the tag parameter in the uri, e.g., mackms:label=test;tag=my-tag. But if we want to not use a tag we need to provide the tag parameter empty, e,g., mackms:label=test;tag= Fixes #595 --- kms/mackms/mackms.go | 67 ++++++++++++++++++++------------------- kms/mackms/mackms_test.go | 21 ++++++++++++ kms/uri/uri.go | 5 +++ kms/uri/uri_test.go | 36 +++++++++++++++++++++ 4 files changed, 97 insertions(+), 32 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 7688b966..11050f05 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -189,12 +189,6 @@ func (k *MacKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons } // Define key attributes - cfTag, err := cf.NewData([]byte(u.tag)) - if err != nil { - return nil, fmt.Errorf("mackms CreateKey failed: %w", err) - } - defer cfTag.Release() - cfLabel, err := cf.NewString(u.label) if err != nil { return nil, fmt.Errorf("mackms CreateKey failed: %w", err) @@ -202,8 +196,15 @@ func (k *MacKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons defer cfLabel.Release() keyAttributesDict := cf.Dictionary{ - security.KSecAttrApplicationTag: cfTag, - security.KSecAttrIsPermanent: cf.True, + security.KSecAttrIsPermanent: cf.True, + } + if u.tag != "" { + cfTag, err := cf.NewData([]byte(u.tag)) + if err != nil { + return nil, fmt.Errorf("mackms CreateKey failed: %w", err) + } + defer cfTag.Release() + keyAttributesDict[security.KSecAttrApplicationTag] = cfTag } if u.useSecureEnclave { // After the first unlock, the data remains accessible until the next @@ -491,12 +492,6 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error { return fmt.Errorf("mackms DeleteKey failed: %w", err) } - cfTag, err := cf.NewData([]byte(u.tag)) - if err != nil { - return fmt.Errorf("mackms DeleteKey failed: %w", err) - } - defer cfTag.Release() - cfLabel, err := cf.NewString(u.label) if err != nil { return fmt.Errorf("mackms DeleteKey failed: %w", err) @@ -505,10 +500,17 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error { for _, keyClass := range []cf.TypeRef{security.KSecAttrKeyClassPublic, security.KSecAttrKeyClassPrivate} { dict := cf.Dictionary{ - security.KSecClass: security.KSecClassKey, - security.KSecAttrApplicationTag: cfTag, - security.KSecAttrLabel: cfLabel, - security.KSecAttrKeyClass: keyClass, + security.KSecClass: security.KSecClassKey, + security.KSecAttrLabel: cfLabel, + security.KSecAttrKeyClass: keyClass, + } + if u.tag != "" { + cfTag, err := cf.NewData([]byte(u.tag)) + if err != nil { + return fmt.Errorf("mackms DeleteKey failed: %w", err) + } + defer cfTag.Release() + dict[security.KSecAttrApplicationTag] = cfTag } // Extract logic to deleteItem to avoid defer on loops if err := deleteItem(dict, u.hash); err != nil { @@ -672,12 +674,6 @@ func deleteItem(dict cf.Dictionary, hash []byte) error { } func getPrivateKey(u *keyAttributes) (*security.SecKeyRef, error) { - cfTag, err := cf.NewData([]byte(u.tag)) - if err != nil { - return nil, err - } - defer cfTag.Release() - cfLabel, err := cf.NewString(u.label) if err != nil { return nil, err @@ -685,12 +681,19 @@ func getPrivateKey(u *keyAttributes) (*security.SecKeyRef, error) { defer cfLabel.Release() dict := cf.Dictionary{ - security.KSecClass: security.KSecClassKey, - security.KSecAttrApplicationTag: cfTag, - security.KSecAttrLabel: cfLabel, - security.KSecAttrKeyClass: security.KSecAttrKeyClassPrivate, - security.KSecReturnRef: cf.True, - security.KSecMatchLimit: security.KSecMatchLimitOne, + security.KSecClass: security.KSecClassKey, + security.KSecAttrLabel: cfLabel, + security.KSecAttrKeyClass: security.KSecAttrKeyClassPrivate, + security.KSecReturnRef: cf.True, + security.KSecMatchLimit: security.KSecMatchLimitOne, + } + if u.tag != "" { + cfTag, err := cf.NewData([]byte(u.tag)) + if err != nil { + return nil, err + } + defer cfTag.Release() + dict[security.KSecAttrApplicationTag] = cfTag } if len(u.hash) > 0 { d, err := cf.NewData(u.hash) @@ -1013,7 +1016,7 @@ func parseURI(rawuri string) (*keyAttributes, error) { return nil, fmt.Errorf("error parsing %q: label is required", rawuri) } tag := u.Get("tag") - if tag == "" { + if tag == "" && !u.Has("tag") { tag = DefaultTag } return &keyAttributes{ @@ -1100,7 +1103,7 @@ func parseSearchURI(rawuri string) (*keySearchAttributes, error) { // mackms:label=my-key;tag=my-tag;hash=010a...;se=true;bio=true label := u.Get("label") // when searching, the label can be empty tag := u.Get("tag") - if tag == "" { + if tag == "" && !u.Has("tag") { tag = DefaultTag } return &keySearchAttributes{ diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index 6bf27402..9d3a23c6 100644 --- a/kms/mackms/mackms_test.go +++ b/kms/mackms/mackms_test.go @@ -332,6 +332,7 @@ func TestMacKMS_GetPublicKey(t *testing.T) { assertion assert.ErrorAssertionFunc }{ {"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 no uri", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "test-p256"}}, r1.PublicKey, assert.NoError}, @@ -357,6 +358,9 @@ func TestMacKMS_CreateKey(t *testing.T) { assert.NoError(t, kms.DeleteKey(&apiv1.DeleteKeyRequest{ Name: "mackms:label=test-p256", })) + assert.NoError(t, kms.DeleteKey(&apiv1.DeleteKeyRequest{ + Name: "mackms:label=test-p256-2;tag=", + })) }) type args struct { @@ -384,6 +388,21 @@ func TestMacKMS_CreateKey(t *testing.T) { require.NotEmpty(tt, u.tag) require.NotEmpty(tt, u.hash) }, assert.NoError}, + {"ok no tag", &MacKMS{}, args{&apiv1.CreateKeyRequest{Name: "mackms:label=test-p256-2;tag="}}, + func(tt require.TestingT, i1 interface{}, i2 ...interface{}) { + require.IsType(tt, &apiv1.CreateKeyResponse{}, i1) + resp := i1.(*apiv1.CreateKeyResponse) + require.NotEmpty(tt, resp.Name) + require.NotNil(tt, resp.PublicKey) + require.Nil(tt, resp.PrivateKey) + require.NotEmpty(tt, resp.CreateSignerRequest) + + u, err := parseURI(resp.Name) + require.NoError(tt, err) + require.NotEmpty(tt, u.label) + require.Empty(tt, u.tag) + require.NotEmpty(tt, u.hash) + }, assert.NoError}, {"fail name", &MacKMS{}, args{&apiv1.CreateKeyRequest{}}, require.Nil, assert.Error}, {"fail uri", &MacKMS{}, args{&apiv1.CreateKeyRequest{Name: "mackms:name=test-p256"}}, require.Nil, assert.Error}, {"fail signatureAlgorithm", &MacKMS{}, args{&apiv1.CreateKeyRequest{ @@ -524,6 +543,8 @@ func Test_parseURI(t *testing.T) { {"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 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 missing label", args{"mackms:hash=0102abcd"}, nil, assert.Error}, } diff --git a/kms/uri/uri.go b/kms/uri/uri.go index a3e325b8..e83fb9c1 100644 --- a/kms/uri/uri.go +++ b/kms/uri/uri.go @@ -105,6 +105,11 @@ func (u *URI) String() string { return u.URL.String() } +// Has checks whether a given key is set. +func (u *URI) Has(key string) bool { + return u.Values.Has(key) || u.URL.Query().Has(key) +} + // Get returns the first value in the uri with the given key, it will return // empty string if that field is not present. func (u *URI) Get(key string) string { diff --git a/kms/uri/uri_test.go b/kms/uri/uri_test.go index 6688ee73..db7dde75 100644 --- a/kms/uri/uri_test.go +++ b/kms/uri/uri_test.go @@ -153,6 +153,42 @@ func TestParseWithScheme(t *testing.T) { } } +func TestURI_Has(t *testing.T) { + mustParse := func(s string) *URI { + u, err := Parse(s) + if err != nil { + t.Fatal(err) + } + return u + } + type args struct { + key string + } + tests := []struct { + name string + uri *URI + args args + want bool + }{ + {"ok", mustParse("yubikey:slot-id=9a"), args{"slot-id"}, true}, + {"ok empty", mustParse("yubikey:slot-id="), args{"slot-id"}, true}, + {"ok query", mustParse("yubikey:pin=123456?slot-id="), args{"slot-id"}, true}, + {"ok empty no equal", mustParse("yubikey:slot-id"), args{"slot-id"}, true}, + {"ok query no equal", mustParse("yubikey:pin=123456?slot-id"), args{"slot-id"}, true}, + {"ok empty no equal other", mustParse("yubikey:slot-id;pin=123456"), args{"slot-id"}, true}, + {"ok query no equal other", mustParse("yubikey:pin=123456?slot-id&pin=123456"), args{"slot-id"}, true}, + {"fail", mustParse("yubikey:pin=123456"), args{"slot-id"}, false}, + {"fail with query", mustParse("yubikey:pin=123456?slot=9a"), args{"slot-id"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.uri.Has(tt.args.key); got != tt.want { + t.Errorf("URI.Has() = %v, want %v", got, tt.want) + } + }) + } +} + func TestURI_Get(t *testing.T) { mustParse := func(s string) *URI { u, err := Parse(s)