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: refresh tokens & refresh grant #1294

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

davenewza
Copy link
Contributor

@davenewza davenewza commented Nov 10, 2023

Refresh Tokens

We now issue a new refresh token from the token endpoint with every successful authentication.

{
  "access_token": "eyJhbGciO...",
  "token_type": "bearer",
  "expires_in": 86400,
  "refresh_token": "WQl1q2yAPzd5SrLnpZSKarFLPJFnnLRIfViJG0BfL7A76VNUnqXgkHiylpZMYeBM"
}

The refresh token can be used to mint a new access token without getting the user involved in reauthenticating. Refresh tokens also give us the means to have stateless access token validation but still giving us the option to revoke access to a user if we need (the recommendation here is actually to have a short-lived access token, which we can spell out in the docs).

The refresh token is opaque (and not a structured JWT) and has a server-side configured expiry. A refresh token is revoked by deleting it from storage.

Storage and performance

We use symmetrical SHA3 hashing when storing the tokens in the database. This is without a salt, which means we can easily compare hash values when looking up tokens.

Querying this table is going to be suboptimal unfortunately - it's a trade-off we need to make if we don't want to store indexed plaintext tokens. There are ways to make this more efficient, but I think we'll be fine for now.

Refresh Grant

The token endpoint now supports the refresh_token grant, which will issue a new access token. The response from the token endpoint will be exactly the same as with any other grant.

Refresh Token Rotation

Basically, this means that when refreshing the access token, we also issue a new refresh token alongside it, and we revoke the original refresh token.

Refresh tokens in SPAs was considered insecure because they typically have long lifetimes and browser storage is susceptible to token theft. However, there are acceptable mitigations outlined in the most recent OAuth2 BCP for browser-based applications stipulating that refresh tokens may be used provided that they adhere to specific recommendations, including having a maximum lifetime and requiring rotation to protect against malicious re-use.

Enabled by default, but we may want to make this configurable for clients which don't want a rotating refresh token.

Things to do

  • Configuration for access token and refresh token expiry time (will also let me write some more tests)
  • Configuration to enable/disable refresh token rotation
  • Revoke endpoint
  • Find a way to clean out expired tokens from database

Telemetry Screenshots

Token Exchange:

image

Token Refresh:

image

@davenewza davenewza force-pushed the ke-1237-add-refresh-tokens branch from 6680ef0 to 97edc57 Compare November 10, 2023 10:16

var tracer = otel.Tracer("github.com/teamkeel/keel/runtime")

// VerifyIdToken will verify the ID token from an OpenID Connect provider.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing changed here - just moved the function

@davenewza davenewza changed the title ke 1237 add refresh tokens feat: refresh tokens & refresh grant Nov 10, 2023
@davenewza davenewza marked this pull request as ready for review November 10, 2023 11:06
@davenewza davenewza requested a review from a team November 10, 2023 11:06
INSERT INTO
keel_refresh_token (token, identity_id, expires_at, created_at)
VALUES
(crypt(?, gen_salt('md5')), ?, ?, ?)`
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the same approach we use for passwords and encrypt in Go?

hashedBytes, err := bcrypt.GenerateFromPassword([]byte(emailPassword.String("password")), bcrypt.DefaultCost)

Copy link
Contributor Author

@davenewza davenewza Nov 10, 2023

Choose a reason for hiding this comment

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

Bcrypt uses a salt when hashing which means that two hashes of the same token will not be the same. This means we can't simply compare the hash values in a database query. We would need to use bcrypt.CompareHashAndPassword on each token from the database until we find our match (and, at worst, having to read the entire table into memory), because bcrypt.CompareHashAndPassword will use the salt when comparing the hash with the new token. That's why I used this Postgres hashing function for this

BUT, your question got me researching. Im no security expert, but I think it's completely unnecessary to hash with salt for tokens (which are cryptographically generated), which means we can use a single round of SHA3 hashing which always produces the same hash. This means we can compare the hash values, which will be tremendously more performant (as we can now index them!)

I have changed it to hash in Go using SH3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im very glad you asked, because this entirely solves the performance challenge

@jonbretman jonbretman closed this Nov 10, 2023
@jonbretman jonbretman reopened this Nov 10, 2023
@davenewza davenewza requested a review from jonbretman November 12, 2023 04:15
chore: test fix and better names

chore: minors

chore: mod tidy and minor refactors

chore: pr feedback

feat: content length fix

feat: refresh tokens wip

feat: rotating refresh tokens and refresh token grant endpoint

fix: tests

fix: tests

fix: test db name gen

fix: using sha3 hash

fix: remove crypto extension

fix: test db naming, improved telemetry
@davenewza davenewza force-pushed the ke-1237-add-refresh-tokens branch from 77e03a2 to cac2d5a Compare November 13, 2023 04:08
@davenewza davenewza merged commit 7fc95de into main Nov 13, 2023
10 checks passed
@davenewza davenewza deleted the ke-1237-add-refresh-tokens branch November 13, 2023 10:38
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.

2 participants