From 3f86b6108f407944a942bc456630a3e1f3aadf94 Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Sun, 18 Oct 2020 22:42:09 -0700 Subject: [PATCH 1/6] Adds OIDC pass_namespace_in_state option --- path_config.go | 32 +++++---- path_config_test.go | 50 +++++++------- path_oidc.go | 18 +++++ path_oidc_test.go | 157 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 36 deletions(-) diff --git a/path_config.go b/path_config.go index ef6f71dd..d450dae4 100644 --- a/path_config.go +++ b/path_config.go @@ -94,6 +94,11 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { }, }, }, + "pass_namespace_in_state": { + Type: framework.TypeBool, + Description: "Pass namespace in the state parameter instead of as a separate query parameter. With this setting the allowed redirect URL in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it.", + Default: false, + }, }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -159,18 +164,19 @@ func (b *jwtAuthBackend) pathConfigRead(ctx context.Context, req *logical.Reques resp := &logical.Response{ Data: map[string]interface{}{ - "oidc_discovery_url": config.OIDCDiscoveryURL, - "oidc_discovery_ca_pem": config.OIDCDiscoveryCAPEM, - "oidc_client_id": config.OIDCClientID, - "oidc_response_mode": config.OIDCResponseMode, - "oidc_response_types": config.OIDCResponseTypes, - "default_role": config.DefaultRole, - "jwt_validation_pubkeys": config.JWTValidationPubKeys, - "jwt_supported_algs": config.JWTSupportedAlgs, - "jwks_url": config.JWKSURL, - "jwks_ca_pem": config.JWKSCAPEM, - "bound_issuer": config.BoundIssuer, - "provider_config": config.ProviderConfig, + "oidc_discovery_url": config.OIDCDiscoveryURL, + "oidc_discovery_ca_pem": config.OIDCDiscoveryCAPEM, + "oidc_client_id": config.OIDCClientID, + "oidc_response_mode": config.OIDCResponseMode, + "oidc_response_types": config.OIDCResponseTypes, + "default_role": config.DefaultRole, + "jwt_validation_pubkeys": config.JWTValidationPubKeys, + "jwt_supported_algs": config.JWTSupportedAlgs, + "jwks_url": config.JWKSURL, + "jwks_ca_pem": config.JWKSCAPEM, + "bound_issuer": config.BoundIssuer, + "provider_config": config.ProviderConfig, + "pass_namespace_in_state": config.PassNamespaceInState, }, } @@ -192,6 +198,7 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque JWTSupportedAlgs: d.Get("jwt_supported_algs").([]string), BoundIssuer: d.Get("bound_issuer").(string), ProviderConfig: d.Get("provider_config").(map[string]interface{}), + PassNamespaceInState: d.Get("pass_namespace_in_state").(bool), } // Run checks on values @@ -354,6 +361,7 @@ type jwtConfig struct { BoundIssuer string `json:"bound_issuer"` DefaultRole string `json:"default_role"` ProviderConfig map[string]interface{} `json:"provider_config"` + PassNamespaceInState bool `json:"pass_namespace_in_state"` ParsedJWTPubKeys []interface{} `json:"-"` } diff --git a/path_config_test.go b/path_config_test.go index 443aa84c..c384deb4 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -16,18 +16,19 @@ func TestConfig_JWT_Read(t *testing.T) { b, storage := getBackend(t) data := map[string]interface{}{ - "oidc_discovery_url": "", - "oidc_discovery_ca_pem": "", - "oidc_client_id": "", - "oidc_response_mode": "", - "oidc_response_types": []string{}, - "default_role": "", - "jwt_validation_pubkeys": []string{testJWTPubKey}, - "jwt_supported_algs": []string{}, - "jwks_url": "", - "jwks_ca_pem": "", - "bound_issuer": "http://vault.example.com/", - "provider_config": map[string]interface{}{}, + "oidc_discovery_url": "", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "", + "oidc_response_mode": "", + "oidc_response_types": []string{}, + "default_role": "", + "jwt_validation_pubkeys": []string{testJWTPubKey}, + "jwt_supported_algs": []string{}, + "jwks_url": "", + "jwks_ca_pem": "", + "bound_issuer": "http://vault.example.com/", + "provider_config": map[string]interface{}{}, + "pass_namespace_in_state": false, } req := &logical.Request{ @@ -160,18 +161,19 @@ func TestConfig_JWKS_Update(t *testing.T) { } data := map[string]interface{}{ - "jwks_url": s.server.URL + "/certs", - "jwks_ca_pem": cert, - "oidc_discovery_url": "", - "oidc_discovery_ca_pem": "", - "oidc_client_id": "", - "oidc_response_mode": "form_post", - "oidc_response_types": []string{}, - "default_role": "", - "jwt_validation_pubkeys": []string{}, - "jwt_supported_algs": []string{}, - "bound_issuer": "", - "provider_config": map[string]interface{}{}, + "jwks_url": s.server.URL + "/certs", + "jwks_ca_pem": cert, + "oidc_discovery_url": "", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "", + "oidc_response_mode": "form_post", + "oidc_response_types": []string{}, + "default_role": "", + "jwt_validation_pubkeys": []string{}, + "jwt_supported_algs": []string{}, + "bound_issuer": "", + "provider_config": map[string]interface{}{}, + "pass_namespace_in_state": false, } req := &logical.Request{ diff --git a/path_oidc.go b/path_oidc.go index 36345600..a47822e7 100644 --- a/path_oidc.go +++ b/path_oidc.go @@ -371,6 +371,20 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f return logical.ErrorResponse("role %q could not be found", roleName), nil } + // If namespace will be passed around in state, don't store it in redirect_uri + namespace := "" + if config.PassNamespaceInState { + inputURI, err := url.Parse(redirectURI) + if err != nil { + return resp, nil + } + qParam := inputURI.Query() + namespace = qParam.Get("namespace") + qParam.Del("namespace") + inputURI.RawQuery = qParam.Encode() + redirectURI = inputURI.String() + } + if !validRedirect(redirectURI, role.AllowedRedirectURIs) { logger.Warn("unauthorized redirect_uri", "redirect_uri", redirectURI) return resp, nil @@ -408,6 +422,10 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f logger.Warn("error generating OAuth state", "error", err) return resp, nil } + if config.PassNamespaceInState && len(namespace) > 0 { + // embed namespace in state in the auth_url + stateID = fmt.Sprintf("%s,ns=%s", stateID, namespace) + } authCodeOpts := []oauth2.AuthCodeOption{ oidc.Nonce(nonce), diff --git a/path_oidc_test.go b/path_oidc_test.go index fafc1321..130ed331 100644 --- a/path_oidc_test.go +++ b/path_oidc_test.go @@ -208,6 +208,163 @@ func TestOIDC_AuthURL(t *testing.T) { }) } +func TestOIDC_AuthURL_namespace(t *testing.T) { + + type testCase struct { + passNamespaceInState string + allowedRedirectURIs []string + incomingRedirectURI string + expectedStateRegEx string + expectedRedirectURI string + expectFail bool + } + + tests := map[string]testCase{ + "namespace as query parameter": { + passNamespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com?namespace=test"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com?namespace=test`, + }, + "namespace as query parameter, bad allowed redirect": { + passNamespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com?namespace=test`, + expectFail: true, + }, + "namespace in state": { + passNamespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27},ns=test`, + expectedRedirectURI: `https://example.com`, + }, + "namespace in state, bad allowed redirect": { + passNamespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com?namespace=test"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectFail: true, + }, + "nested namespace in state": { + passNamespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=org4321/dev", + expectedStateRegEx: `\w{27},ns=org4321/dev`, + expectedRedirectURI: `https://example.com`, + }, + "namespace as query parameter, no namespaces": { + passNamespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com`, + }, + "namespace in state, no namespaces": { + passNamespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com`, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + b, storage := getBackend(t) + + // Configure backend + data := map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "abc", + "oidc_client_secret": "def", + "default_role": "test", + "bound_issuer": "http://vault.example.com/", + "pass_namespace_in_state": test.passNamespaceInState, + } + + // basic configuration + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: storage, + Data: data, + } + + resp, err := b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v\n", err, resp) + } + + // set up test role + rolePayload := map[string]interface{}{ + "user_claim": "email", + "bound_audiences": "vault", + "allowed_redirect_uris": test.allowedRedirectURIs, + } + + req = &logical.Request{ + Operation: logical.CreateOperation, + Path: "role/test", + Storage: storage, + Data: rolePayload, + } + + resp, err = b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v\n", err, resp) + } + + authURLPayload := map[string]interface{}{ + "role": "test", + "redirect_uri": test.incomingRedirectURI, + } + req = &logical.Request{ + Operation: logical.UpdateOperation, + Path: "oidc/auth_url", + Storage: storage, + Data: authURLPayload, + } + + resp, err = b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v\n", err, resp) + } + + rawAuthURL := resp.Data["auth_url"].(string) + if test.expectFail && len(rawAuthURL) > 0 { + t.Fatalf("Expected auth_url to fail (empty), but got %s", rawAuthURL) + } + if test.expectFail && len(rawAuthURL) == 0 { + return + } + + authURL, err := url.Parse(rawAuthURL) + if err != nil { + t.Fatal(err) + } + qParams := authURL.Query() + redirectURI := qParams.Get("redirect_uri") + if test.expectedRedirectURI != redirectURI { + t.Fatalf("expected redirect_uri to match: %s, %s", test.expectedRedirectURI, redirectURI) + } + + state := qParams.Get("state") + matchState, err := regexp.MatchString(test.expectedStateRegEx, state) + if err != nil { + t.Fatal(err) + } + if !matchState { + t.Fatalf("expected state to match regex: %s, %s", test.expectedStateRegEx, state) + } + + }) + } +} + func TestOIDC_Callback(t *testing.T) { t.Run("successful login", func(t *testing.T) { From bbfa423bfae5143a280ceede8feb711ff5108f6b Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Mon, 19 Oct 2020 14:39:37 -0700 Subject: [PATCH 2/6] pass_namespace_in_state -> namespace_in_state --- path_config.go | 32 +++++++-------- path_config_test.go | 52 ++++++++++++------------ path_oidc.go | 4 +- path_oidc_test.go | 96 ++++++++++++++++++++++----------------------- 4 files changed, 92 insertions(+), 92 deletions(-) diff --git a/path_config.go b/path_config.go index d450dae4..af0dc0f6 100644 --- a/path_config.go +++ b/path_config.go @@ -94,7 +94,7 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { }, }, }, - "pass_namespace_in_state": { + "namespace_in_state": { Type: framework.TypeBool, Description: "Pass namespace in the state parameter instead of as a separate query parameter. With this setting the allowed redirect URL in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it.", Default: false, @@ -164,19 +164,19 @@ func (b *jwtAuthBackend) pathConfigRead(ctx context.Context, req *logical.Reques resp := &logical.Response{ Data: map[string]interface{}{ - "oidc_discovery_url": config.OIDCDiscoveryURL, - "oidc_discovery_ca_pem": config.OIDCDiscoveryCAPEM, - "oidc_client_id": config.OIDCClientID, - "oidc_response_mode": config.OIDCResponseMode, - "oidc_response_types": config.OIDCResponseTypes, - "default_role": config.DefaultRole, - "jwt_validation_pubkeys": config.JWTValidationPubKeys, - "jwt_supported_algs": config.JWTSupportedAlgs, - "jwks_url": config.JWKSURL, - "jwks_ca_pem": config.JWKSCAPEM, - "bound_issuer": config.BoundIssuer, - "provider_config": config.ProviderConfig, - "pass_namespace_in_state": config.PassNamespaceInState, + "oidc_discovery_url": config.OIDCDiscoveryURL, + "oidc_discovery_ca_pem": config.OIDCDiscoveryCAPEM, + "oidc_client_id": config.OIDCClientID, + "oidc_response_mode": config.OIDCResponseMode, + "oidc_response_types": config.OIDCResponseTypes, + "default_role": config.DefaultRole, + "jwt_validation_pubkeys": config.JWTValidationPubKeys, + "jwt_supported_algs": config.JWTSupportedAlgs, + "jwks_url": config.JWKSURL, + "jwks_ca_pem": config.JWKSCAPEM, + "bound_issuer": config.BoundIssuer, + "provider_config": config.ProviderConfig, + "namespace_in_state": config.NamespaceInState, }, } @@ -198,7 +198,7 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque JWTSupportedAlgs: d.Get("jwt_supported_algs").([]string), BoundIssuer: d.Get("bound_issuer").(string), ProviderConfig: d.Get("provider_config").(map[string]interface{}), - PassNamespaceInState: d.Get("pass_namespace_in_state").(bool), + NamespaceInState: d.Get("namespace_in_state").(bool), } // Run checks on values @@ -361,7 +361,7 @@ type jwtConfig struct { BoundIssuer string `json:"bound_issuer"` DefaultRole string `json:"default_role"` ProviderConfig map[string]interface{} `json:"provider_config"` - PassNamespaceInState bool `json:"pass_namespace_in_state"` + NamespaceInState bool `json:"namespace_in_state"` ParsedJWTPubKeys []interface{} `json:"-"` } diff --git a/path_config_test.go b/path_config_test.go index c384deb4..95f409bb 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -16,19 +16,19 @@ func TestConfig_JWT_Read(t *testing.T) { b, storage := getBackend(t) data := map[string]interface{}{ - "oidc_discovery_url": "", - "oidc_discovery_ca_pem": "", - "oidc_client_id": "", - "oidc_response_mode": "", - "oidc_response_types": []string{}, - "default_role": "", - "jwt_validation_pubkeys": []string{testJWTPubKey}, - "jwt_supported_algs": []string{}, - "jwks_url": "", - "jwks_ca_pem": "", - "bound_issuer": "http://vault.example.com/", - "provider_config": map[string]interface{}{}, - "pass_namespace_in_state": false, + "oidc_discovery_url": "", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "", + "oidc_response_mode": "", + "oidc_response_types": []string{}, + "default_role": "", + "jwt_validation_pubkeys": []string{testJWTPubKey}, + "jwt_supported_algs": []string{}, + "jwks_url": "", + "jwks_ca_pem": "", + "bound_issuer": "http://vault.example.com/", + "provider_config": map[string]interface{}{}, + "namespace_in_state": false, } req := &logical.Request{ @@ -161,19 +161,19 @@ func TestConfig_JWKS_Update(t *testing.T) { } data := map[string]interface{}{ - "jwks_url": s.server.URL + "/certs", - "jwks_ca_pem": cert, - "oidc_discovery_url": "", - "oidc_discovery_ca_pem": "", - "oidc_client_id": "", - "oidc_response_mode": "form_post", - "oidc_response_types": []string{}, - "default_role": "", - "jwt_validation_pubkeys": []string{}, - "jwt_supported_algs": []string{}, - "bound_issuer": "", - "provider_config": map[string]interface{}{}, - "pass_namespace_in_state": false, + "jwks_url": s.server.URL + "/certs", + "jwks_ca_pem": cert, + "oidc_discovery_url": "", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "", + "oidc_response_mode": "form_post", + "oidc_response_types": []string{}, + "default_role": "", + "jwt_validation_pubkeys": []string{}, + "jwt_supported_algs": []string{}, + "bound_issuer": "", + "provider_config": map[string]interface{}{}, + "namespace_in_state": false, } req := &logical.Request{ diff --git a/path_oidc.go b/path_oidc.go index a47822e7..daa56d4d 100644 --- a/path_oidc.go +++ b/path_oidc.go @@ -373,7 +373,7 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f // If namespace will be passed around in state, don't store it in redirect_uri namespace := "" - if config.PassNamespaceInState { + if config.NamespaceInState { inputURI, err := url.Parse(redirectURI) if err != nil { return resp, nil @@ -422,7 +422,7 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f logger.Warn("error generating OAuth state", "error", err) return resp, nil } - if config.PassNamespaceInState && len(namespace) > 0 { + if config.NamespaceInState && len(namespace) > 0 { // embed namespace in state in the auth_url stateID = fmt.Sprintf("%s,ns=%s", stateID, namespace) } diff --git a/path_oidc_test.go b/path_oidc_test.go index 130ed331..2bb8f2f0 100644 --- a/path_oidc_test.go +++ b/path_oidc_test.go @@ -211,63 +211,63 @@ func TestOIDC_AuthURL(t *testing.T) { func TestOIDC_AuthURL_namespace(t *testing.T) { type testCase struct { - passNamespaceInState string - allowedRedirectURIs []string - incomingRedirectURI string - expectedStateRegEx string - expectedRedirectURI string - expectFail bool + namespaceInState string + allowedRedirectURIs []string + incomingRedirectURI string + expectedStateRegEx string + expectedRedirectURI string + expectFail bool } tests := map[string]testCase{ "namespace as query parameter": { - passNamespaceInState: "false", - allowedRedirectURIs: []string{"https://example.com?namespace=test"}, - incomingRedirectURI: "https://example.com?namespace=test", - expectedStateRegEx: `\w{27}`, - expectedRedirectURI: `https://example.com?namespace=test`, + namespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com?namespace=test"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com?namespace=test`, }, "namespace as query parameter, bad allowed redirect": { - passNamespaceInState: "false", - allowedRedirectURIs: []string{"https://example.com"}, - incomingRedirectURI: "https://example.com?namespace=test", - expectedStateRegEx: `\w{27}`, - expectedRedirectURI: `https://example.com?namespace=test`, - expectFail: true, + namespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com?namespace=test`, + expectFail: true, }, "namespace in state": { - passNamespaceInState: "true", - allowedRedirectURIs: []string{"https://example.com"}, - incomingRedirectURI: "https://example.com?namespace=test", - expectedStateRegEx: `\w{27},ns=test`, - expectedRedirectURI: `https://example.com`, + namespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectedStateRegEx: `\w{27},ns=test`, + expectedRedirectURI: `https://example.com`, }, "namespace in state, bad allowed redirect": { - passNamespaceInState: "true", - allowedRedirectURIs: []string{"https://example.com?namespace=test"}, - incomingRedirectURI: "https://example.com?namespace=test", - expectFail: true, + namespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com?namespace=test"}, + incomingRedirectURI: "https://example.com?namespace=test", + expectFail: true, }, "nested namespace in state": { - passNamespaceInState: "true", - allowedRedirectURIs: []string{"https://example.com"}, - incomingRedirectURI: "https://example.com?namespace=org4321/dev", - expectedStateRegEx: `\w{27},ns=org4321/dev`, - expectedRedirectURI: `https://example.com`, + namespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com?namespace=org4321/dev", + expectedStateRegEx: `\w{27},ns=org4321/dev`, + expectedRedirectURI: `https://example.com`, }, "namespace as query parameter, no namespaces": { - passNamespaceInState: "false", - allowedRedirectURIs: []string{"https://example.com"}, - incomingRedirectURI: "https://example.com", - expectedStateRegEx: `\w{27}`, - expectedRedirectURI: `https://example.com`, + namespaceInState: "false", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com`, }, "namespace in state, no namespaces": { - passNamespaceInState: "true", - allowedRedirectURIs: []string{"https://example.com"}, - incomingRedirectURI: "https://example.com", - expectedStateRegEx: `\w{27}`, - expectedRedirectURI: `https://example.com`, + namespaceInState: "true", + allowedRedirectURIs: []string{"https://example.com"}, + incomingRedirectURI: "https://example.com", + expectedStateRegEx: `\w{27}`, + expectedRedirectURI: `https://example.com`, }, } @@ -277,13 +277,13 @@ func TestOIDC_AuthURL_namespace(t *testing.T) { // Configure backend data := map[string]interface{}{ - "oidc_discovery_url": "https://team-vault.auth0.com/", - "oidc_discovery_ca_pem": "", - "oidc_client_id": "abc", - "oidc_client_secret": "def", - "default_role": "test", - "bound_issuer": "http://vault.example.com/", - "pass_namespace_in_state": test.passNamespaceInState, + "oidc_discovery_url": "https://team-vault.auth0.com/", + "oidc_discovery_ca_pem": "", + "oidc_client_id": "abc", + "oidc_client_secret": "def", + "default_role": "test", + "bound_issuer": "http://vault.example.com/", + "namespace_in_state": test.namespaceInState, } // basic configuration From b0211022c9a857e6bb97844d9d9c4653e36b48bd Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Wed, 21 Oct 2020 09:20:48 -0700 Subject: [PATCH 3/6] Only attempt to remove namespace if present Checks whether there actually was a namespace query parameter before removing it and re-encoding the remaining query parameters. --- path_oidc.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/path_oidc.go b/path_oidc.go index daa56d4d..88cd95c4 100644 --- a/path_oidc.go +++ b/path_oidc.go @@ -371,7 +371,9 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f return logical.ErrorResponse("role %q could not be found", roleName), nil } - // If namespace will be passed around in state, don't store it in redirect_uri + // If namespace will be passed around in state, and it has been provided as + // a redirectURI query parameter, remove it from redirectURI, and append it + // to the state (later in this function) namespace := "" if config.NamespaceInState { inputURI, err := url.Parse(redirectURI) @@ -380,9 +382,11 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f } qParam := inputURI.Query() namespace = qParam.Get("namespace") - qParam.Del("namespace") - inputURI.RawQuery = qParam.Encode() - redirectURI = inputURI.String() + if len(namespace) > 0 { + qParam.Del("namespace") + inputURI.RawQuery = qParam.Encode() + redirectURI = inputURI.String() + } } if !validRedirect(redirectURI, role.AllowedRedirectURIs) { From fc3439a2a5cdef147b29a19221028546374eb4bf Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Tue, 27 Oct 2020 14:14:26 -0700 Subject: [PATCH 4/6] default namespace_in_state to true --- path_config.go | 6 +++++- path_config_test.go | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/path_config.go b/path_config.go index 8b5117e3..5c3d4a1e 100644 --- a/path_config.go +++ b/path_config.go @@ -92,7 +92,11 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { "namespace_in_state": { Type: framework.TypeBool, Description: "Pass namespace in the state parameter instead of as a separate query parameter. With this setting the allowed redirect URL in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it.", - Default: false, + Default: true, + DisplayAttrs: &framework.DisplayAttributes{ + Name: "Namespace in state", + Value: true, + }, }, }, diff --git a/path_config_test.go b/path_config_test.go index 95f409bb..a10d007c 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -137,6 +137,7 @@ func TestConfig_JWT_Write(t *testing.T) { OIDCResponseTypes: []string{}, BoundIssuer: "http://vault.example.com/", ProviderConfig: map[string]interface{}{}, + NamespaceInState: true, } conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) @@ -344,6 +345,7 @@ func TestConfig_OIDC_Write(t *testing.T) { OIDCResponseTypes: []string{}, OIDCDiscoveryURL: "https://team-vault.auth0.com/", ProviderConfig: map[string]interface{}{}, + NamespaceInState: true, } conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) @@ -435,6 +437,7 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) { "provider": "azure", "extraOptions": "abound", }, + NamespaceInState: true, } conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) @@ -491,6 +494,7 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) { OIDCResponseTypes: []string{}, OIDCDiscoveryURL: "https://team-vault.auth0.com/", ProviderConfig: map[string]interface{}{}, + NamespaceInState: true, } conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) From 4ed5ad52f129bbcb7b0772d667c8edf3279a3be7 Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Wed, 28 Oct 2020 14:54:20 -0700 Subject: [PATCH 5/6] default to true only on new config write --- path_config.go | 23 ++++-- path_config_test.go | 182 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 5 deletions(-) diff --git a/path_config.go b/path_config.go index 5c3d4a1e..312a14d2 100644 --- a/path_config.go +++ b/path_config.go @@ -91,11 +91,9 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { }, "namespace_in_state": { Type: framework.TypeBool, - Description: "Pass namespace in the state parameter instead of as a separate query parameter. With this setting the allowed redirect URL in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it.", - Default: true, + Description: "Pass namespace in the OIDC state parameter instead of as a separate query parameter. With this setting, the allowed redirect URL(s) in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it. Defaults to true for new configs.", DisplayAttrs: &framework.DisplayAttributes{ - Name: "Namespace in state", - Value: true, + Name: "Namespace in OIDC state", }, }, }, @@ -197,7 +195,22 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque JWTSupportedAlgs: d.Get("jwt_supported_algs").([]string), BoundIssuer: d.Get("bound_issuer").(string), ProviderConfig: d.Get("provider_config").(map[string]interface{}), - NamespaceInState: d.Get("namespace_in_state").(bool), + } + + // Check if the config already exists, to determine if this is a create or + // an update + existingConfig, err := b.config(ctx, req.Storage) + if err != nil { + return nil, err + } + if nsInState, ok := d.GetOk("namespace_in_state"); ok { + config.NamespaceInState = nsInState.(bool) + } else if existingConfig == nil { + // new configs default to true + config.NamespaceInState = true + } else { + // maintain the existing value + config.NamespaceInState = existingConfig.NamespaceInState } // Run checks on values diff --git a/path_config_test.go b/path_config_test.go index a10d007c..45730a1d 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -508,6 +508,188 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) { }) } +func TestConfig_OIDC_Create_Namespace(t *testing.T) { + type testCase struct { + create map[string]interface{} + expected jwtConfig + } + tests := map[string]testCase{ + "namespace_in_state not specified": { + create: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: true, + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + "namespace_in_state true": { + create: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": true, + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: true, + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + "namespace_in_state false": { + create: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": false, + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: false, + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + b, storage := getBackend(t) + + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: storage, + Data: test.create, + } + resp, err := b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) + assert.NoError(t, err) + assert.Equal(t, &test.expected, conf) + }) + } + +} + +func TestConfig_OIDC_Update_Namespace(t *testing.T) { + type testCase struct { + existing map[string]interface{} + update map[string]interface{} + expected jwtConfig + } + tests := map[string]testCase{ + "existing false, update to true": { + existing: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": false, + }, + update: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": true, + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: true, + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + "existing false, update something else": { + existing: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": false, + }, + update: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "default_role": "ui", + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: false, + DefaultRole: "ui", + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + "existing true, update to false": { + existing: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": true, + }, + update: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": false, + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: false, + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + "existing true, update something else": { + existing: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "namespace_in_state": true, + }, + update: map[string]interface{}{ + "oidc_discovery_url": "https://team-vault.auth0.com/", + "default_role": "ui", + }, + expected: jwtConfig{ + OIDCDiscoveryURL: "https://team-vault.auth0.com/", + NamespaceInState: true, + DefaultRole: "ui", + OIDCResponseTypes: []string{}, + JWTSupportedAlgs: []string{}, + JWTValidationPubKeys: []string{}, + ProviderConfig: map[string]interface{}{}, + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + b, storage := getBackend(t) + + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: storage, + Data: test.existing, + } + resp, err := b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + req.Data = test.update + resp, err = b.HandleRequest(context.Background(), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + conf, err := b.(*jwtAuthBackend).config(context.Background(), storage) + assert.NoError(t, err) + assert.Equal(t, &test.expected, conf) + }) + } + +} + const ( testJWTPubKey = `-----BEGIN PUBLIC KEY----- MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEEVs/o5+uQbTjL3chynL4wXgUg2R9 From 58a98414d080de5949cc6645ae16ccef3333d1f2 Mon Sep 17 00:00:00 2001 From: Theron Voran Date: Wed, 28 Oct 2020 16:39:27 -0700 Subject: [PATCH 6/6] default to true in UI and extra comment --- path_config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/path_config.go b/path_config.go index 312a14d2..0b3c2682 100644 --- a/path_config.go +++ b/path_config.go @@ -93,7 +93,8 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { Type: framework.TypeBool, Description: "Pass namespace in the OIDC state parameter instead of as a separate query parameter. With this setting, the allowed redirect URL(s) in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it. Defaults to true for new configs.", DisplayAttrs: &framework.DisplayAttributes{ - Name: "Namespace in OIDC state", + Name: "Namespace in OIDC state", + Value: true, }, }, }, @@ -198,7 +199,8 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque } // Check if the config already exists, to determine if this is a create or - // an update + // an update, since req.Operation is always 'update' in this handler, and + // there's no existence check defined. existingConfig, err := b.config(ctx, req.Storage) if err != nil { return nil, err