Repository navigation
feat(qwp): refreshing token providers for Entra ID authentication - #102
Draft
bluestreak01 wants to merge 8 commits into
Draft
bluestreak01 wants to merge 8 commits into
bluestreak01 wants to merge 8 commits into
Conversation
Add a cross-language specification for QWP clients that authenticate with rotating bearer tokens (Microsoft Entra ID managed identities and service principals), plus the Java findings and implementation plan. - design/qwp-token-provider-spec.md (v0.3, decisions resolved): the token-source contract, a proactively refreshing shared token cache, when clients fetch tokens, failure policy by phase (one retry after 401), connect-string keys (token_provider, azure_resource, azure_client_id), connection health, an optional authentication- outage deadline, redaction rules and conformance tests C1-C23. - design/entra-id-qwp-auth.md: findings for the Java client with code references, the design rationale, and the four-step Java plan (§10). Security-relevant server findings are reported privately and are not part of this change.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AzureTokenProviderFactory factory = new AzureTokenProviderFactory(); | ||
| Map<String, String> params = new HashMap<>(); | ||
| params.put(TokenProviderSpec.KEY_AZURE_RESOURCE, "api://qdb"); | ||
| params.put(TokenProviderSpec.KEY_AZURE_CLIENT_ID, "0a1b2c3d-4e5f-6a7b-8c9d-0e1f2a3b4c5d"); |
Contributor
There was a problem hiding this comment.
🛑 Gitleaks has detected a secret with rule-id generic-api-key in commit 576383b.
If this secret is a true positive, please rotate the secret ASAP.
If this secret is a false positive, you can add the fingerprint below to your .gitleaksignore file and commit the change to this branch.
echo 576383b498f6ab57f6d34fae2e696f2f8210ae08:azure/src/test/java/io/questdb/client/azure/test/AzureTokenSourceTest.java:generic-api-key:137 >> .gitleaksignore
Member
Author
There was a problem hiding this comment.
False positive, and no longer applicable to this PR.
- Not a secret. The flagged value
0a1b2c3d-4e5f-6a7b-8c9d-0e1f2a3b4c5dwas a synthetic test fixture forazure_client_id(sequential hex pattern). An Azure client ID is a public identifier, not a credential, so there is nothing to rotate. The rule fired on theKEY_prefix ofKEY_AZURE_CLIENT_IDplus the string's entropy (~4.0 vs the rule's 3.5 threshold). - Commit is gone. 576383b is no longer part of this PR; the branch was rewritten. Its replacement, c9f2b96, uses the low-entropy placeholder
aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeeeon the same line. - Verified clean. The latest Gitleaks run (gitleaks 8.24.3, range
e99c7084^..6048875f) reports no leaks. A local run of the same version reproduces this finding on 576383b, and finds nothing on c9f2b96, the fullorigin/main..HEADrange, or the merge commit 16cfb41 (which--no-mergesskips in CI).
No .gitleaksignore entry added: the fingerprint would reference a commit that is no longer in the branch.
Implement the dynamic-credential specification (design/qwp-token-provider-spec.md, v0.3) and the four-step Java plan in design/entra-id-qwp-auth.md, section 10. - Token cache: RefreshingTokenProvider, ExpiringToken, TokenSource and TokenUnavailableException. Proactive jittered refresh at about half the token lifetime, a 60 s hand-out floor, single-flight cold waits bounded by cold_wait, jittered backoff honouring Retry-After, rate-limited forced refresh, and token redaction everywhere. - Client integration: one immediate same-endpoint retry after a refreshable 401 (WWW-Authenticate aware; never for a 403 or a static credential) on every ingest connect path, orphan drains included, and on egress connect and failover. SYNC startup retries a retryable provider failure within its budget (D6); any other provider exception still fails fast (D8). Egress errors name the failure class, and a query client whose failover reconnect failed reconnects on its next execute() instead of staying unusable (the suspected dead pooled worker, reproduced by a test before the fix). - Connect string: token_provider, azure_resource and azure_client_id on both clients (wss:: only; exclusive with static credentials and application-supplied providers), resolved through a ServiceLoader SPI and a process-wide ref-counted registry with a 60 s linger. Validation never fetches a token. - New optional module azure/ (org.questdb:questdb-client-azure): token_provider=azure on DefaultAzureCredential with the spec's error classification. Java 8 floor, released together with the client. - Connection health: Sender.health(), QwpQueryClient.health() and an aggregate QuestDB.health(); an optional auth_failure_max_duration_millis deadline for authentication outages. - Tests for conformance scenarios C1-C23, plus a TLS mode for the test WebSocket server (its key is generated at test time). README and design docs updated; the spec's Appendix B now names the published artifact org.questdb:questdb-client-azure.
bluestreak01
force-pushed
the
feat/qwp-entra-token-provider
branch
from
October 2, 2026 10:15
576383b to
c9f2b96
Compare
The spec classified Azure Identity's "no credential available in the chain" as permanent (section 7.5), while section 4 calls network failures and IMDS 404/410 retryable. Inside DefaultAzureCredential an unreachable IMDS produces exactly that message: the chain probes IMDS once, with a short timeout and no retries, so a transient IMDS outage failed a SYNC startup fast. Spec v0.4 follows Azure Identity's own split between fail-fast discovery and a resilient single credential: - 7.1, 7.2: a new key, azure_credential (default, managed_identity, workload_identity, environment), with its validation rules. - 7.5: library errors are classified by how the credential was selected. With one credential, configuration errors are permanent and endpoint or network failures retryable. In the discovery chain, "no credential available" is retryable and the client warns once. Errors should carry the library's innermost reason. - 4: a source that discovers its credential by probing must not treat "nothing found" as permanent on that evidence alone. - 10: conformance test C24. 12: decision D10, which records why an SMBIOS host check was rejected. Appendix B: the Java binding. Appendix D: precedents from Azure Identity, MongoDB, Google, AWS and Apache Druid. The Java implementation still follows v0.3.
Implements spec v0.4's azure_credential and aligns the spec with the code, so the two no longer disagree. - azure_credential=default|managed_identity|workload_identity|environment is accepted by the Sender, QwpQueryClient and the QuestDB facade, and validated per spec 7.2: an unknown value lists the supported ones, and azure_client_id cannot be combined with environment. default is normalized away, so existing connect strings take the same code path and share providers as before. - The other values build that credential directly. Passing AZURE_TOKEN_CREDENTIALS to DefaultAzureCredentialBuilder.configuration(), as Appendix B prescribed, does not work in azure-identity 1.18: it selects the credential but keeps the chain's one-shot IMDS probe, because each credential re-reads the selector from the process-wide configuration. A managed identity built directly retries an unreachable endpoint and rides out an outage. - A managed identity used alone reports "identity not assigned" only in a nested MSAL message. AzureTokenSource now classifies it as permanent; otherwise a sync startup retries that misconfiguration for its whole connect budget (measured: 316 s instead of ~25 s). - Retryable errors lead with their socket-level cause, such as a refused connection, which managed identity otherwise hides behind "see inner exception". Only java.net socket messages are echoed. - The discovery chain's "no credential available" stays permanent, so a host without a credential still fails fast. Instead, the provider warns once when DefaultAzureCredential starts as a chain, recommending azure_credential in production. - A workload identity missing its platform configuration no longer fails inside a client's build(): every fetch fails as permanent. The spec is amended to match (sections 4, 7.5, C24, D10, Appendix B and D), and entra-id-qwp-auth.md now points at v0.4. Tests: AzureManagedIdentityLibraryTest runs the real library in a child JVM against a local IMDS stub (MSAL reads the endpoint only from the process environment) for C24: an outage ridden out, an unreachable endpoint retryable, an unassigned identity permanent, the chain failing fast and warning once, and the platform selector silencing the warning. Plus AzureCredentialSelectionTest, new AzureTokenSourceTest cases, and TokenProviderConfigTest and config-honored coverage for the new key.
Brings in #104 (pool-wide sf_max_total_bytes budget and graceful sender close). Conflict in Sender.java: this branch moved the WebSocket build() branch into buildWebSocket(HttpTokenProvider), while #104 edited that block in place. Resolved by keeping the move and applying #104's edits to buildWebSocket: resolveSfMaxSegmentBytes()/resolveSfMaxTotalBytes(), newCursorEngine(..., sfSharedBudget, ...) for both engine constructions, and the sfSharedBudget argument on both quarantineTornSlot() calls. A token_provider sender built by the pool now charges the pool's shared segment budget like any other pooled sender.
QueryWorker.shutdown() closed the query client once its bounded 5 s join expired, even with the dispatch thread still inside execute(). A reconnect walk parked in a native connect or WebSocket upgrade does not see the shutdown interrupt, so it outlived the close: it went on to a healthy endpoint, then wrote the binds into the bind buffer close() had freed (SIGSEGV), or published an I/O thread and connection on the closed client (leak). The failover reconnect could already reach this; the reconnect on the next execute() after a failed failover made it reachable from every query on such a client. When the join expires with the dispatch thread still busy, the thread now closes its own client on its way out of runLoop(). One CAS decides which side closes, so the client is closed exactly once and never under a running execute(). Shutdown latency is unchanged and the per-query path is untouched.
executeImpl's failover-budget-exhausted exit returned without telling the connection health tracker, unlike its sibling exits. A query whose connection died on the last attempt the budget allowed, or after failover_max_duration_ms ran out, left QwpQueryClient.health() - and the QuestDB.health() aggregate - reporting CONNECTED until the next execute(). With failover_max_attempts=1 every later query fails on the same latched failure without reconnecting, so the client reported CONNECTED for its whole life while each query failed. That exit now records the loss with connectionLost(), as the other exits do: RECONNECTING, since the next execute() fails over. With a single attempt per execute() it also calls failed(): the client never reconnects, as with failover=off, so FAILED is the true state. Health is read only through health(), so query behaviour is unchanged, and the success path never reaches this code. Tests: ConnectionHealthTest covers a connection lost on the last allowed attempt (RECONNECTING, then CONNECTED on the next query) and with a single attempt (FAILED, and the next query fails without reconnecting).
The auth_failure_max_duration_millis javadoc and the ERROR log the deadline emits promised that unacknowledged rows stay in on-disk store-and-forward for a later sender or an orphan drain. That only holds with sf_dir: the option is accepted without one, memory-mode segments are malloc'd, orphan drains require sf_dir, and spec section 8.5 says memory-only data is lost when the sender closes. Document both modes in the javadoc, and make the log line name the outcome for the sender's actual backing (engine.sfDir() is null only in memory mode).
Contributor
[PR Coverage check]😍 pass : 1241 / 1388 (89.41%) file detail
|
Contributor
|
Tandem: questdb/questdb#7799 (submodule bump to |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Support for QWP clients that authenticate with rotating bearer tokens: Microsoft Entra ID managed identities and service principals, typically obtained through
DefaultAzureCredential. No QuestDB credentials are stored.Today a static
token=is captured once. Once it expires, every reconnect presents the dead token. The existingHttpTokenProviderhook already re-pulls a token on every connect round. What's missing is an expiry-aware shared cache, failure classification, a refresh signal on 401, and a connect-string way to select a provider.Status: draft, spec only
This PR currently contains the design. The Java implementation follows in the steps below.
design/qwp-token-provider-spec.md: the cross-language specification (v0.3, all decisions resolved). Other clients (Rust/Python, Go, .NET, Node) implement this document.design/entra-id-qwp-auth.md: findings for the Java client (with code references), the design rationale, and the Java implementation plan (§10).Plan (design doc §10; test IDs refer to spec §10)
ExpiringToken,TokenSource,RefreshingTokenProvider,TokenUnavailableException. Tests C1–C8, C20.onTokenRejected. Tests C9–C16, C23.token_provider,azure_resource,azure_client_id), the shared registry, and thequestdb-client-azuremodule. Tests C17–C19.Separate tickets, not in this PR:
Security-relevant server findings were reported privately (see
SECURITY.md) and are not part of this PR.