getsentry / getsentry/sentry

Consider removing or extending refresh token expiration for OAuth apps

Open
#107,873 1 comment 2 reactions 0 assignees View on GitHub
Auth Improvement
Dominant language
Python
Stars
44.8k
Forks
4.9k
Avg merge
21h 10m
Merged PRs (30d)
635

Description

## Problem

Our OAuth token implementation has several issues deviating from OAuth 2.1 / RFC 9700 best practices:

1. **Refresh tokens expire too aggressively** — Share a 30-day `expires_at` with access tokens on one `ApiToken` row. Dormant integrations must re-authorize.
2. **Refresh token rotation has no grace period** — Concurrent refreshes permanently lose the refresh token.
3. **Only one active access token** — Lives on the same row. Concurrent refreshes overwrite each other.
4. **Access tokens are too long-lived** — 30 days vs industry norm of 1–8 hours.
5. **Root cause**: `ApiToken` conflates the authorization (refresh token, user, scopes) and the credential (access token, expiry) on a single row.

## Spec Requirements (RFC 9700 / OAuth 2.1)

### Refresh Token Rotation

**RFC 9700 §2.2.2** (exact text):
> "Refresh tokens for public clients **MUST** be sender-constrained or use refresh token rotation as described in Section 4.14. [RFC6749] already mandates that refresh tokens for confidential clients can only be used by the client for which they were issued."

**RFC 9700 §4.14.2** (exact text):
> "Refresh token rotation is **RECOMMENDED**."

Summary:
- **Public clients** (`client_secret=None`): **MUST** rotate (or use sender-constrained tokens)
- **Confidential clients** (have `client_secret`): Rotation is **RECOMMENDED**, not MUST

### Reuse Detection

**RFC 9700 §4.14.2** (exact text):
> "The authorization server **SHOULD** detect refresh token reuse. If a refresh token is used twice, the authorization server **SHOULD** revoke all access tokens based on that refresh token."

### Refresh Token Expiration

**OAuth 2.1 §1.3.2**: "There is no property defined to communicate the expiration of a refresh token to the client."

**RFC 9700**: Uses **SHOULD** for inactivity-based expiration: "Refresh tokens SHOULD expire if the client has been inactive for some time." Duration is at server discretion. "Non-use" means the refresh token hasn't been exchanged for a new access token ([Google's definition](https://developers.google.com/identity/protocols/oauth2)).

### Access Tokens

**RFC 9700 §2.1.1**: "Access tokens **SHOULD** be sender-constrained or have a short lifetime."

No spec requirement limiting to one active access token per grant.

## Industry Comparison

| Platform | AT TTL | RT Lifetime | RT Rotation | Grace Period | Multiple Active ATs |
|----------|--------|-------------|-------------|-------------|-------------------|
| [Google](https://developers.google.com/identity/protocols/oauth2) | 1h | No hard expiry; 6mo inactivity revocation | No | N/A | Yes |
| [GitHub](https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/refreshing-user-access-tokens) | 8h | 6 months | Yes | None | Yes (up to 10/scope) |
| [Okta](https://developer.okta.com/docs/guides/refresh-tokens/main/) | 5min–24h | Configurable | Yes | **30s** (0–60s) | Yes |
| [Auth0](https://auth0.com/docs/secure/tokens/refresh-tokens/refresh-token-rotation) | Configurable | Configurable | Yes | **Reuse interval** | Yes |
| [Microsoft Entra](https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens) | 60–90min | 90-day sliding | Yes | N/A | Yes |
| **Sentry** | **30 days** | **30 days (shared)** | Yes, no grace | **None** | **No** |

All platforms use separate storage for access and refresh tokens. [Django OAuth Toolkit](https://django-oauth-toolkit.readthedocs.io/en/latest/models.html), [Spring Security OAuth2](https://github.com/spring-attic/spring-security-oauth/blob/main/spring-security-oauth2/src/test/resources/schema.sql), and [PHP League OAuth2](https://github.com/thephpleague/oauth2-server) all use separate `AccessToken` / `RefreshToken` models with FK relationships.

## Proposed Architecture

### Schema: Separate Models

```
ApiRefreshToken (new)
├── token / hashed_token (unique)
├── user_id, application_id (FKs)
├── scopes
├── last_used_at ← updated on each refresh; inactivity clock
├── date_added
├── revoked_at (nullable) ← soft revocation for reuse detection
├── previous_token_hash (nullable) ← hash of the rotated-out RT (grace period)
├── rotated_at (nullable) ← when the previous RT was rotated out

ApiAccessToken (new)
├── token / hashed_token (unique)
├── refresh_token_id (FK → ApiRefreshToken)
├── expires_at (short: 8 hours)
├── date_added
```

### Refresh Token Rotation with Grace Period

Rotate for **all client types** — public clients MUST per spec, and applying uniformly is simpler and gives reuse detection everywhere.

#### Normal refresh (single request):

1. Client sends `grant_type=refresh_token&refresh_token=rt_old`
2. Server looks up `ApiRefreshToken` by `hashed_token = hash(rt_old)` → found
3. Check `is_inactive()` → `last_used_at` within 6 months? If not, reject
4. Generate `rt_new`
5. Update the row:
- `previous_token_hash = hashed_token` (save the old hash)
- `rotated_at = now()`
- `token = rt_new`, `hashed_token = hash(rt_new)`
- `last_used_at = now()`
6. Create new `ApiAccessToken` row (8h expiry)
7. Return `{access_token: at_new, refresh_token: rt_new}`

#### Concurrent refresh within grace period (30 seconds):

1. Process B sends `refresh_token=rt_old` (already rotated by Process A)
2. Lookup by `hashed_token = hash(rt_old)` → **not found** (row now has `rt_new`)
3. Lookup by `previous_token_hash = hash(rt_old)` → **found**
4. Check: `now() - rotated_at < 30 seconds`? → **yes, within grace period**
5. This is a legitimate concurrent request, not theft
6. Create another `ApiAccessToken` row (both processes get valid ATs)
7. Return `{access_token: at_new_2, refresh_token: rt_new}` (same RT that Process A got)

#### Reuse after grace period (theft detection):

1. Attacker sends `refresh_token=rt_old` after 30 seconds
2. Lookup by `hashed_token` → not found
3. Lookup by `previous_token_hash` → found, but `now() - rotated_at >= 30 seconds`
4. **Reuse detected** → set `revoked_at = now()` on the token
5. All access tokens linked to this refresh token become invalid
6. Return `invalid_grant`
7. Legitimate client must re-authorize (both client and attacker are locked out — per RFC 9700 SHOULD)

### Behavior Summary

| Behavior | All Client Types |
|----------|-----------------|
| RT rotation | Yes, on every refresh |
| RT grace period | 30 seconds — old RT accepted, returns same new RT |
| RT reuse detection | After grace period → revoke entire token family |
| RT expiration | Inactivity-based: 6 months of non-use |
| AT lifetime | 8 hours |
| Multiple active ATs | Yes — each refresh creates a new row |
| AT auth lookup | By `hashed_token` on `ApiAccessToken` (same pattern as today) |

### Migration Strategy

- **Backward compatibility** — Existing `ApiToken` rows must keep working during migration (read from both old and new tables)
- **Hybrid cloud / silo implications** — `RpcApiToken` and `ApiTokenReplica` need updates
- **Sentry App tokens** — Separate flow (`sentry_apps/token_exchange/`), needs consideration
- **User auth tokens** (`AuthTokenType.USER`) — No refresh token, may stay on current model
- **AT TTL change** — Breaking change for clients not implementing refresh. Needs deprecation period.

### Key Files

- `src/sentry/models/apitoken.py` — Split into two new models
- `src/sentry/web/frontend/oauth_token.py` — Token endpoint (refresh grant, grace period logic)
- `src/sentry/api/authentication.py` — Token lookup in auth middleware
- `src/sentry/auth/services/auth/model.py` — `RpcApiToken` cross-silo model
- `src/sentry/api/serializers/models/apitoken.py` — API response serializer
- `src/sentry/models/apiapplication.py` — `is_public` property (already exists)

### References

- [RFC 9700 — OAuth 2.0 Security BCP](https://datatracker.ietf.org/doc/rfc9700/)
- [OAuth 2.1 draft (draft-ietf-oauth-v2-1-14)](https://datatracker.ietf.org/doc/draft-ietf-oauth-v2-1/)
- [Django OAuth Toolkit models](https://django-oauth-toolkit.readthedocs.io/en/latest/models.html)
- [Okta refresh token rotation & grace period](https://developer.okta.com/docs/guides/refresh-tokens/main/)
- [Auth0 refresh token rotation & reuse detection](https://auth0.com/docs/secure/tokens/refresh-tokens/refresh-token-rotation)
- [Auth0 reuse interval explanation](https://support.auth0.com/center/s/article/Refresh-token-leeway)

Contributor guide

Open the contributing guide

Research direction

Start by reading src/sentry/models/apitoken.py and tracing the refresh grant in src/sentry/web/frontend/oauth_token.py. Then inspect token lookup in src/sentry/api/authentication.py and the cross-silo model in src/sentry/auth/services/auth/model.py. Done means the proposed separated token storage, rotation and reuse behavior, migration compatibility, and related serializers and replicas are implemented and verified.

Written by the indexing model from the issue text.

Assessment

Tech stack
django, python
Domain
api, authentication, backend, security
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.