RemoveKey shifts later keys into the removed index, so a forward loop skipped
the key after each removal. Two adjacent alg=none keys left the second one in
the set handed to signature verification.
Client assertion signing is algorithm-agnostic, but the fixtures only
produced RS256 keys, so nothing proved it. Covers RSA, ECDSA and Ed25519,
including both RFC 9864 identifiers.
Also drops a kty assignment that jwx ignores and that would have mislabelled
non-RSA keys had it taken effect.
Fail at startup rather than on the first token request when the identity
provider does not accept the client assertion algorithm. The check is
skipped when the provider omits the field, which is optional in Discovery.
Both the assertion signer and the provider validation derived the algorithm
from the key and had to handle a missing "alg" that NewClientConfig already
rejects. Validate it once at construction and keep the result.
The flag never described id_token verification. It assigns an algorithm to
provider JWKS keys that omit "alg", which is a compatibility shim for
providers such as Azure and ID-porten.
BREAKING CHANGE: openid.id-token-signing-alg is now openid.jwks-fallback-alg.
A config file with the old key fails to start. The old environment variable
is ignored and the value falls back to RS256.
Both were reported by deadcode: retry.WithBase was never used, and
PrometheusMiddleware.Initialize is a leftover from the chi-prometheus
code this middleware was adapted from.
When an equal collector was already registered, the caller kept using
its own instance, whose observations are never scraped. RegisterCollector
now returns the registered collector so callers can adopt it.
Each finding was reviewed individually; the annotations record why the
flagged code is safe rather than suppressing the rules globally.
- G101: viper configuration keys, not credentials
- G117: the marshalled value is the plaintext input to the encryption
that immediately follows, or a token endpoint response that is
required to carry tokens
- G118: the background deletion must outlive the request, so the
request context deliberately is not used
- G124: cookie attributes are validated in config.Cookie.Validate
- G710: the redirect targets are relative by construction, validated by
the url validators, or read from the provider's metadata document
The retry callback assigned to the enclosing if-scoped err instead of
declaring its own. Nothing read that variable after the handler
returned, so behaviour is unchanged.
Requests to the token and pushed authorization endpoints carry the
client secret or a signed client assertion. Following a redirect would
forward those credentials to a host the provider never advertised.
The metadata document was fetched with http.Get, which has no deadline,
so a hung connection blocked startup indefinitely. The response status
was never checked either, leaving an error page to surface as a JSON
decoding error.
The fetch now runs with a 10s deadline derived from the context passed
in from main, goes through the shared transport so it is traced and
pooled, and rejects any non-200 response.
Registration errors were discarded, leaving the collector silently
absent from the metrics endpoint while the vectors still recorded.
Duplicate registrations remain ignored; they are expected when several
instances are constructed in the same process.
Satisfies errcheck. Errors that carry no actionable information are
ignored explicitly: writes to an already-committed response, and closing
a fully read response body or file.
Test fixture setup asserts with require.NoError instead, since a failure
there means the fixture itself is broken.
Introduce SecondsNotApplicable for *_in_seconds fields whose feature is
disabled (no inactivity timeout, auto-refresh off), replacing scattered
magic -1 values and the post-construction mutation in Verbose(). JSON
output is unchanged.
Consolidate the refresh schedule into a single NextRefresh function (leeway +
inactivity half-life, floored at the cooldown). ShouldRefresh now defers to it,
so the explicit cooldown gate is redundant and removed. Add doc comments naming
the mechanisms. No behavior change.
Refreshes are activity-based and synchronous-on-expiry, so the proactive
leeway only hides refresh latency and provides retry headroom; it is not
what prevents serving expired tokens. Shrink it from 5m to 30s to align
with identity provider guidance against refreshing long before expiry,
while keeping decent end-user UX.
The previous if/else only checked for WarnLevel, causing any other
level (e.g. InfoLevel in 24fb9b1) to fall through to Errorf. Use Logf to
dispatch on the actual level passed by callers.
Move metrics and probe listeners from main.go into server.go and
manage all servers with errgroup. This replaces log.Fatalf goroutines
with proper error propagation and ties all server lifetimes together
so a failure in any listener triggers graceful shutdown of the others.
Without IdleTimeout, clients holding keep-alive connections open indefinitely
caused server-side goroutines (and their ~16KB of buffers) to accumulate
linearly until OOM.
Some providers require that the `typ` header has a value exactly equal
to `client-authentication+jwt` in accordance with changes introduced by
RFC7523bis.
This commit allows for opting in to setting the `typ` header with this new value.
The default behaviour is to use the previous de facto standard value, `JWT`.
Once the changes in RFC7523bis lands in the affected standards and
identity providers start supporting the new `typ` header (Entra ID being
notable for not supporting this as of this commit), we will default to
use `client-authentication+jwt`.
This adds an optional health probe listener, mostly for use in rutime
environments where you want separate public and private listeners.
The existing /oauth2/ping endpoint on the main listener is kept for
backwards compatibility.
This drops the calling function, filepath and line number attributes
for redisotel tracing. The underlying implementation here calls
runtime.Callers, which are more expensive than we'd like.
While RFC 6749 specify this field as recommended:
> If omitted, the authorization server SHOULD provide the
> expiration time via other means or document the default value.
and equivalently the OIDC Core spec specifies the same field as optional,
we will explicitly enforce that these fields are returned from the AS.
This isn't a breaking change as the existing session refresh logic implicitly
depends on this field and its value.
While there are probably some providers that omit the `expires_in` field
or sets it to zero with the intent of returning access tokens that do not
expire, we assume these are relatively rare. We might revisit this
at some point in the future, should our assumptions be wrong.
Trong Huu Nguyen
