Repository navigation
Encode HTTP Basic credentials as UTF-8, not in the platform default charset - #29694
Conversation
…harset Five places build an Authorization/Proxy-Authorization header by Base64-encoding "user:password" through String.getBytes() with no argument, which uses the JVM's default charset: camel-as2 AS2HeaderUtils.addAuthorizationHeader camel-keycloak KeycloakProducer (token revocation) camel-keycloak KeycloakTokenIntrospector camel-smpp SmppConnectionFactory.connectProxy camel-wordpress WordpressBasicAuthentication RFC 7617 says the credentials are encoded with UTF-8 unless the server asks for another charset, and that is what Keycloak, WordPress and ordinary HTTP proxies expect. The default charset is only UTF-8 when nothing overrides it: -Dfile.encoding, file.encoding=COMPAT, or a JVM before 18 following the platform locale, all give something else. A user name or password holding a non-ASCII character then produces a different Authorization header on different machines, and the server rejects it with 401 even though the credentials are right. Since the bytes are Base64-encoded, nothing in the error points at the encoding. All five now pass StandardCharsets.UTF_8 explicitly. Credentials made only of ASCII characters encode to exactly the same bytes as before. camel-smpp's connectProxy also wrote the CONNECT request line, the proxy headers and the line separators, and read the proxy's response, in the default charset; those are converted too, so the whole exchange is built one way.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, a nice tidy fix, and the PR description is very clear about why the failure is so hard to diagnose. I checked the monorepo: these five were the only places that build a Basic / Proxy-Authorization header from the platform default charset (camel-oauth's OAuthTokenRequest and the AS2 proxy auth in AS2ClientConnection already use UTF-8), so the client side is consistent after this. AS2HeaderUtilsTest.addAuthorizationHeaderEncodesCredentialsAsUtf8 pins the exact header value, and the escaped ä keeps the source ASCII-safe.
A few notes, none blocking:
- [Suggestion, follow-up] The receiving side has the same issue: camel-netty-http's
HttpServerChannelHandler.extractBasicAuthSubjectdecodes the Basic credentials withCharset.defaultCharset()(HttpServerChannelHandler.java:387). Switching it to UTF-8 would be a good small follow-up PR. - [Question] For
SmppConnectionFactory.connectProxy, switching the CONNECT line, headers and reply reader to UTF-8 looks fine (ASCII in practice). Was it mainly for consistency? HTTP/1.x doesn't really define non-ASCIIproxyHeadersvalues. - [Nit] Camel commits and PR titles usually carry a JIRA key (
CAMEL-XXXX: ...).
On the upgrade guide: main requires Java 21, where the default charset is already UTF-8, so only deployments that override file.encoding and use non-ASCII credentials see a different header, and they now get the RFC 7617 behaviour. No migration note needed.
Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
All five credential-encoding fixes are correct per RFC 7617 §2, which mandates UTF-8 for the user-id:password string before Base64-encoding in HTTP Basic auth. The fix is well-motivated and consistently applied across all call sites.
One issue to address: the AS2 test is a tautology on the JVMs that Camel CI actually runs.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Recomputing the expectation the way the code does makes the assertion vacuous on a JVM whose default charset is already UTF-8, which is every JVM Camel CI runs. The expected header is now written out, and the ISO-8859-1 encoding of the same credentials is asserted to differ, so the test says what has to go on the wire rather than agreeing with whatever the code did.
|
Thanks both, and thanks for sweeping the monorepo for the remaining call sites — good to know The tautology is fixed. @gnodet-bot is right: I computed the expectation the same way the code does, so on Java 21 (default charset UTF-8) the assertion agreed with whatever the code produced. The test now asserts a written-out value and that the ISO-8859-1 encoding of the same credentials differs from it: assertEquals("Basic dXNlcjpww6Rzc3dvcmQ=", message.getFirstHeader(AS2Header.AUTHORIZATION).getValue());
// what the default charset would have produced instead, had it not been UTF-8
assertNotEquals("Basic " + Base64.getEncoder().encodeToString(("user:" + password).getBytes(ISO_8859_1)),
message.getFirstHeader(AS2Header.AUTHORIZATION).getValue());So it now pins the wire format rather than restating the implementation. ( On The receiving side is a good catch — JIRA — I have no credentials for issues.apache.org from this environment, so I cannot create the ticket. If you want a |
|
The receiving-side follow-up is open as #29711 — |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The second commit addresses the AS2 test tautology precisely.
addAuthorizationHeaderEncodesCredentialsAsUtf8 test (new commit):
- Uses a fixed expected value
"Basic dXNlcjpww6Rzc3dvcmQ="— this is exactly the correct UTF-8 Base64 of"user:pässword", verified against RFC 7617 - The
assertNotEqualson the ISO-8859-1 encoding (dXNlcjpw5HNzd29yZA==) is meaningful — the two encodings genuinely differ, so the assertion is not vacuous on any JVM - Comment explains the intent clearly: "makes the assertion vacuous on a JVM whose default charset is already UTF-8 ... the expected header is now written out"
The original five fixes (AS2, Keycloak×2, SMPP, WordPress) remain correct and are ready to merge. davsclaus has already approved; this is a clean, well-scoped bug fix.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: still good to merge, the new commit resolves the tautology point.
addAuthorizationHeaderEncodesCredentialsAsUtf8 now asserts the written-out Basic dXNlcjpww6Rzc3dvcmQ=, which is the UTF-8 Base64 of user:pässword, so the test pins the wire format instead of restating the implementation. The extra assertNotEquals against the ISO-8859-1 form is redundant after the assertEquals, but it documents intent, so no need to change it. Thanks also for answering the SMPP question and for opening #29711 for the receiving side; the two PRs touch disjoint files.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 13 of 723 tested, 27 compile-only — current: 13 all testedMaveniverse Scalpel detected 13 affected modules (current approach: 13). Skip-tests mode would test 13 modules (4 direct + 9 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (13)
Modules with tests skipped (27)
All tested modules (40 modules, 5m 39s total)Total reactor time: 5m 39s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed the producer side. Every one of the five spots now Base64-encodes user:password from its UTF-8 bytes, which is what RFC 7617 asks for and what browsers and Camel's own consumers expect:
- camel-as2
AS2HeaderUtils.addAuthorizationHeader - camel-keycloak
KeycloakProducer.revokeTokenandKeycloakTokenIntrospector.introspect - camel-smpp
SmppConnectionFactory.connectProxy(the CONNECT line, the proxy headers and the response reader are pinned to UTF-8 too, so the whole exchange is charset-stable, not just the credential) - camel-wordpress
WordpressBasicAuthentication
ASCII credentials are unchanged, so only a non-ASCII user name or password moves, and it moves to the correct bytes. The AS2 test is written well: it asserts the literal Basic dXNlcjpww6Rzc3dvcmQ= that has to go on the wire for user:pässword instead of recomputing it the way the code does, and the assertNotEquals against the ISO-8859-1 encoding pins down that the charset is exactly what changed.
Two things to consider, neither blocking:
- This is the encode half. The matching decode fix for the netty-http server is #29711, and the two agree on the same
dXNlcjpww6Rzc3dvcmQ=vector, which is good to see. - For a deployment on Java 17 whose default charset is not UTF-8 (Windows-1252, say) and which uses non-ASCII Basic credentials, the Authorization header value on the wire changes. That is the bug being fixed, but it is also a visible behavior change, so a line in the 4.24 upgrade guide under changed header values would help anyone in that narrow case. On Java 18 and later the default charset is already UTF-8 (JEP 400), so most deployments see nothing change.
One pointer, unrelated to this PR but in the same family: camel-workday AuthClientForIntegration.createPostMethod builds its Basic header with Arrays.toString(Base64.getEncoder().encode(...)), which emits [66, 97, ...] rather than a Base64 string. That is a separate and more serious defect sitting on top of the same getBytes() charset issue, so it is better fixed on its own rather than folded in here.
Claude Code on behalf of oscerd
Problem
Five places build an
Authorization: Basic(orProxy-Authorization: Basic) header by Base64-encodinguser:passwordthrough the no-argumentString.getBytes(), which uses the JVM's default charset:AS2HeaderUtils.addAuthorizationHeaderKeycloakProducer(token revocation)KeycloakTokenIntrospectorSmppConnectionFactory.connectProxyWordpressBasicAuthenticationRFC 7617 §2.1 says the credentials are encoded with UTF-8 unless the server names another charset, and that is what Keycloak, WordPress and ordinary HTTP proxies expect.
The JVM default is only UTF-8 when nothing overrides it.
-Dfile.encoding=ISO-8859-1,file.encoding=COMPAT, or any JVM before 18 (which follows the platform locale, so US-ASCII under theC/POSIXlocale that containers often use) all give something else. A user name or password containing a non-ASCII character then produces a different Authorization header on different machines, and the server answers 401 although the credentials are correct. Because the bytes are Base64-encoded, nothing in the failure points at the encoding — this is the kind of defect that gets debugged for a day.Fix
Pass
StandardCharsets.UTF_8explicitly in all five places. Credentials made only of ASCII characters encode to exactly the same bytes as before, so nothing changes for the common case.camel-smpp'sconnectProxyalso wrote theCONNECTrequest line, the configured proxy headers and the line separators, and read the proxy's response, in the default charset. Those are converted too, so the whole exchange is built one way rather than two.Tests
AS2HeaderUtilsTest.addAuthorizationHeaderEncodesCredentialsAsUtf8adds a Basic header for a password with a non-ASCII character and asserts the exact header value against the Base64 of the UTF-8 bytes. It fails onmainunder any JVM whose default charset is not UTF-8, and pins the encoding from now on.The other four call sites are the same one-line change inside methods that do network I/O (an HTTP POST to Keycloak, a CXF
WebClientconfiguration, a proxyCONNECT), so they have no seam to assert on without a server; the shared behaviour is what the AS2 test covers.