luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

identity: do not allow a role's token_ttl to be longer than verification_ttl (#12151) 

* 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>
This commit is contained in:
John-Michael Faircloth 2021-07-28 20:34:52 -05:00 committed by GitHub
parent fa29e780f0
commit 39c744ca4e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 190 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
changelog/12151.txt Normal file
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
```

135
vault/identity_store_oidc.go
View File

@ -11,6 +11,7 @@ import (
"errors"
"fmt"
"net/url"
"sort"
"strings"
"time"
@ -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 {
@ -556,6 +576,51 @@ func (i *IdentityStore) pathOIDCReadKey(ctx context.Context, req *logical.Reques
}, nil
}
// 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 {
return nil, err
}
var tempRole role
roles := make(map[string]role)
for _, roleName := range roleNames {
entry, err := req.Storage.Get(ctx, roleConfigPath+roleName)
if err != nil {
return nil, err
}
if entry != nil {
if err := entry.DecodeJSON(&tempRole); err != nil {
return nil, err
}
if tempRole.Key == targetKeyName {
roles[roleName] = tempRole
}
}
}
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)
@ -567,35 +632,14 @@ func (i *IdentityStore) pathOIDCDeleteKey(ctx context.Context, req *logical.Requ
i.oidcLock.Lock()
// it is an error to delete a key that is actively referenced by a role
roleNames, err := req.Storage.List(ctx, roleConfigPath)
roleNames, err := i.roleNamesReferencingTargetKeyName(ctx, req, targetKeyName)
if err != nil {
i.oidcLock.Unlock()
return nil, err
}
var role *role
rolesReferencingTargetKeyName := make([]string, 0)
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()
return nil, err
}
if role.Key == targetKeyName {
rolesReferencingTargetKeyName = append(rolesReferencingTargetKeyName, roleName)
}
}
}
if len(rolesReferencingTargetKeyName) > 0 {
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
}
@ -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(),
}
@ -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) {
@ -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 {
@ -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)
}
@ -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
}

86
vault/identity_store_oidc_test.go
View File

@ -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)
@ -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)
@ -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)