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(oauth2.go): introduce JWT creation and validation for CLI sessions #1142

Merged
merged 4 commits into from
Jan 31, 2024
Merged

feat(oauth2.go): introduce JWT creation and validation for CLI sessions #1142

merged 4 commits into from
Jan 31, 2024

Conversation

ale8k
Copy link
Contributor

@ale8k ale8k commented Jan 30, 2024

Description

Introduces session based JWT tokens for use within the CLI device flow.

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

Notes for code reviewers

Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

Just some questions

internal/auth/auth.go Outdated Show resolved Hide resolved
internal/auth/oauth2.go Outdated Show resolved Hide resolved
internal/auth/oauth2.go Show resolved Hide resolved
internal/auth/oauth2.go Show resolved Hide resolved
internal/auth/oauth2.go Show resolved Hide resolved
internal/auth/oauth2.go Outdated Show resolved Hide resolved
internal/auth/oauth2.go Show resolved Hide resolved
Copy link
Contributor

@mina1460 mina1460 left a comment

Choose a reason for hiding this comment

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

looks good, but I am always worried with symmetric key encryption, I prefer ES256 or RSA, especially that brute forcing HS256 is possible, and HS256 can dangerous if not protected with TLS or if it can be accessed from the outside network (maybe by poor on-prem config).

return nil, errors.E(op, err, "failed to build access token")
}

freshToken, err := jwt.Sign(token, jwt.WithKey(jwa.HS256, []byte(secretKey)))
Copy link
Contributor

Choose a reason for hiding this comment

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

are we positive on the use of HS256 (symmetric encryption)? why not RS256 or other asymmetric encryption

Copy link
Contributor

Choose a reason for hiding this comment

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

the safer alternative doesn't look that much more complicated, but I am not an expert on any of the auth stuff

serialized, err := jwt.Sign(token, jwt.WithKey(jwa.RS256, signingKey, jws.WithProtectedHeaders(hdrs)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only we need to know the contents, asymmetric is purely if the receiving party needs to validate / read contents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Signing the token with RS256 or HS256 does not influence whether the token is readable though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, but why would we have a pub/priv key that's only kept server side?

internal/auth/oauth2.go Show resolved Hide resolved
internal/auth/oauth2_test.go Show resolved Hide resolved
internal/auth/oauth2.go Show resolved Hide resolved
@ale8k ale8k merged commit d153bd9 into canonical:feature-oidc Jan 31, 2024
4 checks passed
ale8k added a commit that referenced this pull request Feb 5, 2024
* feat(logindevice): implements LoginDevice

Login device now stores the oauth response on the controllerroot such that it can be shared with the
second step of the flow, GetDeviceAccessToken upon a successful login from a user. It is expected
that the same websocket be used for both facade calls. This prevents the need to associate a
LoginDevice call to a GetDeviceAccessToken. In addition to this, the library actually expects the
oauth response from device to be sent to deviceaccesstoken.

6714

* PR comments

* pr comments

* Wait on ci so all containers healthy

* Fix CI

* fix

* remove "finished" from dc

* feat(oauth2.go): introduce JWT creation and validation for CLI sessions (#1142)

* feat(oauth2.go): introduce JWT creation and validation for CLI sessions

* feat(package doc for auth): adds a package doc for auth

* feat(auth): update error code

* PR comments

* Plug oauth authenticator into jimm (#1144)

* feat(oauth2.go): introduce JWT creation and validation for CLI sessions

* feat(package doc for auth): adds a package doc for auth

* feat(auth): update error code

* feat(oauthauthenticator): plug in the authenticator params and create it on the JIMM service

* PR comments

* feat(pr fixes): pr fixes

* fix

* add ref

* remove else

* Connect directly to realm for tests

* typo

* Add auth to test utils

* fix service tests

* debug line to help fix tests in ci

* PR comments

* pr comments

* Wait on ci so all containers healthy

* Fix CI

* fix

* remove "finished" from dc

* feat(remove req param for logindevice): remove param

---------

Co-authored-by: Ales Stimec <ales.stimec@canonical.com>
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.

4 participants