Support HTTP method based allowlists #789

NickMeves · 2020-09-23T02:00:43Z

Bring over the HTTP Method allowlist & improved validation organization from this defunct PR: #685

Description

Add --skip-auth-route option that takes the form METHOD=PathRegex or PathRegex (for backwards compatible with --skip-auth-regex with the eventual intent on deprecating that option).

Motivation and Context

REST APIs might have very different functionality between a GET vs other HTTP methods that alter state. This supports allowlisting an HTTP method for a given regex path.

E.g. GET /data/1 can be allowlisted to require no authentication, while POST /data/1 requires authentication to update the record.

How Has This Been Tested?

Unit Tests. TODO - interactive tests

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

NickMeves · 2020-09-23T02:02:05Z

Leaving this in draft form for now. I mostly shifted parts we wanted to keep from the RulesEngine PR.

I want to convert the new allowlist tests to Ginkgo before I flip this to ready for review.

NickMeves · 2020-09-23T02:04:48Z

oauthproxy.go

+// buildRoutesAllowlist builds a []route from either the legacy SkipAuthRegex
+// option (paths only support) or newer SkipAuthRoutes option (method=path
+// support)
+func buildRoutesAllowlist(opts *options.Options) ([]*allowedRoute, error) {


I moved configuration out of the options validation (to further the goal of that purely validating and doing no setup).

I know this isn't the best spot, but it gets it with the other very related configuration builders in this file that will eventually be refactored together in another PR to a better spot.

NickMeves · 2020-09-23T02:06:23Z

pkg/validation/options_test.go

@@ -78,12 +77,19 @@ func TestClientSecretFileOption(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to create temp file: %v", err)
 	}
-	f.WriteString("testcase")
+	_, err = f.WriteString("testcase")


I don't know why my IDE catches these when I try to commit but GoSec doesn't 🤷‍♂️

These are unrelated to this PR, just handling errors that could be thrown.

NickMeves · 2020-09-24T03:41:30Z

oauthproxy_test.go

@@ -1482,28 +1482,28 @@ func TestAuthOnlyEndpointSetBasicAuthFalseRequestHeaders(t *testing.T) {
 }

 func TestAuthSkippedForPreflightRequests(t *testing.T) {
-	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


naming conflict with imported upstream package -- This is unrelated cleanup that I was warned about when I attempted to commit.

NickMeves · 2020-09-26T19:47:19Z

oauthproxy.go

+			path   string
+		)
+
+		parts := strings.Split(methodPath, "=")


There's 1 case where this logic won't be fully backwards compatible with migrating former --skip-auth-regex entries to this without a method:

Regexes with a = in it (e.g querystring) so /my/path\?q=data will view /my/path\?q as the HTTP method to allow and data as the path regex.

The hack for this scenario is =/my/path\?q=data. Just isn't graceful. Forcing a regex wrapper (E.g. /regex/ or #regex# etc) would work? But would make the migration harder for 99% of use cases.

I lean toward leave the corner case not fully compatible, understand its hacky because we are using single configs, and know this will be better with structured configs.

Maybe worth noting this in the changelog?

JoelSpeed

Looks great! Left a few suggestions, PTAL

Don't think there's any major changes required here.

Do we have an open issue and/or follow up PR to clean up the allowed-regex option?

JoelSpeed · 2020-10-03T10:22:17Z

oauthproxy.go

+			path   string
+		)
+
+		parts := strings.Split(methodPath, "=")


Maybe worth noting this in the changelog?

JoelSpeed · 2020-10-03T10:23:43Z

oauthproxy.go

@@ -70,6 +76,7 @@ type OAuthProxy struct {
 	AuthOnlyPath      string
 	UserInfoPath      string

+	allowedRoutes           []*allowedRoute


Does this need to be to be a pointer or could it just be []allowedRoute?

I weighed the same thing when I was coding this 😄

Switching it to non-pointer since you brought it up too. The struct is pretty lightweight already, pointer doesn't buy us much.

JoelSpeed · 2020-10-03T10:26:16Z

oauthproxy.go

+			path   string
+		)
+
+		parts := strings.Split(methodPath, "=")


What about strings.SplitN here? Then you wouldn't need to rejoin the remainder strings later?

IIRC SplitN with N==2 should produce a maximum of 2 strings, but if the character is not found, will return a slice of length 1 with the original string in it

JoelSpeed · 2020-10-03T10:30:47Z

oauthproxy_test.go

+		expectedMethods []string
+		expectedRegexes []string


Could use a dummy struct for this?

type testAllowedRoute struct { method string regexString string } ... assert.Equal(t, route.method, expectedRoute[i].method) assert.Equal(t, route.regexp.String(), expectedRoute[i].regexString)

The benefit of this being that the method and regex are paired, which I would say makes the test easier to read, WDYT?

JoelSpeed · 2020-10-03T10:33:27Z

pkg/validation/allowlist.go

+	msgs := []string{}
+	for _, route := range o.SkipAuthRoutes {
+		var regex string
+		parts := strings.Split(route, "=")


Could use SplitN here as well

NickMeves · 2020-10-05T19:52:39Z

Thanks for the notes @JoelSpeed

I addressed everything and rebased.

JoelSpeed

Thanks

Signed-off-by: hansedong <skipper1314@gmail.com>

NickMeves added the enhancement label Sep 23, 2020

NickMeves commented Sep 23, 2020

View reviewed changes

NickMeves force-pushed the skip-auth-http-method branch 2 times, most recently from 7dcbe78 to e2d198d Compare September 23, 2020 22:49

NickMeves marked this pull request as ready for review September 24, 2020 03:39

NickMeves requested a review from a team as a code owner September 24, 2020 03:39

NickMeves commented Sep 24, 2020

View reviewed changes

NickMeves force-pushed the skip-auth-http-method branch 2 times, most recently from 9371bad to 4b5cf8c Compare September 26, 2020 19:42

NickMeves commented Sep 26, 2020

View reviewed changes

JoelSpeed reviewed Oct 3, 2020

View reviewed changes

NickMeves force-pushed the skip-auth-http-method branch from 4b5cf8c to 0ef3fb2 Compare October 5, 2020 19:39

NickMeves force-pushed the skip-auth-http-method branch from 0ef3fb2 to d3a05b7 Compare October 6, 2020 17:40

JoelSpeed previously approved these changes Oct 7, 2020

View reviewed changes

Nick Meves added 5 commits October 7, 2020 10:13

Support HTTP method based allowlists

183cb12

Add tests for skip auth functionality

cfd3de8

Convert allowlist validation test to Ginkgo

fa4ba5e

Add startup logging for skipped auth routes

89a8ac8

Improve AllowedRoute test table formatting

b7b7ade

NickMeves dismissed JoelSpeed’s stale review via b7b7ade October 7, 2020 17:13

NickMeves force-pushed the skip-auth-http-method branch from d3a05b7 to b7b7ade Compare October 7, 2020 17:13

JoelSpeed approved these changes Oct 7, 2020

View reviewed changes

JoelSpeed merged commit d8b0d90 into oauth2-proxy:master Oct 7, 2020

Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024

improve log output format (oauth2-proxy#789)

b8a0111

Signed-off-by: hansedong <skipper1314@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support HTTP method based allowlists #789

Support HTTP method based allowlists #789

NickMeves commented Sep 23, 2020 •

edited

Loading

NickMeves commented Sep 23, 2020

NickMeves Sep 23, 2020

NickMeves Sep 23, 2020

NickMeves Sep 24, 2020

NickMeves Sep 26, 2020

JoelSpeed Oct 3, 2020

JoelSpeed left a comment

JoelSpeed Oct 3, 2020

JoelSpeed Oct 3, 2020

NickMeves Oct 5, 2020

JoelSpeed Oct 3, 2020

JoelSpeed Oct 3, 2020

JoelSpeed Oct 3, 2020

NickMeves commented Oct 5, 2020

JoelSpeed left a comment

Support HTTP method based allowlists #789

Support HTTP method based allowlists #789

Conversation

NickMeves commented Sep 23, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

NickMeves commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves commented Oct 5, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

NickMeves commented Sep 23, 2020 •

edited

Loading