Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for requesting an audience to the OAuth2 Introspection pr… #678

Merged

Conversation

corrideat
Copy link
Contributor

@corrideat corrideat commented Mar 23, 2021

Related issue

#677

Proposed changes

Adding an option to request an audience, and adding the audience to the preauth request.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

@corrideat corrideat changed the title Add support for requesting an audience to the OAuth2 Introspection pr… feat: Add support for requesting an audience to the OAuth2 Introspection pr… Mar 23, 2021
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

@@ -265,11 +266,18 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth
var rt http.RoundTripper

if c.PreAuth != nil && c.PreAuth.Enabled {
var ep url.Values

if c.PreAuth.Audience != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a test for this? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I can do that. However, there are no tests currently for this (for example for scopes), and I'm not sure how to write it.

I added this:

index 2487b09..dba83d0 100644
--- a/pipeline/authn/authenticator_oauth2_introspection_test.go
+++ b/pipeline/authn/authenticator_oauth2_introspection_test.go
@@ -474,6 +474,45 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
                                },
                                expectErr: false,
                        },
+                       {
+                               d:      "should pass because audience and scopes match configuration",
+                               r:      &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+                               config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true,"scope":["foo","bar"]} }`),
+                               setup: func(t *testing.T, m *httprouter.Router) {
+                                       m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+                                               require.NoError(t, r.ParseForm())
+                                               require.Equal(t, "", r.Form.Get("audience"))
+                                               require.Equal(t, "foo bar", r.Form.Get("scope"))
+                                       })
+                               },
+                               expectErr: false,
+                       },
+                       {
+                               d:      "should pass because audience and scopes match configuration",
+                               r:      &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+                               config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true,"scope":["foo","bar"]} }`),
+                               setup: func(t *testing.T, m *httprouter.Router) {
+                                       m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+                                               require.NoError(t, r.ParseForm())
+                                               require.Equal(t, "", r.Form.Get("audience"))
+                                               require.Equal(t, "foo bar", r.Form.Get("scope"))
+                                       })
+                               },
+                               expectErr: false,
+                       },
+                       {
+                               d:      "should pass because audience and scopes should not be requested",
+                               r:      &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+                               config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true} }`),
+                               setup: func(t *testing.T, m *httprouter.Router) {
+                                       m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+                                               require.NoError(t, r.ParseForm())
+                                               require.Equal(t, "", r.Form.Get("audience"))
+                                               require.Equal(t, "", r.Form.Get("scope"))
+                                       })
+                               },
+                               expectErr: false,
+                       },
                } {
                        t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) {
                                router := httprouter.New()
@@ -484,6 +523,8 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
                                defer ts.Close()
 
                                tc.config, _ = sjson.SetBytes(tc.config, "introspection_url", ts.URL+"/oauth2/introspect")
+                               tc.config, _ = sjson.SetBytes(tc.config, "pre_authorization.token_url", ts.URL+"/oauth2/token")
+
                                sess := new(AuthenticationSession)
                                err := a.Authenticate(tc.r, sess, tc.config, nil)
                                if tc.expectErr {

Which fails with:

--- FAIL: TestAuthenticatorOAuth2Introspection (4.42s)
    --- FAIL: TestAuthenticatorOAuth2Introspection/method=authenticate (4.40s)
        --- FAIL: TestAuthenticatorOAuth2Introspection/method=authenticate/case=24/description=should_pass_because_audience_and_scopes_match_configuration (1.64s)
            authenticator_oauth2_introspection_test.go:536: 
                	Error Trace:	authenticator_oauth2_introspection_test.go:536
                	Error:      	Received unexpected error:
                	            	Post "http://127.0.0.1:36911/oauth2/introspect": unexpected end of JSON input
                	            	github.com/ory/oathkeeper/pipeline/authn.(*AuthenticatorOAuth2Introspection).Authenticate
                	            		/home/user/oathkeeper/pipeline/authn/authenticator_oauth2_introspection.go:194
                	            	github.com/ory/oathkeeper/pipeline/authn_test.TestAuthenticatorOAuth2Introspection.func1.23
                	            		/home/user/oathkeeper/oathkeeper/pipeline/authn/authenticator_oauth2_introspection_test.go:529
                	            	testing.tRunner
                	            		/usr/lib/go-1.16/src/testing/testing.go:1194
                	            	runtime.goexit
                	            		/usr/lib/go-1.16/src/runtime/asm_amd64.s:1371
                	Test:       	TestAuthenticatorOAuth2Introspection/method=authenticate/case=24/description=should_pass_because_audience_and_scopes_match_configuration

Copy link
Member

Choose a reason for hiding this comment

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

If you expect introspection to pass, you need to add a JSON payload to the response along the lines of: https://github.com/ory/oathkeeper/blob/master/pipeline/authn/authenticator_oauth2_introspection_test.go#L462-L474

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@aeneasr aeneasr self-assigned this Mar 29, 2021
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@aeneasr aeneasr merged commit 2405810 into ory:master Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants