Skip to content

Commit

Permalink
feat: add delegate authenticator
Browse files Browse the repository at this point in the history
## Problem Summary

This merge request addresses issue !669 surrounding the behavior
of the "noop" authentication method, which underwent changes in commit
6f8ab4f. 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

ory#1152
ory#669

closes 1152
  • Loading branch information
yunier-rojas committed Mar 2, 2024
1 parent 817943a commit 55254d1
Show file tree
Hide file tree
Showing 19 changed files with 311 additions and 4 deletions.
11 changes: 11 additions & 0 deletions .schema/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
9 changes: 9 additions & 0 deletions .schemas/authenticators.delegate.schema.json
Original file line number Diff line number Diff line change
@@ -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
}
11 changes: 11 additions & 0 deletions .schemas/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
52 changes: 52 additions & 0 deletions api/decision_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions api/health_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{}})
Expand Down
4 changes: 4 additions & 0 deletions cmd/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -54,6 +55,9 @@ func init() {
},
{
"handler": "anonymous"
},
{
"handler": "delegate"
}
],
"authorizer": {
Expand Down
3 changes: 3 additions & 0 deletions driver/configuration/config_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
7 changes: 7 additions & 0 deletions driver/configuration/provider_koanf_public_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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()))
Expand Down
1 change: 1 addition & 0 deletions driver/registry_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
5 changes: 5 additions & 0 deletions internal/config/.oathkeeper.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions middleware/grpc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ func TestMiddleware(t *testing.T) {
authenticators:
noop:
enabled: true
delegate:
enabled: true
anonymous:
enabled: true
bearer_token:
Expand Down
1 change: 1 addition & 0 deletions pipeline/authn/authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
39 changes: 39 additions & 0 deletions pipeline/authn/authenticator_delegate.go
Original file line number Diff line number Diff line change
@@ -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
}
37 changes: 37 additions & 0 deletions pipeline/authn/authenticator_delegate_test.go
Original file line number Diff line number Diff line change
@@ -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))
})
}
67 changes: 67 additions & 0 deletions proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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}}
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions proxy/request_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 55254d1

Please sign in to comment.