Skip to content

Commit

Permalink
identity: do not allow a role's token_ttl to be longer than verificat…
Browse files Browse the repository at this point in the history 
…ion_ttl (#12151) (#12213)

* do not allow token_ttl to be longer than verification_ttl

* add verification when updating an existing key

When updating a key, ensure any roles referencing the key do not already
have a token_ttl greater than the key's verification_ttl

* add changelog

* remove unneeded UT check and comment

* refactor based on PR comments

- remove make slice in favor of var delcaration
- remove unneeded if check
- validate expiry value during token generation
- update changelog as bug

* refactor get roles referencing target key names logic

* add note about thread safety to helper func

* update func comment

* sort array and refactor func names

* add warning to return response

* remove unnecessary code from unit test

* Update vault/identity_store_oidc.go

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: John-Michael Faircloth <fairclothjm@users.noreply.github.com>
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
  • Loading branch information
@calvn @fairclothjm @austingebauer
3 people committed Jul 30, 2021
1 parent 571a30c commit f882564
Show file tree
Hide file tree
Showing 3 changed files with 188 additions and 32 deletions.
3 changes: 3 additions & 0 deletions changelog/12151.txt
View file
@@ -0,0 +1,3 @@
```release-note:bug
identity: do not allow a role's token_ttl to be longer than the signing key's verification_ttl
```
131 changes: 99 additions & 32 deletions vault/identity_store_oidc.go
View file
Expand Up @@ -11,6 +11,7 @@ import (
"errors"
"fmt"
"net/url"
"sort"
"strings"
"time"

Expand Down Expand Up @@ -472,6 +473,25 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica
return logical.ErrorResponse("verification_ttl cannot be longer than 10x rotation_period"), nil
}

if req.Operation == logical.UpdateOperation {
// ensure any roles referencing this key do not already have a token_ttl
// greater than the key's verification_ttl
roles, err := i.rolesReferencingTargetKeyName(ctx, req, name)
if err != nil {
return nil, err
}
for _, role := range roles {
if role.TokenTTL > key.VerificationTTL {
errorMessage := fmt.Sprintf(
"unable to update key %q because it is currently referenced by one or more roles with a token ttl greater than %d seconds",
name,
key.VerificationTTL/time.Second,
)
return logical.ErrorResponse(errorMessage), nil
}
}
}

if allowedClientIDsRaw, ok := d.GetOk("allowed_client_ids"); ok {
key.AllowedClientIDs = allowedClientIDsRaw.([]string)
} else if req.Operation == logical.CreateOperation {
Expand Down Expand Up @@ -556,46 +576,70 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques
}, nil
}

// handleOIDCDeleteKey is used to delete a key
func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
ns, err := namespace.FromContext(ctx)
if err != nil {
return nil, err
}

targetKeyName := d.Get("name").(string)

i.oidcLock.Lock()

// it is an error to delete a key that is actively referenced by a role
// rolesReferencingTargetKeyName returns a map of role names to roles referenced by targetKeyName.
// Note: this is not threadsafe. It is to be called with Lock already held.
func (i *IdentityStore) rolesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) (map[string]role, error) {
roleNames, err := req.Storage.List(ctx, roleConfigPath)
if err != nil {
i.oidcLock.Unlock()
return nil, err
}

var role *role
rolesReferencingTargetKeyName := make([]string, 0)
var tempRole role
roles := make(map[string]role)
for _, roleName := range roleNames {
entry, err := req.Storage.Get(ctx, roleConfigPath+roleName)
if err != nil {
i.oidcLock.Unlock()
return nil, err
}
if entry != nil {
if err := entry.DecodeJSON(&role); err != nil {
i.oidcLock.Unlock()
if err := entry.DecodeJSON(&tempRole); err != nil {
return nil, err
}
if role.Key == targetKeyName {
rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, roleName)
if tempRole.Key == targetKeyName {
roles[roleName] = tempRole
}
}
}

if len(rolesReferencingTargetKeyName) > 0 {
return roles, nil
}

// roleNamesReferencingTargetKeyName returns a slice of strings of role
// names referenced by targetKeyName.
// Note: this is not threadsafe. It is to be called with Lock already held.
func (i *IdentityStore) roleNamesReferencingTargetKeyName(ctx context.Context, req *logical.Request, targetKeyName string) ([]string, error) {
roles, err := i.rolesReferencingTargetKeyName(ctx, req, targetKeyName)
if err != nil {
return nil, err
}

var names []string
for key, _ := range roles {
names = append(names, key)
}
sort.Strings(names)
return names, nil
}

// handleOIDCDeleteKey is used to delete a key
func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
ns, err := namespace.FromContext(ctx)
if err != nil {
return nil, err
}

targetKeyName := d.Get("name").(string)

i.oidcLock.Lock()

roleNames, err := i.roleNamesReferencingTargetKeyName(ctx, req, targetKeyName)
if err != nil {
return nil, err
}

if len(roleNames) > 0 {
errorMessage := fmt.Sprintf("unable to delete key %q because it is currently referenced by these roles: %s",
targetKeyName, strings.Join(rolesReferencingTargetKeyName, ", "))
targetKeyName, strings.Join(roleNames, ", "))
i.oidcLock.Unlock()
return logical.ErrorResponse(errorMessage), logical.ErrInvalidRequest
}
Expand Down Expand Up @@ -747,13 +791,21 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical.
return nil, err
}

retResp := &logical.Response{}
expiry := role.TokenTTL
if expiry > key.VerificationTTL {
expiry = key.VerificationTTL
retResp.AddWarning(fmt.Sprintf("a role's token ttl cannot be longer "+
"than the verification_ttl of the key it references, setting token ttl to %d", expiry))
}

now := time.Now()
idToken := idToken{
Issuer: config.effectiveIssuer,
Namespace: ns.ID,
Subject: req.EntityID,
Audience: role.ClientID,
Expiry: now.Add(role.TokenTTL).Unix(),
Expiry: now.Add(expiry).Unix(),
IssuedAt: now.Unix(),
}

Expand Down Expand Up @@ -782,13 +834,12 @@ func (i *IdentityStore) pathOIDCGenerateToken(ctx context.Context, req *logical.
return nil, fmt.Errorf("error signing OIDC token: %w", err)
}

return &logical.Response{
Data: map[string]interface{}{
"token": signedIdToken,
"client_id": role.ClientID,
"ttl": int64(role.TokenTTL.Seconds()),
},
}, nil
retResp.Data = map[string]interface{}{
"token": signedIdToken,
"client_id": role.ClientID,
"ttl": int64(role.TokenTTL.Seconds()),
}
return retResp, nil
}

func (tok *idToken) generatePayload(logger hclog.Logger, template string, entity *identity.Entity, groups []*identity.Group) ([]byte, error) {
Expand Down Expand Up @@ -867,7 +918,7 @@ func (i *IdentityStore) pathOIDCRoleExistenceCheck(ctx context.Context, req *log
return role != nil, nil
}

// handleOIDCCreateRole is used to create a new role or update an existing one
// pathOIDCCreateUpdateRole is used to create a new role or update an existing one
func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
ns, err := namespace.FromContext(ctx)
if err != nil {
Expand Down Expand Up @@ -938,6 +989,22 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
role.TokenTTL = time.Duration(d.Get("ttl").(int)) * time.Second
}

// get the key referenced by this role
var key namedKey
entry, err := req.Storage.Get(ctx, namedKeyConfigPath+role.Key)
if err != nil {
return nil, err
}
if entry != 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 {
role.ClientID = clientID.(string)
}
Expand All @@ -952,7 +1019,7 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic
}

// store role (which was either just created or updated)
entry, err := logical.StorageEntryJSON(roleConfigPath+name, role)
entry, err = logical.StorageEntryJSON(roleConfigPath+name, role)
if err != nil {
return nil, err
}
Expand Down
86 changes: 86 additions & 0 deletions vault/identity_store_oidc_test.go
View file
Expand Up @@ -113,6 +113,48 @@ func TestOIDC_Path_OIDCRoleRole(t *testing.T) {
}
}

// TestOIDC_Path_OIDCRole_InvalidTokenTTL tests the TokenTTL validation
func TestOIDC_Path_OIDCRole_InvalidTokenTTL(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": int64(60),
},
Storage: storage,
})

// Create a test role "test-role1" with a ttl longer than the
// verification_ttl -- should fail with error
resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/role/test-role1",
Operation: logical.CreateOperation,
Data: map[string]interface{}{
"key": "test-key",
"ttl": int64(3600),
},
Storage: storage,
})
expectError(t, resp, err)

// Read "test-role1"
respReadTestRole1, err3 := c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/role/test-role1",
Operation: logical.ReadOperation,
Storage: storage,
})
// Ensure that "test-role1" was not created
expectSuccess(t, respReadTestRole1, err3)
if respReadTestRole1 != nil {
t.Fatalf("Expected a nil response but instead got:\n%#v", respReadTestRole1)
}
}

// TestOIDC_Path_OIDCRole tests the List operation for roles
func TestOIDC_Path_OIDCRole(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
Expand Down Expand Up @@ -284,6 +326,49 @@ func TestOIDC_Path_OIDCKeyKey(t *testing.T) {
expectSuccess(t, resp, err)
}

// TestOIDC_Path_OIDCKey_InvalidTokenTTL tests the TokenTTL validation
func TestOIDC_Path_OIDCKey_InvalidTokenTTL(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
ctx := namespace.RootContext(nil)
storage := &logical.InmemStorage{}

// Create a test key "test-key" -- should succeed
resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/key/test-key",
Operation: logical.CreateOperation,
Data: map[string]interface{}{
"verification_ttl": "4m",
},
Storage: storage,
})
expectSuccess(t, resp, err)

// Create a role that depends on test key
resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/role/allowed-test-role",
Operation: logical.UpdateOperation,
Data: map[string]interface{}{
"key": "test-key",
"ttl": "4m",
},
Storage: storage,
})
expectSuccess(t, resp, err)

// Update "test-key" -- should fail since allowed-test-role ttl is less than 2m
resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/key/test-key",
Operation: logical.UpdateOperation,
Data: map[string]interface{}{
"rotation_period": "10m",
"verification_ttl": "2m",
"allowed_client_ids": "allowed-test-role",
},
Storage: storage,
})
expectError(t, resp, err)
}

// TestOIDC_Path_OIDCKey tests the List operation for keys
func TestOIDC_Path_OIDCKey(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
Expand Down Expand Up @@ -1100,6 +1185,7 @@ func expectSuccess(t *testing.T, resp *logical.Response, err error) {
}

func expectError(t *testing.T, resp *logical.Response, err error) {
t.Helper()
if err == nil {
if resp == nil || !resp.IsError() {
t.Fatalf("expected error but got success; error:\n%v\nresp: %#v", err, resp)
Expand Down

0 comments on commit f882564

Please sign in to comment.