Merge pull request #600 from smallstep/mariano/nulltags

Add support for using mackms keys with no tags

kms/mackms/mackms.go

@@ -95,6 +95,7 @@ var signatureAlgorithmMapping = map[apiv1.SignatureAlgorithm]algorithmAttributes
 // CreateKey methods can create keys with the following URIs:
 //   - mackms:label=my-name
 //   - mackms:label=my-name;tag=com.smallstep.crypto
+//   - mackms;label=my-name;tag=
 //   - mackms;label=my-name;se=true;bio=true
 //
 // GetPublicKey and CreateSigner accepts the above URIs as well as the following
@@ -107,7 +108,8 @@ var signatureAlgorithmMapping = map[apiv1.SignatureAlgorithm]algorithmAttributes
 //     represents the key name. You will be able to see the keys in the Keychain,
 //     looking for the value.
 //   - "tag" corresponds with kSecAttrApplicationTag. It defaults to
-//     com.smallstep.crypto.
+//     com.smallstep.crypto. If tag is an empty string ("tag="), the attribute
+//     will not be set.
 //   - "se" is a boolean. If set to true, it will store the key in the
 //     Secure Enclave. This option requires the application to be code-signed
 //     with the appropriate entitlements.
@@ -189,21 +191,22 @@ 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)
 	}
 	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 +494,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 +502,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,25 +676,26 @@ 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
 	}
 	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 +1018,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 +1105,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{

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},
 	}

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 {

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)