Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

token/renewal: return full set of token and identity policies in the … #8535

Merged
merged 3 commits into from
Mar 31, 2020
Merged

token/renewal: return full set of token and identity policies in the … #8535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eae923a
token/renewal: return full set of token and identity policies in the …
calvn Mar 11, 2020
0d54971
extend tests to cover additional token and identity policies on a token
calvn Mar 11, 2020
77a9e2a
verify identity_policies returned on login and renewals
calvn Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions .../external_tests/policy/no_default_test.go → vault/external_tests/policy/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import (
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/api"
"github.com/hashicorp/vault/builtin/credential/ldap"
credUserpass "github.com/hashicorp/vault/builtin/credential/userpass"
ldaphelper "github.com/hashicorp/vault/helper/testhelpers/ldap"
vaulthttp "github.com/hashicorp/vault/http"
"github.com/hashicorp/vault/sdk/helper/strutil"
"github.com/hashicorp/vault/sdk/logical"
"github.com/hashicorp/vault/vault"
)
Expand Down Expand Up @@ -181,3 +183,144 @@ func TestPolicy_NoConfiguredPolicy(t *testing.T) {
t.Fatalf("failed to renew lease, got: %v", secret.Auth.LeaseDuration)
}
}

func TestPolicy_TokenRenewal(t *testing.T) {
cases := []struct {
name string
tokenPolicies []string
identityPolicies []string
}{
{
"default only",
nil,
nil,
},
{
"with token policies",
[]string{"token-policy"},
nil,
},
{
"with identity policies",
nil,
[]string{"identity-policy"},
},
{
"with token and identity policies",
[]string{"token-policy"},
[]string{"identity-policy"},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
coreConfig := &vault.CoreConfig{
CredentialBackends: map[string]logical.Factory{
"userpass": credUserpass.Factory,
},
}
cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
HandlerFunc: vaulthttp.Handler,
})
cluster.Start()
defer cluster.Cleanup()

core := cluster.Cores[0].Core
vault.TestWaitActive(t, core)
client := cluster.Cores[0].Client

// Enable userpass auth
err := client.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{
Type: "userpass",
})
if err != nil {
t.Fatal(err)
}

// Add a user to userpass backend
data := map[string]interface{}{
"password": "testpassword",
}
if len(tc.tokenPolicies) > 0 {
data["token_policies"] = tc.tokenPolicies
}
_, err = client.Logical().Write("auth/userpass/users/testuser", data)
if err != nil {
t.Fatal(err)
}

// Set up entity if we're testing against an identity_policies
if len(tc.identityPolicies) > 0 {
auths, err := client.Sys().ListAuth()
if err != nil {
t.Fatal(err)
}
userpassAccessor := auths["userpass/"].Accessor

resp, err := client.Logical().Write("identity/entity", map[string]interface{}{
"name": "test-entity",
"policies": tc.identityPolicies,
})
if err != nil {
t.Fatal(err)
}
entityID := resp.Data["id"].(string)

// Create an alias
resp, err = client.Logical().Write("identity/entity-alias", map[string]interface{}{
"name": "testuser",
"mount_accessor": userpassAccessor,
"canonical_id": entityID,
})
if err != nil {
t.Fatal(err)
}
}

// Authenticate
secret, err := client.Logical().Write("auth/userpass/login/testuser", map[string]interface{}{
"password": "testpassword",
})
if err != nil {
t.Fatal(err)
}
clientToken := secret.Auth.ClientToken

// Verify the policies exist in the login response
expectedTokenPolicies := append([]string{"default"}, tc.tokenPolicies...)
if !strutil.EquivalentSlices(secret.Auth.TokenPolicies, expectedTokenPolicies) {
t.Fatalf("token policy mismatch:\nexpected: %v\ngot: %v", expectedTokenPolicies, secret.Auth.TokenPolicies)
}

if !strutil.EquivalentSlices(secret.Auth.IdentityPolicies, tc.identityPolicies) {
t.Fatalf("identity policy mismatch:\nexpected: %v\ngot: %v", tc.identityPolicies, secret.Auth.IdentityPolicies)
}

expectedPolicies := append(expectedTokenPolicies, tc.identityPolicies...)
if !strutil.EquivalentSlices(secret.Auth.Policies, expectedPolicies) {
t.Fatalf("policy mismatch:\nexpected: %v\ngot: %v", expectedPolicies, secret.Auth.Policies)
}

// Renew token
secret, err = client.Logical().Write("auth/token/renew", map[string]interface{}{
"token": clientToken,
})
if err != nil {
t.Fatal(err)
}

// Verify the policies exist in the renewal response
if !strutil.EquivalentSlices(secret.Auth.TokenPolicies, expectedTokenPolicies) {
t.Fatalf("policy mismatch:\nexpected: %v\ngot: %v", expectedTokenPolicies, secret.Auth.TokenPolicies)
}

if !strutil.EquivalentSlices(secret.Auth.IdentityPolicies, tc.identityPolicies) {
t.Fatalf("identity policy mismatch:\nexpected: %v\ngot: %v", tc.identityPolicies, secret.Auth.IdentityPolicies)
}

if !strutil.EquivalentSlices(secret.Auth.Policies, expectedPolicies) {
t.Fatalf("policy mismatch:\nexpected: %v\ngot: %v", expectedPolicies, secret.Auth.Policies)
}
})
}
}
49 changes: 29 additions & 20 deletions vault/request_handling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,9 +836,8 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp
}

// Only the token store is allowed to return an auth block, for any
// other request this is an internal error. We exclude renewal of a token,
// since it does not need to be re-registered
if resp != nil && resp.Auth != nil && !strings.HasPrefix(req.Path, "auth/token/renew") {
// other request this is an internal error.
if resp != nil && resp.Auth != nil {
if !strings.HasPrefix(req.Path, "auth/token/") {
c.logger.Error("unexpected Auth response for non-token backend", "request_path", req.Path)
retErr = multierror.Append(retErr, ErrInternalError)
Expand Down Expand Up @@ -868,24 +867,34 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp
return nil, nil, ErrInternalError
}

resp.Auth.TokenPolicies = policyutil.SanitizePolicies(resp.Auth.Policies, policyutil.DoNotAddDefaultPolicy)
switch resp.Auth.TokenType {
case logical.TokenTypeBatch:
case logical.TokenTypeService:
if err := c.expiration.RegisterAuth(ctx, &logical.TokenEntry{
TTL: auth.TTL,
Policies: auth.TokenPolicies,
Path: resp.Auth.CreationPath,
NamespaceID: ns.ID,
}, resp.Auth); err != nil {
// Best-effort clean up on error, so we log the cleanup error as
// a warning but still return as internal error.
if err := c.tokenStore.revokeOrphan(ctx, resp.Auth.ClientToken); err != nil {
c.logger.Warn("failed to clean up token lease during auth/token/ request", "request_path", req.Path, "error", err)
// We skip expiration manager registration for token renewal since it
// does not need to be re-registered
if strings.HasPrefix(req.Path, "auth/token/renew") {
// We build the "policies" list to be returned by starting with
// token policies, and add identity policies right after this
// conditional
resp.Auth.Policies = policyutil.SanitizePolicies(resp.Auth.TokenPolicies, policyutil.DoNotAddDefaultPolicy)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we assign to Policies here instead of TokenPolicies as in the other branch?

Assuming my intuition is correct - that the assignment should be the same regardless of whether or not this is a renewal - I think the code might be a bit clearer if you do the assignment as a first step, then extend the following switch statement, like:

resp.Auth.TokenPolicies = policyutil.SanitizePolicies(resp.Auth.Policies, policyutil.DoNotAddDefaultPolicy)
switch {
case strings.HasPrefix(req.Path, "auth/token/renew"): // do nothing
case resp.Auth.TokenType == logical.TokenTypeBatch: // do nothing
case resp.Auth.TokenType == logical.TokenTypeService: // do registration
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work because resp.Auth.Policies returned by a renewal response after routing is empty (the token policies are returned under resp.Auth.TokenPolicies), and hence why we need to add them to resp.Auth.Policies and not the other way around.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I understood why it's like that, but my timeboxed foray into the code for answers didn't bear fruit. Anyway, I don't think this will break anything, and it solves the existing problem, so I'll let it go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug a bit further to try to understand it a bit more as well. The Auth object in resp.Auth comes from the associated lease entry in the token, which is assigned in renewAuthEntry that we hit during the renewal request flow. It looks like the policies field omits the default policy, and thus either 1) does not contain the full set of token policies or 2) stores a null object for that field if default is the only policy (as seen by the examples below when reading the lease entry in storage). That inconsistency may or may not need to be addressed separately, but in any case using token_policies as the starting set for token policies is the more reliable way to build resp.Auth.Policies.

User bob on userpass with no policy attached, just the default:

  "auth": {
    "lease": 2764798000000000,
    "max_ttl": 0,
    "renewable": true,
    "internal_data": null,
    "display_name": "userpass-bob",
    "policies": null,
    "token_policies": [
      "default"
    ],
...

User bob with "test" policy attached:

  "auth": {
    "lease": 2764798000000000,
    "max_ttl": 0,
    "renewable": true,
    "internal_data": null,
    "display_name": "userpass-bob",
    "policies": [
      "test"
    ],
    "token_policies": [
      "default",
      "test"
    ],

} else {
resp.Auth.TokenPolicies = policyutil.SanitizePolicies(resp.Auth.Policies, policyutil.DoNotAddDefaultPolicy)

switch resp.Auth.TokenType {
case logical.TokenTypeBatch:
case logical.TokenTypeService:
if err := c.expiration.RegisterAuth(ctx, &logical.TokenEntry{
TTL: auth.TTL,
Policies: auth.TokenPolicies,
Path: resp.Auth.CreationPath,
NamespaceID: ns.ID,
}, resp.Auth); err != nil {
// Best-effort clean up on error, so we log the cleanup error as
// a warning but still return as internal error.
if err := c.tokenStore.revokeOrphan(ctx, resp.Auth.ClientToken); err != nil {
c.logger.Warn("failed to clean up token lease during auth/token/ request", "request_path", req.Path, "error", err)
}
c.logger.Error("failed to register token lease during auth/token/ request", "request_path", req.Path, "error", err)
retErr = multierror.Append(retErr, ErrInternalError)
return nil, auth, retErr
}
c.logger.Error("failed to register token lease during auth/token/ request", "request_path", req.Path, "error", err)
retErr = multierror.Append(retErr, ErrInternalError)
return nil, auth, retErr
}
}

Expand Down