Skip to content

Commit 8469be9

Browse files
glasstigerclaude
andcommitted
Enforce OIDC token store lock staleness floor
build() now rejects a FileTokenStore whose lockStaleMillis is below 4x httpTimeoutMillis. The cross-process lock presumes a holder has crashed once its lock outlives that staleness window and steals it; if the window is shorter than the worst-case time a live refresh holds the lock (send + await + parse + body drain, each bounded by httpTimeoutMillis), a peer can steal a live holder's lock mid-refresh and reopen the rotating-refresh-token race the lock exists to prevent. The store cannot see the client's timeout, so build() enforces the invariant where both values are known. The default store (600s) and any non-coordinating TokenStore are unaffected. Also add the two load-path security tests this subsystem lacked: - a groups-in-token instance rejects a persisted entry whose served id token carries CR/LF (adopt() must validate the id token, not the access token, in that mode) and falls back to the device flow; - a persisted served token carrying a non-ASCII char (> 0x7e), not just a control char, is rejected on load. Each new test was confirmed to fail without its fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent aba8399 commit 8469be9

3 files changed

Lines changed: 114 additions & 0 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/auth/FileTokenStore.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,12 @@ public void save(TokenStoreKey key, PersistedToken token) {
280280
}
281281
}
282282

283+
long getLockStaleMillis() {
284+
// exposed package-private so OidcDeviceAuth.build() can verify this window dominates the worst-case time
285+
// a coordinated refresh holds the lock, before a peer could otherwise judge a live lock stale and steal it
286+
return lockStaleMillis;
287+
}
288+
283289
private static void createLockFile(Path lock) throws IOException {
284290
try {
285291
Files.createFile(lock, FILE_ATTRS);

core/src/main/java/io/questdb/client/cutlass/auth/OidcDeviceAuth.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,11 @@ public class OidcDeviceAuth implements QuietCloseable {
132132
// tokens fail to parse with "String is too long".
133133
private static final int JSON_LEXER_CACHE_SIZE = 1024;
134134
private static final int JSON_LEXER_MAX_VALUE_BYTES = 1 << 20;
135+
// the worst case a coordinated refresh can hold a FileTokenStore cross-process lock, as a multiple of
136+
// httpTimeoutMillis: one refresh under the lock runs send + await + parse, plus a body drain on a parse
137+
// failure, each separately bounded by httpTimeoutMillis. build() rejects a FileTokenStore whose
138+
// lock-staleness window does not exceed this, so a peer never judges a live holder's lock stale mid-refresh
139+
private static final int LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE = 4;
135140
// upper bound on the device code lifetime (the device authorization response's expires_in), so a
136141
// hostile or buggy provider cannot make the client poll for an absurd duration; matches the Python client
137142
private static final int MAX_DEVICE_CODE_TTL_SECONDS = 1800;
@@ -1579,6 +1584,23 @@ public OidcDeviceAuth build() {
15791584
// discovery, so the documented guarantee holds for the explicit builder too
15801585
validateEndpointOrigins(parsedTokenEndpoint, deviceEndpoint, issuerEndpoint);
15811586
ClientTlsConfiguration tls = tlsConfig != null ? tlsConfig : defaultTlsConfig();
1587+
// a FileTokenStore steals a lock older than its staleness window, presuming a crashed holder; that
1588+
// window must exceed the worst-case time a live refresh holds the lock (the refresh under the lock is
1589+
// bounded by httpTimeoutMillis, up to LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE times it), or a peer could steal
1590+
// a live holder's lock mid-refresh and reopen the rotating-refresh-token race the lock prevents. The
1591+
// store cannot see this client's timeout, so enforce the invariant here, where both are known, rather
1592+
// than leave the caller to size it by hand. A non-coordinating TokenStore is exempt - it takes no lock.
1593+
if (tokenStore instanceof FileTokenStore) {
1594+
long minStaleMillis = (long) LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE * httpTimeoutMillis;
1595+
long staleMillis = ((FileTokenStore) tokenStore).getLockStaleMillis();
1596+
if (staleMillis < minStaleMillis) {
1597+
throw new OidcAuthException()
1598+
.put("the FileTokenStore lockStaleMillis (").put(staleMillis)
1599+
.put(") must be at least ").put(LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE)
1600+
.put("x httpTimeoutMillis (").put(minStaleMillis)
1601+
.put("), otherwise a slow refresh's live cross-process lock could be stolen by a peer mid-refresh");
1602+
}
1603+
}
15821604
return new OidcDeviceAuth(this, tls);
15831605
}
15841606

core/src/test/java/io/questdb/client/test/cutlass/auth/OidcDeviceAuthPersistenceTest.java

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,34 @@ public void testAdoptDerivesTtlFromExpiry() throws Exception {
8585
});
8686
}
8787

88+
@Test(timeout = 30_000)
89+
public void testBuildRejectsFileTokenStoreWithTooSmallStaleWindow() throws Exception {
90+
assertMemoryLeak(() -> {
91+
try (MockOidcServer server = new MockOidcServer((method, path, body) -> MockOidcServer.json(200, "{}"))) {
92+
Path dir = storeDir();
93+
// a lock-staleness window below LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE (4) x httpTimeoutMillis would let a
94+
// peer judge a live holder's lock stale and steal it mid-refresh, reopening the rotating-refresh-
95+
// token race the lock prevents; build() must reject the combination rather than ship the race
96+
try {
97+
baseBuilder(server)
98+
.httpTimeoutMillis(30_000)
99+
.tokenStore(new FileTokenStore(dir, 3_000, 119_999))
100+
.build();
101+
Assert.fail("a lockStaleMillis below 4x httpTimeoutMillis must be rejected");
102+
} catch (OidcAuthException expected) {
103+
Assert.assertTrue(expected.getMessage(), expected.getMessage().contains("lockStaleMillis"));
104+
}
105+
// exactly 4x httpTimeoutMillis is the boundary and builds
106+
try (OidcDeviceAuth ignored = baseBuilder(server)
107+
.httpTimeoutMillis(30_000)
108+
.tokenStore(new FileTokenStore(dir, 3_000, 120_000))
109+
.build()) {
110+
// building at the boundary succeeds
111+
}
112+
}
113+
});
114+
}
115+
88116
@Test(timeout = 30_000)
89117
public void testBuilderRejectsHttpTimeoutAboveCap() throws Exception {
90118
assertMemoryLeak(() -> {
@@ -557,6 +585,37 @@ public void testTamperedFileWithCrlfTokenFallsBackToDeviceFlow() throws Exceptio
557585
});
558586
}
559587

588+
@Test(timeout = 30_000)
589+
public void testTamperedIdTokenRejectedOnLoadWithGroupsInToken() throws Exception {
590+
assertMemoryLeak(() -> {
591+
AtomicInteger device = new AtomicInteger();
592+
AtomicInteger token = new AtomicInteger();
593+
MockOidcServer.Handler handler = (method, path, body) -> {
594+
if (DEVICE_PATH.equals(path)) {
595+
device.incrementAndGet();
596+
return MockOidcServer.json(200, deviceAuthJson());
597+
}
598+
token.incrementAndGet();
599+
return MockOidcServer.json(200, tokenJson("ACCESS-FRESH", "ID-FRESH", "REFRESH-FRESH", 3600));
600+
};
601+
try (MockOidcServer server = new MockOidcServer(handler)) {
602+
Path dir = storeDir();
603+
// groups-in-token mode serves the ID token, so adopt() must validate the ID token, not the access
604+
// token. A genuine on-disk file (valid groups fingerprint) with a CLEAN access token but a CR/LF
605+
// id token must be rejected and fall back to the device flow, never routing the tampered id token
606+
// onto the wire. A bug that validated the access token would accept this entry and serve "I\r\nD".
607+
new FileTokenStore(dir).save(keyForGroups(server),
608+
new PersistedToken("ACCESS-CLEAN", "I\r\nD", "REFRESH-1", System.currentTimeMillis() + 300_000, 300_000));
609+
try (OidcDeviceAuth auth = baseBuilder(server).groupsInToken(true).tokenStore(new FileTokenStore(dir)).build()) {
610+
String result = auth.signIn();
611+
Assert.assertEquals("ID-FRESH", result);
612+
Assert.assertNotEquals("I\r\nD", result);
613+
}
614+
Assert.assertTrue("a rejected on-disk id token must fall back to the device flow", device.get() >= 1);
615+
}
616+
});
617+
}
618+
560619
@Test(timeout = 30_000)
561620
public void testTamperedServedTokenRejectedOnLoad() throws Exception {
562621
assertMemoryLeak(() -> {
@@ -582,6 +641,33 @@ public void testTamperedServedTokenRejectedOnLoad() throws Exception {
582641
});
583642
}
584643

644+
@Test(timeout = 30_000)
645+
public void testTamperedServedTokenWithNonAsciiRejectedOnLoad() throws Exception {
646+
assertMemoryLeak(() -> {
647+
AtomicInteger device = new AtomicInteger();
648+
MockOidcServer.Handler handler = (method, path, body) -> {
649+
if (DEVICE_PATH.equals(path)) {
650+
device.incrementAndGet();
651+
return MockOidcServer.json(200, deviceAuthJson());
652+
}
653+
return MockOidcServer.json(200, tokenJson("ACCESS-FRESH", null, "REFRESH-1", 3600));
654+
};
655+
try (MockOidcServer server = new MockOidcServer(handler)) {
656+
FakeTokenStore fake = new FakeTokenStore();
657+
// hasOnlyTokenChars rejects a non-ASCII char (> 0x7e), not just a control char: a persisted served
658+
// token carrying one (here U+00E9) is the byte the ASCII Authorization-header writer would
659+
// truncate, so adopt() must reject the entry and fall back rather than serve a corrupt credential
660+
fake.loadReturns = new PersistedToken("ACC\u00e9SS", null, "REFRESH-1", System.currentTimeMillis() + 300_000, 300_000);
661+
try (OidcDeviceAuth auth = baseBuilder(server).tokenStore(fake).build()) {
662+
String result = auth.signIn();
663+
Assert.assertEquals("ACCESS-FRESH", result);
664+
Assert.assertNotEquals("ACC\u00e9SS", result);
665+
}
666+
Assert.assertTrue("a rejected non-ASCII persisted token must fall back to the device flow", device.get() >= 1);
667+
}
668+
});
669+
}
670+
585671
private static OidcDeviceAuth.Builder baseBuilder(MockOidcServer server) {
586672
return OidcDeviceAuth.builder()
587673
.clientId("questdb")

0 commit comments

Comments
 (0)