Add HTTP Basic authentication as a third auth scheme - #169
Conversation
Support Basic alongside NTLM and Kerberos: AuthScheme.BASIC / AuthenticationEnum.BASIC, a --basic CLI option, and a stateless BasicAuthScheme whose Authorization header repeats on every request (no message protection, so HTTPS is the intended transport). A new requestAuthorization() hook on the light AuthScheme carries the per-request header, and a shared PlaintextSoapAuthScheme base deduplicates the plaintext-SOAP behavior of Basic and Kerberos. Basic over HTTP and HTTPS is covered by in-process protocol tests against a fake WSMan server, plus builder, factory, enum, and CLI tests. Docs (authentication, cli, preparing-the-host, migration guides, index) and the changelog are updated.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cd7d03c370
ℹ️ 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".
| // Basic is stateless and rides the Authorization header of every request, so it works | ||
| // over both transports. The credential is the caller's username as given: a | ||
| // domain-qualified name keeps its domain prefix, which is how the server locates it. | ||
| schemes.add(new BasicAuthScheme(winRMEndpoint.getRawUsername(), password)); |
There was a problem hiding this comment.
When BASIC appears with another scheme, this list is wrapped in FallbackAuthScheme, but that wrapper does not override requestAuthorization() and therefore returns the interface default of null. Since BasicAuthScheme.authenticate() also returns null, every Basic attempt in an ordered fallback sends no Authorization header and receives a 401; delegate requestAuthorization() to the active candidate so the advertised Basic fallback behavior works.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 672727e. FallbackAuthScheme now overrides requestAuthorization() and forwards it to the active candidate, so a Basic scheme inside an ordered fallback (e.g. BASIC, NTLM) now carries its Authorization header on every request instead of the interface default null being used. Added FallbackAuthSchemeTest.requestAuthorizationForwardsToTheActiveStatelessScheme and requestAuthorizationIsNullWhileUnauthenticated to pin the behavior down (including that a single-scheme fallback — a BASIC-only list is wrapped too — works the same way).
| */ | ||
| final class BasicAuthScheme extends PlaintextSoapAuthScheme { | ||
|
|
||
| private final String authorizationHeader; |
There was a problem hiding this comment.
When a closed client remains referenced, this immutable field retains a Base64-reversible copy of the username and password indefinitely; close() only reaches reset(), which clears the authenticated flag but not this header. Additionally, buildCredential() creates an immutable plaintext String, so wiping the caller's char[] does not satisfy the documented single-wipeable-copy contract. Keep the derived credential in wipeable storage and erase it during client disposal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 672727e. The derived header is now held as a wipeable byte[] (not an immutable String) and is erased in reset() — which close() always runs — so it cannot outlive the caller's char[]. Because WsmanClient needs the header again after a dropped connection is re-established, the scheme keeps the caller's char[] by reference (the same contract NtlmAuthScheme already holds via WinRMSession), and re-derives the header from the still-live array in authenticate(); if the caller has already wiped the password when a re-authentication is attempted, requestAuthorization() throws loudly instead of silently sending a header of zeros. Separately, buildAuthorizationHeader() now encodes the password straight from the char[] via a CharsetEncoder/CharBuffer view, so no String copy of the secret is ever formed. Covered by BasicAuthSchemeTest.resetErasesTheDerivedCredential and thePasswordArrayIsNotCopiedIntoAnImmutableString.
| @@ -0,0 +1,75 @@ | |||
| package org.metricshub.winrm.light; | |||
There was a problem hiding this comment.
This newly added file starts directly with the package declaration and omits the project's required license header, so the Maven license verification will reject the change. Add the standard header before the package declaration.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one turned out to be a false positive — the build does not require a header on this file. The inherited license-maven-plugin configuration (from the oss-parent POM) scopes check-file-header to:
<includes>
<include>main/java/**/*.java</include>
</includes>so src/test/java/** is not checked, and every existing test file in the project (e.g. WinRMClientTest, CliArgumentsTest, WsmanProtocolTest) starts directly with its package declaration. Concretely: mvn license:update-file-header scans the main sources and leaves the test files untouched, and the subsequent mvn verify (which runs check-file-header with failOnMissingHeader=true) passes — this PR's CI is green. I've therefore left the file matching the project's established test-file convention. Happy to add a header here instead if the convention should change (which would mean updating the plugin includes and re-headering every existing test).
| ``` | ||
|
|
||
| The server must have Basic authentication enabled | ||
| (`winrm set winrm/config/service @{AllowBasicAuth=true}`); see |
There was a problem hiding this comment.
On a host where Basic is currently disabled, this command fails because AllowBasicAuth is not a property at the service root. The WinRM configuration exposes Basic under Service\Auth, as shown in Microsoft's WinRM configuration reference, so the documented command must target winrm/config/service/auth and set Basic=true; otherwise users cannot prepare the host as instructed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — you're right, AllowBasicAuth is not a WinRM setting. Per the Microsoft documentation the service exposes Basic under the service auth section (default False), so the documented command was failing on a host where Basic is disabled. Fixed in 672727e across all references: authentication.md now documents winrm set winrm/config/service/auth @{Basic=true}, and the preparing-the-host.md table and bullet, the CHANGELOG entry, and the migration page now all use the correct setting name and location.
| * **`Basic` and `CredSSP` stay `False` unless you use Basic.** NTLM and Kerberos need neither. To | ||
| use HTTP Basic, enable `AllowBasicAuth` on the service and connect over HTTPS |
There was a problem hiding this comment.
The new host-preparation text says Basic must be enabled when using this scheme, but README.md lines 57–59 still state unconditionally that Basic does not need to be changed. Users following the quick-start prerequisites will therefore leave authentication disabled and receive 401 responses; update that existing summary to include the Basic-auth exception.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 672727e — the README's host-prerequisites bullet now removes Basic from the "do not need to be changed" list and calls out the one exception: users of the HTTP Basic scheme must enable the service's Basic setting (winrm/config/service/auth), with a link to the Preparing the Windows Host page for the exact command.
| NTLM, | ||
| KERBEROS; | ||
| KERBEROS, | ||
| BASIC; |
There was a problem hiding this comment.
When the JVM default locale is Turkish or Azerbaijani, getValueOf("basic") uppercases the final i to İ, producing BASİC, which is absent from VALUES_OF; the newly added scheme therefore cannot be parsed from ordinary lowercase configuration in those environments. Use toUpperCase(Locale.ROOT) for the lookup normalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 672727e: getValueOf now normalizes with toUpperCase(Locale.ROOT). Added AuthenticationEnumTest.testGetValueOfIsInsensitiveToTheDefaultLocale, which sets the JVM default locale to Turkish and asserts basic / kerberos / ntlm still resolve (with a finally block restoring the previous default so it can't leak into other tests).
| * **Basic is HTTPS-only.** winrm4j offers `AuthSchemes.BASIC`, which here maps to | ||
| `authentication(AuthScheme.BASIC)`. This client has no Basic message protection, so it requires | ||
| `https()` — the credential and payload travel inside the TLS tunnel. The host must have |
There was a problem hiding this comment.
In the migration scenario described here, the implementation does not require https(): LightWinRMService accepts Basic over HTTP and the new CLI and factory tests explicitly exercise that configuration. A caller relying on this HTTPS-only statement can omit an application-side check and then silently transmit the credential and SOAP payload in cleartext, so either enforce HTTPS at build time or revise this text to state that HTTPS is strongly recommended but not enforced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 672727e. I chose the documentation fix rather than enforcing HTTPS at build(): Basic over plain HTTP is technically possible and the client intentionally accepts it (mirroring winrm4j, which also does not refuse), and the docs should describe what the code does rather than the other way round. migrating-from-winrm4j.md now says the client accepts the scheme over both transports but you must use https(), and that — unlike the Kerberos case, which is refused at build() — the client will not stop a cleartext transmission. The CHANGELOG and authentication.md wording was tightened the same way ("accepted over both transports, must be used over HTTPS in practice").
…tial, fix docs Review feedback on the Basic authentication change, addressed in one pass: FallbackAuthScheme now forwards requestAuthorization() to the active candidate so a Basic scheme inside an ordered fallback sends its header on every request instead of relying on the interface default of null; BasicAuthScheme keeps the derived Authorization header as a wipeable byte[], erased on reset() (which close() always runs) and re-derived from the caller's still-live char[] on re-auth, and builds the header by encoding the password straight from the char[] (CharBuffer view) so no String copy of the secret is ever formed; AuthenticationEnum.getValueOf now normalizes with Locale.ROOT so the Turkish/Armenian dotted-i does not break lookup; the docs now target the real WinRM service setting (Basic under winrm/config/service/auth, not AllowBasicAuth), the README host-setup summary notes the Basic exception, and the migration page states Basic is accepted over both transports but must be used over HTTPS rather than claiming it is HTTPS-only. Adds fallback forwarding and credential-erase tests, and a locale-independence test for the enum lookup. mvn verify passes: 231 tests, 0 Checkstyle / PMD / CPD / SpotBugs findings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 672727ef75
ℹ️ 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".
| BasicAuthScheme(final String username, final char[] password) { | ||
| this.username = username; | ||
| this.password = password; | ||
| this.authorization = buildAuthorizationHeader(username, password); |
There was a problem hiding this comment.
The new wipe in reset() does not cover a Basic scheme that never becomes active: with an ordered list such as [NTLM, BASIC], this constructor eagerly derives the reversible credential, but FallbackAuthScheme.reset() resets only the active NTLM candidate. If NTLM succeeds and the closed client remains referenced, the inactive Basic candidate therefore retains authorization after the caller wipes the password; derive it lazily or dispose every fallback candidate when closing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e307dcd — I took your second suggested option: FallbackAuthScheme.reset() now walks all candidates rather than just the active one, so a Basic scheme that never became active (e.g. [NTLM, BASIC] where NTLM wins) has its derived credential erased on close() and on a dropped connection. startIndex is left untouched, so the fallback order survives and the next authenticate() still retries the last-accepted scheme first. New regression test: FallbackAuthSchemeTest.resetClearsEveryCandidateNotJustTheActiveOne asserts both candidates are reset and that the first scheme is the one retried.
| if (authorization == null) { | ||
| throw new IllegalStateException("The Basic credential was erased before the connection was re-authenticated"); | ||
| } | ||
| return new String(authorization, StandardCharsets.US_ASCII); |
There was a problem hiding this comment.
The new byte[] field does not fully resolve the earlier immutable-copy issue because every Basic request converts the reversible credential into a fresh, non-wipeable String here, which HttpTransport then copies through its string-built request header. Resetting only erases authorization, so applications using Basic still cannot rely on the documented post-close wipe guarantee; carry the header through the transport in wipeable byte storage instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right that the per-request String is a copy of the reversible credential, but I don't think it breaks the wipe guarantee, and it's the exact mechanism the other schemes already use:
- The guarantee is about what remains after
close():close()fences the client (closed = true, later requests fail) and runsauth.reset(), which zeroesauthorization. From that point no further per-request strings are ever produced from the credential, so the caller can zero itschar[]with nothing left behind. - The per-request string is transient — immediately folded into the header buffer and garbage-collected — and identical to what NTLM/Kerberos already do: their
Negotiate <token>is also a freshly builtStringper first request, carried through the sameHttpTransport.post(..., String authorization). So this is scheme-agnostic transport behavior, not a Basic-specific retention. - Carrying the header through the transport in byte storage would mean a new
HttpTransportoverload and switching every scheme's token path. That's a real interface change I'd rather make as a separate task covering NTLM/Kerberos together, rather than special-casing Basic here.
Happy to file that as a follow-up if the stronger guarantee is wanted.
| The server must have Basic authentication enabled on the WinRM service — the `Basic` setting under | ||
| the service's `auth` section, `False` by default: | ||
| `winrm set winrm/config/service/auth @{Basic=true}`; see |
There was a problem hiding this comment.
The corrected service/auth command is still insufficient for the supported plain-HTTP Basic configuration exercised by the new factory and protocol tests: with the WinRM service default AllowUnencrypted=false, Windows refuses the unprotected Basic/SOAP messages even after Basic=true. Document that HTTP Basic also requires winrm/config/service AllowUnencrypted=true (with the existing security warning), or enforce HTTPS instead; otherwise users following this setup cannot connect. Microsoft's WinRM configuration reference documents the service default and its rejection of unencrypted traffic.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e307dcd. authentication.md now notes that over plain HTTP, in addition to Basic=true, the service must also set AllowUnencrypted=true (the service's default AllowUnencrypted=false would refuse the unprotected SOAP), and frames it as precisely what the HTTPS recommendation exists to avoid. The AllowUnencrypted bullet in preparing-the-host.md carries the same exception.
| setting that a Java client never reads. The one exception is the service's `Basic` setting, which | ||
| only users of the HTTP Basic scheme must enable |
There was a problem hiding this comment.
Adding Basic here leaves the README's existing capability summaries contradictory: line 11 still says the library has only two authentication types, and lines 205–206 still describe support as only NTLM and Kerberos. Because the new scheme directly invalidates those existing overview statements, update them to include Basic while keeping the README terse.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e307dcd — both summaries now include Basic while staying terse: the top-of-file bullet reads "one of three authentication types (NTLM, Kerberos, or Basic)", and the "The WinRM client" section now says the client supports "NTLM over HTTP (with message encryption) and HTTPS, Kerberos (SPNEGO) over HTTPS, and HTTP Basic over HTTPS".
…plain-HTTP Basic, fix README summaries Review round 2 on the Basic authentication change: FallbackAuthScheme.reset() now erases the state of EVERY candidate, not just the active one, so a Basic scheme that never became active (e.g. [NTLM, BASIC] where NTLM wins) does not retain its reversible credential after close() - new resetClearsEveryCandidateNotJustTheActiveOne test pins this down, including that the fallback order survives. The docs now note that plain-HTTP Basic (accepted by the client, not recommended) additionally requires AllowUnencrypted=true on the service, since the default refusal of unencrypted traffic would reject the unprotected SOAP - this is exactly what the HTTPS recommendation avoids. The README's capability summaries (three authentication types, scheme list) now include Basic. mvn verify passes: 232 tests, 0 Checkstyle / PMD / CPD / SpotBugs findings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e307dcdad5
ℹ️ 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".
| public void reset() { | ||
| // close() always reaches here, and the Base64 value is a reversible copy of the credential: | ||
| // erase it so wiping the caller's char[] afterward leaves no live copy of the password. |
There was a problem hiding this comment.
When close() races an in-flight operation or an open streaming handle, WsmanClient.close() fails connectionPermit.tryAcquire() and explicitly skips auth.reset() (lines 1352–1368); after the hard transport close unblocks the worker, no later reset is scheduled. The current close path is fresh evidence that the earlier assumption that close always reaches this wipe is false, so a referenced closed client can retain the reversible Basic credential indefinitely. Ensure authentication state is disposed after the active operation releases the permit, or provide a safe wipe path that does not require acquiring it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right — when close() can't acquire the connection permit (a worker still holds it, blocked on a socket read), the Basic credential survived inside the closed client. Fixed: WsmanClient now wipes the auth state on connection release, not only on close().
- New
WsmanClient.releaseConnection()wraps every operation-sideconnectionPermit.release(). Because the channel is serial (one permit), the operation that releases is the last one in flight, so resetting the scheme there is race-free. It wipes whenclosed, plus a post-release sweep covering the interleaving whereclose()setsclosedand failstryAcquirejust after the pre-release check.close()is unchanged: when it does acquire the permit it still resets there. BasicAuthScheme.authorizationis nowvolatile(reset()can run on another thread), andreset()is re-entrant: it captures the field in a local before erasing, sinceclose()and the releasing worker are different threads with no shared lock. The same local-capture is applied toKerberosAuthScheme(double-disposing aGSSContextis the hazard; theNtlmAuthSchemereset is plain field nulls and already safe).
Covered by BasicAuthCloseRaceTest: a worker blocked on a delayed server response holds the permit while close() runs; the test asserts the credential is erased (requestAuthorization() throws, isAuthenticated() is false) and that the worker terminates. BasicAuthSchemeTest.resetIsIdempotentAndSafeToCallTwice locks in the double-wipe safety.
| } else if (auth == AuthenticationEnum.BASIC) { | ||
| // Basic is stateless and rides the Authorization header of every request, so it works | ||
| // over both transports. The credential is the caller's username as given: a | ||
| // domain-qualified name keeps its domain prefix, which is how the server locates it. | ||
| schemes.add(new BasicAuthScheme(winRMEndpoint.getRawUsername(), password)); |
There was a problem hiding this comment.
When callers supply whitespace around or within the username, the builder accepts it and WinRMEndpoint deliberately removes all whitespace before parsing the account, so NTLM authenticates with the normalized value. Basic instead receives getRawUsername() and encodes those original whitespace characters into the credential; for example, credentials(" DOMAIN\\user ", ...) sends a different username and is rejected with 401. Preserve the domain prefix while using the endpoint's normalized account representation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — NTLM and Kerberos use the endpoint's normalized (whitespace-stripped) domain/username, but Basic was built from getRawUsername(), which kept the caller's string including whitespace. A padded domain\user would have sent a header the server can't match.
Fixed at the source: WinRMEndpoint now stores the whitespace-stripped account in rawUsername (it computed the stripped user for the domain split and threw it away otherwise), so getRawUsername() returns the same normalized account the other schemes use — a domain-qualified name still keeps its domain prefix, which is how the server locates the account. Updated the getRawUsername() Javadoc, the LightWinRMService comment, and the BasicAuthScheme @param.
Covered by two tests: WinRMEndpointTest asserts getRawUsername() is whitespace-stripped for a padded domain\user, and WsmanProtocolTest.basicSendsTheWhitespaceNormalizedAccountOnTheWire proves the wire credential is the normalized FAKE\user (the fake server 401s any non-normalized header, so a successful round trip is the proof).
…n in-flight operation; normalize the Basic account Close-race credential wipe (codex comment on BasicAuthScheme.close): - WsmanClient gains releaseConnection(): every operation-side permit release now goes through it. On release it wipes the auth scheme when the client is closed, so when close() cannot acquire the connection permit (a timed-out worker still holds it) the last in-flight operation still erases the connection-bound secrets instead of leaving them in a referenced client. A post-release sweep covers the interleaving where close() sets closed and fails tryAcquire just after the pre-release check. - BasicAuthScheme.authorization is volatile (reset() can run on another thread); its Javadoc documents the release-time wipe. - The wipe is now re-entrant (close() and the releasing worker are different threads with no shared lock): BasicAuthScheme and KerberosAuthScheme capture the field in a local before erasing/disposing, so a concurrent reset() cannot null it mid-erase. Basic account normalization (codex comment on LightWinRMService): - WinRMEndpoint stores the whitespace-stripped account in rawUsername (it previously kept the caller's raw string with whitespace), so the Basic scheme sends the same normalized account as the domain/username pair used by NTLM/Kerberos. getRawUsername() Javadoc and the service/scheme comments updated to match. Tests: - BasicAuthCloseRaceTest: a worker blocked on a delayed server response holds the permit while close() runs; asserts the credential is erased (no authenticated flag, requestAuthorization() throws) and the worker terminates. - WsmanProtocolTest: Basic over a whitespace-padded account sends the normalized header (the server 401s anything else). - BasicAuthSchemeTest: reset() is idempotent / safe to call twice. - WinRMEndpointTest: getRawUsername() is whitespace-stripped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84c884b866
ℹ️ 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".
| // Store the whitespace-stripped account for protocols that do not distinguish domain and | ||
| // username (Basic, where the whole string is the account), consistent with the normalized | ||
| // domain/username pair used by the other protocols; then split it into the pair for them. | ||
| rawUsername = user; |
There was a problem hiding this comment.
Assigning the normalized value here changes the established public behavior of WinRMEndpoint.getRawUsername(), which previously returned the username exactly as supplied. Callers passing whitespace for display, diagnostics, or identity-key purposes now receive a different value, and equals()/hashCode() also silently collapse endpoints that were previously distinct. Keep the original string in rawUsername and pass a separate normalized account value to BasicAuthScheme; this breaking API change is not documented.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good call — normalizing rawUsername in place was the wrong layer: it changed the public getRawUsername() contract and, more importantly, equals()/hashCode() (both hash/compare rawUsername), so two endpoints that differed only by whitespace would silently collapse. Reverting the endpoint to keep the caller's value verbatim restores the established behavior.
The normalization now happens where the value is actually consumed: LightWinRMService rebuilds the Basic account from the endpoint's already whitespace-stripped domain/username parts (the same account NTLM/Kerberos use), domain-qualified when the endpoint has one. The wire credential is normalized without touching the public accessor, and there's no new public surface to document.
WinRMEndpointTest now asserts getRawUsername() returns the whitespace-padded input verbatim, and WsmanProtocolTest asserts the raw value is verbatim while the wire header is the normalized FAKE\user (the fake server 401s any other value, so the round trip is the proof).
| * `AllowUnencrypted`, `CredSSP` and `TrustedHosts` do **not** need to be changed: over plain HTTP | ||
| the payload is protected by NTLM message encryption, and `TrustedHosts` is a Windows-client | ||
| setting that a Java client never reads. The one exception is the service's `Basic` setting, which | ||
| only users of the HTTP Basic scheme must enable |
There was a problem hiding this comment.
For the supported plain-HTTP Basic configuration, this summary still says AllowUnencrypted never needs changing and calls the service's Basic flag the sole exception. The final authentication.md now confirms that Windows also requires AllowUnencrypted=true for this case, so users following the README prerequisites will enable only Basic and still be rejected. Qualify the statement for NTLM/Kerberos or mention both Basic-over-HTTP settings.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — the README contradicted the authentication.md page, which documents that Basic over plain HTTP also requires AllowUnencrypted=true. The bullet is now qualified to NTLM (where plain HTTP is indeed safe via NTLM message encryption, and TrustedHosts is a Windows-client setting the Java client never reads), and the HTTP Basic exception explicitly names both settings: enable the service's Basic flag, and — over plain HTTP only — AllowUnencrypted=true.
| } else if (auth == AuthenticationEnum.BASIC) { | ||
| // Basic is stateless and rides the Authorization header of every request, so it works | ||
| // over both transports. The credential is the (whitespace-stripped) raw username: a |
There was a problem hiding this comment.
Enabling Basic over both transports makes the public WinRMClient.Builder.http() Javadoc false: it still states that SOAP messages are NTLM-encrypted, but .http().authentication(BASIC) sends both the credential and SOAP in cleartext. A caller relying on that API-level transport guarantee can expose secrets even though the implementation intentionally permits this combination; update the method documentation to make the guarantee conditional on NTLM and warn about Basic.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Builder.http() no longer states the NTLM-encryption guarantee unconditionally. It now reads: with NTLM (the default scheme) the SOAP is NTLM-encrypted, so plain HTTP is protected; but with the Basic scheme both the credential and the SOAP travel in cleartext over HTTP, so use https() with Basic (and Kerberos over plain HTTP is rejected, since it can't be protected). The guarantee is now correctly conditional on the scheme, not the transport.
… Basic account at the service, fix two docs Basic account normalization without breaking the public API: - WinRMEndpoint.getRawUsername() previously kept the caller's string verbatim; the earlier round normalized it in place, which silently changed equals()/hashCode() (both use rawUsername) and the accessor's contract. Reverted: rawUsername is again the verbatim caller value, restoring the established public behavior. - The normalized account is now built where it is consumed: LightWinRMService rebuilds the Basic account from the endpoint's whitespace-stripped domain/username parts (the same account NTLM/Kerberos use), domain-qualified when the endpoint has one. The wire credential is normalized without touching the public accessor. Docs corrected for the HTTP Basic over plain-HTTP combination: - WinRMClient.Builder.http() Javadoc: the NTLM-encryption guarantee is now stated as conditional on NTLM; Basic over HTTP is cleartext (credential and SOAP), so use HTTPS. - README: the "AllowUnencrypted / CredSSP / TrustedHosts need no change" bullet is qualified to NTLM; the HTTP Basic exception now also names AllowUnencrypted=true (plain HTTP only), matching authentication.md. Tests updated to the new contract: - WinRMEndpointTest: getRawUsername() returns the whitespace-padded input verbatim. - WsmanProtocolTest: asserts the raw username is verbatim while the wire credential is the normalized account (the server 401s anything else).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb63bf27df
ℹ️ 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".
| * races an operation that still holds it (a timed-out worker blocked on a socket read, an open | ||
| * streaming handle) close skips the dispose and relies on the LAST operation to release the | ||
| * permit here. The connection is a serial channel (one permit), so the releasing operation is |
There was a problem hiding this comment.
When close() encounters an open but idle WQL or command cursor, that cursor still holds connectionPermit, so close() skips auth.reset() and this new cleanup runs only if the caller later consumes or closes the cursor. Fresh evidence in the final code is that openWql() returns while retaining the permit and only WqlEnumeration.next()/close() calls releaseConnection(); if the closed client and abandoned cursor remain referenced, the reversible Basic credential is therefore retained indefinitely. Ensure closing the client disposes the credential even when an idle streaming handle never releases its permit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba037c6. You're right that the wipe was being skipped in exactly that case: close() only ran auth.reset() when it could acquire the connection permit, and an open-but-idle cursor holds the permit indefinitely, so close() never reached the reset and the Basic credential lingered until the caller later consumed or closed the cursor.
The fix makes close() the single, unconditional disposal point. The auth.reset() moved out of the if (locked) block into the finally, after transport.close(). The auth state (NTLM keys, the Basic credential, the Kerberos context) is bound to the connection, not the socket, so it needs no permit to be erased. Now:
- idle handle holds the permit (the case you flagged) ->
close()can't acquire it, skips the shell Delete, but still disposes the auth state in the finally. The credential is erased the moment the client closes. close()does acquire the permit -> identical to before (dispose, then the finally dispose is a no-op since it's idempotent).
releaseConnection() no longer carries the redundant pre-release sweep; it keeps only the post-release backstop for the one case close() cannot see (a worker that re-authenticates after close()'s own reset).
| // disposing: a concurrent reset() that nulls the field cannot make this one dispose a | ||
| // half-disposed GSSContext. Disposing the same context twice is the hazard, not the double | ||
| // null-store, so the local (not the field) is what guards it. | ||
| final GSSContext ctx = context; |
There was a problem hiding this comment.
When close() races the new post-release reset sweep, two threads can enter reset() together, and copying context to a local does not claim it: both can read the same non-null value before either reaches context = null, then call dispose() concurrently. In the remote-command case, the releasing thread can also overwrite a replacement context created while close() re-authenticates to send its best-effort shell Delete, leaving that replacement undisposed. Atomically clear or synchronize access to context before disposing the claimed value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba037c6. Two distinct hazards here, both addressed:
-
The double-dispose race: I was relying on "read into a local, then null the field," which you correctly point out is not a claim. Two threads can both read the same non-null
contextbefore either clears it and both calldispose().contextis now anAtomicReference<GSSContext>, andreset()claims it withgetAndSet(null)- a single atomic step - so exactly one thread disposes the context and any concurrentreset()sees null and skips.authenticate()installs viacontext.set(...)and does its work (requestMutualAuth, requestCredDeleg, initSecContext) on a local before publishing. -
The replacement context from close() re-authenticating: this was a real leak, and it is closed by the same change that fixes the idle-handle case in the other comment.
close()now disposes the auth state unconditionally in its finally block, after it has (optionally) run its best-effort shell Delete (which is the re-authentication that could create a replacement context). So whatever context is live whenclose()'s dispose runs is claimed and disposed, including a replacement created moments earlier. The only residual case - a worker that re-authenticates afterclose()'s dispose, which can happen because the Kerberos GSS exchange does no socket I/O - is caught by the post-release sweep inreleaseConnection(), which now uses the same atomic claim, so it cannot double-dispose either.
| * credential and the SOAP in cleartext, so use {@link #https()} with Basic (Kerberos over | ||
| * plain HTTP is rejected, since it cannot be protected). |
There was a problem hiding this comment.
When HTTP is combined with an ordered list such as authentication(KERBEROS, NTLM), Kerberos is not rejected: resolveAuthScheme() silently omits it and builds an NTLM-only client. This new unconditional Javadoc claim can therefore make callers believe an invalid Kerberos transport will fail closed while the client actually downgrades to another scheme; qualify the statement for Kerberos-only configurations or enforce rejection for every list containing Kerberos.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba037c6 - I chose to enforce rejection for every list containing Kerberos rather than weakening the Javadoc. resolveAuthScheme() now throws WinRMException as soon as it sees KERBEROS in the list over plain HTTP, instead of silently omitting it and building an NTLM-only (or Basic/NTLM) client. The old schemes.isEmpty() escape-hatch branch is gone since the throw happens in the loop.
This makes the actual behavior match the contract the API already advertised:
- the fluent
build()Javadoc says the configuration is "rejected (e.g. Kerberos requested over HTTP)" - it never scoped that to Kerberos-only; authentication.mdsays "Requesting Kerberos on a plain-HTTP client fails at build()";- the CLI already enforces this (CliArguments rejects
--kerberoswithout--https).
So [KERBEROS, NTLM] over HTTP now fails at build() with the same "Kerberos over WinRM requires HTTPS" message as the Kerberos-only case - no silent downgrade to NTLM. If a caller wants Kerberos-when-available with NTLM as a fallback over HTTP, they should build an HTTPS client (NTLM works fine over HTTPS) or simply not list Kerberos; the previous behavior of pretending to try Kerberos and quietly not doing so was the footgun.
WinRMExecutorFactoryTest had a test encoding the old behavior (mixedKerberosNtlmFallsBackToNtlmOverHttp); it now asserts the rejection (mixedKerberosNtlmRejectedOverHttp).
…und 5) Make close() the single, unconditional disposal point for authentication state: it now resets the scheme in the finally block after the transport is closed, even when an idle streaming handle still holds the connection permit (previously the reset was skipped when close() could not acquire the permit, leaving the reversible Basic credential in a referenced-but-closed client). releaseConnection() keeps only the post-release backstop for a worker that re-authenticates after close(). Make the Kerberos GSSContext claim atomic (AtomicReference + getAndSet(null)) so the two threads that can reset() at once (close() and the last in-flight operation) cannot both dispose the same context; the replacement context from close()'s re-authentication is disposed because it runs before the reset. Enforce the Kerberos-requires-HTTPS contract fail-closed for ANY scheme list containing Kerberos over HTTP (not just Kerberos-only), matching the builder's documented 'rejected' contract and the CLI's --kerberos-requires---https guard, instead of silently dropping it from an ordered fallback list. Update the corresponding factory test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba037c6217
ℹ️ 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".
| .credentials("DOMAIN\\Administrator", password) | ||
| .authentication(AuthScheme.NTLM) // NTLM only (also the default) | ||
| // .authentication(AuthScheme.KERBEROS) // Kerberos only | ||
| // .authentication(AuthScheme.BASIC) // HTTP Basic only |
There was a problem hiding this comment.
Uncommenting this newly added alternative leaves the builder on its default HTTP transport, so the example sends both the Basic credential and SOAP payload in cleartext despite the Basic section explicitly instructing users to use HTTPS. Split this into an HTTPS example or add .https() so following the documented configuration does not expose credentials.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
| // Fail closed: Kerberos cannot be protected over plain HTTP, so reject it for any | ||
| // list rather than silently downgrading to the remaining schemes. | ||
| throw new WinRMException( |
There was a problem hiding this comment.
Rejecting every HTTP list containing Kerberos makes the documented authentication(KERBEROS, NTLM) examples invalid because both the authentication-page snippet and the README quick-start use the builder's default HTTP transport. Users following either example now fail at build() instead of getting ordered fallback; add .https() to those examples or explicitly qualify this fallback as HTTPS-only.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
| The credential is the user name exactly as given to `credentials(...)`. A domain-qualified name | ||
| (`DOMAIN\user`) keeps its domain prefix on the wire, which is how a domain controller locates the | ||
| account; a bare name is used as-is. |
There was a problem hiding this comment.
When the supplied username contains whitespace—including an internal space in an account name—WinRMEndpoint removes all whitespace and resolveAuthScheme() reconstructs Basic's account from those normalized parts, so the header does not contain the username “exactly as given” and can authenticate as a different account or receive a 401. Document the normalization here, or preserve the supplied account when constructing the Basic credential.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
| final byte[] token = newContext.initSecContext(new byte[0], 0, 0); | ||
| context.set(newContext); |
There was a problem hiding this comment.
When initSecContext() throws—for example because the service principal is unavailable—the newly created GSSContext has not yet been stored in context, so the subsequent fallback/reset or client close cannot dispose it. Repeated failed Kerberos attempts can therefore leak implementation/native GSS resources; dispose newContext on failure or transfer it into atomically managed ownership before initialization.
Useful? React with 👍 / 👎.
| basicMode = true; | ||
| expectedBasicHeader = expectedAuthorization; |
There was a problem hiding this comment.
When these tests run, withBasicAuth() writes basicMode and expectedBasicHeader on the test thread while handleConnection() reads them on a separately spawned server thread, but neither field is volatile or protected by a shared lock. The Java memory model therefore permits the request thread to observe Basic mode as disabled, or enabled with a stale null expected header, making the new Basic protocol tests intermittently enter the NTLM path or return 401; publish both values safely, with the expected header visible before enabling the mode.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds HTTP Basic as a third authentication scheme alongside NTLM (default) and Kerberos (SPNEGO), across the fluent API, the legacy entry points, the CLI, and the site documentation.
API
AuthScheme.BASIC— fluent builder:WinRMClient.builder(host).credentials(user, password).authentication(AuthScheme.BASIC).build()AuthenticationEnum.BASIC— for the legacyWinRMWqlExecutor/WinRMCommandExecutorentry points--basicoption, mutually exclusive with--ntlmand--kerberosWire behavior
Basic is stateless: the
Basic <base64(user:password)>credential rides theAuthorizationheader of every request, and the SOAP payload travels as plaintext (no message protection). Because of that, the documentation steers users to HTTPS, where TLS protects both credential and payload. A domain-qualified user name (DOMAIN\user) keeps its domain prefix on the wire; the host must haveAllowBasicAuthenabled on the WinRM service.BASICparticipates in the ordered-fallback list like the other schemes.Implementation
BasicAuthScheme(light backend): encodes the credential straight from the caller'schar[]password (never aStringcopy, per the credential contract) and holds the result as an immutable header valueAuthScheme.requestAuthorization()hook on the light interface: NTLM/Kerberos returnnull(token rides the first request only), stateless schemes repeat their header on every requestPlaintextSoapAuthSchemebase class shared byBasicAuthSchemeandKerberosAuthScheme(plaintext-SOAP pass-throughs + authenticated flag) — also resolves the CPD duplication the two classes would otherwise shareTests
Authorizationheader is repeated on every request (Enumerate + Pull), and a wrong-credential case surfacing the standardWinRMAuthenticationExceptionmessageBasicAuthSchemeTest: header encoding, statelessness, reset, plaintext pass-through, and that the passwordchar[]is not retainedAuthenticationEnumTest,WinRMExecutorFactoryTest(Basic over HTTP and HTTPS, fallback lists),WinRMClientBuilderTest(Basic accepted over both transports), andCliArgumentsTest(--basicparsing and mutual-exclusion errors)Docs
authentication.md(new Basic section),cli.md(option + section),index.md,legacy.md,preparing-the-host.md(AllowBasicAuthguidance),migrating-from-winrm4j.md(AuthSchemes.BASICnow maps instead of "none"), and a CHANGELOG entry.mvn verifypasses: 228 tests, 0 Checkstyle / PMD / CPD / SpotBugs findings.README.md is intentionally unchanged (deliberately terse per project convention).