smolnar82 opened a new pull request, #1457:
URL: https://github.com/apache/knox/pull/1457

   [KNOX-3516](https://issues.apache.org/jira/browse/KNOX-3516) - Log and 
describe JWT rejections caused by an untrusted issuer
   
   ## What changes were proposed in this pull request?
   
   When a JWT's `iss` is neither in `jwt.expected.issuer` nor a 
runtime-registered trusted issuer, `AbstractJWTFilter.validateToken(...)` 
rejected the request with a 401 and logged nothing. It also passed a `null` 
error message, so the client got a bare 401 on the plain path and 
`{"error":"invalid_request"}` with no `error_description` on the token-exchange 
path. The failure could not be seen on either side.
   
   This PR:
   
   - **Logs the rejection** at WARN: `Rejected token {0} ({1}) with unexpected 
issuer {2}: ...`. It uses the redacted token display text and token id, the 
same as the other rejection paths. The display text is always present, which 
matters because foreign tokens (K8s service-account tokens, external OIDC) 
usually have no `knox.id`, so the token id is `null`.
   - **Tells the client why**: the error is now `Token issuer '<iss>' is not 
trusted`. On token exchange it is returned as `error_description`; on the plain 
path it is passed to `sendError(401, ...)`.
   - **Sanitizes the issuer** with the new 
`TokenUtils.getIssuerDisplayText(String)`. The issuer is read before the 
signature is verified, so the caller controls it. Control characters are 
replaced to prevent log forging, and the length is capped at 256 characters to 
limit log flooding.
   - **Keeps the fixed text at the end of the error message.** 
`SSOCookieFederationFilter.isInvalidSsoCookie()` decides by the end of the 
error string (`" is disabled"` / `" exceeded idle timeout"`). If the issuer 
ended the message, a crafted `iss` could trigger cookie removal, session 
invalidation and global logout.
   
   ## How was this patch tested?
   
   New and updated unit tests; `mvn verify` passes on `gateway-spi` and 
`gateway-provider-security-jwt`, including checkstyle.
   
   - `UnexpectedIssuerRejectionTest` (new):
     - a foreign token (no `knox.id`) from an untrusted issuer produces the log 
record with the expected arguments and the expected 401 message;
     - CR/LF in `iss` is neutralized in both the log and the client message;
     - an issuer ending in `" is disabled"` / `" exceeded idle timeout"` cannot 
produce an error string with that ending.
   - `TokenUtilsTest`: `getIssuerDisplayText` with a normal issuer, null, C0/C1 
control characters, truncation, and a value exactly at the length limit.
   - `JWTFederationFilterTokenExchangeTest`:
     - `testUntrustedIssuerRejectedNoHttpCall` and `testServiceUnavailable` now 
require the `error_description`;
     - `testNonTokenExchangeRegistryIssuerRejected` now verifies the 
`sendError` message.
   
   ## Integration Tests
   N/A


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to