Implement RFC 8628 #826

nsklikas · 2024-09-30T06:50:13Z

BREAKING CHANGES: This patch breaks up `OAuth2AuthorizeExplicitFactory` into
`OAuth2AuthorizeExplicitAuthFactory` and `Oauth2AuthorizeExplicitTokenFactory`

Checklist

I have read the contributing guidelines and signed the CLA.
I have referenced an issue containing the design document if my change introduces a new feature.
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 approval (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 the necessary documentation within the code base (if appropriate).

Further comments

This PR is based on the work done on #701, by @supercairos and @BuzzBumbleBee. That PR was based on an older version of fosite and was missing some features/tests.

Comments:

Due to some dependency issues we had to switch to go.uber.org/mock/gomock (github.com/golang/mock/gomock is deprecated)
We had to do a small refactor of the AuthorizeExplicitGrantHandler to generalize it to reduce code duplication

CLAassistant · 2024-09-30T06:50:20Z

All committers have signed the CLA.

nsklikas · 2024-09-30T07:14:18Z

Looks like the conformity tests are failing because it tries to use this version of fosite on hydra master. But they pass on the hydra PR with this version of fosite. Changes are needed on the hydra config DefaultProvider object, how should we fix this?

aeneasr

Thank you very much for this massive PR! There's still thousands of lines to review, but here are some first remarks.

Primarily, some of the diffs are hard to read because they re-organize code into different files. Would it be possible to focus the changes on what's required to change and leave the refactoring out? We can always re-organize the code later, but given the sensitivity of the files touched (core token methods), it will be significantly easier to review those changes. Thank you!

aeneasr · 2024-10-09T12:10:26Z

go.mod

-go 1.20
+go 1.21
+
+toolchain go1.21.4


Is 1.21 needed?

It was required for the update of ory/x, I will roll it back to the lateset version that supports 1.20.

aeneasr · 2024-10-09T12:13:32Z

fosite.go

+type DeviceEndpointHandlers []DeviceEndpointHandler
+
+// Append adds an DeviceEndpointHandlers to this list. Ignores duplicates based on reflect.TypeOf.
+func (a *DeviceEndpointHandlers) Append(h DeviceEndpointHandler) {


Add a test for this? :)

aeneasr · 2024-10-09T12:20:59Z

handler/rfc8628/strategy_hmacsha.go

+	key := code + "_limiter"
+
+	keyBytes := []byte(key)
+	object, err := h.RateLimiterCache.Get(keyBytes)


This won't work in real world scenarios. The cache is in memory which means:

It won't work if more than one instance (which is almost always the case) of the library/hydra running

It won't persist across restarts

In my view, rate limiting should be the concern of the firewall/ingress/reverse proxy and I think we could simplify this PR a bit by removing this part or making it optional and/or more generic

Agreed on all your points. The reason we included this change is to be spec compliant and the reason we didn't implement a more sophisticated solution (which would probably require an external caching system) is that rate limiting should be handled by the ingress/firewall. But looking back at it, it is probably better to leave it unhandled, instead of having partly correct logic.

It is probably better to remove this and leave it as future work.

aeneasr · 2024-10-09T12:22:22Z

handler/oauth2/flow_authorize_code_token.go

@@ -5,197 +5,99 @@ package oauth2



This diff is quite challenging to read, would it be possible to reduce the refactoring and separating into different files? Since this is security relevant code, the smaller the diff the faster the review :)

We will try to revert the refactored code and push the changes in the coming days

aeneasr · 2024-10-09T12:46:48Z

Looks like the conformity tests are failing because it tries to use this version of fosite on hydra master. But they pass on the hydra PR with this version of fosite. Changes are needed on the hydra config DefaultProvider object, how should we fix this?

You should be able to change the target commit in the CI file to target your PR in hydra, which should make the tests pass

zepatrik

This looks great already, however due to the security implications I agree with @aeneasr that we should pull out some of the unrelated refactoring into separate PRs to reduce the size and complexity of this one, especially the migration from github.com/golang/mock/gomock to go.uber.org/mock/gomock and maybe also some other code style changes.
I did not yet fully review all parts, but these are some preliminary suggestions already.

device_request_handler.go

zepatrik · 2024-10-11T14:59:25Z

device_request_handler.go

+	scope := RemoveEmpty(strings.Split(request.Form.Get("scope"), " "))
+	for _, permission := range scope {
+		if !f.Config.GetScopeStrategy(ctx)(request.Client.GetScopes(), permission) {
+			return errorsx.WithStack(ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", permission))
+		}
+	}
+	request.SetRequestedScopes(scope)


Consider re-using the scope strategy across loop iterations:

Suggested change

scope := RemoveEmpty(strings.Split(request.Form.Get("scope"), " "))

for _, permission := range scope {

if !f.Config.GetScopeStrategy(ctx)(request.Client.GetScopes(), permission) {

return errorsx.WithStack(ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", permission))

}

}

request.SetRequestedScopes(scope)

scopes := RemoveEmpty(strings.Split(request.Form.Get("scope"), " "))

scopeStrategy := f.Config.GetScopeStrategy(ctx)

for _, scope := range scopes {

if !scopeStrategy(request.Client.GetScopes(), scope) {

return errorsx.WithStack(ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", scope))

}

}

request.SetRequestedScopes(scope)

device_request_handler.go

zepatrik · 2024-10-11T15:06:14Z

device_request_handler_test.go

+			ar, err := fosite.NewDeviceRequest(context.Background(), r)
+			if c.expectedError != nil {
+				assert.EqualError(t, err, c.expectedError.Error())
+			} else {
+				require.NoError(t, err)
+				assert.NotNil(t, ar.GetRequestedAt())
+			}


Reduce logic in tests:

Suggested change

ar, err := fosite.NewDeviceRequest(context.Background(), r)

if c.expectedError != nil {

assert.EqualError(t, err, c.expectedError.Error())

} else {

require.NoError(t, err)

assert.NotNil(t, ar.GetRequestedAt())

}

ar, err := fosite.NewDeviceRequest(context.Background(), r)

require.ErrorIs(t, err, c.expectedError)

if c.expectedError == nil {

assert.NotNil(t, ar.GetRequestedAt())

}

device_request_handler_test.go

zepatrik · 2024-10-11T15:30:57Z

device_request_handler_test.go

+				assert.EqualError(t, err, c.expectedError.Error())
+			} else {
+				require.NoError(t, err)
+				assert.NotNil(t, req.GetRequestedAt())


req.GetRequestedAt() returns a time.Time. This can never be nil.

Suggested change

assert.NotNil(t, req.GetRequestedAt())

assert.NotZero(t, req.GetRequestedAt())

zepatrik · 2024-10-11T15:33:19Z

device_request_test.go

+func TestDeviceRequest(t *testing.T) {
+	r := NewDeviceRequest()
+	r.Client = &DefaultClient{}
+	r.SetRequestedScopes([]string{"17", "42"})
+	assert.True(t, r.GetRequestedScopes().Has("17", "42"))
+	assert.Equal(t, r.Client, r.GetClient())
+}


This is not testing the DeviceRequest, but the Request. The test adds no value, as DeviceRequest has no logic to be tested so IMO this can be removed.

zepatrik · 2024-10-11T15:37:03Z

device_response.go

+// FromJson populates a response's fields from a json
+func (d *DeviceResponse) FromJson(r io.Reader) error {
+	return json.NewDecoder(r).Decode(&d)
+}
+
+// ToJson writes a response as a json
+func (d *DeviceResponse) ToJson(rw io.Writer) error {
+	return json.NewEncoder(rw).Encode(&d)
+}


This should implement the json.Marshaler and json.Unmarshaler interfaces instead. As there is no specific logic implemented here, the methods can just be removed.

zepatrik · 2024-10-11T15:46:08Z

errors.go

+	ErrPollingRateLimited = &RFC6749Error{
+		DescriptionField: "The authorization request was rate-limited to prevent system overload.",
+		HintField:        "Ensure that you don't call the token endpoint sooner than the polling interval",
+		ErrorField:       errPollingIntervalRateLimited,
+		CodeField:        http.StatusTooManyRequests,
+	}
+	ErrDeviceExpiredToken = &RFC6749Error{
+		DescriptionField: "The device_code has expired, and the device authorization session has concluded.",
+		ErrorField:       errDeviceExpiredToken,
+		CodeField:        http.StatusBadRequest,
+	}


I suggest to keep the exact same naming for the errors as in the RFC to make it easier to understand.

Suggested change

ErrPollingRateLimited = &RFC6749Error{

DescriptionField: "The authorization request was rate-limited to prevent system overload.",

HintField: "Ensure that you don't call the token endpoint sooner than the polling interval",

ErrorField: errPollingIntervalRateLimited,

CodeField: http.StatusTooManyRequests,

}

ErrDeviceExpiredToken = &RFC6749Error{

DescriptionField: "The device_code has expired, and the device authorization session has concluded.",

ErrorField: errDeviceExpiredToken,

CodeField: http.StatusBadRequest,

}

ErrSlowDown = &RFC6749Error{

DescriptionField: "The authorization request was rate-limited to prevent system overload.",

HintField: "Ensure that you don't call the token endpoint sooner than the polling interval",

ErrorField: errSlowDown,

CodeField: http.StatusBadRequest, // http.StatusTooManyRequests makes semantically more sense but it is not specified in the RFC (therefore the default StatusBadRequest should be used)

}

ErrDeviceExpiredToken = &RFC6749Error{

DescriptionField: "The device_code has expired, and the device authorization session has concluded.",

ErrorField: errDeviceExpiredToken,

CodeField: http.StatusBadRequest,

}

zepatrik · 2024-10-14T09:24:30Z

handler/oauth2/flow_authorize_code_token.go

I am wondering whether the split up is actually required. Can't we just do some internal refactoring to reuse the code, instead of this breaking change?
Regardless, I agree with @aeneasr that this should happen in a separate PR.

zepatrik · 2024-10-14T11:07:46Z

handler/rfc8628/strategy_hmacsha.go

+
+// GenerateUserCode generates a user_code
+func (h *DefaultDeviceStrategy) GenerateUserCode(ctx context.Context) (string, string, error) {
+	seq, err := randx.RuneSequence(8, []rune(randx.AlphaUpper))


The RFC discusses extensively the accessibility of user codes wrt length and character sets. As this is "just" the default implementation, these defaults are probably fine.
However, I think that it would make sense to make length and character set configurable, just so fosite users don't have to fork the strategy just to adjust the character set.

zepatrik · 2024-10-14T13:55:45Z

handler/oauth2/flow_generic_code_token.go

+}
+
+// CodeHandler handles authorization/device code related operations.
+type CodeHandler interface {


As the device auth code grant has two codes (user & device), the naming should be clear at any point which one is used. Here I'm struggling really to understand which one it is.

I strongly agree with this, I've been trying to follow along with an implementation (outside of Hydra), and it's been extremely unclear what the correct steps to handle user_code's are.

nsklikas · 2024-10-18T12:13:46Z

Thank you for the reviews.

I reverted the refactor on the oauth2 handlers and tried to address all the comments. Also pinned the ci to use hydra from the PR branch and now the tests pass. Please have another go.

handler/rfc7523/handler_test.go

zepatrik · 2024-10-21T15:39:53Z

device_write_test.go

+	resp.SetUserCode("AAAA")
+	resp.SetDeviceCode("BBBB")
+	resp.SetInterval(int(
+		oauth2.Config.GetDeviceAuthTokenPollingInterval(context.TODO()).Round(time.Second).Seconds(),


I'd suggest to add a simple

ctx := context.Background()

in the beginning of each test, and use that as the context everywhere.

zepatrik · 2024-10-21T15:42:24Z

device_write_test.go

+		oauth2.Config.GetDeviceAuthTokenPollingInterval(context.TODO()).Round(time.Second).Seconds(),
+	))
+	resp.SetExpiresIn(int64(
+		time.Now().Round(time.Second).Add(oauth2.Config.GetDeviceAndUserCodeLifespan(context.TODO())).Second(),


Suggested change

time.Now().Round(time.Second).Add(oauth2.Config.GetDeviceAndUserCodeLifespan(context.TODO())).Second(),

oauth2.Config.GetDeviceAndUserCodeLifespan(ctx),

handler/rfc8628/auth_handler.go

handler/rfc8628/storage.go

handler/openid/flow_device_auth.go

feat: add the access token endpoint handling for device authorization flow

… auth/device code is expired

…ssertion

fix: add the OIDC handler for device flow

* fix: fix the refresh token issue * fix: fix the OIDC token missing issue

nsklikas requested a review from aeneasr as a code owner September 30, 2024 06:50

nsklikas force-pushed the canonical-master branch from 7589ecc to 73d570f Compare September 30, 2024 06:54

nsklikas mentioned this pull request Sep 30, 2024

feat: implement RFC 8628 ory/hydra#3851

Open

7 tasks

aeneasr reviewed Oct 9, 2024

View reviewed changes

zepatrik requested changes Oct 14, 2024

View reviewed changes

zepatrik reviewed Oct 14, 2024

View reviewed changes

nsklikas force-pushed the canonical-master branch 3 times, most recently from 24db6b9 to aa568e6 Compare October 18, 2024 10:09

nsklikas commented Oct 18, 2024

View reviewed changes

handler/rfc7523/handler_test.go Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch from aa568e6 to bf057ec Compare October 18, 2024 14:12

zepatrik reviewed Oct 22, 2024

View reviewed changes

nsklikas force-pushed the canonical-master branch from bf057ec to 35c89bf Compare October 23, 2024 12:02

nsklikas requested review from aeneasr, J-tt and zepatrik October 29, 2024 08:47

nsklikas force-pushed the canonical-master branch 2 times, most recently from bfa2f4e to 904e20c Compare November 6, 2024 14:17

nsklikas force-pushed the canonical-master branch 2 times, most recently from e5035e8 to 94653ee Compare November 13, 2024 10:57

nsklikas commented Nov 15, 2024

View reviewed changes

handler/openid/flow_device_auth.go Outdated Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch 2 times, most recently from c778674 to ae6e4e3 Compare November 18, 2024 09:33

nsklikas force-pushed the canonical-master branch from ae6e4e3 to a5ad00e Compare December 6, 2024 12:00

nsklikas mentioned this pull request Dec 18, 2024

feat: implement RFC 8628 ory/hydra#3912

Open

7 tasks

fix: fix tests

63fbfee

nsklikas and others added 21 commits December 18, 2024 10:23

fix: generalize validateAuthorizeAudience method

6985372

feat: add device flow base logic

57a7b87

fix: add handler for device authorization req

285b822

fix: add device flow handlers to compose

daefbb5

fix: update memory storage

00f227f

chore: update integration tests

409019e

fix: review comments

4ecf113

feat: implement the access token handling for device authorization flow

342f7a9

feat: add the access token endpoint handling for device authorization flow

fix: passing the correct authorization request when validating if the…

c3942d7

… auth/device code is expired

feat: error handling for authorization pending in device flow

39876db

test: reorganize the testcases

9a51d0c

chore: resolve comments

e9f95a4

fix: fix oauth2 core storage interface and device flow session type a…

188c831

…ssertion

fix: implement rate limiting

ad1b856

fix: do not validate request when creating response

a6837eb

fix: add the OIDC handler for device flow (#13)

4cba133

fix: add the OIDC handler for device flow

fix: fix the refresh token issue (#14)

a56d930

* fix: fix the refresh token issue * fix: fix the OIDC token missing issue

fix: use correct grant lifespan to issue tokens

b6f5efe

fix: handle the user code generation duplication

545e9a2

chore: migrate to uber/gomock

ee2c5f8

refactor: revert oauth handler changes

2909a08

nsklikas force-pushed the canonical-master branch from a5ad00e to acb3361 Compare December 18, 2024 11:15

nsklikas added 7 commits December 19, 2024 16:11

ci: use hydra from branch

32b005f

fix: remove rate limiting implementation

2c635ba

fix: make user code creation configurable

b6b5463

refactor: simplify handler and test logic

c22c070

refactor: merge user and device code storage

45462ac

refactor: enhance deviceRequest struct

b30ae67

fix: do not create openid session on device auth request

7309753

nsklikas force-pushed the canonical-master branch from acb3361 to 7309753 Compare December 19, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 8628 #826

Implement RFC 8628 #826

nsklikas commented Sep 30, 2024 •

edited

Loading

CLAassistant commented Sep 30, 2024 •

edited

Loading

nsklikas commented Sep 30, 2024

aeneasr left a comment

aeneasr Oct 9, 2024

nsklikas Oct 17, 2024

aeneasr Oct 9, 2024

aeneasr Oct 9, 2024

nsklikas Oct 14, 2024

aeneasr Oct 9, 2024

nsklikas Oct 14, 2024

aeneasr commented Oct 9, 2024

zepatrik left a comment

zepatrik Oct 11, 2024

zepatrik Oct 11, 2024

zepatrik Oct 11, 2024

zepatrik Oct 11, 2024 •

edited

Loading

zepatrik Oct 11, 2024

zepatrik Oct 11, 2024

zepatrik Oct 14, 2024

zepatrik Oct 14, 2024

zepatrik Oct 14, 2024

J-tt Oct 14, 2024

nsklikas commented Oct 18, 2024

zepatrik Oct 21, 2024

zepatrik Oct 21, 2024

	assert.NotNil(t, req.GetRequestedAt())
	assert.NotZero(t, req.GetRequestedAt())

	time.Now().Round(time.Second).Add(oauth2.Config.GetDeviceAndUserCodeLifespan(context.TODO())).Second(),
	oauth2.Config.GetDeviceAndUserCodeLifespan(ctx),

Implement RFC 8628 #826

Are you sure you want to change the base?

Implement RFC 8628 #826

Conversation

nsklikas commented Sep 30, 2024 • edited Loading

Related Design Document

Checklist

Further comments

CLAassistant commented Sep 30, 2024 • edited Loading

nsklikas commented Sep 30, 2024

aeneasr 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

Choose a reason for hiding this comment

aeneasr commented Oct 9, 2024

zepatrik 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

zepatrik Oct 11, 2024 • edited Loading

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

nsklikas commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsklikas commented Sep 30, 2024 •

edited

Loading

CLAassistant commented Sep 30, 2024 •

edited

Loading

zepatrik Oct 11, 2024 •

edited

Loading