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

IAM 892 - login handler and authentication middleware implementation according to spec ID036 #338

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

BarcoMasile
Copy link
Contributor

@BarcoMasile BarcoMasile commented Jun 25, 2024

Description

This PR introduces the middleware functionalities to authenticate invocations via either a bearer token (access token only, no refresh) or ID and access token (with possible refresh in case one of the tokens is expired and a refresh token is available and valid).
This PR also changes the behavior of the callback endpoint which previously returned a json response with three tokens, now redirects to the content of UIPrefix as a URL. This needs to be fixed/improved, and I opened the issue to track this - #337.

Middleware request authentication flow

  1. if the request is against an endpoint whose prefix is allowlisted, then no checks are run, otherwise...
  2. check for bearer token in the Authorization header (CLI use case)
    2.a if available, we rely on that as an access token: if validated we go on, otherwise return 401
  3. check for both ID and access tokens (browser user use case)
    3.a if both valid, we validate them and if all good we serve the request, otherwise
    3.b if one of them is not valid and the error is oidc.TokenExpiredError we try to refresh
    3.b.1 if refresh is fine then cookies get set again and the request is served
    3.c if one of them is not valid and the error is not a token expired error then return 401
  4. if all went well set the Principal in the context and serve the request
  5. if anything went wrong return 401 and delete all cookies

NB: all encrypted token cookies now rely on the "refresh token" expiration (set via OAUTH2_REFRESHTOKEN_COOKIE_TTL_SECONDS env var): reason is that we need id token and access token be set as cookie for longer than their expiration, otherwise we would never get TokenExpiredError which is needed to trigger the refresh flow. If for example id token cookie expiration is equal to the token expiration, the backend would not be able to receive the expired token (in the just expired cookie), so the backend would just return a 401 since no cookie would be found on the incoming request.

About cookie attributes, some of them must be explicitly set, that's why we have this:

  Name:     name,
  Value:    encrypted,
  Path:     path,
  Domain:   "",
  Expires:  expires,
  MaxAge:   int(ttl.Seconds()),
  Secure:   true,
  HttpOnly: true,
  SameSite: http.SameSiteLaxMode,

Also, both expires and max-age are set to support most browser implementations.

Changes

  • introduced OAuth2UserSessionCookieTTLSeconds config
  • added methods on Cookie manager with tests to cover them
  • Principal object has been expanded on to take advantage of claims coming from the ID token, and also the orignal raw id token, access token and refresh token are kept in the same Principal structure that they generated
  • middleware has been expanded along with test coverage

@BarcoMasile BarcoMasile requested a review from a team as a code owner June 25, 2024 13:58
@BarcoMasile BarcoMasile force-pushed the IAM-892-cookie-auth-middleware branch from 5a88794 to 3818bfe Compare June 26, 2024 14:03
shipperizer
shipperizer previously approved these changes Jun 26, 2024
Copy link
Contributor

@shipperizer shipperizer left a comment

Choose a reason for hiding this comment

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

citing the Maxibon advertisement: "2 is meigl che 1"

happier with the split logic

minor comments but good to go imho

pkg/authentication/middlewares.go Show resolved Hide resolved
pkg/authentication/middlewares.go Show resolved Hide resolved
@BarcoMasile
Copy link
Contributor Author

I went ahead and squashed the last commit

shipperizer
shipperizer previously approved these changes Jun 27, 2024
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

A couple of comments/questions, some may be off:

  • Why store the access token in a cookie if it is not used? I suppose it is there to implement userinfo authentication, but we could instead use the id token on the introspection endpoint.
  • Is it sensible to store the tokens in different cookies? It complicates the flow, by making it harder to reason (when examining the security of the flow, eg what happens if I mix and match cookies from different users?)

pkg/authentication/oidc.go Show resolved Hide resolved
pkg/authentication/oidc.go Show resolved Hide resolved
pkg/authentication/middlewares.go Show resolved Hide resolved
pkg/authentication/middlewares.go Outdated Show resolved Hide resolved
pkg/authentication/middlewares.go Show resolved Hide resolved
pkg/authentication/handlers.go Outdated Show resolved Hide resolved
internal/config/specs.go Outdated Show resolved Hide resolved
@BarcoMasile BarcoMasile force-pushed the IAM-892-cookie-auth-middleware branch from 126ef68 to b1890ea Compare June 28, 2024 12:51
@BarcoMasile
Copy link
Contributor Author

A couple of comments/questions, some may be off:

  • Why store the access token in a cookie if it is not used? I suppose it is there to implement userinfo authentication, but we could instead use the id token on the introspection endpoint.
  • Is it sensible to store the tokens in different cookies? It complicates the flow, by making it harder to reason (when examining the security of the flow, eg what happens if I mix and match cookies from different users?)

I replied to the first point in one of the comments, about the other point: unfortunately we have to rely on multiple cookies since the RFC(s) impose a 4096 bytes limit for cookie size, attributes included. A typical encrypted cookie of ours is more than 3k bytes, so it would not be possible to merge them as they are.

nsklikas
nsklikas previously approved these changes Jul 1, 2024
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Minor fix, but otherwise lgtm

OAUTH2.md Outdated Show resolved Hide resolved
@BarcoMasile BarcoMasile merged commit 61d0482 into main Jul 1, 2024
6 checks passed
@BarcoMasile BarcoMasile deleted the IAM-892-cookie-auth-middleware branch July 1, 2024 07:42
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.

3 participants