Repository navigation
camel-netty-http: decode Basic auth credentials as UTF-8 - #29711
Dhairya-007 wants to merge 3 commits into
Conversation
extractBasicAuthSubject read the Base64-decoded credentials back with the
platform default charset:
String userAndPw = out.toString(Charset.defaultCharset());
RFC 7617 says a client encodes "user:password" with UTF-8, which is what
browsers and Camel's own producers send. A server whose JVM default is not
UTF-8 - -Dfile.encoding, file.encoding=COMPAT, or a JVM before 18 following
the platform locale - therefore reads a different password than the one that
was sent as soon as it contains a non-ASCII character, and the request is
rejected with 401 although the credentials are right.
This is the receiving side of the credential encoding fixed on the sending
side in apache#29694, and was spotted in its review.
The Base64 text itself is also converted to bytes explicitly, as US-ASCII,
which is what Base64 is; that one is byte-identical in any ASCII-compatible
charset.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: the UTF-8 fix is correct, thanks for following up on #29694 so quickly. One small, pre-existing buffer leak sits on the line this PR rewrites, so it would be good to fix it here.
I checked the change: out.toString(StandardCharsets.UTF_8) matches RFC 7617 and what Camel's own producers send after #29694, and US-ASCII for the Base64 text is right. Both test vectors are correct (dXNlcjpww6Rzc3dvcmQ= is user:pässword in UTF-8), and JUnit assertions match the rest of camel-netty-http. The two PRs touch disjoint files, so they can merge in any order.
[Important] Pooled buffer leak on malformed input (HttpServerChannelHandler.java:383-384): NettyConverter.toByteBuffer(byte[]) allocates from ByteBufAllocator.DEFAULT (pooled), and Base64.decode(buf) runs before the try. Netty's decoder throws IllegalArgumentException on an invalid Base64 character or a bad length, so a malformed Authorization: Basic ... header sent by any client leaves buf unreleased. Since this PR rewrites that line, could we move the decode inside the try?
ByteBuf buf = NettyConverter.toByteBuffer(decoded.getBytes(StandardCharsets.US_ASCII));
ByteBuf out = null;
try {
out = Base64.decode(buf);
String userAndPw = out.toString(StandardCharsets.UTF_8);
...
} finally {
buf.release();
if (out != null) {
out.release();
}
}(An alternative is java.util.Base64.getDecoder().decode(decoded.trim()) with no ByteBufs at all. Catching the IllegalArgumentException and returning null, so the client gets a 401 instead of an error, would also be reasonable.)
[Suggestion] A third case in HttpServerChannelHandlerBasicAuthTest with a malformed value (e.g. Basic %%%) would pin whichever behaviour you choose.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed against the producer/encode side (#29694); the two are the matching halves of the same RFC 7617 fix and they line up on the same dXNlcjpww6Rzc3dvcmQ= vector.
Two charset changes in extractBasicAuthSubject, both right:
decoded.getBytes()becomesdecoded.getBytes(US_ASCII).decodedis the Base64 text, whose alphabet is ASCII by definition, so US_ASCII is both correct and no longer at the mercy of the default charset.out.toString(Charset.defaultCharset())becomesout.toString(UTF_8), so a non-ASCII user name or password is read back as the bytes the client actually sent.
The new test covers the three cases that matter: a non-ASCII credential decoded as UTF-8, a plain ASCII credential, and a non-Basic scheme returning null. Reusing the same pässword vector as the encode side is a nice touch.
One consideration, not blocking: a netty-http server on Java 17 with a non-UTF-8 default charset that today authenticates non-ASCII credentials will read those credentials differently after this change (correctly, per the RFC). If #29694 adds an upgrade-guide note for the header-value change, a sentence here would be worth it so the server side is covered too. Java 18 and later already default to UTF-8, so most deployments are unaffected.
Claude Code on behalf of oscerd
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: thanks for the update; the new commit is the formatter re-wrapping the test's class Javadoc (it matches the patch CI produced exactly), so the buffer leak from my earlier review is still open.
[Important] Pooled buffer leak on malformed input is still there (HttpServerChannelHandler.java:383-384): Base64.decode(buf) still runs before the try, so when a client sends a malformed Authorization: Basic ... value, Netty's decoder throws IllegalArgumentException and the pooled buf from NettyConverter.toByteBuffer(...) is never released. The fix I suggested earlier still applies: move the decode inside the try and release out only when it is not null, or decode with java.util.Base64 so no ByteBufs are involved. Optionally, catch the IllegalArgumentException and return null so the client gets a 401.
[Suggestion] A third case in HttpServerChannelHandlerBasicAuthTest with a malformed value (e.g. Basic %%%) would pin whichever behaviour you choose.
Everything else (the UTF-8 / US-ASCII charsets and the two test vectors) still looks good, and the PR merges cleanly with main.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Base64.decode(buf) ran before the try whose finally released the buffer, so a client sending a malformed Authorization: Basic value made Netty's decoder throw IllegalArgumentException and leaked the pooled ByteBuf that NettyConverter.toByteBuffer had allocated - once per bad request. Rather than juggle the two buffers, this drops them: java.util.Base64 decodes straight to a byte[], so there is nothing to release and no leak to get wrong. A value that is not base64 is now answered with no principal, which means the request gets 401 instead of an error out of the handler.
|
Thanks — the leak is real and it is on the exact line I was rewriting, so it belongs here. Fixed, and I took your alternative rather than the String encoded = StringHelper.after(auth, " ");
byte[] credentials;
try {
// the value is base64 encoded, so we need to decode that
credentials = Base64.getDecoder().decode(encoded.trim());
} catch (IllegalArgumentException e) {
// a client sent something that is not base64 at all: no principal, so the request is
// answered with 401 rather than an error
LOG.debug("Cannot decode the Basic Auth header, it is not valid base64");
return null;
}
String userAndPw = new String(credentials, StandardCharsets.UTF_8);With Added the malformed test, and verifying it caught a mistake worth mentioning: my first version also asserted that On @oscerd's upgrade-guide point: #29694 concluded no note was needed, since main requires Java 21 where the default charset is already UTF-8 and only a deployment overriding |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 40 of 723 tested, 27 compile-only — current: 40 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 40). Skip-tests mode would test 40 modules (1 direct + 39 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (40)
Modules with tests skipped (27)
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the update. Since my last review, d89d1cb decodes the Basic value with java.util.Base64 instead of Netty's ByteBuf decoder (HttpServerChannelHandler.java:381-399), so there is no pooled buffer left to leak, and a value that is not Base64 now gives no principal, which the handler answers with 401 (lines 203-218). shouldIgnoreAValueThatIsNotBase64 pins that. Both earlier points are covered, and it lines up with #29694 (now on main) on the encode side: UTF-8 both ways.
LGTM once CI is green.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Problem
HttpServerChannelHandler.extractBasicAuthSubjectreads the Base64-decoded credentials back with the platform default charset:RFC 7617 §2.1 says the client encodes
user:passwordwith UTF-8, which is what browsers and Camel's own producers send. Anetty-httpconsumer running on a JVM whose default charset is not UTF-8 —-Dfile.encoding,file.encoding=COMPAT, or a JVM before 18 following the platform locale — therefore reconstructs a different password than the one that was sent, as soon as it contains a non-ASCII character, and answers 401 although the credentials are correct.This is the receiving side of what #29694 fixed on the sending side; @davsclaus spotted it while reviewing that PR.
Fix
out.toString(StandardCharsets.UTF_8)for the credentials.decoded.getBytes(StandardCharsets.US_ASCII)for the Base64 text itself, which is ASCII by definition — byte-identical in any ASCII-compatible charset, but it was the same unchecked pattern two lines up.The now-unused
java.nio.charset.Charsetimport is dropped.Tests
New
HttpServerChannelHandlerBasicAuthTest, which calls theprotected staticmethod directly from the same package:shouldDecodeCredentialsAsUtf8— the headerBasic dXNlcjpww6Rzc3dvcmQ=, which isuser:pässwordencoded as a client sends it, has to come back aspässword. The expected values are written out rather than recomputed from the input, so the test states what the wire format means instead of agreeing with the implementation.shouldDecodeAsciiCredentials—scott:tiger, the ordinary case, unchanged.shouldIgnoreAnotherScheme— aBearerheader yields no principal.As on #29694: Camel's CI runs Java 21, where the default charset is already UTF-8, so the first test is green there either way. It fails on a JVM started with a non-UTF-8
file.encoding, and it pins the behaviour from now on.