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.

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

identity/oidc: allow filtering the list providers response by an allowed_client_id #16181

Merged
merged 4 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions http/logical.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ func buildLogicalRequestNoAuth(perfStandby bool, w http.ResponseWriter, r *http.
path += "/"
}

data = parseQuery(r.URL.Query())
Copy link
Contributor Author

@austingebauer austingebauer Jun 29, 2022

Choose a reason for hiding this comment

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

This passes the query parameters to backends for LIST operations. I don't see why we couldn't do this, but perhaps there is some context I'm missing 🤔

If we don't want to allow query parameters for LIST operations, I'm okay changing this to use a GET.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically the "GET" verb can also translate to a logical.ListOperation, but that case omits parsing and passing in the rest of the query parameters into the logical.Request.Data.

Should we also consider how to handle query params there if we make it available here? Would there be any backwards compatibility concerns if so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a good callout. I can't think of any compatibility concerns. The "GET" with ?list=true would result in a ListOperation handler being called, and it would be up to that handler implementation to parse data from query parameters. I don't think any ListOperation handlers are parsing parameters today, so I can't think of a concern.

That said, I'm okay with just allowing query parameters for "LIST" at this point. It's a small change, and it makes sense to use query parameters to filter lists. If users want consistent behavior with the GET-style list, we can always address it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the value in keeping the behavior of GET with ?list=true and LIST the same in regards to query params. But I agree that it is a small change and we could wait to see if users want this available for GET ?list=true.


case "OPTIONS", "HEAD":
default:
return nil, nil, http.StatusMethodNotAllowed, nil
Expand Down
58 changes: 52 additions & 6 deletions vault/identity_store_oidc_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,19 @@ type provider struct {
effectiveIssuer string
}

// allowedClientID returns true if the given client ID is in
// the provider's set of allowed client IDs or its allowed client
// IDs contains the wildcard "*" char.
func (p *provider) allowedClientID(clientID string) bool {
for _, allowedID := range p.AllowedClientIDs {
switch allowedID {
case "*", clientID:
return true
}
}
return false
}

type providerDiscovery struct {
Issuer string `json:"issuer"`
Keys string `json:"jwks_uri"`
Expand Down Expand Up @@ -356,6 +369,14 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path {
},
{
Pattern: "oidc/provider/?$",
Fields: map[string]*framework.FieldSchema{
"allowed_client_id": {
Type: framework.TypeString,
austingebauer marked this conversation as resolved.
Show resolved Hide resolved
Description: "Filters the set of returned OIDC providers to those " +
"that allow the given value in their set of allowed_client_ids.",
Query: true,
},
},
Operations: map[logical.Operation]framework.OperationHandler{
logical.ListOperation: &framework.PathOperation{
Callback: i.pathOIDCListProvider,
Expand Down Expand Up @@ -1301,6 +1322,34 @@ func (i *IdentityStore) pathOIDCListProvider(ctx context.Context, req *logical.R
if err != nil {
return nil, err
}

// If allowed_client_id is provided as a query parameter, filter the set of
// returned OIDC providers to those that allow the given value in their set
// of allowed_client_ids.
if clientIDRaw, ok := d.GetOk("allowed_client_id"); ok {
clientID := clientIDRaw.(string)
if clientID == "" {
return logical.ListResponse(providers), nil
}

filtered := make([]string, 0)
for _, name := range providers {
provider, err := i.getOIDCProvider(ctx, req.Storage, name)
if err != nil {
return nil, err
}
if provider == nil {
continue
}

if provider.allowedClientID(clientID) {
filtered = append(filtered, name)
}
}

providers = filtered
}

return logical.ListResponse(providers), nil
}

Expand Down Expand Up @@ -1592,8 +1641,7 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ
if client == nil {
return authResponse("", state, ErrAuthInvalidClientID, "client with client_id not found")
}
if !strutil.StrListContains(provider.AllowedClientIDs, "*") &&
!strutil.StrListContains(provider.AllowedClientIDs, clientID) {
if !provider.allowedClientID(clientID) {
return authResponse("", state, ErrAuthUnauthorizedClient, "client is not authorized to use the provider")
}

Expand Down Expand Up @@ -1812,8 +1860,7 @@ func (i *IdentityStore) pathOIDCToken(ctx context.Context, req *logical.Request,
}

// Validate that the client is authorized to use the provider
if !strutil.StrListContains(provider.AllowedClientIDs, "*") &&
!strutil.StrListContains(provider.AllowedClientIDs, clientID) {
if !provider.allowedClientID(clientID) {
return tokenResponse(nil, ErrTokenInvalidClient, "client is not authorized to use the provider")
}

Expand Down Expand Up @@ -2133,8 +2180,7 @@ func (i *IdentityStore) pathOIDCUserInfo(ctx context.Context, req *logical.Reque
}

// Validate that the client is authorized to use the provider
if !strutil.StrListContains(provider.AllowedClientIDs, "*") &&
!strutil.StrListContains(provider.AllowedClientIDs, clientID) {
if !provider.allowedClientID(clientID) {
return userInfoResponse(nil, ErrUserInfoAccessDenied, "client is not authorized to use the provider")
}

Expand Down
84 changes: 84 additions & 0 deletions vault/identity_store_oidc_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3350,6 +3350,90 @@ func TestOIDC_Path_OIDC_Provider_List(t *testing.T) {
expectStrings(t, respListProvidersAfterDelete.Data["keys"].([]string), expectedStrings)
}

func TestOIDC_Path_OIDC_Provider_List_Filter(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
ctx := namespace.RootContext(nil)
s := new(logical.InmemStorage)

// Create providers with different allowed_client_ids values
providers := []struct {
name string
allowedClientIDs []string
}{
{name: "p0", allowedClientIDs: []string{"*"}},
{name: "p1", allowedClientIDs: []string{"abc"}},
{name: "p2", allowedClientIDs: []string{"abc", "def"}},
{name: "p3", allowedClientIDs: []string{"abc", "def", "ghi"}},
{name: "p4", allowedClientIDs: []string{"ghi"}},
{name: "p5", allowedClientIDs: []string{"jkl"}},
}
for _, p := range providers {
resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/provider/" + p.name,
Operation: logical.CreateOperation,
Storage: s,
Data: map[string]interface{}{
"allowed_client_ids": p.allowedClientIDs,
},
})
expectSuccess(t, resp, err)
}

tests := []struct {
name string
clientIDFilter string
expectedProviders []string
}{
{
name: "list providers with client_id filter subset 1",
clientIDFilter: "abc",
expectedProviders: []string{"p0", "p1", "p2", "p3"},
austingebauer marked this conversation as resolved.
Show resolved Hide resolved
},
{
name: "list providers with client_id filter subset 2",
clientIDFilter: "def",
expectedProviders: []string{"p0", "p2", "p3"},
},
{
name: "list providers with client_id filter subset 3",
clientIDFilter: "ghi",
expectedProviders: []string{"p0", "p3", "p4"},
},
{
name: "list providers with client_id filter subset 4",
clientIDFilter: "jkl",
expectedProviders: []string{"p0", "p5"},
},
{
name: "list providers with client_id filter only matching glob",
clientIDFilter: "globmatch_only",
expectedProviders: []string{"p0"},
},
{
name: "list providers with empty client_id filter returns all",
clientIDFilter: "",
expectedProviders: []string{"p0", "p1", "p2", "p3", "p4", "p5"},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// List providers with the allowed_client_id query parameter
resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
Path: "oidc/provider",
Operation: logical.ListOperation,
Storage: s,
Data: map[string]interface{}{
"allowed_client_id": tc.clientIDFilter,
},
})
expectSuccess(t, resp, err)

// Assert the filtered set of providers is returned
require.Equal(t, tc.expectedProviders, resp.Data["keys"])
})
}
}

// TestOIDC_Path_OpenIDProviderConfig tests read operations for the
// openid-configuration path
func TestOIDC_Path_OpenIDProviderConfig(t *testing.T) {
Expand Down