Skip to content

Commit

Permalink
Add ADR for authentication with OAuth2
Browse files Browse the repository at this point in the history
RISDEV-6410
  • Loading branch information
SebastianRossa committed Feb 14, 2025
1 parent 7e9b3dc commit 4da4a7c
Showing 1 changed file with 46 additions and 0 deletions.
46 changes: 46 additions & 0 deletions doc/adr/0014-authentication.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# 0. Template

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

Replace 0. Template with the number of this ADR, 14? and also a useful name like "OAuth2 Authentication Migration" ?


Date: 2025-02-14

## Status

Accepted

## Context

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

Do we need a more specific context? I am just gonna go full length, please shorten it or decide if it is not needed:

Previously, our authentication mechanism was based on OAuth2 but was misconfigured. The backend acted as an OAuth2 client, redirecting users through the frontend to Keycloak for authentication. Upon successful login, the backend retrieved the access token from Keycloak and stored it alongside the session in Redis. A session ID was sent to the frontend via a Set-Cookie header, and subsequent requests used this session ID for authentication.

And then as an intro to the problems listed:
However, this approach led to several problems:

I would also move the sentence about what we are aiming, meaning the transition/migration we are doing, after listing the problems:

To address these issues, we are transitioning to a frontend-driven OAuth2 authentication model.

Our application requires a secure authentication mechanism. We are considering replacing our current session-based authentication with OAuth2.

This comment was marked as resolved.

Copy link
@andreasphil

andreasphil Feb 17, 2025

Member

Also just a language nitpick: we're not "considering", we're implementing this

With the OAuth2 based authentication we want to address the following problems:

1. Our identity provider (Keycloak) has no control over application access after the initial login
2. A session can last 12 hours or longer which gives an attacker a lot of time in case of a breach

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

Maybe also mention here that not only was the session valid for 12 hours but the Keycloak token saved within that session (redis) was expired after 5 min and not refresh (this was the problem we realized that lead us to implement a new approach)

3. There is no interoperability between Neuris systems meaning users would have to authenticate for each system

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

Is this something we have then "fixed" with the new implementation? If a user has different roles, can he/she access several applications (like norms and internal portal) in the same browser window but just logging in once?

This comment has been minimized.

Copy link
@andreasphil

andreasphil Feb 18, 2025

Member

I think so, even though it's hard to test in the current state of the different applications. This is an issue and a goal, so I'll leave it here for now. If we noticed that we still have this issue in the future, we can revisit the ADR.

4. The OAuth2 concept as we used it was misused since a client was used instead of a resource server

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

I think this needs some rephrasing. maybe:

Incorrect OAuth2 implementation: The backend was configured as an OAuth2 client instead of a resource server.


## Decision
We have decided to implement OAuth2 for authentication instead of continuing with session-based authentication.
In detail this means:
- In our setup the frontend will serve as a OAuth2 client while the backend serves as a resource server
- Thus, Keycloak regains control over application access
- A token only lasts 1 minute reducing the impact on theft while still keeping the app usable
- We will use the OAuth2 authentication code flow with PKCE
- PKCE is necessary since we don't have a confidential client which could be used with a client secret
- We will use Keycloak.js as client library because it is
- lightweight
- focused on what we need
- is directly maintained by RedHat together with the Keycloak app.
- The backend uses the Spring Boot standard library for OAuth2 Resource Server which is recommended by the Keycloak developers
- We use JWTs (Java Web Token)

This comment was marked as resolved.

Copy link
@malte-laukoetter

malte-laukoetter Feb 14, 2025

Member

JSON Web Token

- Which works without introspection (as opposed to opaque tokens) thus, we need fewer network calls

This comment was marked as resolved.

Copy link
@andreasphil

andreasphil Feb 17, 2025

Member

Still an open question if we don't need introspection though

- There is no sensitive data in the token we would have to obfuscate
- A threat modelling workshop is held to understand possible threats coming from this change

This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

"has been held" ? Andreas mention we could also link to the confluence page with all threats gathered


This comment was marked as resolved.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

The topic of the token refresh is missing?

## Consequences

This comment has been minimized.

Copy link
@andreasphil

andreasphil Feb 17, 2025

Member

I agree with the others that the categorization doesn't really fit the consequences. My proposal would be to just remove "positive"/'negative", it's not that many and for me they're all mostly in the "not good, not bad, just good to know" category

This comment has been minimized.

Copy link
@andreasphil

andreasphil Feb 17, 2025

Member

Add consequence: this opens up new possible threats and things we need to consider, so we conducted a threat modeling

### Positive:

This comment has been minimized.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

And I think there are at least three more positive aspects to be listed (after moving these to the negative category as they are mis-labelled):

1. Improved security and control: Keycloak now manages session expiration and revocation centrally.
2. Reduced attack surface: The removal of long-lived backend sessions minimizes risk in case of a breach.
4. Enhanced interoperability: Users can authenticate once and access multiple Neuris systems without repeated logins.
(depending if it is the case, question asked at the top)

1. Since the frontend now handles the tokens developers have to keep in mind to keep the token in memory within a closure
2. The frontend has to be served before authentication thus allowing URLs serving the frontend while not exposing any sensitive URLs

This comment has been minimized.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

I would rephrase this sentence to make it more readable

The frontend must be served before authentication, requiring careful routing and security considerations to not expose any sensitive URLs


### Negative:
1. Redis is removed since our authentication setup is now stateless and Redis was only used to synchronize state between pods

This comment has been minimized.

Copy link
@malte-laukoetter

malte-laukoetter Feb 14, 2025

Member

i would classify this as a positive: one thing less to maintain

This comment has been minimized.

Copy link
@malte-laukoetter

malte-laukoetter Feb 14, 2025

Member

on a second thought: are the two groups wrongly labeled? The positive consequence sound more like negative once to me

This comment has been minimized.

Copy link
@VictorDelCampo

VictorDelCampo Feb 15, 2025

Member

I agree, removing redis is actually positive. And the other topics are negative. I think yes it was just a small error when labelling


## Alternatives Considered
1. Continuing with Session-Based Authentication: While simpler to implement, it lacks the scalability and interoperability offered by OAuth2
2. BFF pattern or various proxys: The idea was rejected since it would add an extra service to maintain while not substantially offering additional security

0 comments on commit 4da4a7c

Please sign in to comment.