An easy-to-host PDS on the ATProtocol, iPhone and MacOS. Maintain control of your keys and data, always.
fix(relay): address auth middleware round 2 PR review
Critical:
- Fix i64 overflow in DPoP freshness check: widen to i128 before
subtracting to prevent panic (debug) or bypass (release) when
iat = i64::MIN
- Add debug log in verify_access_token _ arm so InvalidSignature /
key rotation failures are visible in operator logs
Important:
- Reject cnf present without jkt (cnf:{} now returns 401 instead of
silently downgrading to plain Bearer)
- Add dpop_stale_proof_returns_401 and dpop_future_dated_proof_returns_401
tests covering both sides of the ±60 s freshness window
- Fix misleading "signature verification failed" log — now says
"decoding or signature verification failed"
- Reorder DPoP validation: verify signature before computing/comparing
thumbprint (defence-in-depth: prove key control before trusting claims)
Suggestions:
- Switch make_dpop_proof jti from fixed string to Uuid::new_v4() so
tests won't interfere when jti deduplication is added
- Refactor make_dpop_proof to delegate to make_dpop_proof_with_iat
+123
-39
+123
-39
crates/relay/src/auth/mod.rs
+123
-39
crates/relay/src/auth/mod.rs
···
130
130
// 3. Decode and verify the access token (HS256).
131
131
let claims = verify_access_token(token_str, state)?;
132
132
133
-
// 4. Enforce DPoP binding (RFC 9449 §7.1): if the access token carries a
134
-
// `cnf.jkt` binding, the DPoP proof header is mandatory. Accepting the
135
-
// token as a plain Bearer would allow an attacker with a stolen access
136
-
// token to bypass the key binding entirely.
137
-
let token_is_dpop_bound = claims.cnf.as_ref().map_or(false, |c| c.jkt.is_some());
138
-
if token_is_dpop_bound && !has_dpop {
139
-
return Err(ApiError::new(
140
-
ErrorCode::InvalidToken,
141
-
"DPoP-bound token requires a DPoP proof header",
142
-
));
133
+
// 4. Enforce DPoP binding (RFC 9449 §7.1).
134
+
// When `cnf` is present the token carries a proof-of-possession claim; we
135
+
// must require a DPoP proof to honour that binding.
136
+
// * `cnf` present but no `jkt` → explicit rejection: a future cnf variant
137
+
// (e.g. `x5t#S256` for cert binding) could be silently downgraded to plain
138
+
// Bearer if we only check `jkt.is_some()`.
139
+
// * `cnf.jkt` present but no DPoP header → downgrade attack; reject.
140
+
if let Some(cnf) = &claims.cnf {
141
+
if cnf.jkt.is_none() {
142
+
return Err(ApiError::new(
143
+
ErrorCode::InvalidToken,
144
+
"access token cnf present without jkt binding",
145
+
));
146
+
}
147
+
if !has_dpop {
148
+
return Err(ApiError::new(
149
+
ErrorCode::InvalidToken,
150
+
"DPoP-bound token requires a DPoP proof header",
151
+
));
152
+
}
143
153
}
144
154
145
155
// 5. Resolve scope enum.
···
230
240
ErrorKind::ExpiredSignature => {
231
241
ApiError::new(ErrorCode::TokenExpired, "token has expired")
232
242
}
233
-
_ => ApiError::new(ErrorCode::InvalidToken, "invalid token"),
243
+
_ => {
244
+
tracing::debug!(error = %e, error_kind = ?e.kind(), "access token verification failed");
245
+
ApiError::new(ErrorCode::InvalidToken, "invalid token")
246
+
}
234
247
}
235
248
})
236
249
}
···
283
296
));
284
297
}
285
298
286
-
// Compute JWK thumbprint (RFC 7638) from the embedded public key.
287
-
let thumbprint = jwk_thumbprint(&dpop_header.jwk).map_err(|e| {
288
-
tracing::debug!(error = %e, "failed to compute JWK thumbprint from DPoP proof header");
289
-
invalid()
290
-
})?;
291
-
292
-
// Verify that the access token was bound to this DPoP key.
293
-
let bound_thumbprint = access_claims
294
-
.cnf
295
-
.as_ref()
296
-
.and_then(|c| c.jkt.as_deref())
297
-
.ok_or_else(|| {
298
-
ApiError::new(ErrorCode::InvalidToken, "access token missing DPoP binding")
299
-
})?;
300
-
if thumbprint != bound_thumbprint {
301
-
tracing::debug!("DPoP proof key thumbprint does not match cnf.jkt in access token");
302
-
return Err(ApiError::new(
303
-
ErrorCode::InvalidToken,
304
-
"DPoP key thumbprint does not match token binding",
305
-
));
306
-
}
307
-
308
-
// Verify the DPoP JWT signature using the embedded public JWK.
299
+
// Verify the DPoP JWT signature first (before making any binding decisions based
300
+
// on the embedded JWK — defence-in-depth: prove key control before trusting claims).
309
301
let jwk: jsonwebtoken::jwk::Jwk =
310
302
serde_json::from_value(dpop_header.jwk.clone()).map_err(|e| {
311
303
tracing::debug!(error = %e, "failed to parse JWK from DPoP proof header");
···
327
319
validation.validate_aud = false;
328
320
329
321
let dpop_data = decode::<DPopClaims>(dpop_token, &decoding_key, &validation).map_err(|e| {
330
-
tracing::debug!(error = %e, "DPoP proof signature verification failed");
322
+
tracing::debug!(error = %e, "DPoP proof decoding or signature verification failed");
331
323
invalid()
332
324
})?;
333
325
let dpop_claims = dpop_data.claims;
334
326
327
+
// Compute JWK thumbprint (RFC 7638) and verify the access token was bound to this key.
328
+
// Signature has already been verified above, so the JWK is authentic.
329
+
let thumbprint = jwk_thumbprint(&dpop_header.jwk).map_err(|e| {
330
+
tracing::debug!(error = %e, "failed to compute JWK thumbprint from DPoP proof header");
331
+
invalid()
332
+
})?;
333
+
let bound_thumbprint = access_claims
334
+
.cnf
335
+
.as_ref()
336
+
.and_then(|c| c.jkt.as_deref())
337
+
.ok_or_else(|| {
338
+
ApiError::new(ErrorCode::InvalidToken, "access token missing DPoP binding")
339
+
})?;
340
+
if thumbprint != bound_thumbprint {
341
+
tracing::debug!("DPoP proof key thumbprint does not match cnf.jkt in access token");
342
+
return Err(ApiError::new(
343
+
ErrorCode::InvalidToken,
344
+
"DPoP key thumbprint does not match token binding",
345
+
));
346
+
}
347
+
335
348
// Require `jti` for replay protection (existence check only — full deduplication
336
349
// per RFC 9449 §11.1 requires a server-side nonce store, not yet implemented).
337
350
if dpop_claims.jti.is_empty() {
···
378
391
ApiError::new(ErrorCode::InternalError, "internal server error")
379
392
})?
380
393
.as_secs() as i64;
381
-
if (now - dpop_claims.iat).abs() > 60 {
394
+
// Widen to i128 before subtracting to prevent i64 overflow when a malicious
395
+
// client sends iat = i64::MIN (debug panic; release wraparound bypass).
396
+
let diff = (now as i128) - (dpop_claims.iat as i128);
397
+
if diff.unsigned_abs() > 60 {
382
398
return Err(ApiError::new(ErrorCode::InvalidToken, "DPoP proof is stale"));
383
399
}
384
400
···
516
532
serde_json::json!({ "kty": "EC", "crv": "P-256", "x": x, "y": y })
517
533
}
518
534
519
-
/// Build a valid DPoP proof JWT signed with the given P-256 key.
535
+
/// Build a valid DPoP proof JWT signed with the given P-256 key using current time as `iat`.
520
536
fn make_dpop_proof(key: &SigningKey, htm: &str, htu: &str) -> String {
537
+
make_dpop_proof_with_iat(key, htm, htu, now_secs() as i64)
538
+
}
539
+
540
+
/// Build a DPoP proof JWT with an explicit `iat` — used to test freshness rejection.
541
+
fn make_dpop_proof_with_iat(key: &SigningKey, htm: &str, htu: &str, iat: i64) -> String {
521
542
let jwk = dpop_key_to_jwk(key);
522
543
let header = serde_json::json!({
523
544
"typ": "dpop+jwt",
···
527
548
let payload = serde_json::json!({
528
549
"htm": htm,
529
550
"htu": htu,
530
-
"iat": now_secs() as i64,
531
-
"jti": "test-jti-unique-value",
551
+
"iat": iat,
552
+
"jti": uuid::Uuid::new_v4().to_string(),
532
553
});
533
554
534
555
let hdr_b64 =
···
945
966
let dpop_proof =
946
967
format!("{hdr_b64}.{pay_b64}.{}", URL_SAFE_NO_PAD.encode(sig.to_bytes().as_ref() as &[u8]));
947
968
969
+
let req = Request::builder()
970
+
.uri("/protected")
971
+
.header("Authorization", format!("Bearer {token}"))
972
+
.header("DPoP", dpop_proof)
973
+
.body(Body::empty())
974
+
.unwrap();
975
+
let resp = protected_app(state).oneshot(req).await.unwrap();
976
+
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
977
+
let json = json_body(resp).await;
978
+
assert_eq!(json["error"]["code"], "INVALID_TOKEN");
979
+
}
980
+
981
+
#[tokio::test]
982
+
async fn dpop_stale_proof_returns_401() {
983
+
let state = test_state().await;
984
+
let dpop_key = SigningKey::random(&mut OsRng);
985
+
let thumbprint = dpop_key_thumbprint(&dpop_key);
986
+
let token = mint_token(
987
+
"did:plc:alice",
988
+
"com.atproto.access",
989
+
3600,
990
+
&state.jwt_secret,
991
+
Some(serde_json::json!({ "jkt": thumbprint })),
992
+
);
993
+
// iat = now - 120: 120 s in the past — outside the ±60 s freshness window.
994
+
let dpop_proof = make_dpop_proof_with_iat(
995
+
&dpop_key,
996
+
"GET",
997
+
&format!("{}/protected", state.config.public_url),
998
+
now_secs() as i64 - 120,
999
+
);
1000
+
let req = Request::builder()
1001
+
.uri("/protected")
1002
+
.header("Authorization", format!("Bearer {token}"))
1003
+
.header("DPoP", dpop_proof)
1004
+
.body(Body::empty())
1005
+
.unwrap();
1006
+
let resp = protected_app(state).oneshot(req).await.unwrap();
1007
+
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
1008
+
let json = json_body(resp).await;
1009
+
assert_eq!(json["error"]["code"], "INVALID_TOKEN");
1010
+
}
1011
+
1012
+
#[tokio::test]
1013
+
async fn dpop_future_dated_proof_returns_401() {
1014
+
let state = test_state().await;
1015
+
let dpop_key = SigningKey::random(&mut OsRng);
1016
+
let thumbprint = dpop_key_thumbprint(&dpop_key);
1017
+
let token = mint_token(
1018
+
"did:plc:alice",
1019
+
"com.atproto.access",
1020
+
3600,
1021
+
&state.jwt_secret,
1022
+
Some(serde_json::json!({ "jkt": thumbprint })),
1023
+
);
1024
+
// iat = now + 120: 120 s in the future — also outside the ±60 s window.
1025
+
// This exercises the abs() branch (unsigned_abs() after widening).
1026
+
let dpop_proof = make_dpop_proof_with_iat(
1027
+
&dpop_key,
1028
+
"GET",
1029
+
&format!("{}/protected", state.config.public_url),
1030
+
now_secs() as i64 + 120,
1031
+
);
948
1032
let req = Request::builder()
949
1033
.uri("/protected")
950
1034
.header("Authorization", format!("Bearer {token}"))