Repository navigation
release: structures 3.8.0 - #28
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Authentication could hang for 15+ minutes when an OIDC provider stopped answering. The JWKS client had no response timeout and reused pooled connections, so a request written to a connection silently dropped by a NAT or load balancer waited on TCP retransmission before failing. - Only fetch for issuers that match an enabled provider's authority; tokens from other issuers are rejected before any network call - Connect and end-to-end timeouts on every fetch, configurable, and no connection pooling for these rare fetches - Failed fetches, empty key sets and discovery documents without a jwks_uri are not cached; concurrent lookups share one fetch - Cache the whole key set; an unknown kid refetches at most once per refresh cooldown, so rotation works without a fetch per bad token - Match .local on the host only for the insecure dev client - Optional per-provider jwks-uri for when the authority's public URL is not reachable from the cluster JwksResilienceTest covers each case against a fake IdP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups on the JWKS fetch changes: - Key sets and discovery documents are refreshed with Caffeine's refreshAfterWrite instead of expiring. Lookups are served the cached document during a refresh, and a failed refresh keeps it, so a token with a made up kid, or an expired discovery entry, during an IdP outage no longer breaks valid logins - An unknown kid refreshes via cache.refresh(), deduplicated, and the cooldown also limits failed refreshes - jwksUri is @JsonIgnore, so an in-cluster IdP URL is not published in the unauthenticated frontend configuration - Issuer to provider matching lives in one place, OidcSecurityServiceProperties.findEnabledProviders - Build the JWK parser once; drop the warn that duplicated Caffeine's own log of failed loads - Shut down each fake IdP's executor in JwksResilienceTest Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The frontend configuration endpoint is unauthenticated and serialized the whole OidcSecurityServiceProperties, so backend-only settings (audience, required roles, roles claim path, allowAnyDomain, jwksUri, tenantIdFieldName and the JWKS fetch settings) were published with it. It now serves OidcFrontendConfiguration, an allow-list of the fields the frontend reads, for enabled providers only. A new setting is not sent unless it is added there, so the @JsonIgnore annotations are dropped. No frontend change is needed: it never read the removed fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…owed issuers Second review follow-ups: - Tokens with an unknown kid within the cooldown of a refresh now use that refresh, so a burst of tokens signed with a rotated key all wait on one fetch instead of all but the first being rejected - A failed refresh keeps the cached copy until the next refresh is due, instead of every lookup refetching; the 24 h key set staleness bound is checked in getKey since that resets Caffeine's write time - After a fetch fails with nothing cached, lookups fail with its error for jwks-retry-backoff (5 s) before fetching again - A token whose issuer is not the authority of an enabled provider, unknown or disabled, fails with AuthenticationException "Issuer not allowed: <iss>". OidcSecurityService completes with the original error rather than a CompletionException, so the gateway reports it - Disabled providers are sent to the frontend again, enabled false, since its basic auth toggle counts configured providers - Log rejected unknown key ids; Helm and README cover jwksUri and the fetch settings; fix the refreshAfterWrite javadoc; small cleanups Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- tenantIdFieldName, and per provider allowAnyDomain, frontEndRoles and additionalScopes, each rendered only when set - rolesClaimPath is rendered only when set too. Left empty it rendered as "", which OidcSecurityService took as a roles path, so every token was rejected with "No roles found in claims". A blank path now counts as unset in OidcSecurityService as well - values.yaml documents each option, and that provider metadata cannot be set this way: Spring cannot bind map entries from environment variables under the oidc-security-service prefix Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Send the frontend only the endpoint keys of provider metadata, which the backend also merges into every Participant; none sends null, as the frontend treats any metadata object as endpoint overrides - A failed refresh for an unknown kid reports the fetch error instead of "key not found" - Check key set staleness before the lookup and compare by value, so a failed refresh's new future cannot bypass the 24 h bound and an idle deployment does not fetch twice - Rejections of unverified tokens log at warn at most once a minute, with the token's claims shortened and control characters replaced - Drop the Accept header, as before, so strict JWKS endpoints are not refused; trust-all dev TLS also covers *.localhost - Start unknown-kid refreshes outside ConcurrentHashMap.compute; one shared CompletionException unwrap - Helm refuses JWKS durations without a unit, since Spring would read a bare number as milliseconds Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compared with other JWKS clients, the 24 h staleness cap sat on the strict side: a network problem lasting a day rejected tokens that were still valid. Follow Microsoft.IdentityModel and go-oidc instead. - The last fetched keys are used for as long as an outage lasts; the new jwks-max-staleness caps that for deployments that prefer to fail closed. A failed refresh logs the age of the keys it keeps - jwks-refresh-interval (1 h) makes the routine refresh configurable - The unknown kid cooldown counts only from refreshes unknown key ids started, so a rotation just after a routine fetch is picked up - Tests for an outage with and without the cap, a failed routine refresh retried after the interval, keys published ahead of use, and a rotation right after a fetch - Docs: behaviour per situation, comparison with other libraries, and rotating Keycloak keys without rejected logins; README and Helm cover the new settings Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Evict stale key sets with computeIfPresent: asMap().remove(key, value) reads the entry, which started the refresh the eviction is meant to avoid, so a down IdP was asked twice per lookup - A refresh an unknown kid starts is always used, so a zero cooldown no longer rejects every rotated key; fetch durations are validated at startup instead of failing at the first lookup - Rate-limit logging of failed JWT validation, which anyone can trigger with a forged signature, through the same RejectionLog as the JWKS rejections; log JWKS keys that cannot be parsed at warn - Skip null provider metadata values instead of failing the frontend configuration endpoint - Helm accepts durations in any case, as Spring does - Correct the refreshAfterWrite javadoc on what a failed refresh does Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The discovery document was the last fetch on a hardcoded interval (24 h). It now refreshes on jwks-refresh-interval like key sets, so one setting controls how often Structures refreshes from the IdP. It still has no maximum age: a failed refresh keeps the cached document. Tests cover one lookup refreshing both, and a failed discovery refresh leaving key lookups working. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A review of the JWT -> discovery document -> JWKS key flow, and of the docs against the code: - RejectionLog sanitizes every string it logs. JJWT copies the token's alg header into its error message, so a forged alg could put a newline into the log. One rate-limit window is shared by all rejection logs - jwks-max-staleness must be longer than jwks-refresh-interval; shorter, healthy keys were evicted before a refresh was due, so logins waited on a fetch every time - An unknown kid refresh that throws while starting completes its shared future, so lookups waiting on it cannot hang; drop the unreachable null branch - A successful load clears the previous refresh error, so it is not reported for a later missing key; the refresh failure is logged with the rejection - Docs: staleness counted from the last successful fetch, lazy refresh on idle pods, a changed jwks_uri loading without fallback, what clients see, and the components section and tuning notes Tests: forged alg in the log, a changed jwks_uri used and failing, discovery retry backoff, a lookup not waiting on either refresh, the staleness/interval check, and an exact unknown kid fetch count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Found in the KinD smoke test: for a discovery document, which has no age, the warning read "Keeping the cached OIDC discovery document for <issuer> refreshing it failed". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix: OIDC JWKS fetching: timeouts, outage-tolerant caching, issuer allow-list
A write that lost an optimistic locking race reached callers as a 500. The async client hands a 409 back as the low level ResponseException, since the transport only asks the low level client to pass 400-405 through. Its message also named the Elasticsearch host and request URI, which STOMP callers saw. Save, update, bulkSave and bulkUpdate now turn a 409 into the new VersionConflictException, with the reason read from the response body. The REST failure handler maps it to 409, and the OpenAPI spec documents 409 on the write operations of versioned and stream structures. A bulk request fails with a conflict only when every failed item conflicted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Shared structures only show the data of the session's tenant, so a user signed into the wrong tenant saw an empty or unexpected view with no clue why. The header now shows the tenant the server resolved for the session, and the avatar menu shows the user and tenant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A 409 also happens without a version field, when another write changes an entity while an update is applied. Elasticsearch is still not asked to retry it, since that would apply the partial update over the other write without the caller knowing; the message says to send the request again instead of to read a version the entity does not have. The OpenAPI spec lists 409 on update and bulk update for every Structure. The message always says "version conflict", even when no reason could be read. A bulk request is a conflict only when it has conflicts and nothing else failed, and the message counts the items not written. The tenant badge tooltip now just says "Your tenant". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Found in the review of PR #26: a bulk call with items Elasticsearch rejected under load (429) or failed with a 5xx answers 400, so clients treat it as bad data and don't retry. Noted in the write throttling plan, with the status each kind of failure should map to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Stream structures only create, so their conflicts now say the id already exists instead of telling the caller to re-read a version. - A bulk conflict without item reasons still says "version conflict", and the reasons and the advice no longer run together. - Bulk errors list at most 20 reasons plus "and N more", and a set finds duplicates, so a large call no longer builds a quadratic, multi-megabyte message. - The reason is parsed from the response bytes, since Elasticsearch's compatibility content type names no charset and the string fallback was ISO-8859-1. Also records, in the write throttling doc, that non-409 errors from the low level client still reach callers with the Elasticsearch host and URI in the message. That fix is held for its own PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 20-reason cap from the last commit meant callers could not tell which items past the first 20 failed. They find failed items by reading the document ids out of the reasons, so every distinct reason is listed again, as before this branch. A set still drops duplicates. The throttling doc records the redesign: telling bulk callers which items failed with a structured result instead of a message to parse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Over STOMP: a save without a version for an existing id, a bulk update where one of three items is stale (the other two are written), and a bulk save of existing ids. Over REST: a bulk update with a stale item answers 409. The STOMP cases match the message from its start, which shows the client gets the exception's own message, not a wrapper's. Run against KinD with this branch's image: 11 of 11 pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-tenant-badge fix: 409 for version conflicts, and show the signed-in tenant
A minor version: #25 adds public configuration (the oidc-security-service jwks-* settings, also in Helm), and #26 adds the public VersionConflictException and answers 409 where callers got 500. The Helm chart moves to 1.2.0 for the new OIDC options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
build: version 3.8.0, with docker-compose, KinD and Helm pointed at it
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
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.
OIDC logins no longer hang when an identity provider stops answering, conflicting writes answer 409 instead of 500, and the UI shows which tenant you're signed into.
issmust equal theauthorityof an enabled provider. Otherwise it fails withIssuer not allowed: <iss>before anything is fetched.jwks-max-stalenesscan cap that.kids refresh at most once per cooldown.oidc-security-service.jwks-*settings, also in Helm./app-config.override.jsonnow serves only the fields the UI reads.rolesClaimPathrendered as""and rejected every token.@Version) reached callers as a 500, with the Elasticsearch host and request URI in the message.VersionConflictException: REST answers 409, the TS client rejects with a message startingVersion conflict writing <Structure>, and Java callers can catch the type.@Version, two racing updates conflict too, and the message says to send the request again. Structures doesn't retry those itself, since that would apply the change over the other write silently.appVersion, image tags, chart1.2.0) point at 3.8.0.Upgrade notes
authoritybefore deploying. It must match theissclaim in that provider's tokens exactly, scheme, host, port and path included. A mismatch now fails every login from that provider withIssuer not allowed.version conflict, but now starts withVersion conflict writing <Structure>instead of the raw Elasticsearch text.Known gaps, recorded in
docs/performance/ELASTICSEARCH_WORKLOAD.mdTesting
JwksResilienceTest(32 cases), plus 74/74 auth and frontend-config tests, including the Keycloak-backed ones.VersionConflictTests,ElasticVersionConflictsTestand the full Gradle suite pass. The TS e2e versioned suites pass 11/11 against KinD with the branch image.🤖 Generated with Claude Code