From 55254d164a303eb7aec8f6f7e595d75ce40bbee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yunier=20Rojas=20Garc=C3=ADa?= Date: Sat, 2 Mar 2024 21:21:18 +0100 Subject: [PATCH 1/3] feat: add `delegate` authenticator ## Problem Summary This merge request addresses issue !669 surrounding the behavior of the "noop" authentication method, which underwent changes in commit 6f8ab4f7d. Reverting these changes to restore the previous behavior is challenging due to the potential for breaking existing systems. To mitigate this risk, we propose implementing a new authenticator named "delegate" to replicate the original behavior of the "noop" method. ## Ideal Solution To address this issue, our proposed solution is to implement a new authenticator named "delegate" that replicates the original behavior of the "noop" method. This approach ensures that existing systems in production remain stable and unaffected by changes, while also providing users who prefer the old behavior with an option to utilize it. By introducing the "delegate" authenticator, we mitigate the risk of breaking changes while offering flexibility to users who require the previous behavior. ## Changes Proposed 1. **New Authenticator Module**: This MR adds a new authenticator module named "delegate" to replicate the original behavior of the "noop" method. 3. **Integration Tests**: Integration tests will be added to validate the functionality of the "delegate" authenticator, ensuring compatibility and reliability. 4. **Documentation Updates**: Documentation will be updated to include details about the new "delegate" authenticator, its configuration options, and usage examples. ## Related Issues https://github.com/ory/oathkeeper/issues/1152 https://github.com/ory/oathkeeper/issues/669 closes 1152 --- .schema/config.schema.json | 11 +++ .schemas/authenticators.delegate.schema.json | 9 +++ .schemas/config.schema.json | 11 +++ api/decision_test.go | 52 ++++++++++++++ api/health_test.go | 1 + cmd/root_test.go | 4 ++ driver/configuration/config_keys.go | 3 + .../provider_koanf_public_test.go | 7 ++ driver/registry_memory.go | 1 + internal/config/.oathkeeper.yaml | 5 ++ middleware/grpc_middleware_test.go | 2 + pipeline/authn/authenticator.go | 1 + pipeline/authn/authenticator_delegate.go | 39 +++++++++++ pipeline/authn/authenticator_delegate_test.go | 37 ++++++++++ proxy/proxy_test.go | 67 +++++++++++++++++++ proxy/request_handler.go | 8 +-- proxy/request_handler_test.go | 34 ++++++++++ spec/config.schema.json | 18 +++++ .../authenticators.delegate.schema.json | 5 ++ 19 files changed, 311 insertions(+), 4 deletions(-) create mode 100644 .schemas/authenticators.delegate.schema.json create mode 100644 pipeline/authn/authenticator_delegate.go create mode 100644 pipeline/authn/authenticator_delegate_test.go create mode 100644 spec/pipeline/authenticators.delegate.schema.json diff --git a/.schema/config.schema.json b/.schema/config.schema.json index 4dcd8a8032..006440bca2 100644 --- a/.schema/config.schema.json +++ b/.schema/config.schema.json @@ -1301,6 +1301,17 @@ } } }, + "delegate": { + "title": "Delegate Operation (delegate)", + "description": "The [`delegate` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#delegate).", + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { + "$ref": "#/definitions/handlerSwitch" + } + } + }, "unauthorized": { "title": "Unauthorized", "description": "The [`unauthorized` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#unauthorized).", diff --git a/.schemas/authenticators.delegate.schema.json b/.schemas/authenticators.delegate.schema.json new file mode 100644 index 0000000000..3baf653f6d --- /dev/null +++ b/.schemas/authenticators.delegate.schema.json @@ -0,0 +1,9 @@ +{ + "$id": "https://raw.githubusercontent.com/ory/oathkeeper/master/.schemas/authenticators.delegate.schema.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "title": "Delegate Authenticator Configuration", + "description": "This section is optional when the authenticator is disabled.", + "properties": {}, + "additionalProperties": false +} diff --git a/.schemas/config.schema.json b/.schemas/config.schema.json index 216788c8cd..2bd9b5f471 100644 --- a/.schemas/config.schema.json +++ b/.schemas/config.schema.json @@ -1009,6 +1009,17 @@ } } }, + "delegate": { + "title": "Delegate Operation (delegate)", + "description": "The [`delegate` authenticator](https://www.ory.sh/docs/oathkeeper/pipeline/authn#delegate).", + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { + "$ref": "#/definitions/handlerSwitch" + } + } + }, "unauthorized": { "title": "Unauthorized", "description": "The [`unauthorized` authenticator](https://www.ory.sh/docs/oathkeeper/pipeline/authn#unauthorized).", diff --git a/api/decision_test.go b/api/decision_test.go index d4c6d07236..f8e5f2f722 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -35,6 +35,7 @@ import ( func TestDecisionAPI(t *testing.T) { conf := internal.NewConfigurationWithDefaults() conf.SetForTest(t, configuration.AuthenticatorNoopIsEnabled, true) + conf.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorUnauthorizedIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorAnonymousIsEnabled, true) conf.SetForTest(t, configuration.AuthorizerAllowIsEnabled, true) @@ -81,6 +82,36 @@ func TestDecisionAPI(t *testing.T) { Upstream: rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true}, } + ruleDelegateAuthenticator := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-delegate/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: ""}, + } + ruleDelegateAuthenticatorModifyUpstream := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-delegate/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true}, + } + + ruleDelegateAuthenticatorGLOB := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-delegate/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: ""}, + } + ruleDelegateAuthenticatorModifyUpstreamGLOB := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-delegate/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true}, + } + for k, tc := range []struct { url string code int @@ -299,6 +330,27 @@ func TestDecisionAPI(t *testing.T) { code: http.StatusOK, authz: "", }, + { + d: "should fail because url does exist but is matched by two rulesRegexp", + url: ts.URL + "/decisions" + "/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticator, ruleDelegateAuthenticator}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorGLOB, ruleDelegateAuthenticatorGLOB}, + code: http.StatusInternalServerError, + }, + { + d: "should pass", + url: ts.URL + "/decisions" + "/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticator}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorGLOB}, + code: http.StatusOK, + }, + { + d: "should pass", + url: ts.URL + "/decisions" + "/strip-path/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticatorModifyUpstream}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorModifyUpstreamGLOB}, + code: http.StatusOK, + }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { testFunc := func(strategy configuration.MatchingStrategy) { diff --git a/api/health_test.go b/api/health_test.go index 2b8ae5f1ae..455b292b23 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -39,6 +39,7 @@ func TestHealth(t *testing.T) { conf.SetForTest(t, configuration.AuthorizerAllowIsEnabled, true) conf.SetForTest(t, configuration.AuthorizerDenyIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorNoopIsEnabled, true) + conf.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorAnonymousIsEnabled, true) conf.SetForTest(t, configuration.MutatorNoopIsEnabled, true) conf.SetForTest(t, "mutators.header.config", map[string]interface{}{"headers": map[string]interface{}{}}) diff --git a/cmd/root_test.go b/cmd/root_test.go index 0339102c22..cfb0010012 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -30,6 +30,7 @@ func init() { os.Setenv("SERVE_API_PORT", fmt.Sprintf("%d", apiPort)) os.Setenv("SERVE_PROXY_PORT", fmt.Sprintf("%d", proxyPort)) os.Setenv("AUTHENTICATORS_NOOP_ENABLED", "1") + os.Setenv("AUTHENTICATORS_DELEGATE_ENABLED", "1") os.Setenv("AUTHENTICATORS_ANONYMOUS_ENABLED", "true") os.Setenv("AUTHORIZERS_ALLOW_ENABLED", "true") os.Setenv("MUTATORS_NOOP_ENABLED", "true") @@ -54,6 +55,9 @@ func init() { }, { "handler": "anonymous" + }, + { + "handler": "delegate" } ], "authorizer": { diff --git a/driver/configuration/config_keys.go b/driver/configuration/config_keys.go index 88e725ba06..53cd13b055 100644 --- a/driver/configuration/config_keys.go +++ b/driver/configuration/config_keys.go @@ -55,6 +55,9 @@ const ( // noop AuthenticatorNoopIsEnabled Key = "authenticators.noop.enabled" + // delegate + AuthenticatorDelegateIsEnabled Key = "authenticators.delegate.enabled" + // cookie session AuthenticatorCookieSessionIsEnabled Key = "authenticators.cookie_session.enabled" diff --git a/driver/configuration/provider_koanf_public_test.go b/driver/configuration/provider_koanf_public_test.go index 50cbf91309..5c05dc9ded 100644 --- a/driver/configuration/provider_koanf_public_test.go +++ b/driver/configuration/provider_koanf_public_test.go @@ -146,6 +146,7 @@ func BenchmarkPipelineEnabled(b *testing.B) { for n := 0; n < b.N; n++ { p.AuthorizerIsEnabled("allow") p.AuthenticatorIsEnabled("noop") + p.AuthenticatorIsEnabled("delegate") p.MutatorIsEnabled("noop") } } @@ -247,6 +248,12 @@ func TestKoanfProvider(t *testing.T) { require.NoError(t, a.Validate(nil)) }) + t.Run("authenticator=delegate", func(t *testing.T) { + a := authn.NewAuthenticatorDelegate(p) + assert.True(t, p.AuthenticatorIsEnabled(a.GetID())) + require.NoError(t, a.Validate(nil)) + }) + t.Run("authenticator=cookie_session", func(t *testing.T) { a := authn.NewAuthenticatorCookieSession(p, trace.NewNoopTracerProvider()) assert.True(t, p.AuthenticatorIsEnabled(a.GetID())) diff --git a/driver/registry_memory.go b/driver/registry_memory.go index bcf3d57200..abe383f16f 100644 --- a/driver/registry_memory.go +++ b/driver/registry_memory.go @@ -361,6 +361,7 @@ func (r *RegistryMemory) prepareAuthn() { authn.NewAuthenticatorBearerToken(r.c, r.trc.Provider()), authn.NewAuthenticatorJWT(r.c, r), authn.NewAuthenticatorNoOp(r.c), + authn.NewAuthenticatorDelegate(r.c), authn.NewAuthenticatorOAuth2ClientCredentials(r.c, r.Logger()), authn.NewAuthenticatorOAuth2Introspection(r.c, r.Logger(), r.trc.Provider()), authn.NewAuthenticatorUnauthorized(r.c), diff --git a/internal/config/.oathkeeper.yaml b/internal/config/.oathkeeper.yaml index 39a02cb7fc..66dd22bd1d 100644 --- a/internal/config/.oathkeeper.yaml +++ b/internal/config/.oathkeeper.yaml @@ -179,6 +179,11 @@ authenticators: # Set enabled to true if the authenticator should be enabled and false to disable the authenticator. Defaults to false. enabled: true + # Configures the delegate authenticator + delegate: + # Set enabled to true if the authenticator should be enabled and false to disable the authenticator. Defaults to false. + enabled: true + # Configures the oauth2_client_credentials authenticator oauth2_client_credentials: # Set enabled to true if the authenticator should be enabled and false to disable the authenticator. Defaults to false. diff --git a/middleware/grpc_middleware_test.go b/middleware/grpc_middleware_test.go index 398648c149..ed3c51ec8c 100644 --- a/middleware/grpc_middleware_test.go +++ b/middleware/grpc_middleware_test.go @@ -98,6 +98,8 @@ func TestMiddleware(t *testing.T) { authenticators: noop: enabled: true + delegate: + enabled: true anonymous: enabled: true bearer_token: diff --git a/pipeline/authn/authenticator.go b/pipeline/authn/authenticator.go index fa07eb1daf..92ef7d2aee 100644 --- a/pipeline/authn/authenticator.go +++ b/pipeline/authn/authenticator.go @@ -18,6 +18,7 @@ import ( ) var ErrAuthenticatorNotResponsible = errors.New("Authenticator not responsible") +var ErrAuthenticatorDelegate = errors.New("Authentication should be delegated") var ErrAuthenticatorNotEnabled = herodot.DefaultError{ ErrorField: "authenticator matching this route is misconfigured or disabled", CodeField: http.StatusInternalServerError, diff --git a/pipeline/authn/authenticator_delegate.go b/pipeline/authn/authenticator_delegate.go new file mode 100644 index 0000000000..405ff21a8c --- /dev/null +++ b/pipeline/authn/authenticator_delegate.go @@ -0,0 +1,39 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package authn + +import ( + "encoding/json" + "net/http" + + "github.com/ory/oathkeeper/driver/configuration" + "github.com/ory/oathkeeper/pipeline" +) + +type AuthenticatorDelegate struct { + c configuration.Provider +} + +func NewAuthenticatorDelegate(c configuration.Provider) *AuthenticatorDelegate { + return &AuthenticatorDelegate{c: c} +} + +func (a *AuthenticatorDelegate) GetID() string { + return "delegate" +} + +func (a *AuthenticatorDelegate) Validate(config json.RawMessage) error { + if !a.c.AuthenticatorIsEnabled(a.GetID()) { + return NewErrAuthenticatorNotEnabled(a) + } + + if err := a.c.AuthenticatorConfig(a.GetID(), config, nil); err != nil { + return NewErrAuthenticatorMisconfigured(a, err) + } + return nil +} + +func (a *AuthenticatorDelegate) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { + return ErrAuthenticatorDelegate +} diff --git a/pipeline/authn/authenticator_delegate_test.go b/pipeline/authn/authenticator_delegate_test.go new file mode 100644 index 0000000000..d4933486c8 --- /dev/null +++ b/pipeline/authn/authenticator_delegate_test.go @@ -0,0 +1,37 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package authn_test + +import ( + "testing" + + "github.com/ory/oathkeeper/driver/configuration" + "github.com/ory/oathkeeper/internal" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAuthenticatorDelegate(t *testing.T) { + t.Parallel() + conf := internal.NewConfigurationWithDefaults() + reg := internal.NewRegistry(conf) + + a, err := reg.PipelineAuthenticator("delegate") + require.NoError(t, err) + assert.Equal(t, "delegate", a.GetID()) + + t.Run("method=authenticate", func(t *testing.T) { + err := a.Authenticate(nil, nil, nil, nil) + require.Error(t, err) + }) + + t.Run("method=validate", func(t *testing.T) { + conf.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) + require.NoError(t, a.Validate(nil)) + + conf.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, false) + require.Error(t, a.Validate(nil)) + }) +} diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 579919529a..5cc78e137c 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -46,6 +46,7 @@ func TestProxy(t *testing.T) { defer ts.Close() conf.SetForTest(t, configuration.AuthenticatorNoopIsEnabled, true) + conf.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorUnauthorizedIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorAnonymousIsEnabled, true) conf.SetForTest(t, configuration.AuthorizerAllowIsEnabled, true) @@ -84,6 +85,35 @@ func TestProxy(t *testing.T) { Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, } + ruleDelegateAuthenticator := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-delegate/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: backend.URL}, + } + ruleDelegateAuthenticatorModifyUpstream := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-delegate/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, + } + ruleDelegateAuthenticatorGlob := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-delegate/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: backend.URL}, + } + ruleDelegateAuthenticatorModifyUpstreamGlob := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-delegate/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "delegate"}}, + Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, + } + // acceptRuleStripHost := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "/users/", PreserveHost: true}} // acceptRuleStripHostWithoutTrailing := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "/users", PreserveHost: true}} // acceptRuleStripHostWithoutTrailing2 := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "users", PreserveHost: true}} @@ -394,6 +424,43 @@ func TestProxy(t *testing.T) { r.Header.Set("Connection", "x-arbitrary") }, }, + { + d: "should fail because url does exist but is matched by two rules", + url: ts.URL + "/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticator, ruleDelegateAuthenticator}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorGlob, ruleDelegateAuthenticatorGlob}, + code: http.StatusInternalServerError, + }, + { + d: "should pass", + url: ts.URL + "/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticator}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorGlob}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + messages: []string{ + "authorization=bearer token", + "url=/authn-delegate/1234", + "host=" + x.ParseURLOrPanic(backend.URL).Host, + }, + }, + { + d: "should pass", + url: ts.URL + "/strip-path/authn-delegate/1234", + rulesRegexp: []rule.Rule{ruleDelegateAuthenticatorModifyUpstream}, + rulesGlob: []rule.Rule{ruleDelegateAuthenticatorModifyUpstreamGlob}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + messages: []string{ + "authorization=bearer token", + "url=/authn-delegate/1234", + "host=" + x.ParseURLOrPanic(ts.URL).Host, + }, + }, } { t.Run(fmt.Sprintf("description=%s", tc.d), func(t *testing.T) { testFunc := func(t *testing.T, strategy configuration.MatchingStrategy, rules []rule.Rule) { diff --git a/proxy/request_handler.go b/proxy/request_handler.go index 7c2aaae9f3..9a5b20c0fb 100644 --- a/proxy/request_handler.go +++ b/proxy/request_handler.go @@ -208,10 +208,10 @@ func (d *requestHandler) HandleRequest(r *http.Request, rl *rule.Rule) (session case authn.ErrAuthenticatorNotResponsible.Error(): // The authentication handler is not responsible for handling this request, skip to the next handler break - // case ErrAuthenticatorBypassed.Error(): - // The authentication handler says that no further authentication/authorization is required, and the request should - // be forwarded to its final destination. - // return nil + case authn.ErrAuthenticatorDelegate.Error(): + //The authentication handler says that no further authentication/authorization is required, and the request should + //be forwarded to its final destination. + return session, nil case helper.ErrUnauthorized.ErrorField: d.r.Logger().Info(err) return nil, err diff --git a/proxy/request_handler_test.go b/proxy/request_handler_test.go index 4fe5a0ea6f..f2d451d573 100644 --- a/proxy/request_handler_test.go +++ b/proxy/request_handler_test.go @@ -273,6 +273,7 @@ func TestRequestHandler(t *testing.T) { d: "should fail because the rule is missing authn, authz, and mutator even when some pipelines are enabled", setup: func(t *testing.T, config configuration.Provider) { config.SetForTest(t, configuration.AuthenticatorNoopIsEnabled, true) + config.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) config.SetForTest(t, configuration.AuthorizerAllowIsEnabled, true) config.SetForTest(t, configuration.MutatorNoopIsEnabled, true) }, @@ -419,6 +420,39 @@ func TestRequestHandler(t *testing.T) { Mutators: []rule.Handler{{Handler: "invalid-id"}}, }, }, + { + d: "should pass", + setup: func(t *testing.T, config configuration.Provider) { + config.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) + }, + expectErr: false, + r: newTestRequest("http://localhost"), + rule: rule.Rule{ + Authenticators: []rule.Handler{{Handler: "delegate"}}, + }, + }, + { + d: "should fail when authn is invalid because not enabled", + setup: func(t *testing.T, config configuration.Provider) { + config.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, false) + }, + expectErr: true, + r: newTestRequest("http://localhost"), + rule: rule.Rule{ + Authenticators: []rule.Handler{{Handler: "delegate"}}, + }, + }, + { + d: "should pass when authn since delegate skips authz and mutators", + setup: func(t *testing.T, config configuration.Provider) { + config.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) + }, + expectErr: false, + r: newTestRequest("http://localhost"), + rule: rule.Rule{ + Authenticators: []rule.Handler{{Handler: "delegate"}}, + }, + }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { // log, hook := test.NewNullLogger() diff --git a/spec/config.schema.json b/spec/config.schema.json index ce6ef286d5..a89359ba40 100644 --- a/spec/config.schema.json +++ b/spec/config.schema.json @@ -354,6 +354,13 @@ }, "additionalProperties": false }, + "configAuthenticatorsDelegate": { + "type": "object", + "title": "Delegate Authenticator Configuration", + "description": "This section is optional when the authenticator is disabled.", + "properties": {}, + "additionalProperties": false + }, "configAuthenticatorsCookieSession": { "type": "object", "title": "Cookie Session Authenticator Configuration", @@ -1329,6 +1336,17 @@ } } }, + "delegate": { + "title": "Delegate Operation (delegate)", + "description": "The [`delegate` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#delegate).", + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { + "$ref": "#/definitions/handlerSwitch" + } + } + }, "unauthorized": { "title": "Unauthorized", "description": "The [`unauthorized` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#unauthorized).", diff --git a/spec/pipeline/authenticators.delegate.schema.json b/spec/pipeline/authenticators.delegate.schema.json new file mode 100644 index 0000000000..6ab68bec26 --- /dev/null +++ b/spec/pipeline/authenticators.delegate.schema.json @@ -0,0 +1,5 @@ +{ + "$id": "/.schema/authenticators.delegate.schema.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "$ref": "/.schema/config.schema.json#/definitions/configAuthenticatorsDelegate" +} From 83f4387effdc2b4a6411d21be702a6952208f7c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yunier=20Rojas=20Garc=C3=ADa?= Date: Sun, 3 Mar 2024 15:50:47 +0100 Subject: [PATCH 2/3] chore(tests): Increase test coverage https://app.codecov.io/gh/ory/oathkeeper/pull/1153 --- pipeline/authn/authenticator_delegate.go | 8 ++------ proxy/proxy_test.go | 10 ---------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/pipeline/authn/authenticator_delegate.go b/pipeline/authn/authenticator_delegate.go index 405ff21a8c..271fd9d976 100644 --- a/pipeline/authn/authenticator_delegate.go +++ b/pipeline/authn/authenticator_delegate.go @@ -23,17 +23,13 @@ func (a *AuthenticatorDelegate) GetID() string { return "delegate" } -func (a *AuthenticatorDelegate) Validate(config json.RawMessage) error { +func (a *AuthenticatorDelegate) Validate(_ json.RawMessage) error { if !a.c.AuthenticatorIsEnabled(a.GetID()) { return NewErrAuthenticatorNotEnabled(a) } - - if err := a.c.AuthenticatorConfig(a.GetID(), config, nil); err != nil { - return NewErrAuthenticatorMisconfigured(a, err) - } return nil } -func (a *AuthenticatorDelegate) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { +func (a *AuthenticatorDelegate) Authenticate(r *http.Request, _ *AuthenticationSession, _ json.RawMessage, _ pipeline.Rule) error { return ErrAuthenticatorDelegate } diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 5cc78e137c..f2b43f030b 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -437,12 +437,7 @@ func TestProxy(t *testing.T) { rulesRegexp: []rule.Rule{ruleDelegateAuthenticator}, rulesGlob: []rule.Rule{ruleDelegateAuthenticatorGlob}, code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, messages: []string{ - "authorization=bearer token", - "url=/authn-delegate/1234", "host=" + x.ParseURLOrPanic(backend.URL).Host, }, }, @@ -452,12 +447,7 @@ func TestProxy(t *testing.T) { rulesRegexp: []rule.Rule{ruleDelegateAuthenticatorModifyUpstream}, rulesGlob: []rule.Rule{ruleDelegateAuthenticatorModifyUpstreamGlob}, code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, messages: []string{ - "authorization=bearer token", - "url=/authn-delegate/1234", "host=" + x.ParseURLOrPanic(ts.URL).Host, }, }, From 2bbd8102bfa76a132857d6b57c7ffca6943743b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yunier=20Rojas=20Garc=C3=ADa?= Date: Sun, 3 Mar 2024 15:59:21 +0100 Subject: [PATCH 3/3] chore(tests): Ensure delegate bypass authz & mutators --- proxy/request_handler_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/proxy/request_handler_test.go b/proxy/request_handler_test.go index f2d451d573..f08be7649b 100644 --- a/proxy/request_handler_test.go +++ b/proxy/request_handler_test.go @@ -453,6 +453,19 @@ func TestRequestHandler(t *testing.T) { Authenticators: []rule.Handler{{Handler: "delegate"}}, }, }, + { + d: "should pass with delegate even with invalid authz and mutators", + setup: func(t *testing.T, config configuration.Provider) { + config.SetForTest(t, configuration.AuthenticatorDelegateIsEnabled, true) + }, + expectErr: false, + r: newTestRequest("http://localhost"), + rule: rule.Rule{ + Authenticators: []rule.Handler{{Handler: "delegate"}}, + Authorizer: rule.Handler{Handler: "invalid-id"}, + Mutators: []rule.Handler{{Handler: "invalid-id"}}, + }, + }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { // log, hook := test.NewNullLogger()