identity: enforce key param and key existence on role creation (#12208)

* identity: handle creation of role without a key parameter

* update docs to not require key parameter for creation of a role

* add changelog

* require key param when creating a role

* lock create/update role; remove now redundant key check

* update changelog and UTs

* update change log to refelct actual implementation

* remove deprecated test case

changelog/12208.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+identity: disallow creation of role without a key parameter
+```

vault/identity_store_oidc.go

@@ -243,6 +243,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path {
 				"key": {
 					Type:        framework.TypeString,
 					Description: "The OIDC key to use for generating tokens. The specified key must already exist.",
+					Required:    true,
 				},
 				"template": {
 					Type:        framework.TypeString,
@@ -942,6 +943,9 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
 
 	name := d.Get("name").(string)
 
+	i.oidcLock.Lock()
+	defer i.oidcLock.Unlock()
+
 	var role role
 	if req.Operation == logical.UpdateOperation {
 		entry, err := req.Storage.Get(ctx, roleConfigPath+name)
@@ -961,6 +965,10 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
 		role.Key = d.Get("key").(string)
 	}
 
+	if role.Key == "" {
+		return logical.ErrorResponse("the key parameter is required"), nil
+	}
+
 	if template, ok := d.GetOk("template"); ok {
 		role.Template = template.(string)
 	} else if req.Operation == logical.CreateOperation {
@@ -1010,14 +1018,15 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
 	if err != nil {
 		return nil, err
 	}
-	if entry != nil {
+	if entry == nil {
-		if err := entry.DecodeJSON(&key); err != nil {
+		return logical.ErrorResponse("cannot find key %q", role.Key), nil
-			return nil, err
+	}
-		}
 
-		if role.TokenTTL > key.VerificationTTL {
+	if err := entry.DecodeJSON(&key); err != nil {
-			return logical.ErrorResponse("a role's token ttl cannot be longer than the verification_ttl of the key it references"), nil
+		return nil, err
-		}
+	}
+	if role.TokenTTL > key.VerificationTTL {
+		return logical.ErrorResponse("a role's token ttl cannot be longer than the verification_ttl of the key it references"), nil
 	}
 
 	if clientID, ok := d.GetOk("client_id"); ok {

vault/identity_store_oidc_test.go

@@ -18,6 +18,164 @@ import (
 	"gopkg.in/square/go-jose.v2/jwt"
 )
 
+// TestOIDC_Path_OIDC_RoleNoKeyParameter tests that a role cannot be created
+// without a key parameter
+func TestOIDC_Path_OIDC_RoleNoKeyParameter(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test role "test-role1" without a key param -- should fail
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.CreateOperation,
+		Storage:   storage,
+	})
+	expectError(t, resp, err)
+	// validate error message
+	expectedStrings := map[string]interface{}{
+		"the key parameter is required": true,
+	}
+	expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings)
+}
+
+// TestOIDC_Path_OIDC_RoleNilKeyEntry tests that a role cannot be created when
+// a key parameter is provided but the key does not exist
+func TestOIDC_Path_OIDC_RoleNilKeyEntry(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test role "test-role1" with a non-existent key -- should fail
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"key": "test-key",
+		},
+		Storage: storage,
+	})
+	expectError(t, resp, err)
+	// validate error message
+	expectedStrings := map[string]interface{}{
+		"cannot find key \"test-key\"": true,
+	}
+	expectStrings(t, []string{resp.Data["error"].(string)}, expectedStrings)
+}
+
+// TestOIDC_Path_OIDCRole_UpdateNoKey test that we cannot update a role without
+// prividing a key param
+func TestOIDC_Path_OIDCRole_UpdateNoKey(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test key "test-key"
+	c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/key/test-key",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"verification_ttl": "2m",
+			"rotation_period":  "2m",
+		},
+		Storage: storage,
+	})
+
+	// Create a test role "test-role1" with a valid key -- should succeed
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"key": "test-key",
+			"ttl": "1m",
+		},
+		Storage: storage,
+	})
+	expectSuccess(t, resp, err)
+
+	// Update "test-role1" without prividing a key param -- should succeed
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"ttl": "2m",
+		},
+		Storage: storage,
+	})
+	expectSuccess(t, resp, err)
+
+	// Read "test-role1" again and validate
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.ReadOperation,
+		Storage:   storage,
+	})
+	expectSuccess(t, resp, err)
+	expected := map[string]interface{}{
+		"key":       "test-key",
+		"ttl":       int64(120),
+		"template":  "",
+		"client_id": resp.Data["client_id"],
+	}
+	if diff := deep.Equal(expected, resp.Data); diff != nil {
+		t.Fatal(diff)
+	}
+}
+
+// TestOIDC_Path_OIDCRole_UpdateEmptyKey test that we cannot update a role with an
+// empty key
+func TestOIDC_Path_OIDCRole_UpdateEmptyKey(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test key "test-key"
+	c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/key/test-key",
+		Operation: logical.CreateOperation,
+		Storage:   storage,
+	})
+
+	// Create a test role "test-role1" with a valid key -- should succeed
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"key": "test-key",
+		},
+		Storage: storage,
+	})
+	expectSuccess(t, resp, err)
+
+	// Update "test-role1" with valid parameters -- should fail
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"key": "",
+		},
+		Storage: storage,
+	})
+	expectError(t, resp, err)
+
+	// Read "test-role1" again and validate
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/role/test-role1",
+		Operation: logical.ReadOperation,
+		Storage:   storage,
+	})
+	expectSuccess(t, resp, err)
+	expected := map[string]interface{}{
+		"key":       "test-key",
+		"ttl":       int64(86400),
+		"template":  "",
+		"client_id": resp.Data["client_id"],
+	}
+	if diff := deep.Equal(expected, resp.Data); diff != nil {
+		t.Fatal(diff)
+	}
+}
+
 // TestOIDC_Path_OIDCRoleRole tests CRUD operations for roles
 func TestOIDC_Path_OIDCRoleRole(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)
@@ -113,41 +271,6 @@ func TestOIDC_Path_OIDCRoleRole(t *testing.T) {
 	}
 }
 
-// TestOIDC_Path_OIDCRole_NoKey tests that a role can be created with a non-existent key
-func TestOIDC_Path_OIDCRole_NoKey(t *testing.T) {
-	c, _, _ := TestCoreUnsealed(t)
-	ctx := namespace.RootContext(nil)
-	storage := &logical.InmemStorage{}
-
-	// Create a test role "test-role1" with a non-existent key -- should succeed
-	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
-		Path:      "oidc/role/test-role1",
-		Operation: logical.CreateOperation,
-		Data: map[string]interface{}{
-			"key": "test-key",
-		},
-		Storage: storage,
-	})
-	expectSuccess(t, resp, err)
-
-	// Read "test-role1" and validate
-	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
-		Path:      "oidc/role/test-role1",
-		Operation: logical.ReadOperation,
-		Storage:   storage,
-	})
-	expectSuccess(t, resp, err)
-	expected := map[string]interface{}{
-		"key":       "test-key",
-		"ttl":       int64(86400),
-		"template":  "",
-		"client_id": resp.Data["client_id"],
-	}
-	if diff := deep.Equal(expected, resp.Data); diff != nil {
-		t.Fatal(diff)
-	}
-}
-
 // TestOIDC_Path_OIDCRole_InvalidTokenTTL tests the TokenTTL validation
 func TestOIDC_Path_OIDCRole_InvalidTokenTTL(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)