From 383a373a2a31e0fe3508637551cba136c694731f Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Mon, 26 Dec 2022 18:02:23 +0000 Subject: [PATCH 1/3] Fix aspects of `auth/token/create` request parsing Fixes #18550 Currently, the `auth/token/create` family of APIs (`create`, `create-orphan`, `create/{role}`) does non-standard parsing of requests, by directly using `mapstructure.WeakDecode(request.Data, ...)` instead of using the standard `framework.FieldData` abstraction. Furthermore, the fields declared for these APIs are incorrect, leading to inappropriate OpenAPI generation, and inappropriate warnings about ignored parameters. Detailed changes: * Factor out triplicated definitions of common fields across these three APIs. * Remove incorrect `role_name` field from `create-orphan`. * Add missing `lease` deprecated field. * Rename incorrectly named `metadata` field to `meta`, and change from `TypeMap` to `TypeKVPairs` to reflect actual underlying Go type is `map[string]string`. * Remove entirely incorrect `format` field. * Add declarative `Default: true` to `renewable` field, to match behaviour currently implemented in code. * Having fixed the field definitions to match current usage, remove the secondary decoding of the request via `mapstructure` inside `handleCreateCommon`, and migrate to using `FieldData` APIs like a normal operation function. --- vault/token_store.go | 382 +++++++++++++++---------------------------- 1 file changed, 130 insertions(+), 252 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index f8e7b63b2416..f06d63b93540 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -38,7 +38,6 @@ import ( "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/plugin/pb" "github.com/hashicorp/vault/vault/tokens" - "github.com/mitchellh/mapstructure" ) const ( @@ -135,6 +134,77 @@ var ( ) func (ts *TokenStore) paths() []*framework.Path { + commonFieldsForCreate := map[string]*framework.FieldSchema{ + "display_name": { + Type: framework.TypeString, + Description: "Name to associate with this token", + }, + "explicit_max_ttl": { + Type: framework.TypeString, + Description: "Explicit Max TTL of this token", + }, + "entity_alias": { + Type: framework.TypeString, + Description: "Name of the entity alias to associate with this token", + }, + "num_uses": { + Type: framework.TypeInt, + Description: "Max number of uses for this token", + }, + "period": { + Type: framework.TypeString, + Description: "Renew period", + }, + "renewable": { + Type: framework.TypeBool, + Description: "Allow token to be renewed past its initial TTL up to system/mount maximum TTL", + Default: true, + }, + "ttl": { + Type: framework.TypeString, + Description: "Time to live for this token", + }, + "lease": { + Type: framework.TypeString, + Description: "Use 'ttl' instead", + Deprecated: true, + }, + "type": { + Type: framework.TypeString, + Description: "Token type", + }, + "no_default_policy": { + Type: framework.TypeBool, + Description: "Do not include default policy for this token", + }, + "id": { + Type: framework.TypeString, + Description: "Value for the token", + }, + "meta": { + Type: framework.TypeKVPairs, + Description: "Arbitrary key=value metadata to associate with the token", + }, + "no_parent": { + Type: framework.TypeBool, + Description: "Create the token with no parent", + }, + "policies": { + Type: framework.TypeStringSlice, + Description: "List of policies for the token", + }, + } + + fieldsForCreateWithRole := map[string]*framework.FieldSchema{ + "role_name": { + Type: framework.TypeString, + Description: "Name of the role", + }, + } + for k, v := range commonFieldsForCreate { + fieldsForCreateWithRole[k] = v + } + p := []*framework.Path{ { Pattern: "roles/?$", @@ -161,69 +231,7 @@ func (ts *TokenStore) paths() []*framework.Path { { Pattern: "create-orphan$", - Fields: map[string]*framework.FieldSchema{ - "role_name": { - Type: framework.TypeString, - Description: "Name of the role", - }, - "display_name": { - Type: framework.TypeString, - Description: "Name to associate with this token", - }, - "explicit_max_ttl": { - Type: framework.TypeString, - Description: "Explicit Max TTL of this token", - }, - "entity_alias": { - Type: framework.TypeString, - Description: "Name of the entity alias to associate with this token", - }, - "num_uses": { - Type: framework.TypeInt, - Description: "Max number of uses for this token", - }, - "period": { - Type: framework.TypeString, - Description: "Renew period", - }, - "renewable": { - Type: framework.TypeBool, - Description: "Allow token to be renewed past its initial TTL up to system/mount maximum TTL", - }, - "ttl": { - Type: framework.TypeString, - Description: "Time to live for this token", - }, - "type": { - Type: framework.TypeString, - Description: "Token type", - }, - "no_default_policy": { - Type: framework.TypeBool, - Description: "Do not include default policy for this token", - }, - "id": { - Type: framework.TypeString, - Description: "Value for the token", - }, - "metadata": { - Type: framework.TypeMap, - Description: "Arbitrary key=value metadata to associate with the token", - }, - "no_parent": { - Type: framework.TypeBool, - Description: "Create the token with no parent", - }, - "policies": { - Type: framework.TypeStringSlice, - Description: "List of policies for the token", - }, - "format": { - Type: framework.TypeString, - Query: true, - Description: "Return json formatted output", - }, - }, + Fields: commonFieldsForCreate, Callbacks: map[logical.Operation]framework.OperationFunc{ logical.UpdateOperation: ts.handleCreateOrphan, @@ -236,69 +244,7 @@ func (ts *TokenStore) paths() []*framework.Path { { Pattern: "create/" + framework.GenericNameRegex("role_name"), - Fields: map[string]*framework.FieldSchema{ - "role_name": { - Type: framework.TypeString, - Description: "Name of the role", - }, - "display_name": { - Type: framework.TypeString, - Description: "Name to associate with this token", - }, - "explicit_max_ttl": { - Type: framework.TypeString, - Description: "Explicit Max TTL of this token", - }, - "entity_alias": { - Type: framework.TypeString, - Description: "Name of the entity alias to associate with this token", - }, - "num_uses": { - Type: framework.TypeInt, - Description: "Max number of uses for this token", - }, - "period": { - Type: framework.TypeString, - Description: "Renew period", - }, - "renewable": { - Type: framework.TypeBool, - Description: "Allow token to be renewed past its initial TTL up to system/mount maximum TTL", - }, - "ttl": { - Type: framework.TypeString, - Description: "Time to live for this token", - }, - "type": { - Type: framework.TypeString, - Description: "Token type", - }, - "no_default_policy": { - Type: framework.TypeBool, - Description: "Do not include default policy for this token", - }, - "id": { - Type: framework.TypeString, - Description: "Value for the token", - }, - "metadata": { - Type: framework.TypeMap, - Description: "Arbitrary key=value metadata to associate with the token", - }, - "no_parent": { - Type: framework.TypeBool, - Description: "Create the token with no parent", - }, - "policies": { - Type: framework.TypeStringSlice, - Description: "List of policies for the token", - }, - "format": { - Type: framework.TypeString, - Query: true, - Description: "Return json formatted output", - }, - }, + Fields: fieldsForCreateWithRole, Callbacks: map[logical.Operation]framework.OperationFunc{ logical.UpdateOperation: ts.handleCreateAgainstRole, @@ -311,65 +257,7 @@ func (ts *TokenStore) paths() []*framework.Path { { Pattern: "create$", - Fields: map[string]*framework.FieldSchema{ - "display_name": { - Type: framework.TypeString, - Description: "Name to associate with this token", - }, - "explicit_max_ttl": { - Type: framework.TypeString, - Description: "Explicit Max TTL of this token", - }, - "entity_alias": { - Type: framework.TypeString, - Description: "Name of the entity alias to associate with this token", - }, - "num_uses": { - Type: framework.TypeInt, - Description: "Max number of uses for this token", - }, - "period": { - Type: framework.TypeString, - Description: "Renew period", - }, - "renewable": { - Type: framework.TypeBool, - Description: "Allow token to be renewed past its initial TTL up to system/mount maximum TTL", - }, - "ttl": { - Type: framework.TypeString, - Description: "Time to live for this token", - }, - "type": { - Type: framework.TypeString, - Description: "Token type", - }, - "no_default_policy": { - Type: framework.TypeBool, - Description: "Do not include default policy for this token", - }, - "id": { - Type: framework.TypeString, - Description: "Value for the token", - }, - "metadata": { - Type: framework.TypeMap, - Description: "Arbitrary key=value metadata to associate with the token", - }, - "no_parent": { - Type: framework.TypeBool, - Description: "Create the token with no parent", - }, - "policies": { - Type: framework.TypeStringSlice, - Description: "List of policies for the token", - }, - "format": { - Type: framework.TypeString, - Query: true, - Description: "Return json formatted output", - }, - }, + Fields: commonFieldsForCreate, Callbacks: map[logical.Operation]framework.OperationFunc{ logical.UpdateOperation: ts.handleCreate, @@ -2631,27 +2519,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // Check if the client token has sudo/root privileges for the requested path isSudo := ts.System().(extendedSystemView).SudoPrivilege(ctx, req.MountPoint+req.Path, req.ClientToken) - // Read and parse the fields - var data struct { - ID string - Policies []string - Metadata map[string]string `mapstructure:"meta"` - NoParent bool `mapstructure:"no_parent"` - NoDefaultPolicy bool `mapstructure:"no_default_policy"` - Lease string - TTL string - Renewable *bool - ExplicitMaxTTL string `mapstructure:"explicit_max_ttl"` - DisplayName string `mapstructure:"display_name"` - NumUses int `mapstructure:"num_uses"` - Period string - Type string `mapstructure:"type"` - EntityAlias string `mapstructure:"entity_alias"` - } - if err := mapstructure.WeakDecode(req.Data, &data); err != nil { - return logical.ErrorResponse(fmt.Sprintf( - "Error decoding request: %s", err)), logical.ErrInvalidRequest - } + policies := d.Get("policies").([]string) // If the context's namespace is different from the parent and this is an // orphan token creation request, then this is an admin token generation for @@ -2675,18 +2543,13 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque return logical.ErrorResponse("root or sudo privileges required to directly generate a token in a child namespace"), logical.ErrInvalidRequest } - if strutil.StrListContains(data.Policies, "root") { + if strutil.StrListContains(policies, "root") { return logical.ErrorResponse("root tokens may not be created from a parent namespace"), logical.ErrInvalidRequest } } - renewable := true - if data.Renewable != nil { - renewable = *data.Renewable - } - tokenType := logical.TokenTypeService - tokenTypeStr := data.Type + tokenTypeStr := d.Get("type").(string) if role != nil { switch role.TokenType { case logical.TokenTypeDefault, logical.TokenTypeDefaultService: @@ -2704,23 +2567,28 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque return logical.ErrorResponse(fmt.Sprintf("role being used for token creation contains invalid token type %q", role.TokenType.String())), nil } } + + renewable := d.Get("renewable").(bool) + explicitMaxTTL := d.Get("explicit_max_ttl").(string) + numUses := d.Get("num_uses").(int) + period := d.Get("period").(string) switch tokenTypeStr { case "", "service": case "batch": var badReason string switch { - case data.ExplicitMaxTTL != "": - dur, err := parseutil.ParseDurationSecond(data.ExplicitMaxTTL) + case explicitMaxTTL != "": + dur, err := parseutil.ParseDurationSecond(explicitMaxTTL) if err != nil { return logical.ErrorResponse(`"explicit_max_ttl" value could not be parsed`), nil } if dur != 0 { badReason = "explicit_max_ttl" } - case data.NumUses != 0: + case numUses != 0: badReason = "num_uses" - case data.Period != "": - dur, err := parseutil.ParseDurationSecond(data.Period) + case period != "": + dur, err := parseutil.ParseDurationSecond(period) if err != nil { return logical.ErrorResponse(`"period" value could not be parsed`), nil } @@ -2738,14 +2606,14 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } // Verify the number of uses is positive - if data.NumUses < 0 { + if numUses < 0 { return logical.ErrorResponse("number of uses cannot be negative"), logical.ErrInvalidRequest } // Verify the entity alias var explicitEntityID string - if data.EntityAlias != "" { + if entityAliasRaw := d.Get("entity_alias").(string); entityAliasRaw != "" { // Parameter is only allowed in combination with token role if role == nil { return logical.ErrorResponse("'entity_alias' is only allowed in combination with token role"), logical.ErrInvalidRequest @@ -2753,8 +2621,8 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // Convert entity alias to lowercase to match the fact that role.AllowedEntityAliases // has also been lowercased. An entity alias will keep its case formatting, but be - // treated as lowercase during any value cheek anywhere. - entityAlias := strings.ToLower(data.EntityAlias) + // treated as lowercase during any value check anywhere. + entityAlias := strings.ToLower(entityAliasRaw) // Check if there is a concrete match if !strutil.StrListContains(role.AllowedEntityAliases, entityAlias) && @@ -2770,7 +2638,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // Create alias for later processing alias := &logical.Alias{ - Name: data.EntityAlias, + Name: entityAliasRaw, MountAccessor: mountValidationResp.Accessor, MountType: mountValidationResp.Type, } @@ -2793,7 +2661,16 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque explicitEntityID = entity.ID } - // Setup the token entry + // By using GetOk here, we set TokenEntry.Meta to nil instead of an empty map, when no meta was passed in the + // token creation API call. This maintains closer compatibility with previous behaviour of Vault, helping to avoid + // user surprise, and saving the need to update quite a few tests which expect this to be nil in such a case. + meta, ok := d.GetOk("meta") + var metaMap map[string]string + if ok { + metaMap = meta.(map[string]string) + } + + // Set up the token entry te := logical.TokenEntry{ Parent: req.ClientToken, @@ -2802,9 +2679,9 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // have an official mount Path: fmt.Sprintf("auth/token/%s", req.Path), - Meta: data.Metadata, + Meta: metaMap, DisplayName: "token", - NumUses: data.NumUses, + NumUses: numUses, CreationTime: time.Now().Unix(), NamespaceID: ns.ID, Type: tokenType, @@ -2840,15 +2717,15 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } // Attach the given display name if any - if data.DisplayName != "" { - full := "token-" + data.DisplayName + if displayName := d.Get("display_name").(string); displayName != "" { + full := "token-" + displayName full = displayNameSanitize.ReplaceAllString(full, "-") full = strings.TrimSuffix(full, "-") te.DisplayName = full } // Allow specifying the ID of the token if the client has root or sudo privileges - if data.ID != "" { + if id := d.Get("id").(string); id != "" { if !isSudo { return logical.ErrorResponse("root or sudo privileges required to specify token id"), logical.ErrInvalidRequest @@ -2857,7 +2734,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque return logical.ErrorResponse("token IDs can only be manually specified in the root namespace"), logical.ErrInvalidRequest } - te.ID = data.ID + te.ID = id } resp := &logical.Response{} @@ -2869,6 +2746,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // should be stripped out regardless, *but*, the logic of when it should // and shouldn't be added is kept because we want to do subset comparisons // based on adding default when it's correct to do so. + noDefaultPolicy := d.Get("no_default_policy").(bool) switch { case role != nil && (len(role.AllowedPolicies) > 0 || len(role.DisallowedPolicies) > 0 || len(role.AllowedPoliciesGlob) > 0 || len(role.DisallowedPoliciesGlob) > 0): @@ -2883,15 +2761,15 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // isn't in the disallowed list, add it. This is in line with the idea // that roles, when allowed/disallowed ar set, allow a subset of // policies to be set disjoint from the parent token's policies. - if !data.NoDefaultPolicy && !role.TokenNoDefaultPolicy && + if !noDefaultPolicy && !role.TokenNoDefaultPolicy && !strutil.StrListContains(role.DisallowedPolicies, "default") && !strutil.StrListContainsGlob(role.DisallowedPoliciesGlob, "default") { localAddDefault = true } // Start with passed-in policies as a baseline, if they exist - if len(data.Policies) > 0 { - finalPolicies = policyutil.SanitizePolicies(data.Policies, localAddDefault) + if len(policies) > 0 { + finalPolicies = policyutil.SanitizePolicies(policies, localAddDefault) } var sanitizedRolePolicies, sanitizedRolePoliciesGlob []string @@ -2938,17 +2816,17 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } } - data.Policies = finalPolicies + policies = finalPolicies // We are creating a token from a parent namespace. We should only use the input // policies. case ns.ID != parent.NamespaceID: - addDefault = !data.NoDefaultPolicy + addDefault = !noDefaultPolicy // No policies specified, inherit parent - case len(data.Policies) == 0: + case len(policies) == 0: // Only inherit "default" if the parent already has it, so don't touch addDefault here - data.Policies = policyutil.SanitizePolicies(parent.Policies, policyutil.DoNotAddDefaultPolicy) + policies = policyutil.SanitizePolicies(parent.Policies, policyutil.DoNotAddDefaultPolicy) // When a role is not in use or does not specify allowed/disallowed, only // permit policies to be a subset unless the client has root or sudo @@ -2956,7 +2834,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // the client specified for it not to be added. case !isSudo: // Sanitize passed-in and parent policies before comparison - sanitizedInputPolicies := policyutil.SanitizePolicies(data.Policies, policyutil.DoNotAddDefaultPolicy) + sanitizedInputPolicies := policyutil.SanitizePolicies(policies, policyutil.DoNotAddDefaultPolicy) sanitizedParentPolicies := policyutil.SanitizePolicies(parent.Policies, policyutil.DoNotAddDefaultPolicy) if !strutil.StrListSubset(sanitizedParentPolicies, sanitizedInputPolicies) { @@ -2967,19 +2845,19 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // add it. Note that if they have explicitly put "default" in // data.Policies it will still be added because NoDefaultPolicy // controls *automatic* adding. - if !data.NoDefaultPolicy && strutil.StrListContains(parent.Policies, "default") { + if !noDefaultPolicy && strutil.StrListContains(parent.Policies, "default") { addDefault = true } // Add default by default in this case unless requested not to case isSudo: - addDefault = !data.NoDefaultPolicy + addDefault = !noDefaultPolicy } - te.Policies = policyutil.SanitizePolicies(data.Policies, addDefault) + te.Policies = policyutil.SanitizePolicies(policies, addDefault) // Yes, this is a little inefficient to do it like this, but meh - if data.NoDefaultPolicy { + if noDefaultPolicy { te.Policies = strutil.StrListDelete(te.Policies, "default") } @@ -3018,7 +2896,7 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque te.BoundCIDRs = role.TokenBoundCIDRs } - case data.NoParent: + case d.Get("no_parent").(bool): // Only allow an orphan token if the client has sudo policy if !isSudo { return logical.ErrorResponse("root or sudo privileges required to create orphan token"), @@ -3055,8 +2933,8 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } var explicitMaxTTLToUse time.Duration - if data.ExplicitMaxTTL != "" { - dur, err := parseutil.ParseDurationSecond(data.ExplicitMaxTTL) + if explicitMaxTTL != "" { + dur, err := parseutil.ParseDurationSecond(explicitMaxTTL) if err != nil { return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } @@ -3068,8 +2946,8 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } var periodToUse time.Duration - if data.Period != "" { - dur, err := parseutil.ParseDurationSecond(data.Period) + if period != "" { + dur, err := parseutil.ParseDurationSecond(period) if err != nil { return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } @@ -3089,8 +2967,8 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque } // Parse the TTL/lease if any - if data.TTL != "" { - dur, err := parseutil.ParseDurationSecond(data.TTL) + if ttl := d.Get("ttl").(string); ttl != "" { + dur, err := parseutil.ParseDurationSecond(ttl) if err != nil { return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } @@ -3098,9 +2976,9 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque return logical.ErrorResponse("ttl must be positive"), logical.ErrInvalidRequest } te.TTL = dur - } else if data.Lease != "" { + } else if lease := d.Get("lease").(string); lease != "" { // This block is compatibility - dur, err := time.ParseDuration(data.Lease) + dur, err := time.ParseDuration(lease) if err != nil { return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } From 468f66bf386147002c55389b2e01516556781b22 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Thu, 29 Dec 2022 11:38:12 +0000 Subject: [PATCH 2/3] Add changelog --- changelog/18556.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/18556.txt diff --git a/changelog/18556.txt b/changelog/18556.txt new file mode 100644 index 000000000000..a48dacde5ba2 --- /dev/null +++ b/changelog/18556.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/token: Fix parsing of `auth/token/create` fields to avoid incorrect warnings about ignored parameters +``` From 38d96026416513866bc41ee09a21f952c0e71935 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Fri, 7 Jul 2023 08:05:28 +0100 Subject: [PATCH 3/3] Rephrase comment. --- vault/token_store.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index 5d880f50afd5..d25aa629df2d 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -2768,9 +2768,8 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque explicitEntityID = entity.ID } - // By using GetOk here, we set TokenEntry.Meta to nil instead of an empty map, when no meta was passed in the - // token creation API call. This maintains closer compatibility with previous behaviour of Vault, helping to avoid - // user surprise, and saving the need to update quite a few tests which expect this to be nil in such a case. + // GetOk is used here solely to preserve the distinction between an absent/nil map and an empty map, to match the + // behaviour of previous Vault versions - rather than introducing a potential slight compatibility issue for users. meta, ok := d.GetOk("meta") var metaMap map[string]string if ok {