AbsaOSS / login-service

AbsaOSS Common Login gateway using JWT Public key signatures
Apache License 2.0
2 stars 0 forks source link

Feature/75 refresh token simple extend #78

Closed dk1844 closed 7 months ago

dk1844 commented 9 months ago

This PR brings the ability to refresh access tokens as outlined in #75. Specifically:

/token/generate also yields a refresh token in Cookie

Previously, /token/generate only returned a single (access, but it was not marked as such) token. Now, the access token & refresh token are the body

{
    "token": "..."
}

Otherwise unchanged, except that a new field type=access|refresh has been added to signify the token type and refresh token is set to a Cookie refresh=.... E.g.:

{
  "sub": "ab01234",
  "exp": 1696941933,
  "iat": 1696941903,
  "kid": "aaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbbbbbbb",
  "groups": [
    "MyAwesomeGroup1",
    "AdminOfAllUniverse"
  ],
  "email": "Daniel.Kavan@absa.africa",
  "displayname": "Daniel Kavan (CZ)",
  "type": "access"
}

The refresh token only contains sub=username and type=refresh (apart from the needed iat/exp), e.g.

{
  "sub": "ab01234",
  "exp": 1696941564,
  "iat": 1696941504,
  "type": "refresh"
}

/token/refresh added to get a refreshed access token

An endpoint /token/refresh has been added to refresh the current access token via an issued refresh token. You can send exactly what you received from /token/generate before your refresh token expires (access may have expired already) )- i.e. access token in payload, refresh token in Cookie.

refresh-access-token-cookie

Closes #75

github-actions[bot] commented 9 months ago

JaCoCo code coverage report - scala:2.12.17

File Coverage [82.23%] :green_apple:
InMemoryKeyConfig.scala 100% :green_apple:
TokenController.scala 100% :green_apple:
RestMessage.scala 100% :green_apple:
PublicKey.scala 100% :green_apple:
SecurityConfig.scala 100% :green_apple:
JWTService.scala 94.52% :green_apple:
AccessToken.scala 88.1% :green_apple:
RestResponseEntityExceptionHandler.scala 82.22% :green_apple:
ConfigProvider.scala 79.81% :x:
KeyConfig.scala 73.98% :x:
AwsSecretsManagerKeyConfig.scala 51.02% :x:
OptionUtils.scala 50% :x:
Total Project Coverage 75.58% :green_apple:
jakipatryk commented 8 months ago

Shouldn't the refresh token be returned as HTTP-only cookie?

dk1844 commented 8 months ago

Shouldn't the refresh token be returned as HTTP-only cookie?

A secure=true and httpOnly=true cookie is an option. This is useful if used by browsers. However, on the other hand, there may be situations where the cookie could prove problematic - clients may need special care to follow those, or I think I read about other situations ... basically, this is more approachable for programmatic clients - simply a payload-based solution.

The FE client can save the refresh token to local/session storage.

If needed, I can rewrite it to secure,http-only cookie.

//edit: I remember now: if you set the refresh token to cookie, I think is unnecessarily sent with every request. But you don't need that, you only need it to be used when you are actually refreshing. (this may even be unwanted from security perspective -- this may increase the chance of the refresh token being stolen?

We could ask @fateeand, too. @fateeand what is your preference or can you work with both (refresh token being in the payload to be saved to localStorage -- I am sure that this you can work with. What about the secure,httpOnly cookie?)

fateeand commented 8 months ago

Shouldn't the refresh token be returned as HTTP-only cookie?

A secure=true and httpOnly=true cookie is an option. This is useful if used by browsers. However, on the other hand, there may be situations where the cookie could prove problematic - clients may need special care to follow those, or I think I read about other situations ... basically, this is more approachable for programmatic clients - simply a payload-based solution.

The FE client can save the refresh token to local/session storage.

If needed, I can rewrite it to secure,http-only cookie.

//edit: I remember now: if you set the refresh token to cookie, I think is unnecessarily sent with every request. But you don't need that, you only need it to be used when you are actually refreshing. (this may even be unwanted from security perspective -- this may increase the chance of the refresh token being stolen?

We could ask @fateeand, too. @fateeand what is your preference or can you work with both (refresh token being in the payload to be saved to localStorage -- I am sure that this you can work with. What about the secure,httpOnly cookie?)

Since the refresh token rotation is implemented (/token/refresh returns new refresh token, right?) we can store it in local storage. In this case refresh token lifetime is also short and linked to access token lifetime.

Read this article for reference https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/#You-Can-Store-Refresh-Token-In-Local-Storage

dk1844 commented 8 months ago

Since the refresh token rotation is implemented (/token/refresh returns new refresh token, right?) we can store it in local storage. In this case refresh token lifetime is also short and linked to access token lifetime.

Read this article for reference https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/#You-Can-Store-Refresh-Token-In-Local-Storage

Currently, the service does not return a new refresh token on each /token/refresh, but rather behave as if it does, because we may implement this in the future to limit possible misuse.

But its validity will only < working day, so it is still fine.

fateeand commented 8 months ago

Currently, the service does not return a new refresh token on each /token/refresh, but rather behave as if it does, because we may implement this in the future to limit possible misuse.

But its validity will only < working day, so it is still fine.

Ok, so for now this refresh token doesn't have much sense from the security point of view. If the attacker will manage to obtain it once, then he can get new access tokens within the working day, basically same story as having long-living access token. At least we won't force users to relogin and a bit more secure anyways. I will keep frontend-based user inactivity tracking functionality as well for now.

So looking forward to a refresh token rotation mechanism, and we should be safe.

dk1844 commented 8 months ago

Shouldn't the refresh token be returned as HTTP-only cookie?

Redone as a secure HTTP-only cookie. Tests updated, swagger updated. Rebased to the lastest master. @jakipatryk @fateeand

@TheLydonKing please note a small change in the config - generate-in-memory.rotation-time -> key-rotation-time

fateeand commented 7 months ago

Please don't forget to set access-exp-time to 9 hours before merging (a temporary measure until UI support becomes available both in Unify and Aqueduct)

TheLydonKing commented 7 months ago

Please don't forget to set access-exp-time to 9 hours before merging (a temporary measure until UI support becomes available both in Unify and Aqueduct)

I'll do that on the deployment side with the latest Terraform additions