From c8772a6b4fbbaa76bfc5e1b75ca6602cfb31f3ee Mon Sep 17 00:00:00 2001 From: Vlad Rusu Date: Fri, 12 Feb 2021 10:27:26 +0200 Subject: [PATCH 1/4] create custom forward request authenticator --- .codeclimate.yml | 8 + .../authenticators.forward.schema.json | 5 + .schemas/authenticators.forward.schema.json | 27 +++ driver/registry_memory.go | 1 + pipeline/authn/authenticator_forward.go | 168 ++++++++++++++++ pipeline/authn/authenticator_forward_test.go | 186 ++++++++++++++++++ spec/config.schema.json | 77 ++++++++ 7 files changed, 472 insertions(+) create mode 100644 .codeclimate.yml create mode 100644 .schema/pipeline/authenticators.forward.schema.json create mode 100644 .schemas/authenticators.forward.schema.json create mode 100644 pipeline/authn/authenticator_forward.go create mode 100644 pipeline/authn/authenticator_forward_test.go diff --git a/.codeclimate.yml b/.codeclimate.yml new file mode 100644 index 0000000000..95788af2da --- /dev/null +++ b/.codeclimate.yml @@ -0,0 +1,8 @@ +version: "2" +plugins: + gofmt: + enabled: true + golint: + enabled: true + govet: + enabled: true diff --git a/.schema/pipeline/authenticators.forward.schema.json b/.schema/pipeline/authenticators.forward.schema.json new file mode 100644 index 0000000000..4b14586b05 --- /dev/null +++ b/.schema/pipeline/authenticators.forward.schema.json @@ -0,0 +1,5 @@ +{ + "$id": "/.schema/authenticators.forward.schema.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "$ref": "/.schema/config.schema.json#/definitions/configAuthenticatorsForward" +} diff --git a/.schemas/authenticators.forward.schema.json b/.schemas/authenticators.forward.schema.json new file mode 100644 index 0000000000..6451b8cad5 --- /dev/null +++ b/.schemas/authenticators.forward.schema.json @@ -0,0 +1,27 @@ +{ + "$id": "https://raw.githubusercontent.com/ory/oathkeeper/master/.schemas/authenticators.cookie_session.schema.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "title": "Forward Authenticator Configuration", + "description": "This section is optional when the authenticator is disabled.", + "properties": { + "service_url": { + "title": "Service URL", + "type": "string", + "format": "uri", + "description": "The origin to proxy requests to. If the response is a 200 with body `{ \"subject\": \"...\", \"extra\": {} }`. The request will pass the subject through successfully, otherwise it will be marked as unauthorized.\n\n>If this authenticator is enabled, this value is required.", + "examples": [ + "https://service-host" + ] + }, + "preserve_path": { + "title": "Preserve Path", + "type": "boolean", + "description": "When set to true, any path specified in `check_session_url` will be preserved instead of overwriting the path with the path from the original request" + } + }, + "required": [ + "service_url" + ], + "additionalProperties": false +} diff --git a/driver/registry_memory.go b/driver/registry_memory.go index ae793b2b7a..610a8245f7 100644 --- a/driver/registry_memory.go +++ b/driver/registry_memory.go @@ -372,6 +372,7 @@ func (r *RegistryMemory) prepareAuthn() { authn.NewAuthenticatorOAuth2ClientCredentials(r.c, r.Logger()), authn.NewAuthenticatorOAuth2Introspection(r.c, r.Logger()), authn.NewAuthenticatorUnauthorized(r.c), + authn.NewAuthenticatorForward(r.c), } r.authenticators = map[string]authn.Authenticator{} diff --git a/pipeline/authn/authenticator_forward.go b/pipeline/authn/authenticator_forward.go new file mode 100644 index 0000000000..0b4b65adf8 --- /dev/null +++ b/pipeline/authn/authenticator_forward.go @@ -0,0 +1,168 @@ +package authn + +import ( + "bytes" + "encoding/json" + "io" + "io/ioutil" + "net/http" + "net/url" + + "github.com/pkg/errors" + "github.com/tidwall/gjson" + + "github.com/ory/go-convenience/stringsx" + + "github.com/ory/herodot" + + "github.com/ory/oathkeeper/driver/configuration" + "github.com/ory/oathkeeper/helper" + "github.com/ory/oathkeeper/pipeline" +) + +func init() { + gjson.AddModifier("this", func(json, arg string) string { + return json + }) +} + +type AuthenticatorForwardFilter struct { +} + +type AuthenticatorForwardConfiguration struct { + ServiceURL string `json:"service_url"` + PreservePath bool `json:"preserve_path"` + ExtraFrom string `json:"extra_from"` + SubjectFrom string `json:"subject_from"` + Method string `json:"method"` +} + +type AuthenticatorForward struct { + c configuration.Provider +} + +func NewAuthenticatorForward(c configuration.Provider) *AuthenticatorForward { + return &AuthenticatorForward{ + c: c, + } +} + +func (a *AuthenticatorForward) GetID() string { + return "forward" +} + +func (a *AuthenticatorForward) Validate(config json.RawMessage) error { + if !a.c.AuthenticatorIsEnabled(a.GetID()) { + return NewErrAuthenticatorNotEnabled(a) + } + + _, err := a.Config(config) + return err +} + +func (a *AuthenticatorForward) Config(config json.RawMessage) (*AuthenticatorForwardConfiguration, error) { + var c AuthenticatorForwardConfiguration + if err := a.c.AuthenticatorConfig(a.GetID(), config, &c); err != nil { + return nil, NewErrAuthenticatorMisconfigured(a, err) + } + + if len(c.ExtraFrom) == 0 { + c.ExtraFrom = "extra" + } + + if len(c.SubjectFrom) == 0 { + c.SubjectFrom = "subject" + } + + return &c, nil +} + +func (a *AuthenticatorForward) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { + cf, err := a.Config(config) + if err != nil { + return err + } + + method := forwardMethod(r, cf.Method) + + body, err := forwardRequestToAuthenticator(r, method, cf.ServiceURL, cf.PreservePath) + if err != nil { + return err + } + + var ( + subject string + extra map[string]interface{} + + subjectRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cf.SubjectFrom).Raw, "null")) + extraRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cf.ExtraFrom).Raw, "null")) + ) + + if err = json.Unmarshal(subjectRaw, &subject); err != nil { + return helper.ErrForbidden.WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cf.SubjectFrom, body, subjectRaw).WithTrace(err) + } + + if err = json.Unmarshal(extraRaw, &extra); err != nil { + return helper.ErrForbidden.WithReasonf("The configured extra_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cf.ExtraFrom, body, extraRaw).WithTrace(err) + } + + session.Subject = subject + session.Extra = extra + return nil +} + +func forwardMethod(r *http.Request, method string) string { + if len(method) == 0 { + return r.Method + } + return method +} + +func forwardRequestToAuthenticator(r *http.Request, method string, serviceURL string, preservePath bool) (json.RawMessage, error) { + reqUrl, err := url.Parse(serviceURL) + if err != nil { + return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to parse remote URL: %s", err)) + } + + if !preservePath { + reqUrl.Path = r.URL.Path + } + + var forwardRequestBody io.ReadCloser = nil + if r.Body != nil { + body, err := ioutil.ReadAll(r.Body) + if err != nil { + return nil, helper.ErrBadRequest.WithReason(err.Error()).WithTrace(err) + } + + // Unfortunately the body reader needs to be read once to forward the request, + // thus the upstream request will fail miserably without recreating a fresh ReaderCloser + forwardRequestBody = ioutil.NopCloser(bytes.NewReader(body)) + r.Body = ioutil.NopCloser(bytes.NewReader(body)) + } + + req := http.Request{ + Method: method, + URL: reqUrl, + Header: r.Header, + Body: forwardRequestBody, + } + res, err := http.DefaultClient.Do(req.WithContext(r.Context())) + if err != nil { + return nil, helper.ErrForbidden.WithReason(err.Error()).WithTrace(err) + } + + return handleResponse(res) +} + +func handleResponse(r *http.Response) (json.RawMessage, error) { + if r.StatusCode == http.StatusOK { + body, err := ioutil.ReadAll(r.Body) + if err != nil { + return json.RawMessage{}, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Remote server returned error: %+v", err)) + } + return body, nil + } else { + return json.RawMessage{}, errors.WithStack(helper.ErrUnauthorized) + } +} diff --git a/pipeline/authn/authenticator_forward_test.go b/pipeline/authn/authenticator_forward_test.go new file mode 100644 index 0000000000..b8400982dd --- /dev/null +++ b/pipeline/authn/authenticator_forward_test.go @@ -0,0 +1,186 @@ +package authn_test + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ory/oathkeeper/internal" + . "github.com/ory/oathkeeper/pipeline/authn" +) + +func TestAuthenticatorForward(t *testing.T) { + conf := internal.NewConfigurationWithDefaults() + reg := internal.NewRegistry(conf) + session := new(AuthenticationSession) + + pipelineAuthenticator, err := reg.PipelineAuthenticator("forward") + require.NoError(t, err) + + t.Run("method=authenticate", func(t *testing.T) { + t.Run("description=should fail because remote returned 400", func(t *testing.T) { + testServer, _ := makeServiceServer(400, `{}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + session, + json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)), + nil, + ) + require.Error(t, err, "%#v", errors.Cause(err)) + }) + + t.Run("description=should pass because remote returned 200", func(t *testing.T) { + testServer, _ := makeServiceServer(200, `{"subject": "123", "extra": {"foo": "bar"}}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Equal(t, &AuthenticationSession{ + Subject: "123", + Extra: map[string]interface{}{"foo": "bar"}, + }, session) + }) + + t.Run("description=should pass through path, headers, method, and body to auth server", func(t *testing.T) { + testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Len(t, requestRecorder.requests, 1) + + r := requestRecorder.requests[0] + + assert.Equal(t, r.Method, "PUT") + assert.Equal(t, r.URL.Path, "/users/123?query=string") + assert.Equal(t, r.Header.Get("sessionid"), "zyx") + assert.Equal(t, requestRecorder.bodies[0], []byte("Test body")) + assert.Equal(t, &AuthenticationSession{Subject: "123"}, session) + }) + + t.Run("description=should pass through path, headers, and body and use custom method to auth server", func(t *testing.T) { + testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "method": "POST"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Len(t, requestRecorder.requests, 1) + r := requestRecorder.requests[0] + assert.Equal(t, r.Method, "POST") + assert.Equal(t, r.URL.Path, "/users/123?query=string") + assert.Equal(t, r.Header.Get("sessionid"), "zyx") + assert.Equal(t, requestRecorder.bodies[0], []byte("Test body")) + assert.Equal(t, &AuthenticationSession{Subject: "123"}, session) + }) + + t.Run("description=should pass through method, headers and body to auth server when PreservePath is true (preserve the path from config remote URL)", func(t *testing.T) { + testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, ""), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "preserve_path": true}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Len(t, requestRecorder.requests, 1) + r := requestRecorder.requests[0] + assert.Equal(t, r.Method, "PUT") + assert.Equal(t, r.URL.Path, "/") + assert.Equal(t, r.Header.Get("sessionid"), "zyx") + assert.Equal(t, &AuthenticationSession{Subject: "123"}, session) + }) + + t.Run("description=should work with nested extra keys", func(t *testing.T) { + testServer, _ := makeServiceServer(200, `{"subject": "123", "session": {"foo": "bar"}}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "extra_from": "session"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Equal(t, &AuthenticationSession{ + Subject: "123", + Extra: map[string]interface{}{"foo": "bar"}, + }, session) + }) + + t.Run("description=should work with the root key for extra and a custom subject key", func(t *testing.T) { + testServer, _ := makeServiceServer(200, `{"identity": {"id": "123"}, "session": {"foo": "bar"}}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "subject_from": "identity.id", "extra_from": "@this"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Equal(t, &AuthenticationSession{ + Subject: "123", + Extra: map[string]interface{}{"session": map[string]interface{}{"foo": "bar"}, "identity": map[string]interface{}{"id": "123"}}, + }, session) + }) + }) +} + +type ForwardRequestRecorder struct { + requests []*http.Request + bodies [][]byte +} + +func makeServiceServer(statusCode int, responseBody string) (*httptest.Server, *ForwardRequestRecorder) { + requestRecorder := &ForwardRequestRecorder{} + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestRecorder.requests = append(requestRecorder.requests, r) + requestBody, _ := ioutil.ReadAll(r.Body) + requestRecorder.bodies = append(requestRecorder.bodies, requestBody) + w.WriteHeader(statusCode) + w.Write([]byte(responseBody)) + })) + return testServer, requestRecorder +} + +func makeForwardRequest(method string, path string, headers map[string]string, bodyStr string) *http.Request { + var body io.ReadCloser + header := http.Header{} + if bodyStr != "" { + body = ioutil.NopCloser(bytes.NewBufferString(bodyStr)) + header.Add("Content-Length", strconv.Itoa(len(bodyStr))) + } + req := &http.Request{ + Method: method, + URL: &url.URL{Path: path}, + Header: header, + Body: body, + } + for name, value := range headers { + req.Header.Add(name, value) + } + return req +} diff --git a/spec/config.schema.json b/spec/config.schema.json index 24e561a3d1..3c166a5045 100644 --- a/spec/config.schema.json +++ b/spec/config.schema.json @@ -429,6 +429,51 @@ "required": ["check_session_url"], "additionalProperties": false }, + "configAuthenticatorsForward": { + "type": "object", + "title": "Forward Request Authenticator Configuration", + "description": "This section is optional when the authenticator is disabled.", + "properties": { + "service_url": { + "title": "Service Check URL", + "type": "string", + "format": "uri", + "description": "The origin to proxy requests to. If the response is a 200 with body `{ \"subject\": \"...\", \"extra\": {} }`. The request will pass the subject through successfully, otherwise it will be marked as unauthorized.\n\n>If this authenticator is enabled, this value is required.", + "examples": [ + "https://service-host" + ] + }, + "preserve_path": { + "title": "Preserve Path", + "type": "boolean", + "description": "When set to true, any path specified in `check_session_url` will be preserved instead of overwriting the path with the path from the original request" + }, + "extra_from": { + "title": "Extra JSON Path", + "description": "The `extra` field in the ORY Oathkeeper authentication session is set using this JSON Path. Defaults to `extra`, and could be `@this` (for the root element), `foo.bar` (for key foo.bar), or any other valid GJSON path. See [GSJON Syntax](https://github.com/tidwall/gjson/blob/master/SYNTAX.md) for reference.", + "type": "string", + "default": "extra" + }, + "subject_from": { + "title": "Subject JSON Path", + "description": "The `subject` field in the ORY Oathkeeper authentication session is set using this JSON Path. Defaults to `subject`. See [GSJON Syntax](https://github.com/tidwall/gjson/blob/master/SYNTAX.md) for reference.", + "type": "string", + "default": "subject" + }, + "method": { + "title": "Method to pass to the auth server", + "description": "The method to pass to the auth server. Defaults to the original request method.", + "type": "string", + "examples": [ + "GET" + ] + } + }, + "required": [ + "service_url" + ], + "additionalProperties": false + }, "configAuthenticatorsBearerToken": { "type": "object", "title": "Bearer Token Authenticator Configuration", @@ -1342,6 +1387,38 @@ } ] }, + "forward": { + "title": "Forward Request", + "description": "The custom forward request authenticator.", + "type": "object", + "properties": { + "enabled": { + "$ref": "#/definitions/handlerSwitch" + } + }, + "oneOf": [ + { + "properties": { + "enabled": { + "const": true + }, + "config": { + "$ref": "#/definitions/configAuthenticatorsForward" + } + }, + "required": [ + "config" + ] + }, + { + "properties": { + "enabled": { + "const": false + } + } + } + ] + }, "bearer_token": { "title": "Bearer Token", "description": "The [`bearer_token` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#bearer_token).", From 1ccc89d82eeb90c8a698f044a290538a37d4c9e1 Mon Sep 17 00:00:00 2001 From: Vlad Rusu Date: Tue, 24 May 2022 15:59:27 +0300 Subject: [PATCH 2/4] refactor: rename forward authz int remote JSON --- ...=> authenticators.remote_json.schema.json} | 4 +-- driver/registry_memory.go | 2 +- ...orward.go => authenticator_remote_json.go} | 22 ++++++++-------- ...t.go => authenticator_remote_json_test.go} | 26 +++++++++---------- spec/config.schema.json | 10 +++---- 5 files changed, 32 insertions(+), 32 deletions(-) rename .schema/pipeline/{authenticators.forward.schema.json => authenticators.remote_json.schema.json} (60%) rename pipeline/authn/{authenticator_forward.go => authenticator_remote_json.go} (84%) rename pipeline/authn/{authenticator_forward_test.go => authenticator_remote_json_test.go} (85%) diff --git a/.schema/pipeline/authenticators.forward.schema.json b/.schema/pipeline/authenticators.remote_json.schema.json similarity index 60% rename from .schema/pipeline/authenticators.forward.schema.json rename to .schema/pipeline/authenticators.remote_json.schema.json index 4b14586b05..cdabbe4e4b 100644 --- a/.schema/pipeline/authenticators.forward.schema.json +++ b/.schema/pipeline/authenticators.remote_json.schema.json @@ -1,5 +1,5 @@ { - "$id": "/.schema/authenticators.forward.schema.json", + "$id": "/.schema/authenticators.remote_json.schema.json", "$schema": "http://json-schema.org/draft-07/schema#", - "$ref": "/.schema/config.schema.json#/definitions/configAuthenticatorsForward" + "$ref": "/.schema/config.schema.json#/definitions/configAuthenticatorsRemoteJSON" } diff --git a/driver/registry_memory.go b/driver/registry_memory.go index 610a8245f7..e17dd79a51 100644 --- a/driver/registry_memory.go +++ b/driver/registry_memory.go @@ -372,7 +372,7 @@ func (r *RegistryMemory) prepareAuthn() { authn.NewAuthenticatorOAuth2ClientCredentials(r.c, r.Logger()), authn.NewAuthenticatorOAuth2Introspection(r.c, r.Logger()), authn.NewAuthenticatorUnauthorized(r.c), - authn.NewAuthenticatorForward(r.c), + authn.NewAuthenticatorRemoteJSON(r.c), } r.authenticators = map[string]authn.Authenticator{} diff --git a/pipeline/authn/authenticator_forward.go b/pipeline/authn/authenticator_remote_json.go similarity index 84% rename from pipeline/authn/authenticator_forward.go rename to pipeline/authn/authenticator_remote_json.go index 0b4b65adf8..6b9b7bf534 100644 --- a/pipeline/authn/authenticator_forward.go +++ b/pipeline/authn/authenticator_remote_json.go @@ -26,10 +26,10 @@ func init() { }) } -type AuthenticatorForwardFilter struct { +type AuthenticatorRemoteJSONFilter struct { } -type AuthenticatorForwardConfiguration struct { +type AuthenticatorRemoteJSONConfiguration struct { ServiceURL string `json:"service_url"` PreservePath bool `json:"preserve_path"` ExtraFrom string `json:"extra_from"` @@ -37,21 +37,21 @@ type AuthenticatorForwardConfiguration struct { Method string `json:"method"` } -type AuthenticatorForward struct { +type AuthenticatorRemoteJSON struct { c configuration.Provider } -func NewAuthenticatorForward(c configuration.Provider) *AuthenticatorForward { - return &AuthenticatorForward{ +func NewAuthenticatorRemoteJSON(c configuration.Provider) *AuthenticatorRemoteJSON { + return &AuthenticatorRemoteJSON{ c: c, } } -func (a *AuthenticatorForward) GetID() string { - return "forward" +func (a *AuthenticatorRemoteJSON) GetID() string { + return "remote_json" } -func (a *AuthenticatorForward) Validate(config json.RawMessage) error { +func (a *AuthenticatorRemoteJSON) Validate(config json.RawMessage) error { if !a.c.AuthenticatorIsEnabled(a.GetID()) { return NewErrAuthenticatorNotEnabled(a) } @@ -60,8 +60,8 @@ func (a *AuthenticatorForward) Validate(config json.RawMessage) error { return err } -func (a *AuthenticatorForward) Config(config json.RawMessage) (*AuthenticatorForwardConfiguration, error) { - var c AuthenticatorForwardConfiguration +func (a *AuthenticatorRemoteJSON) Config(config json.RawMessage) (*AuthenticatorRemoteJSONConfiguration, error) { + var c AuthenticatorRemoteJSONConfiguration if err := a.c.AuthenticatorConfig(a.GetID(), config, &c); err != nil { return nil, NewErrAuthenticatorMisconfigured(a, err) } @@ -77,7 +77,7 @@ func (a *AuthenticatorForward) Config(config json.RawMessage) (*AuthenticatorFor return &c, nil } -func (a *AuthenticatorForward) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { +func (a *AuthenticatorRemoteJSON) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { cf, err := a.Config(config) if err != nil { return err diff --git a/pipeline/authn/authenticator_forward_test.go b/pipeline/authn/authenticator_remote_json_test.go similarity index 85% rename from pipeline/authn/authenticator_forward_test.go rename to pipeline/authn/authenticator_remote_json_test.go index b8400982dd..0b66b6c6f7 100644 --- a/pipeline/authn/authenticator_forward_test.go +++ b/pipeline/authn/authenticator_remote_json_test.go @@ -20,12 +20,12 @@ import ( . "github.com/ory/oathkeeper/pipeline/authn" ) -func TestAuthenticatorForward(t *testing.T) { +func TestAuthenticatorRemoteJSON(t *testing.T) { conf := internal.NewConfigurationWithDefaults() reg := internal.NewRegistry(conf) session := new(AuthenticationSession) - pipelineAuthenticator, err := reg.PipelineAuthenticator("forward") + pipelineAuthenticator, err := reg.PipelineAuthenticator("remote_json") require.NoError(t, err) t.Run("method=authenticate", func(t *testing.T) { @@ -33,7 +33,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, _ := makeServiceServer(400, `{}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), session, json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)), nil, @@ -45,7 +45,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, _ := makeServiceServer(200, `{"subject": "123", "extra": {"foo": "bar"}}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), nil, @@ -61,7 +61,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), + makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), nil, @@ -82,7 +82,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), + makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "method": "POST"}`, testServer.URL)), nil, @@ -101,7 +101,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, ""), + makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, ""), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "preserve_path": true}`, testServer.URL)), nil, @@ -119,7 +119,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, _ := makeServiceServer(200, `{"subject": "123", "session": {"foo": "bar"}}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "extra_from": "session"}`, testServer.URL)), nil, @@ -135,7 +135,7 @@ func TestAuthenticatorForward(t *testing.T) { testServer, _ := makeServiceServer(200, `{"identity": {"id": "123"}, "session": {"foo": "bar"}}`) defer testServer.Close() err := pipelineAuthenticator.Authenticate( - makeForwardRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), + makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), session, json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "subject_from": "identity.id", "extra_from": "@this"}`, testServer.URL)), nil, @@ -149,13 +149,13 @@ func TestAuthenticatorForward(t *testing.T) { }) } -type ForwardRequestRecorder struct { +type RemoteJSONRequestRecorder struct { requests []*http.Request bodies [][]byte } -func makeServiceServer(statusCode int, responseBody string) (*httptest.Server, *ForwardRequestRecorder) { - requestRecorder := &ForwardRequestRecorder{} +func makeServiceServer(statusCode int, responseBody string) (*httptest.Server, *RemoteJSONRequestRecorder) { + requestRecorder := &RemoteJSONRequestRecorder{} testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestRecorder.requests = append(requestRecorder.requests, r) requestBody, _ := ioutil.ReadAll(r.Body) @@ -166,7 +166,7 @@ func makeServiceServer(statusCode int, responseBody string) (*httptest.Server, * return testServer, requestRecorder } -func makeForwardRequest(method string, path string, headers map[string]string, bodyStr string) *http.Request { +func makeRemoteJSONRequest(method string, path string, headers map[string]string, bodyStr string) *http.Request { var body io.ReadCloser header := http.Header{} if bodyStr != "" { diff --git a/spec/config.schema.json b/spec/config.schema.json index 3c166a5045..855fef0f94 100644 --- a/spec/config.schema.json +++ b/spec/config.schema.json @@ -429,9 +429,9 @@ "required": ["check_session_url"], "additionalProperties": false }, - "configAuthenticatorsForward": { + "configAuthenticatorsRemoteJSON": { "type": "object", - "title": "Forward Request Authenticator Configuration", + "title": "RemoteJSON Request Authenticator Configuration", "description": "This section is optional when the authenticator is disabled.", "properties": { "service_url": { @@ -1387,8 +1387,8 @@ } ] }, - "forward": { - "title": "Forward Request", + "remote_json": { + "title": "Remote JSON Request", "description": "The custom forward request authenticator.", "type": "object", "properties": { @@ -1403,7 +1403,7 @@ "const": true }, "config": { - "$ref": "#/definitions/configAuthenticatorsForward" + "$ref": "#/definitions/configAuthenticatorsRemoteJSON" } }, "required": [ From 854e659e92a1f2dd1e68531d996e8602412d3f9c Mon Sep 17 00:00:00 2001 From: Vlad Rusu Date: Tue, 14 Jun 2022 16:37:40 +0300 Subject: [PATCH 3/4] fix: change behaviour of auth remote_json to send POST request by default to the auth service --- .schemas/authenticators.forward.schema.json | 8 ++--- pipeline/authn/authenticator_remote_json.go | 36 +++++++++++-------- .../authn/authenticator_remote_json_test.go | 25 +++++++++++-- spec/config.schema.json | 26 +++++++------- 4 files changed, 58 insertions(+), 37 deletions(-) diff --git a/.schemas/authenticators.forward.schema.json b/.schemas/authenticators.forward.schema.json index 6451b8cad5..a3ccdb1476 100644 --- a/.schemas/authenticators.forward.schema.json +++ b/.schemas/authenticators.forward.schema.json @@ -10,9 +10,7 @@ "type": "string", "format": "uri", "description": "The origin to proxy requests to. If the response is a 200 with body `{ \"subject\": \"...\", \"extra\": {} }`. The request will pass the subject through successfully, otherwise it will be marked as unauthorized.\n\n>If this authenticator is enabled, this value is required.", - "examples": [ - "https://service-host" - ] + "examples": ["https://service-host"] }, "preserve_path": { "title": "Preserve Path", @@ -20,8 +18,6 @@ "description": "When set to true, any path specified in `check_session_url` will be preserved instead of overwriting the path with the path from the original request" } }, - "required": [ - "service_url" - ], + "required": ["service_url"], "additionalProperties": false } diff --git a/pipeline/authn/authenticator_remote_json.go b/pipeline/authn/authenticator_remote_json.go index 6b9b7bf534..5206e8640c 100644 --- a/pipeline/authn/authenticator_remote_json.go +++ b/pipeline/authn/authenticator_remote_json.go @@ -30,11 +30,12 @@ type AuthenticatorRemoteJSONFilter struct { } type AuthenticatorRemoteJSONConfiguration struct { - ServiceURL string `json:"service_url"` - PreservePath bool `json:"preserve_path"` - ExtraFrom string `json:"extra_from"` - SubjectFrom string `json:"subject_from"` - Method string `json:"method"` + ServiceURL string `json:"service_url"` + PreservePath bool `json:"preserve_path"` + ExtraFrom string `json:"extra_from"` + SubjectFrom string `json:"subject_from"` + Method string `json:"method"` + UseOriginalMethod bool `json:"use_original_method"` } type AuthenticatorRemoteJSON struct { @@ -78,14 +79,14 @@ func (a *AuthenticatorRemoteJSON) Config(config json.RawMessage) (*Authenticator } func (a *AuthenticatorRemoteJSON) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { - cf, err := a.Config(config) + cfg, err := a.Config(config) if err != nil { return err } - method := forwardMethod(r, cf.Method) + method := forwardMethod(r, cfg) - body, err := forwardRequestToAuthenticator(r, method, cf.ServiceURL, cf.PreservePath) + body, err := forwardRequestToAuthenticator(r, method, cfg.ServiceURL, cfg.PreservePath) if err != nil { return err } @@ -94,16 +95,16 @@ func (a *AuthenticatorRemoteJSON) Authenticate(r *http.Request, session *Authent subject string extra map[string]interface{} - subjectRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cf.SubjectFrom).Raw, "null")) - extraRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cf.ExtraFrom).Raw, "null")) + subjectRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cfg.SubjectFrom).Raw, "null")) + extraRaw = []byte(stringsx.Coalesce(gjson.GetBytes(body, cfg.ExtraFrom).Raw, "null")) ) if err = json.Unmarshal(subjectRaw, &subject); err != nil { - return helper.ErrForbidden.WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cf.SubjectFrom, body, subjectRaw).WithTrace(err) + return helper.ErrForbidden.WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.SubjectFrom, body, subjectRaw).WithTrace(err) } if err = json.Unmarshal(extraRaw, &extra); err != nil { - return helper.ErrForbidden.WithReasonf("The configured extra_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cf.ExtraFrom, body, extraRaw).WithTrace(err) + return helper.ErrForbidden.WithReasonf("The configured extra_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.ExtraFrom, body, extraRaw).WithTrace(err) } session.Subject = subject @@ -111,11 +112,16 @@ func (a *AuthenticatorRemoteJSON) Authenticate(r *http.Request, session *Authent return nil } -func forwardMethod(r *http.Request, method string) string { +func forwardMethod(r *http.Request, cfg *AuthenticatorRemoteJSONConfiguration) string { + method := cfg.Method if len(method) == 0 { - return r.Method + if cfg.UseOriginalMethod { + return r.Method + } else { + return http.MethodPost + } } - return method + return cfg.Method } func forwardRequestToAuthenticator(r *http.Request, method string, serviceURL string, preservePath bool) (json.RawMessage, error) { diff --git a/pipeline/authn/authenticator_remote_json_test.go b/pipeline/authn/authenticator_remote_json_test.go index 0b66b6c6f7..185a096907 100644 --- a/pipeline/authn/authenticator_remote_json_test.go +++ b/pipeline/authn/authenticator_remote_json_test.go @@ -63,7 +63,7 @@ func TestAuthenticatorRemoteJSON(t *testing.T) { err := pipelineAuthenticator.Authenticate( makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), session, - json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "use_original_method": true}`, testServer.URL)), nil, ) require.NoError(t, err, "%#v", errors.Cause(err)) @@ -78,6 +78,27 @@ func TestAuthenticatorRemoteJSON(t *testing.T) { assert.Equal(t, &AuthenticationSession{Subject: "123"}, session) }) + t.Run("description=should pass through path, headers, and body to auth server, and use POST method", func(t *testing.T) { + testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) + defer testServer.Close() + err := pipelineAuthenticator.Authenticate( + makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, "Test body"), + session, + json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), + nil, + ) + require.NoError(t, err, "%#v", errors.Cause(err)) + assert.Len(t, requestRecorder.requests, 1) + + r := requestRecorder.requests[0] + + assert.Equal(t, r.Method, "POST") + assert.Equal(t, r.URL.Path, "/users/123?query=string") + assert.Equal(t, r.Header.Get("sessionid"), "zyx") + assert.Equal(t, requestRecorder.bodies[0], []byte("Test body")) + assert.Equal(t, &AuthenticationSession{Subject: "123"}, session) + }) + t.Run("description=should pass through path, headers, and body and use custom method to auth server", func(t *testing.T) { testServer, requestRecorder := makeServiceServer(200, `{"subject": "123"}`) defer testServer.Close() @@ -103,7 +124,7 @@ func TestAuthenticatorRemoteJSON(t *testing.T) { err := pipelineAuthenticator.Authenticate( makeRemoteJSONRequest("PUT", "/users/123?query=string", map[string]string{"sessionid": "zyx"}, ""), session, - json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "preserve_path": true}`, testServer.URL)), + json.RawMessage(fmt.Sprintf(`{"service_url": "%s", "preserve_path": true, "use_original_method": true}`, testServer.URL)), nil, ) require.NoError(t, err, "%#v", errors.Cause(err)) diff --git a/spec/config.schema.json b/spec/config.schema.json index 855fef0f94..26e110f8c7 100644 --- a/spec/config.schema.json +++ b/spec/config.schema.json @@ -439,9 +439,7 @@ "type": "string", "format": "uri", "description": "The origin to proxy requests to. If the response is a 200 with body `{ \"subject\": \"...\", \"extra\": {} }`. The request will pass the subject through successfully, otherwise it will be marked as unauthorized.\n\n>If this authenticator is enabled, this value is required.", - "examples": [ - "https://service-host" - ] + "examples": ["https://service-host"] }, "preserve_path": { "title": "Preserve Path", @@ -462,16 +460,18 @@ }, "method": { "title": "Method to pass to the auth server", - "description": "The method to pass to the auth server. Defaults to the original request method.", + "description": "The method to pass to the auth server. Defaults to the original request method or to POST (see `use_original_method`).", "type": "string", - "examples": [ - "GET" - ] + "examples": ["GET"] + }, + "use_original_method": { + "title": "Use Original Method", + "description": "When set to true, the original request method is sent to the authentication service. If set to false, the POST method is used as a fallback. This flag is ignored when the `method` field is set.", + "type": "boolean", + "default": false } }, - "required": [ - "service_url" - ], + "required": ["service_url"], "additionalProperties": false }, "configAuthenticatorsBearerToken": { @@ -1389,7 +1389,7 @@ }, "remote_json": { "title": "Remote JSON Request", - "description": "The custom forward request authenticator.", + "description": "The [`remote_json` authenticator](https://www.ory.sh/oathkeeper/docs/pipeline/authn#remote_json).", "type": "object", "properties": { "enabled": { @@ -1406,9 +1406,7 @@ "$ref": "#/definitions/configAuthenticatorsRemoteJSON" } }, - "required": [ - "config" - ] + "required": ["config"] }, { "properties": { From 53ead8fa1e52682ef548fe3d7e3d4c4fa94da57f Mon Sep 17 00:00:00 2001 From: Vlad Rusu Date: Thu, 29 Sep 2022 16:43:45 +0300 Subject: [PATCH 4/4] Prepare for merge - Split long lines on multiple lines - Remove unnecessary checks - Move schema to new location in `spec` directory - Replace ioutil with io packages - Add manual close to the original request body stream. --- ...=> authenticators.remote_json.schema.json} | 0 pipeline/authn/authenticator_remote_json.go | 43 ++++++++++++------- .../authn/authenticator_remote_json_test.go | 2 +- .../authenticators.remote_json.schema.json | 0 4 files changed, 28 insertions(+), 17 deletions(-) rename .schemas/{authenticators.forward.schema.json => authenticators.remote_json.schema.json} (100%) rename {.schema => spec}/pipeline/authenticators.remote_json.schema.json (100%) diff --git a/.schemas/authenticators.forward.schema.json b/.schemas/authenticators.remote_json.schema.json similarity index 100% rename from .schemas/authenticators.forward.schema.json rename to .schemas/authenticators.remote_json.schema.json diff --git a/pipeline/authn/authenticator_remote_json.go b/pipeline/authn/authenticator_remote_json.go index 5206e8640c..a9092b4baf 100644 --- a/pipeline/authn/authenticator_remote_json.go +++ b/pipeline/authn/authenticator_remote_json.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "io" - "io/ioutil" "net/http" "net/url" @@ -67,14 +66,6 @@ func (a *AuthenticatorRemoteJSON) Config(config json.RawMessage) (*Authenticator return nil, NewErrAuthenticatorMisconfigured(a, err) } - if len(c.ExtraFrom) == 0 { - c.ExtraFrom = "extra" - } - - if len(c.SubjectFrom) == 0 { - c.SubjectFrom = "subject" - } - return &c, nil } @@ -100,11 +91,19 @@ func (a *AuthenticatorRemoteJSON) Authenticate(r *http.Request, session *Authent ) if err = json.Unmarshal(subjectRaw, &subject); err != nil { - return helper.ErrForbidden.WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.SubjectFrom, body, subjectRaw).WithTrace(err) + return helper. + ErrForbidden. + WithReasonf("The configured subject_from GJSON path returned an error on JSON output: %s", err.Error()). + WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.SubjectFrom, body, subjectRaw). + WithTrace(err) } if err = json.Unmarshal(extraRaw, &extra); err != nil { - return helper.ErrForbidden.WithReasonf("The configured extra_from GJSON path returned an error on JSON output: %s", err.Error()).WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.ExtraFrom, body, extraRaw).WithTrace(err) + return helper. + ErrForbidden. + WithReasonf("The configured extra_from GJSON path returned an error on JSON output: %s", err.Error()). + WithDebugf("GJSON path: %s\nBody: %s\nResult: %s", cfg.ExtraFrom, body, extraRaw). + WithTrace(err) } session.Subject = subject @@ -127,7 +126,10 @@ func forwardMethod(r *http.Request, cfg *AuthenticatorRemoteJSONConfiguration) s func forwardRequestToAuthenticator(r *http.Request, method string, serviceURL string, preservePath bool) (json.RawMessage, error) { reqUrl, err := url.Parse(serviceURL) if err != nil { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to parse remote URL: %s", err)) + return nil, errors.WithStack( + herodot. + ErrInternalServerError.WithReasonf("Unable to parse remote URL: %s", err), + ) } if !preservePath { @@ -136,15 +138,24 @@ func forwardRequestToAuthenticator(r *http.Request, method string, serviceURL st var forwardRequestBody io.ReadCloser = nil if r.Body != nil { - body, err := ioutil.ReadAll(r.Body) + body, err := io.ReadAll(r.Body) if err != nil { return nil, helper.ErrBadRequest.WithReason(err.Error()).WithTrace(err) } + err = r.Body.Close() + if err != nil { + return nil, errors.WithStack( + herodot. + ErrInternalServerError. + WithReasonf("Could not close body reader: %s\n", err), + ) + } + // Unfortunately the body reader needs to be read once to forward the request, // thus the upstream request will fail miserably without recreating a fresh ReaderCloser - forwardRequestBody = ioutil.NopCloser(bytes.NewReader(body)) - r.Body = ioutil.NopCloser(bytes.NewReader(body)) + forwardRequestBody = io.NopCloser(bytes.NewReader(body)) + r.Body = io.NopCloser(bytes.NewReader(body)) } req := http.Request{ @@ -163,7 +174,7 @@ func forwardRequestToAuthenticator(r *http.Request, method string, serviceURL st func handleResponse(r *http.Response) (json.RawMessage, error) { if r.StatusCode == http.StatusOK { - body, err := ioutil.ReadAll(r.Body) + body, err := io.ReadAll(r.Body) if err != nil { return json.RawMessage{}, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Remote server returned error: %+v", err)) } diff --git a/pipeline/authn/authenticator_remote_json_test.go b/pipeline/authn/authenticator_remote_json_test.go index 185a096907..517a73eb4b 100644 --- a/pipeline/authn/authenticator_remote_json_test.go +++ b/pipeline/authn/authenticator_remote_json_test.go @@ -35,7 +35,7 @@ func TestAuthenticatorRemoteJSON(t *testing.T) { err := pipelineAuthenticator.Authenticate( makeRemoteJSONRequest("GET", "/", map[string]string{"sessionid": "zyx"}, ""), session, - json.RawMessage(fmt.Sprintf(`{"check_session_url": "%s"}`, testServer.URL)), + json.RawMessage(fmt.Sprintf(`{"service_url": "%s"}`, testServer.URL)), nil, ) require.Error(t, err, "%#v", errors.Cause(err)) diff --git a/.schema/pipeline/authenticators.remote_json.schema.json b/spec/pipeline/authenticators.remote_json.schema.json similarity index 100% rename from .schema/pipeline/authenticators.remote_json.schema.json rename to spec/pipeline/authenticators.remote_json.schema.json