Skip to content

Commit

Permalink
identity: enforce key param and key existence on role creation (hashi…
Browse files Browse the repository at this point in the history 
…corp#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
  • Loading branch information
@fairclothjm @jartek
fairclothjm authored and jartek committed Sep 11, 2021
1 parent 338bdfe commit eca9703
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 42 deletions.
3 changes: 3 additions & 0 deletions changelog/12208.txt
View file
@@ -0,0 +1,3 @@
```release-note:bug
identity: disallow creation of role without a key parameter
```
23 changes: 16 additions & 7 deletions vault/identity_store_oidc.go
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -1010,14 +1018,15 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
if err != nil {
return nil, err
}
if entry != nil {
if err := entry.DecodeJSON(&key); err != nil {
return nil, err
}
if entry == nil {
return logical.ErrorResponse("cannot find key %q", role.Key), nil
}

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 err := entry.DecodeJSON(&key); err != 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 {
Expand Down
193 changes: 158 additions & 35 deletions vault/identity_store_oidc_test.go
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit eca9703

Please sign in to comment.