Skip to content

Commit aba8399

Browse files
glasstigerclaude
andcommitted
Refine token-store cross-process and load semantics
Run clear() under the cross-process lock. A bare deleteIfExists let a peer mid-refresh atomically rename a fresh file in just after the delete, resurrecting the entry; clear() now deletes inside inLock (which cleans up its own lock and degrades to lock-free if it cannot acquire one). A Files.isDirectory guard keeps clear() a true no-op on a never-used store rather than creating the directory just to run the locked delete. Cross- process clear stays best-effort: a peer holding a live in-memory token may legitimately re-persist afterwards. Derive the adopted token lifetime from the absolute expiry. adopt() clamped expires_at_millis and token_ttl_millis from the file independently, so a tampered file could make them disagree and throw off effectiveSkewMillis (which caps the skew at half the lifetime). Derive tokenTtlMillis from the clamped expiry instead, so the skew basis always matches the authoritative remaining lifetime; a legitimate file is unaffected. Document the cross-process contract more precisely. The README now notes that lock-file staleness is judged by modification time, so coordination assumes a shared clock (a single machine or synchronized clocks; NFS clock skew can mis-judge it), and that clearCache() is best-effort across processes. The frozen on-disk contract now states the document must be a single flat JSON object, which the parser already enforces by rejecting an array-wrapped or non-object shape - so the Python client mirrors it. Order parseAndVerify before parseLongOrZero to match the alphabetical member ordering. Add tests for the expiry-derived lifetime and the clear-on-empty-store no-op; both were proven to fail with their fix reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2128e54 commit aba8399

6 files changed

Lines changed: 77 additions & 16 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ try (OidcDeviceAuth auth = OidcDeviceAuth.fromQuestDB(
248248

249249
The token is stored as **plaintext JSON protected by file permissions**`0600` file, `0700` directory on POSIX systems (Linux, macOS), the same approach `gcloud`, `aws` and `gh` take. On Windows these POSIX permissions cannot be enforced, so the file currently relies on the user-profile directory's default ACL (owner-only ACL hardening is a follow-up); the client prints a one-line warning to `System.err` the first time it cannot enforce them. Enabling persistence therefore writes a long-lived refresh token to disk: anyone who can read the file holds a credential until it expires or is revoked. To encrypt it at rest, supply your own `TokenStore` (backed by an OS keychain or a secrets manager) instead of `FileTokenStore`. A persisted file is treated as untrusted input on load — a tampered, corrupt, oversized, or identity-mismatched entry is ignored (the client falls back to a refresh or an interactive sign-in), and a token carrying control or non-ASCII characters is never placed on the wire.
250250

251-
`FileTokenStore` is safe to share between processes that sign in as the same identity: each update is written atomically (so a concurrent reader never sees a half-written credential), and when the identity provider rotates the refresh token on each refresh, the read-refresh-write is serialized across processes with a lock file so they do not race each other into an unnecessary re-prompt.
251+
`FileTokenStore` is safe to share between processes that sign in as the same identity: each update is written atomically (so a concurrent reader never sees a half-written credential), and when the identity provider rotates the refresh token on each refresh, the read-refresh-write is serialized across processes with a lock file so they do not race each other into an unnecessary re-prompt. The lock file's staleness is judged by its modification time, so this coordination assumes the processes share a clock — a single machine, or machines with synchronized clocks; under significant clock skew (for example a store directory on NFS shared across hosts) a live lock can be mis-judged stale or a dead one never expire. `clearCache()` removes the persisted entry under the same lock, but across processes it is best-effort: a peer that still holds a live in-memory token may legitimately re-persist afterwards (it always forces a fresh sign-in for the calling process).
252252

253253
### Explicit Timestamps
254254

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

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -191,11 +191,22 @@ public static FileTokenStore atDefaultLocation() {
191191

192192
@Override
193193
public void clear(TokenStoreKey key) {
194-
try {
195-
Files.deleteIfExists(tokenFile(key));
196-
} catch (IOException e) {
197-
throw new OidcAuthException(e).put("could not remove the OIDC token store file");
198-
}
194+
if (!Files.isDirectory(directory)) {
195+
return; // nothing is persisted yet; do not create the directory just to clear it
196+
}
197+
// delete under the cross-process lock, like the read-refresh-write, so a peer's in-flight refresh
198+
// cannot resurrect the entry by atomically renaming a fresh file in just after we delete. inLock cleans
199+
// up its own lock file and degrades to lock-free if it cannot acquire one. Cross-process clear is still
200+
// best-effort: a peer holding a live in-memory token may legitimately re-persist later - clearing forces
201+
// a fresh sign-in for THIS process regardless, since the caller resets its in-memory token state.
202+
inLock(key, () -> {
203+
try {
204+
Files.deleteIfExists(tokenFile(key));
205+
} catch (IOException e) {
206+
throw new OidcAuthException(e).put("could not remove the OIDC token store file");
207+
}
208+
return true;
209+
});
199210
}
200211

201212
@Override
@@ -295,14 +306,6 @@ private static boolean nullableEquals(String keyValue, StringSink fileValue) {
295306
return fileHasValue && Chars.equals(keyValue, fileValue);
296307
}
297308

298-
private static long parseLongOrZero(CharSequence value) {
299-
try {
300-
return Numbers.parseLong(value);
301-
} catch (NumericException e) {
302-
return 0;
303-
}
304-
}
305-
306309
private static PersistedToken parseAndVerify(TokenStoreKey key, byte[] bytes) {
307310
if (bytes.length == 0) {
308311
return null;
@@ -339,6 +342,14 @@ private static PersistedToken parseAndVerify(TokenStoreKey key, byte[] bytes) {
339342
return new PersistedToken(accessToken, idToken, refreshToken, parser.expiresAtMillis, parser.tokenTtlMillis);
340343
}
341344

345+
private static long parseLongOrZero(CharSequence value) {
346+
try {
347+
return Numbers.parseLong(value);
348+
} catch (NumericException e) {
349+
return 0;
350+
}
351+
}
352+
342353
private static void putBooleanMember(StringSink sink, String name, boolean value) {
343354
sink.put(',');
344355
putName(sink, name);

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,8 +1038,13 @@ private boolean adopt(PersistedToken token) {
10381038
// positive and serve a garbage-expiry token as valid forever. An already-expired entry still reads as
10391039
// expired and falls through to a refresh rather than being served.
10401040
long maxTokenLifeMillis = MAX_EXPIRES_IN_SECONDS * 1000L;
1041-
tokenTtlMillis = Math.max(0L, Math.min(token.getTokenTtlMillis(), maxTokenLifeMillis));
1042-
expiresAtMillis = Math.max(0L, Math.min(token.getExpiresAtMillis(), System.currentTimeMillis() + maxTokenLifeMillis));
1041+
long now = System.currentTimeMillis();
1042+
expiresAtMillis = Math.max(0L, Math.min(token.getExpiresAtMillis(), now + maxTokenLifeMillis));
1043+
// derive the trusted lifetime from the clamped absolute expiry, not the file's separately-stored ttl, so
1044+
// a tampered file cannot make the two disagree and throw off effectiveSkewMillis (which caps the skew at
1045+
// half the lifetime); for a legitimate file the two already agree, and the expiry clamp bounds this to
1046+
// [0, maxLife]
1047+
tokenTtlMillis = Math.max(0L, expiresAtMillis - now);
10431048
// it is already on disk, so a later non-rotating refresh must not rewrite the file
10441049
lastPersistedRefreshToken = refreshToken;
10451050
return true;

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,18 @@ public void testClearDeletesFile() throws Exception {
170170
});
171171
}
172172

173+
@Test
174+
public void testClearOnEmptyStoreIsNoOp() throws Exception {
175+
assertMemoryLeak(() -> {
176+
Path dir = storeDir(); // a non-existent subdirectory
177+
FileTokenStore store = new FileTokenStore(dir);
178+
// clearing an identity that was never saved must be a no-op and must not create the store directory
179+
// just to run the now-locked delete
180+
store.clear(sampleKey());
181+
Assert.assertFalse("clear must not create the store directory", Files.exists(dir));
182+
});
183+
}
184+
173185
@Test
174186
public void testControlCharactersRoundTrip() throws Exception {
175187
assertMemoryLeak(() -> {

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,35 @@ public class OidcDeviceAuthPersistenceTest {
5656
@Rule
5757
public final TemporaryFolder temp = TemporaryFolder.builder().assureDeletion().build();
5858

59+
@Test(timeout = 30_000)
60+
public void testAdoptDerivesTtlFromExpiry() throws Exception {
61+
assertMemoryLeak(() -> {
62+
AtomicInteger device = new AtomicInteger();
63+
AtomicInteger token = new AtomicInteger();
64+
MockOidcServer.Handler handler = countingHandler(device, token, "ACCESS-FRESH", "REFRESH-FRESH", "ACCESS-2", "REFRESH-2");
65+
try (MockOidcServer server = new MockOidcServer(handler)) {
66+
FakeTokenStore fake = new FakeTokenStore();
67+
// a (tampered) entry whose stored ttl (5m) disagrees with its absolute expiry (now + 10m).
68+
// adopt() must derive the trusted lifetime from the authoritative expiry, not the stored ttl, so
69+
// the effectiveSkewMillis basis matches the real remaining lifetime
70+
long now = System.currentTimeMillis();
71+
fake.loadReturns = new PersistedToken("ACCESS-1", null, "REFRESH-1", now + 600_000, 300_000);
72+
try (OidcDeviceAuth auth = baseBuilder(server).tokenStore(fake).build()) {
73+
Assert.assertEquals("the still-valid persisted token is served", "ACCESS-1", auth.signIn());
74+
long ttl = readPrivateLong(auth, "tokenTtlMillis");
75+
long expiry = readPrivateLong(auth, "expiresAtMillis");
76+
Assert.assertTrue("ttl must be derived from the ~10m expiry, not the stored 5m: " + ttl,
77+
ttl >= 9 * 60_000L);
78+
Assert.assertTrue("ttl must not exceed the clamped 1h lifetime: " + ttl, ttl <= 60 * 60_000L);
79+
Assert.assertTrue("ttl must match expiresAtMillis - now within tolerance",
80+
Math.abs(ttl - (expiry - now)) < 5_000L);
81+
}
82+
Assert.assertEquals("no device flow for a valid persisted token", 0, device.get());
83+
Assert.assertEquals("no refresh for a valid persisted token", 0, token.get());
84+
}
85+
});
86+
}
87+
5988
@Test(timeout = 30_000)
6089
public void testBuilderRejectsHttpTimeoutAboveCap() throws Exception {
6190
assertMemoryLeak(() -> {

design/oidc-token-persistence.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,10 @@ serving — but it defeats *sharing*, leaving each client to re-prompt).
337337
encoding under which every present value round-trips verbatim (a token equal to the string
338338
`"null"` included); a reader treats an absent field as null. The Python client MUST do the
339339
same: omit null fields on write, and treat an absent field as null on read.
340+
341+
The document MUST be a single flat JSON object. A reader rejects any other shape - an array
342+
anywhere (for example a top-level `[ {…} ]` wrapper) or a non-object root - rather than
343+
extract fields from a malformed structure. The Python client MUST do the same.
340344
- **Write protocol (atomicity):** write a sibling temp file created with 0600, flush, then
341345
**atomically rename** over the target — Java `Files.move(tmp, target, ATOMIC_MOVE,
342346
REPLACE_EXISTING)`, Python `os.replace(tmp, target)`. Both are `rename(2)` on POSIX

0 commit comments

Comments
 (0)