[
https://issues.apache.org/jira/browse/KNOX-3501?focusedWorklogId=1045881&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1045881
]
ASF GitHub Bot logged work on KNOX-3501:
----------------------------------------
Author: ASF GitHub Bot
Created on: 06/Oct/26 09:09
Start Date: 06/Oct/26 09:09
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1441:
URL: https://github.com/apache/knox/pull/1441#discussion_r4193487235
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java:
##########
@@ -251,22 +262,95 @@ protected void setJwsTypeVerifier(final FilterConfig
filterConfig, final String
}
protected boolean tokenIsStillValid(final JWT jwtToken) throws
UnknownTokenException {
- Date expires =
getServerManagedStateExpiration(TokenUtils.getTokenId(jwtToken));
+ return tokenIsStillValid(jwtToken, false);
+ }
+
+ /**
+ * @param tolerateUnknownState when true, a token with no server-managed
state record falls back to
+ * its own {@code exp} claim instead of raising {@link
UnknownTokenException}; see
+ * {@link #ALLOW_UNKNOWN_COOKIE_TOKEN_STATE}.
+ */
+ protected boolean tokenIsStillValid(final JWT jwtToken, final boolean
tolerateUnknownState) throws UnknownTokenException {
+ return tokenIsStillValid(jwtToken,
+ resolveServerManagedState(TokenUtils.getTokenId(jwtToken),
tolerateUnknownState, null));
+ }
+
+ private boolean tokenIsStillValid(final JWT jwtToken, final
ServerManagedState state) {
+ Date expires = state.getExpiration();
if (expires == null) {
// if there is no expiration date then the lifecycle is tied entirely to
// the cookie validity - otherwise ensure that the current time is before
// the designated expiration time
expires = jwtToken.getExpiresDate();
+ if (expires == null && state.isAbsentTolerated()) {
+ /*
+ * Tolerating absent state means deferring to the token's own exp
claim, so there has to
+ * be one. With neither a state record nor an exp claim the cookie has
no expiry at all,
+ * it would authenticate forever, and (state being absent) could never
be revoked which
+ * would be a problem so reject it.
+ */
+
log.toleratedCookieTokenHasNoExpiry(Tokens.getTokenIDDisplayText(TokenUtils.getTokenId(jwtToken)),
+ ALLOW_UNKNOWN_COOKIE_TOKEN_STATE);
+ return false;
+ }
}
return expires == null || new Date().before(expires);
}
protected boolean tokenIsStillValid(final String tokenId) throws
UnknownTokenException {
- Date expires = getServerManagedStateExpiration(tokenId);
+ Date expires = resolveServerManagedState(tokenId, false,
null).getExpiration();
return expires == null || (new Date().before(expires));
}
- private Date getServerManagedStateExpiration(final String tokenId) throws
UnknownTokenException {
+ /**
+ * The outcome of consulting server-managed token state for one request.
+ *
+ * <p>A null {@link #getExpiration()} is ambiguous on its own, it covers
+ * 1. "no state service configured",
+ * 2. "token carries no knox.id",
+ * 3. "state record holds no expiry" and
+ * 4. "no state record exists".
+ * {@link #isAbsentTolerated()} singles out the last case ("no state record
exists"),
+ * and only when explicitly opted in,
+ * so that callers can treat a absent record differently from a token Knox
+ * has no opinion about.
+ */
+ private static final class ServerManagedState {
Review Comment:
**Do we need `ServerManagedState` here?** The behavior and the edge-case
coverage look right to me: the no-exp-claim rejection, known-but-expired, and
"metadata throws while expiration succeeds" cases are all real and well-tested,
and the comments are great. My question is narrower: whether this new type
earns its keep.
As far as I can tell it carries exactly two things:
1. a nullable `expiration` (which we already returned as a bare `Date`
before this PR)
2. and the `absentTolerated` boolean.
That boolean is the only genuinely new piece of information, and it's
load-bearing: the metadata lookup must be tolerated only when the *expiration*
lookup actually observed absence, not just when the flag is on (nicely pinned
down by `testCookieAuthKnownStateButUnknownMetadataStillRejected`). So the
distinction is real, but it's only one boolean.
An abstraction like this pays off when several call sites need the bundled
value, or when it collapses tangled logic. Here only `doFullTokenValidation`
consumes both fields together; `tokenIsStillValid(String)` reads only the
expiration and never tolerates, and the new `tokenIsStillValid(JWT, boolean)`
overload ends up test-only (production uses the private `(JWT,
ServerManagedState)` form). So the type is carrying a pair across a single
short hop, and to thread it through we've grown `tokenIsStillValid` from one
shape to four plus the `NO_OPINION`/`ABSENT_TOLERATED`/`of()` sentinels and a
defensive-copy accessor.
I think the same thing reads more simply as a local try/catch at the one
site that needs it:
```java
Date expires;
boolean absentTolerated = false;
try {
expires = getServerManagedStateExpiration(tokenId); // keep the original
throwing helper
} catch (UnknownTokenException e) {
if (!tolerateUnknownState) throw e;
log.unknownCookieTokenStateTolerated(displayableTokenId,
ALLOW_UNKNOWN_COOKIE_TOKEN_STATE);
expires = null;
absentTolerated = true;
}
```
That drops the class, the sentinels, and the two extra overloads, and keeps
the logic where it's used.
One more thing regardless of the above: the name `ServerManagedState` reads
as the authoritative token state (enabled/disabled, idle-timeout, revocation),
which it deliberately doesn't hold; that's still `TokenMetadata`, fetched
separately. If we do keep a return type, could we at least name it for what it
is (e.g. `ServerManagedExpiration`) and, on Java 17, make it a one-line
`record`?
Not blocking - the patch is correct as-is. Mostly want to check whether the
encapsulation is buying us something I'm not seeing.
Issue Time Tracking
-------------------
Worklog Id: (was: 1045881)
Time Spent: 0.5h (was: 20m)
> JWT cookie tokens fail validation when knox.token.exp.server-managed=true
> -------------------------------------------------------------------------
>
> Key: KNOX-3501
> URL: https://issues.apache.org/jira/browse/KNOX-3501
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
> Reporter: Sandeep More
> Assignee: Sandeep More
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently, JWT tokens in a cookie (e.g. hadoop-jwt) fail validation when
> `knox.token.exp.server-managed=true` We need a way to ensure that the
> validation passes for hadoop-jwt even when
> `knox.token.exp.server-managed=true` .
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
