Keep the password as char[] end-to-end so the wipe guarantee holds - #164
Merged
Conversation
The credentials(String, char[]) contract promises that the caller can wipe the single authoritative copy of the password after closing the client, because the builder deliberately does not copy the array. The light backend broke that promise: resolveAuthScheme() converted the char[] to an immutable String that the NTLM and Kerberos schemes then retained, leaving an un-wipeable copy of the secret on the heap (flagged by Codex review on PR #163). The password now flows as the caller's char[] by reference through LightWinRMService, NtlmAuthScheme, WinRMSession, Type3Message, KerberosAuthScheme and CipherGen. The two hashing sinks encode it without going through String: Charset.encode(CharBuffer) has the same malformed-input replacement semantics as String.getBytes(Charset), so the derived hashes are byte-identical, and the transient encodings are zeroed after use. The Kerberos PasswordCallback takes the char[] directly (it clones internally). The legacy LM hash now uppercases per-char instead of via String.toUpperCase; the two differ only on one-to-many mappings the LM OEM charset cannot represent anyway. A new protocol test authenticates with a non-ASCII password (including a surrogate pair) against FakeWsmanServer, whose NTOWFv2 derivation is still String-based, proving the char[] encoding path is byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8084b63b0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The char[] conversion had replaced String.toUpperCase(Locale.ROOT) with per-char Character.toUpperCase in the legacy LM hash, silently changing the derived LM response for passwords containing one-to-many uppercase mappings (e.g. ss expanded to SS before; per-char left it in place and the OEM charset turned it into ?). CipherGen.upperCase(char[]) now reproduces String.toUpperCase exactly without ever creating a String of the secret: a lazily built map (legacy LM path only) probes every code point through the JDK''s own casing data - on public constants, never on the password - and records those whose full uppercase differs from Character.toUpperCase; the password is then uppercased code point by code point through that map. Locale.ROOT uppercasing is context-free, so per-code-point mapping equals the whole-string result. CipherGenTest locks the equivalence: an exhaustive sweep over all code points against String.toUpperCase, expansion-heavy samples, the Charset.encode/String.getBytes parity, and the concrete regression (LM response of the sharp s equals that of ss). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@codex please review again |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The
credentials(String, char[])contract (andauthentication.md) promises that the builder does not copy the password array, so the caller can wipe the single authoritative copy of the secret after closing the client. The light backend broke that promise:LightWinRMService.resolveAuthScheme()didnew String(winRMEndpoint.getPassword()), and the NTLM/Kerberos auth schemes retained that immutable String — a second, un-wipeable copy of the secret. Flagged by Codex review on #163.How
The password now flows as the caller''s
char[]by reference throughLightWinRMService→NtlmAuthScheme/KerberosAuthScheme→WinRMSession→Type3Message→CipherGen, and is never converted to aString:CipherGen.ntlmHash/lmHashencode thechar[]withCharset.encode(CharBuffer), which has the same malformed-input replacement semantics asString.getBytes(Charset)— the derived hashes are byte-identical. Transient encoded copies are zeroed after use.KerberosAuthSchemehands thechar[]straight toPasswordCallback.setPassword(which clones internally), instead ofString.toCharArray().String.toUpperCase; the two differ only on one-to-many mappings (e.g.ß→SS) that the LM OEM charset cannot represent anyway.authentication.mdnow states the end-to-end property explicitly.Testing
New protocol test
ntlmAuthenticatesWithNonAsciiPasswordauthenticates againstFakeWsmanServerwithpässw0rd-€-好-😀(including a surrogate pair). The fake server still derives NTOWFv2 from aString, so a successful handshake proves thechar[]encoding path produces byte-identical hashes.mvn verify siteis green: 212 unit tests + 1 IT pass, 0 checkstyle / PMD / SpotBugs findings.🤖 Generated with Claude Code