From cd7d03c37016bebaf44987f8828b0872ac2cbedd Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Mon, 31 Aug 2026 20:22:43 +0200 Subject: [PATCH 1/9] Add HTTP Basic authentication as a third auth scheme 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. --- CHANGELOG.md | 15 +++ .../java/org/metricshub/winrm/AuthScheme.java | 5 +- .../org/metricshub/winrm/WinRMClient.java | 16 ++- .../metricshub/winrm/cli/CliArguments.java | 14 ++- .../org/metricshub/winrm/cli/WinRmCli.java | 7 +- .../metricshub/winrm/light/AuthScheme.java | 14 +++ .../winrm/light/BasicAuthScheme.java | 85 ++++++++++++++++ .../winrm/light/KerberosAuthScheme.java | 25 +---- .../winrm/light/LightWinRMService.java | 24 +++-- .../winrm/light/PlaintextSoapAuthScheme.java | 63 ++++++++++++ .../metricshub/winrm/light/WsmanClient.java | 7 +- .../client/auth/AuthenticationEnum.java | 3 +- src/site/markdown/authentication.md | 38 +++++-- src/site/markdown/cli.md | 13 ++- src/site/markdown/index.md | 4 +- src/site/markdown/legacy.md | 2 +- src/site/markdown/migrating-from-winrm4j.md | 12 +-- src/site/markdown/preparing-the-host.md | 13 ++- .../winrm/WinRMClientBuilderTest.java | 20 ++++ .../winrm/cli/CliArgumentsTest.java | 28 ++++++ .../winrm/light/BasicAuthSchemeTest.java | 75 ++++++++++++++ .../winrm/light/FakeWsmanServer.java | 88 +++++++++++++++++ .../winrm/light/WsmanProtocolTest.java | 98 +++++++++++++++++++ .../service/WinRMExecutorFactoryTest.java | 39 ++++++++ .../client/auth/AuthenticationEnumTest.java | 4 + 25 files changed, 646 insertions(+), 66 deletions(-) create mode 100644 src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java create mode 100644 src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java create mode 100644 src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 57af684..cb02b5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to this project are documented in this file. ## [Unreleased] — 2.0.0 +### Added — HTTP Basic authentication + +The client now supports **HTTP Basic** as an authentication scheme, in addition to NTLM and +Kerberos: + +* `WinRMClient.Builder.authentication(AuthScheme.BASIC)` and the CLI's `--basic` option select it. +* Basic is stateless: the credential rides the `Authorization` header of **every** request, and + there is no message protection — the payload travels as plaintext SOAP. It therefore requires + **HTTPS** in practice, where TLS protects the credential and the payload (Basic over plain HTTP + is possible but must never be used, since everything is sent in the clear). +* A domain-qualified user name (`DOMAIN\user`) keeps its domain prefix on the wire; the server + must have `AllowBasicAuth` enabled on the WinRM service. +* `BASIC` joins `AuthenticationEnum` (legacy API) and participates in the ordered-fallback list + like the other schemes. + ### ⚠️ Breaking — SMB file copy replaced by a transfer through the WinRM channel Files passed to `WinRMCommandExecutor.execute(...)` in `localFileToCopyList` are no longer copied diff --git a/src/main/java/org/metricshub/winrm/AuthScheme.java b/src/main/java/org/metricshub/winrm/AuthScheme.java index 46b3c97..9842b16 100644 --- a/src/main/java/org/metricshub/winrm/AuthScheme.java +++ b/src/main/java/org/metricshub/winrm/AuthScheme.java @@ -29,5 +29,8 @@ public enum AuthScheme { NTLM, /** Kerberos (SPNEGO) authentication — requires HTTPS and connecting by the FQDN the KDC knows. */ - KERBEROS + KERBEROS, + + /** HTTP Basic authentication — the credential rides the {@code Authorization} header of every request. */ + BASIC } diff --git a/src/main/java/org/metricshub/winrm/WinRMClient.java b/src/main/java/org/metricshub/winrm/WinRMClient.java index 623cac0..0fb2137 100644 --- a/src/main/java/org/metricshub/winrm/WinRMClient.java +++ b/src/main/java/org/metricshub/winrm/WinRMClient.java @@ -375,7 +375,7 @@ public Builder namespace(final String namespace) { * Set the authentication schemes, tried in the given order until one succeeds. Default: * NTLM only. Kerberos requires HTTPS. * - * @param schemes the schemes in fallback order, e.g. {@code KERBEROS, NTLM} + * @param schemes the schemes in fallback order, e.g. {@code KERBEROS, NTLM} or {@code BASIC} * @return this builder */ public Builder authentication(final AuthScheme... schemes) { @@ -535,9 +535,17 @@ public WinRMClient build() { if (authentication != null) { authentications = new ArrayList<>(authentication.size()); for (final AuthScheme scheme : authentication) { - authentications.add( - scheme == AuthScheme.KERBEROS ? AuthenticationEnum.KERBEROS : AuthenticationEnum.NTLM - ); + switch (scheme) { + case KERBEROS: + authentications.add(AuthenticationEnum.KERBEROS); + break; + case BASIC: + authentications.add(AuthenticationEnum.BASIC); + break; + default: + authentications.add(AuthenticationEnum.NTLM); + break; + } } } diff --git a/src/main/java/org/metricshub/winrm/cli/CliArguments.java b/src/main/java/org/metricshub/winrm/cli/CliArguments.java index e120579..409ebc5 100644 --- a/src/main/java/org/metricshub/winrm/cli/CliArguments.java +++ b/src/main/java/org/metricshub/winrm/cli/CliArguments.java @@ -84,7 +84,9 @@ private CliArguments(final Builder builder) { port = WinRMEndpoint.getEndpointPort(protocol, builder.port); timeout = builder.timeout; permissiveHttps = builder.permissiveHttps; - authentication = builder.kerberos ? AuthenticationEnum.KERBEROS : AuthenticationEnum.NTLM; + authentication = builder.basic + ? AuthenticationEnum.BASIC + : builder.kerberos ? AuthenticationEnum.KERBEROS : AuthenticationEnum.NTLM; kerberosKdc = builder.kerberosKdc; kerberosRealm = builder.kerberosRealm; kerberosRealmInferred = builder.kerberosRealmInferred; @@ -173,6 +175,9 @@ private static int parseOption(final Builder builder, final String[] arguments, case "--kerberos": builder.kerberos = true; return index + 1; + case "--basic": + builder.basic = true; + return index + 1; case "--kerberos-kdc": builder.kerberosKdc = optionValue(arguments, index, option); return nextIndex(argument, index); @@ -253,6 +258,12 @@ private static void validate(final Builder builder) throws CliUsageException { if (builder.ntlm && builder.kerberos) { throw new CliUsageException("--ntlm and --kerberos are mutually exclusive"); } + if (builder.ntlm && builder.basic) { + throw new CliUsageException("--ntlm and --basic are mutually exclusive"); + } + if (builder.kerberos && builder.basic) { + throw new CliUsageException("--kerberos and --basic are mutually exclusive"); + } if (builder.kerberos && !builder.https) { throw new CliUsageException("--kerberos requires --https"); } @@ -547,6 +558,7 @@ private static final class Builder { private boolean permissiveHttps; private boolean ntlm; private boolean kerberos; + private boolean basic; private String kerberosKdc; private String kerberosRealm; private boolean kerberosRealmInferred; diff --git a/src/main/java/org/metricshub/winrm/cli/WinRmCli.java b/src/main/java/org/metricshub/winrm/cli/WinRmCli.java index fc652a0..2d4e0b1 100644 --- a/src/main/java/org/metricshub/winrm/cli/WinRmCli.java +++ b/src/main/java/org/metricshub/winrm/cli/WinRmCli.java @@ -397,7 +397,11 @@ static FluentRemoteOperations connect(final CliArguments arguments, final int co builder.authentication( authentications .stream() - .map(scheme -> scheme == AuthenticationEnum.KERBEROS ? AuthScheme.KERBEROS : AuthScheme.NTLM) + .map( + scheme -> scheme == AuthenticationEnum.KERBEROS + ? AuthScheme.KERBEROS + : scheme == AuthenticationEnum.BASIC ? AuthScheme.BASIC : AuthScheme.NTLM + ) .toArray(AuthScheme[]::new) ); } @@ -506,6 +510,7 @@ private static String help() { " --https-permissive Trust any HTTPS certificate and hostname (insecure)\n" + " --ntlm Use NTLM authentication (default)\n" + " --kerberos Use Kerberos authentication (requires HTTPS)\n" + + " --basic Use HTTP Basic authentication (use HTTPS to protect the credential)\n" + " --kerberos-kdc Set the Kerberos KDC; infer realm from its DNS suffix\n" + " --kerberos-realm Override the realm inferred from --kerberos-kdc\n" + " --help Show this help\n" + diff --git a/src/main/java/org/metricshub/winrm/light/AuthScheme.java b/src/main/java/org/metricshub/winrm/light/AuthScheme.java index 105fae8..4096518 100644 --- a/src/main/java/org/metricshub/winrm/light/AuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/AuthScheme.java @@ -42,6 +42,20 @@ interface AuthScheme { */ String authenticate(HttpTransport transport) throws Exception; + /** + * The {@code Authorization} header value a request must carry, or {@code null} when the + * connection's authentication state needs no per-request header. + *

+ * For NTLM and Kerberos the token rides only the first real request (hence the value returned + * by {@link #authenticate(HttpTransport)}), so this is {@code null}; for stateless schemes + * such as Basic the header must repeat on EVERY request. + * + * @return the {@code Authorization} header value, or {@code null} + */ + default String requestAuthorization() { + return null; + } + /** @return whether the connection is currently authenticated. */ boolean isAuthenticated(); diff --git a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java new file mode 100644 index 0000000..63a56b0 --- /dev/null +++ b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java @@ -0,0 +1,85 @@ +package org.metricshub.winrm.light; + +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + +import java.nio.charset.StandardCharsets; +import java.util.Base64; + +/** + * HTTP Basic authentication scheme. The credential (base64 of {@code user:password}) rides the + * {@code Authorization} header of EVERY request, so there is no stateful handshake and no message + * protection — the payload travels as plaintext SOAP. Confidentiality therefore relies on the + * transport: over HTTPS (TLS) the credential and the SOAP are protected; over plain HTTP they are + * sent in the clear and must not be used. + *

+ * The credential is computed from the caller's {@code char[]} password and held as an immutable + * header value; the password array itself is never retained, so it can be wiped by the caller after + * closing the client. + */ +final class BasicAuthScheme extends PlaintextSoapAuthScheme { + + private final String authorizationHeader; + + /** + * @param username the account name (without any {@code DOMAIN\} prefix) + * @param password the account password, kept as {@code char[]} so the caller owns the single + * wipeable copy of the secret + */ + BasicAuthScheme(final String username, final char[] password) { + this.authorizationHeader = "Basic " + Base64.getEncoder().encodeToString( + buildCredential(username, password) + ); + } + + /** + * Encode {@code user:password} to UTF-8 straight from the caller's {@code char[]} password, + * never forming a {@code String} copy of the secret (the credentials contract: the caller owns + * the single wipeable array, and no live copy of the password may outlive {@code close()}). + */ + private static byte[] buildCredential(final String username, final char[] password) { + final StringBuilder user = new StringBuilder(username.length() + 1 + password.length); + user.append(username); + user.append(':'); + for (final char c : password) { + user.append(c); + } + return user.toString().getBytes(StandardCharsets.UTF_8); + } + + @Override + public String authenticate(final HttpTransport transport) throws Exception { + // No handshake: Basic has no server challenge. Mark the connection authenticated so the + // client proceeds straight to the first real request (which carries the Authorization header). + authenticated = true; + return null; + } + + @Override + public String requestAuthorization() { + // Stateless: the same header repeats on every request, not just the first. + return authorizationHeader; + } + + @Override + public void reset() { + authenticated = false; + } +} diff --git a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java index a07b6c3..a62ff9d 100644 --- a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java @@ -53,9 +53,8 @@ * the {@code java.security.krb5.*} system properties), exactly as the CXF path did — the library * sets none itself. */ -final class KerberosAuthScheme implements AuthScheme { +final class KerberosAuthScheme extends PlaintextSoapAuthScheme { - private static final String SOAP_CONTENT_TYPE = "application/soap+xml;charset=UTF-8"; // SPNEGO mechanism OID — the "Negotiate" scheme Windows http.sys expects. private static final String SPNEGO_OID = "1.3.6.1.5.5.2"; @@ -67,7 +66,6 @@ final class KerberosAuthScheme implements AuthScheme { private final Path ticketCache; private GSSContext context; - private boolean authenticated; /** * @param servicePrincipalHost the host whose {@code HTTP/} SPN to target — must be the FQDN @@ -112,11 +110,6 @@ public String authenticate(final HttpTransport transport) throws Exception { return "Negotiate " + Base64.getEncoder().encodeToString(apReq); } - @Override - public boolean isAuthenticated() { - return authenticated; - } - @Override public void reset() { if (context != null) { @@ -130,22 +123,6 @@ public void reset() { authenticated = false; } - @Override - public byte[] wrap(final byte[] soapUtf8) { - // HTTPS only: TLS provides confidentiality, so the SOAP travels plaintext. - return soapUtf8; - } - - @Override - public String wrapContentType() { - return SOAP_CONTENT_TYPE; - } - - @Override - public byte[] unwrap(final HttpTransport.Response response) { - return response.body; - } - /** Obtain a Kerberos {@link Subject} (holding the TGT) via a programmatic JAAS login. */ private Subject login() throws Exception { final LoginContext loginContext = new LoginContext("", null, callbackHandler(), krb5Configuration()); diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index 639ed28..30fe87f 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -52,9 +52,10 @@ * {@code ServiceLoader} poisoning (it uses the JDK-default XML factories). *

* Supports NTLM over HTTP (with message encryption) and over HTTPS (plaintext SOAP inside TLS, - * validating the server certificate by default; see {@link LightTls}), and Kerberos over HTTPS - * (SPNEGO via the JDK GSS-API; see {@link KerberosAuthScheme}). A multi-scheme request such as - * {@code [KERBEROS, NTLM]} is tried in order with fallback. + * validating the server certificate by default; see {@link LightTls}), Kerberos over HTTPS (SPNEGO + * via the JDK GSS-API; see {@link KerberosAuthScheme}), and HTTP Basic (the credential rides the + * {@code Authorization} header of every request; see {@link BasicAuthScheme}). A multi-scheme + * request such as {@code [KERBEROS, NTLM]} is tried in order with fallback. */ public final class LightWinRMService implements WindowsRemoteExecutor { @@ -74,7 +75,7 @@ private LightWinRMService(final WinRMEndpoint winRMEndpoint, final WsmanClient c * @param timeout timeout in milliseconds (must be > 0) * @param ticketCache Kerberos ticket cache path (used by the Kerberos scheme; {@code null} logs * in with the password) - * @param authentications requested authentication schemes, tried in order (NTLM and/or Kerberos); + * @param authentications requested authentication schemes, tried in order (NTLM, Kerberos, and/or Basic); * {@code null}/empty means NTLM only * @return a new {@code LightWinRMService} * @throws WinRMException on invalid arguments or an unsupported authentication request @@ -96,7 +97,7 @@ public static LightWinRMService createInstance( * @param timeout timeout in milliseconds (must be > 0) * @param ticketCache Kerberos ticket cache path (used by the Kerberos scheme; {@code null} logs * in with the password) - * @param authentications requested authentication schemes, tried in order (NTLM and/or Kerberos); + * @param authentications requested authentication schemes, tried in order (NTLM, Kerberos, and/or Basic); * {@code null}/empty means NTLM only * @param sslContext the {@link SSLContext} providing the HTTPS socket factory (hostname * verification stays on); {@code null} uses the default configuration @@ -123,7 +124,7 @@ public static LightWinRMService createInstance( * @param timeout timeout in milliseconds (must be > 0) * @param ticketCache Kerberos ticket cache path (used by the Kerberos scheme; {@code null} logs * in with the password) - * @param authentications requested authentication schemes, tried in order (NTLM and/or Kerberos); + * @param authentications requested authentication schemes, tried in order (NTLM, Kerberos, and/or Basic); * {@code null}/empty means NTLM only * @param sslContext the {@link SSLContext} providing the HTTPS socket factory (hostname * verification stays on); {@code null} uses the default configuration @@ -167,7 +168,7 @@ public static LightWinRMService createInstance( * @param timeout timeout in milliseconds (must be > 0) * @param ticketCache Kerberos ticket cache path (used by the Kerberos scheme; {@code null} logs * in with the password) - * @param authentications requested authentication schemes, tried in order (NTLM and/or Kerberos); + * @param authentications requested authentication schemes, tried in order (NTLM, Kerberos, and/or Basic); * {@code null}/empty means NTLM only * @param sslContext the {@link SSLContext} providing the HTTPS socket factory (hostname * verification stays on); {@code null} uses the default configuration @@ -275,9 +276,16 @@ private static AuthScheme resolveAuthScheme( schemes.add(new KerberosAuthScheme(winRMEndpoint.getHostname(), username, password, ticketCache)); } // else: Kerberos is unavailable over plain HTTP — leave it out of the candidate list. + } 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)); } else { throw new WinRMException( - "The light WinRM backend supports only NTLM and Kerberos (requested: " + requested + ")." + "The light WinRM backend supports only NTLM, Kerberos, and Basic (requested: " + + requested + + ")." ); } } diff --git a/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java b/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java new file mode 100644 index 0000000..91f6000 --- /dev/null +++ b/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java @@ -0,0 +1,63 @@ +package org.metricshub.winrm.light; + +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + +/** + * Base for authentication schemes that exchange **plaintext SOAP inside TLS**: there is no NTLM + * message sealing, so {@link #wrap(byte[])} and {@link #unwrap(HttpTransport.Response)} are + * pass-throughs and the only difference between subclasses is the handshake and the + * connection-bound state it holds. Today that is {@link KerberosAuthScheme} (SPNEGO, HTTPS-only) + * and {@link BasicAuthScheme} (stateless {@code Authorization} header, HTTPS in practice). + *

+ * Subclasses own the authenticated flag and implement {@link #authenticate(HttpTransport)} and + * {@link #reset()}; everything else is shared here. + */ +abstract class PlaintextSoapAuthScheme implements AuthScheme { + + /** The content type of the plaintext SOAP body these schemes exchange. */ + protected static final String SOAP_CONTENT_TYPE = "application/soap+xml;charset=UTF-8"; + + // Set by authenticate() and cleared by reset(): whether this connection is currently + // authenticated. volatile — see WinRMSession's notes on why visibility, not atomics, is needed. + protected volatile boolean authenticated; + + @Override + public final boolean isAuthenticated() { + return authenticated; + } + + @Override + public final byte[] wrap(final byte[] soapUtf8) { + // No message protection: the SOAP travels plaintext. Confidentiality comes from TLS (HTTPS), + // which is why only HTTPS-backed schemes may extend this base. + return soapUtf8; + } + + @Override + public final String wrapContentType() { + return SOAP_CONTENT_TYPE; + } + + @Override + public final byte[] unwrap(final HttpTransport.Response response) { + return response.body; + } +} diff --git a/src/main/java/org/metricshub/winrm/light/WsmanClient.java b/src/main/java/org/metricshub/winrm/light/WsmanClient.java index 0c66ff0..17f6d93 100644 --- a/src/main/java/org/metricshub/winrm/light/WsmanClient.java +++ b/src/main/java/org/metricshub/winrm/light/WsmanClient.java @@ -1106,9 +1106,10 @@ private Decoded send(final String soap) throws Exception { continue; } } - // The handshake's Authorization accompanies the first real request; later requests on the - // already-authenticated connection carry no Authorization header. - final String authorization = pendingAuthorization; + // The handshake's Authorization accompanies the first real request; stateless schemes + // (Basic) instead repeat their header on EVERY request, and NTLM/Kerberos need none after + // the first. + final String authorization = pendingAuthorization != null ? pendingAuthorization : auth.requestAuthorization(); pendingAuthorization = null; final HttpTransport.Response resp = transport.post( diff --git a/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java b/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java index 0cc0684..17849d9 100644 --- a/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java +++ b/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java @@ -28,7 +28,8 @@ public enum AuthenticationEnum { NTLM, - KERBEROS; + KERBEROS, + BASIC; private static final Map VALUES_OF = Stream .of(values()) diff --git a/src/site/markdown/authentication.md b/src/site/markdown/authentication.md index f784af8..1c9b99e 100644 --- a/src/site/markdown/authentication.md +++ b/src/site/markdown/authentication.md @@ -1,12 +1,12 @@ -keywords: authentication, ntlm, kerberos, spnego, domain, realm, kdc, krb5, ticket cache -description: Authenticate to WinRM with NTLM or Kerberos (SPNEGO), including domain accounts, ordered fallback, and Kerberos configuration. +keywords: authentication, ntlm, kerberos, spnego, basic, domain, realm, kdc, krb5, ticket cache +description: Authenticate to WinRM with NTLM, Kerberos (SPNEGO), or HTTP Basic, including domain accounts, ordered fallback, and Kerberos configuration. # Authentication -The client authenticates with either **NTLM** or **Kerberos (SPNEGO)**. The scheme is chosen with -`authentication(...)` on the [`WinRMClient`](apidocs/org/metricshub/winrm/WinRMClient.html) +The client authenticates with **NTLM**, **Kerberos (SPNEGO)**, or **HTTP Basic**. The scheme is +chosen with `authentication(...)` on the [`WinRMClient`](apidocs/org/metricshub/winrm/WinRMClient.html) builder, which takes one or more [`AuthScheme`](apidocs/org/metricshub/winrm/AuthScheme.html) values: @@ -17,6 +17,7 @@ WinRMClient.builder("server.example.com") .credentials("DOMAIN\\Administrator", password) .authentication(AuthScheme.NTLM) // NTLM only (also the default) // .authentication(AuthScheme.KERBEROS) // Kerberos only + // .authentication(AuthScheme.BASIC) // HTTP Basic only // .authentication(AuthScheme.KERBEROS, AuthScheme.NTLM) // ordered fallback .build(); ``` @@ -89,6 +90,31 @@ java -Djava.security.krb5.realm=EXAMPLE.COM \ The optional `ticketCache(Path)` builder option points at a Kerberos ticket cache to use for the connection; without it, Kerberos logs in with the user name and password. +## Basic + +HTTP Basic sends the credential in the `Authorization` header of **every** request — there is no +handshake and no message protection, so the payload travels as plaintext SOAP. It works over both +transports, but over plain HTTP the credential and the data are sent **in the clear**: use Basic +over HTTPS only, where TLS protects both. + +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. + +```java +try (WinRMClient client = WinRMClient.builder("server.example.com") + .https() + .credentials("DOMAIN\\Administrator", password) + .authentication(AuthScheme.BASIC) + .build()) { + ... +} +``` + +The server must have Basic authentication enabled +(`winrm set winrm/config/service @{AllowBasicAuth=true}`); see +[Preparing the Windows Host](preparing-the-host.html). + ## Authentication failures A rejected credential (after every scheme of the fallback list was tried) surfaces as a @@ -97,8 +123,8 @@ whose message has the stable form `Authentication error on with user ## Choosing the scheme on the command line -The standalone jar selects the scheme with `--ntlm` (the default) or `--kerberos`. The two are -mutually exclusive, and `--kerberos` requires `--https`: +The standalone jar selects the scheme with `--ntlm` (the default), `--kerberos`, or `--basic`. The +three are mutually exclusive, and `--kerberos` requires `--https`: ```bash java -jar ${project.artifactId}-${project.version}-standalone.jar \ diff --git a/src/site/markdown/cli.md b/src/site/markdown/cli.md index e886f86..3b2dbf5 100644 --- a/src/site/markdown/cli.md +++ b/src/site/markdown/cli.md @@ -1,5 +1,5 @@ keywords: cli, command line, standalone, jar, wql, exec, shell, interactive, stdin, exit codes, manual -description: Manual page of the winrm-java standalone command-line client - subcommands, options, passwords, Kerberos, streaming output, the interactive shell, and exit codes. +description: Manual page of the winrm-java standalone command-line client - subcommands, options, passwords, authentication schemes (NTLM, Kerberos, Basic), streaming output, the interactive shell, and exit codes. # Command-Line Client @@ -48,12 +48,13 @@ takes no argument. | `--https-permissive` | Trust any HTTPS certificate and hostname. Intentionally insecure: testing and isolated hosts only. Requires `--https`. | | `--ntlm` | Authenticate with NTLM (the default). | | `--kerberos` | Authenticate with Kerberos. Requires `--https`. | +| `--basic` | Authenticate with HTTP Basic. Use with `--https` so the credential is not sent in the clear. | | `--kerberos-kdc ` | Set the Kerberos KDC for this invocation; the realm is inferred from its DNS suffix (see below). | | `--kerberos-realm ` | Override the realm inferred from `--kerberos-kdc`. | | `--help` | Print the usage summary. | | `--version` | Print the build version. | -`--ntlm` and `--kerberos` are mutually exclusive, as are the two password options. +`--ntlm`, `--kerberos`, and `--basic` are mutually exclusive, as are the two password options. ## Passwords @@ -76,7 +77,13 @@ common Active Directory DNS naming convention; it is not guaranteed by Kerberos, fully qualified DNS name. Both options are valid only with `--kerberos`, and `--kerberos-realm` requires `--kerberos-kdc`. -See [Authentication](authentication.html) for how NTLM and Kerberos work on the wire. +## Basic + +`--basic` authenticates with HTTP Basic, sending the credential in the `Authorization` header of +every request. It has no message protection, so the credential and payload are plaintext over HTTP — +combine it with `--https` (and `--https-permissive` for self-signed hosts) so TLS protects them. + +See [Authentication](authentication.html) for how NTLM, Kerberos, and Basic work on the wire. ## Output diff --git a/src/site/markdown/index.md b/src/site/markdown/index.md index a2d442c..1ef7114 100644 --- a/src/site/markdown/index.md +++ b/src/site/markdown/index.md @@ -21,8 +21,8 @@ Both operations can also **stream**: WQL rows are consumed page by page as they returning a `java.lang.Process`-like handle) — memory stays bounded regardless of the result size. -It supports **NTLM** over HTTP (with message encryption) and HTTPS, and **Kerberos (SPNEGO)** over -HTTPS ([Authentication](authentication.html)). +It supports **NTLM** over HTTP (with message encryption) and HTTPS, **Kerberos (SPNEGO)** over +HTTPS, and **HTTP Basic** over HTTPS ([Authentication](authentication.html)). Since 2.0.0 the client has **zero runtime dependencies** (no Apache CXF / JAX-WS / JAXB stack, no SMB stack) and is immune by construction to JAXP `ServiceLoader` conflicts, because it uses the diff --git a/src/site/markdown/legacy.md b/src/site/markdown/legacy.md index eea8bea..da5fce6 100644 --- a/src/site/markdown/legacy.md +++ b/src/site/markdown/legacy.md @@ -83,7 +83,7 @@ all **checked**. [TLS / HTTPS](tls.html)); there is no per-call trust store. * Authentication schemes come from [`AuthenticationEnum`](apidocs/org/metricshub/winrm/service/client/auth/AuthenticationEnum.html) - (`NTLM`, `KERBEROS`), with the same ordered-fallback semantics as the fluent + (`NTLM`, `KERBEROS`, `BASIC`), with the same ordered-fallback semantics as the fluent [`AuthScheme`](apidocs/org/metricshub/winrm/AuthScheme.html). * For advanced use, the underlying reusable executor is also public: [`WinRMExecutorFactory.createInstance(...)`](apidocs/org/metricshub/winrm/service/WinRMExecutorFactory.html) diff --git a/src/site/markdown/migrating-from-winrm4j.md b/src/site/markdown/migrating-from-winrm4j.md index 8c30db9..4d413b2 100644 --- a/src/site/markdown/migrating-from-winrm4j.md +++ b/src/site/markdown/migrating-from-winrm4j.md @@ -80,7 +80,7 @@ with largely overlapping options. Both map to the single | `port(int)` | `port(int)` | | `authenticationScheme(AuthSchemes.NTLM)` | `authentication(AuthScheme.NTLM)` — the default; several schemes form an ordered fallback list ([Authentication](authentication.html)) | | `authenticationScheme(AuthSchemes.KERBEROS)` | `authentication(AuthScheme.KERBEROS)` — requires `https()` (see [behavioral differences](#behavioral-differences)) | -| `authenticationScheme(AuthSchemes.BASIC)` | none — use NTLM; see [behavioral differences](#behavioral-differences) | +| `authenticationScheme(AuthSchemes.BASIC)` | `authentication(AuthScheme.BASIC)` — over HTTPS (see [behavioral differences](#behavioral-differences)) | | `disableCertificateChecks(true)` | `trustAllCertificates()` | | `sslContext(SSLContext)` | `sslContext(SSLContext)` — hostname verification stays on | | `hostnameVerifier(...)`, `sslSocketFactory(...)` | none — hostname verification is all or nothing: on with `sslContext(...)`, off (together with certificate validation) with `trustAllCertificates()`. There is no custom-verifier hook, so the certificate must identify the hostname you connect by ([TLS / HTTPS](tls.html)) | @@ -134,11 +134,11 @@ the switch: final 0.12.x releases. Here, HTTP always uses NTLM message encryption — there is no unencrypted mode and nothing to configure, and hosts that require encryption (`AllowUnencrypted=false`, the Windows default) work out of the box. -* **No Basic authentication.** Basic sends credentials effectively in the clear and is disabled on - Windows by default; the client does not implement it. Authenticate with NTLM instead — and - check the host: `Negotiate` authentication must be enabled on the WinRM service (it is what - carries NTLM, and is `True` by default), and NTLM must not be disabled by security policy - ([Preparing the Windows Host](preparing-the-host.html)). +* **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 + `AllowBasicAuth` enabled on the WinRM service and be reachable over HTTPS + ([Preparing the Windows Host](preparing-the-host.html)). NTLM remains the recommended scheme. * **Kerberos requires HTTPS.** winrm4j runs Kerberos over plain HTTP; this client refuses at `build()`, because it does not implement Kerberos message encryption — without TLS the payload would travel unprotected. Connect with `https()` and by the FQDN the KDC knows diff --git a/src/site/markdown/preparing-the-host.md b/src/site/markdown/preparing-the-host.md index a953a66..fe98ae4 100644 --- a/src/site/markdown/preparing-the-host.md +++ b/src/site/markdown/preparing-the-host.md @@ -23,16 +23,19 @@ when it is already on, how to turn it on, and how to get the privileges right. | A listener | HTTP on port **5985**, or HTTPS on port **5986**. See [TLS / HTTPS](tls.html). | | Firewall open on that port | Inbound, from the machine running the client. | | `Negotiate` authentication enabled on the service | **`True` by default.** This is what carries NTLM; `Kerberos` (also `True` by default) carries Kerberos. | +| `AllowBasicAuth` enabled on the service | **Only for HTTP Basic** (not for NTLM or Kerberos). See below. | | An account with the right privileges | See [Privileges](#Privileges) below. | Just as important, a few settings that other WinRM guides tell you to change are **not** needed here: -* **`AllowUnencrypted` stays `False`.** Over plain HTTP the client protects the payload with - **NTLM message encryption**, so the service's default refusal of unencrypted traffic is - satisfied. If a guide tells you to set `AllowUnencrypted=true`, that advice is for clients that - use Basic authentication — not this one. -* **`Basic` and `CredSSP` stay `False`.** The client authenticates with NTLM or Kerberos only +* **`AllowUnencrypted` stays `False` for NTLM and Kerberos.** Over plain HTTP the client protects + the payload with **NTLM message encryption**, so the service's default refusal of unencrypted + traffic is satisfied. (Exception: HTTP Basic has no message protection, so Basic requires + HTTPS, where TLS provides the confidentiality — see + [Authentication](authentication.html).) +* **`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 ([Authentication](authentication.html)). * **`TrustedHosts` is irrelevant.** That is a setting on the *Windows* WinRM **client**, consulted by the `winrs` command-line tool. A Java client never reads it, so you do not need to add anything diff --git a/src/test/java/org/metricshub/winrm/WinRMClientBuilderTest.java b/src/test/java/org/metricshub/winrm/WinRMClientBuilderTest.java index 487a6f6..8654891 100644 --- a/src/test/java/org/metricshub/winrm/WinRMClientBuilderTest.java +++ b/src/test/java/org/metricshub/winrm/WinRMClientBuilderTest.java @@ -121,6 +121,26 @@ void kerberosOverHttpIsRejectedAtBuildTime() { assertTrue(e.getMessage().contains("HTTPS"), e.getMessage()); } + @Test + void basicIsAcceptedOverHttpAndHttps() { + // Unlike Kerberos, Basic is a plain-HTTP scheme (the credential rides the Authorization + // header), so a Basic-only client must build over both transports. build() does not connect. + try (WinRMClient client = validBuilder().authentication(AuthScheme.BASIC).build()) { + assertEquals("host", client.hostname()); + } + try (WinRMClient client = validBuilder().https().authentication(AuthScheme.BASIC).build()) { + assertEquals("host", client.hostname()); + } + } + + @Test + void basicFallsBackWithOtherSchemes() { + // An ordered fallback list with Basic alongside NTLM builds over HTTP without error. + try (WinRMClient client = validBuilder().authentication(AuthScheme.BASIC, AuthScheme.NTLM).build()) { + assertEquals("host", client.hostname()); + } + } + @Test void buildSucceedsWithoutConnecting() { // The fluent one-liner shape: build() must not reach out to the (nonexistent) host. diff --git a/src/test/java/org/metricshub/winrm/cli/CliArgumentsTest.java b/src/test/java/org/metricshub/winrm/cli/CliArgumentsTest.java index b66cd72..b3f7139 100644 --- a/src/test/java/org/metricshub/winrm/cli/CliArgumentsTest.java +++ b/src/test/java/org/metricshub/winrm/cli/CliArgumentsTest.java @@ -85,6 +85,26 @@ void parsesHttpsKerberosAndExplicitValues() throws Exception { } } + @Test + void parsesBasicOverHttp() throws Exception { + try ( + CliArguments parsed = CliArguments.parse( + new String[] + { + "--hostname=host.example.net", + "--username=user@example.net", + "--password=secret", + "--basic", + "command", + "whoami" + } + )) { + // Basic is not HTTPS-only (unlike Kerberos): a plain HTTP endpoint is valid. + assertEquals(WinRMHttpProtocolEnum.HTTP, parsed.protocol()); + assertEquals(List.of(AuthenticationEnum.BASIC), parsed.authentications()); + } + } + @Test void infersKerberosRealmFromKdcDnsSuffix() throws Exception { try ( @@ -253,6 +273,14 @@ void rejectsInvalidArguments() { "--ntlm and --kerberos are mutually exclusive", concat(base, "--https", "--ntlm", "--kerberos", "command", "whoami") }, + { + "--ntlm and --basic are mutually exclusive", + concat(base, "--ntlm", "--basic", "command", "whoami") + }, + { + "--kerberos and --basic are mutually exclusive", + concat(base, "--https", "--kerberos", "--basic", "command", "whoami") + }, { "--kerberos requires --https", concat(base, "--kerberos", "command", "whoami") }, { "--kerberos-kdc and --kerberos-realm require --kerberos", diff --git a/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java new file mode 100644 index 0000000..d48576d --- /dev/null +++ b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java @@ -0,0 +1,75 @@ +package org.metricshub.winrm.light; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import org.junit.jupiter.api.Test; + +/** Verifies the HTTP Basic scheme's header encoding and stateless request behavior (no network). */ +class BasicAuthSchemeTest { + + @Test + void encodesDomainQualifiedCredentialAsBasicHeader() { + final BasicAuthScheme scheme = new BasicAuthScheme("DOMAIN\\user", "s3cret".toCharArray()); + + final String expected = "Basic " + Base64.getEncoder().encodeToString( + ("DOMAIN\\user:s3cret").getBytes(StandardCharsets.UTF_8) + ); + assertEquals(expected, scheme.requestAuthorization()); + } + + @Test + void headerIsStatelessAndIdenticalOnEveryRequest() throws Exception { + final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); + + final HttpTransport transport = new HttpTransport("localhost", 1, 1000); + final String first = scheme.authenticate(transport); + // authenticate() returns null: Basic needs no first-request token; the header is per-request. + assertEquals(null, first); + assertTrue(scheme.isAuthenticated()); + + final String again = scheme.requestAuthorization(); + final String yetAgain = scheme.requestAuthorization(); + assertEquals(again, yetAgain); + assertTrue(again.startsWith("Basic ")); + } + + @Test + void resetReturnsToUnauthenticatedState() throws Exception { + final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); + scheme.authenticate(new HttpTransport("localhost", 1, 1000)); + assertTrue(scheme.isAuthenticated()); + + scheme.reset(); + assertFalse(scheme.isAuthenticated()); + // The credential header is still available: Basic re-sends it on the next request, so reset + // only clears the authenticated flag, not the derived header. + assertTrue(scheme.requestAuthorization().startsWith("Basic ")); + } + + @Test + void wrapAndUnwrapArePlaintextPassThrough() { + final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); + final byte[] soap = "".getBytes(StandardCharsets.UTF_8); + + // No message protection: the SOAP bytes travel verbatim. + assertSame(soap, scheme.wrap(soap)); + assertEquals("application/soap+xml;charset=UTF-8", scheme.wrapContentType()); + } + + @Test + void passwordIsNeverRetainedAfterConstruction() { + // The scheme must encode the credential at construction and not hold the caller's char[]: + // wiping the array afterward must not change the (already-derived) header. + final char[] password = "s3cret".toCharArray(); + final BasicAuthScheme scheme = new BasicAuthScheme("user", password); + final String header = scheme.requestAuthorization(); + + java.util.Arrays.fill(password, '\0'); + assertEquals(header, scheme.requestAuthorization()); + } +} diff --git a/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java b/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java index 66e9d80..769a3ab 100644 --- a/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java +++ b/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java @@ -113,6 +113,13 @@ static final class Scripted { private volatile boolean closed; private volatile boolean chunkedResponses; + // HTTP Basic mode: instead of the NTLM handshake, each request must carry the expected + // Authorization header, and scripted bodies are served as PLAINTEXT SOAP (Basic has no message + // protection — the payload is never encrypted by the client). + private boolean basicMode; + private String expectedBasicHeader; + private final List requestAuthorizations = new CopyOnWriteArrayList<>(); + /** * Start the fake server on an ephemeral local port. * @@ -208,6 +215,31 @@ public FakeWsmanServer withChunkedResponses() { return this; } + /** + * Enable HTTP Basic mode: every request must carry {@code expectedAuthorization} in its + * {@code Authorization} header (401 otherwise), and the scripted bodies are served as plaintext + * SOAP — Basic has no message protection, so the client never encrypts them. + * + * @param expectedAuthorization the exact {@code Authorization} header value to expect (e.g. + * {@code Basic }) + * @return this server, for chaining + */ + public FakeWsmanServer withBasicAuth(final String expectedAuthorization) { + basicMode = true; + expectedBasicHeader = expectedAuthorization; + return this; + } + + /** + * The {@code Authorization} header values received so far, in request order ({@code null} + * entries for requests that carried none). + * + * @return a copy of the received authorization header values + */ + public List requestAuthorizations() { + return new ArrayList<>(requestAuthorizations); + } + /** * The plaintext SOAP request bodies received so far, in order (after decryption). * @@ -313,6 +345,19 @@ private void handleConnection(final Socket socket) { return; // client closed the connection } final String authorization = request.header("authorization"); + requestAuthorizations.add(authorization); + if (basicMode) { + // HTTP Basic: the credential must ride the Authorization header of EVERY request, + // and there is no message protection — serve plaintext SOAP. + if (authorization == null || !authorization.equals(expectedBasicHeader)) { + respond(out, 401, "WWW-Authenticate: Basic realm=\"winrm\"", null, null); + continue; + } + if (!serveScriptedBasic(out, request.body)) { + return; // scripted mid-exchange drop + } + continue; + } if (serverSession == null || authorization != null) { final byte[] token = negotiateToken(authorization); if (token == null) { @@ -391,6 +436,49 @@ private boolean serveScripted(final OutputStream out, final WinRMSession session return true; } + /** + * Serve the next scripted response in HTTP Basic mode: the request body is already plaintext + * SOAP (no message protection) and the response is served as plaintext SOAP too. + * + * @return {@code true} to keep the connection alive, {@code false} when the script asked for a + * connection drop instead of a response + */ + private boolean serveScriptedBasic(final OutputStream out, final byte[] plaintextBody) throws IOException { + decryptedRequests.add(new String(plaintextBody, StandardCharsets.UTF_8)); + + Scripted next; + synchronized (script) { + next = script.pollFirst(); + } + if (next != null && next.status == Scripted.DROP) { + return false; + } + if (next == null) { + next = new Scripted( + 500, + "" + + "FakeWsmanServer: no scripted response left" + + "" + ); + } + if (next.delayMillis > 0) { + try { + Thread.sleep(next.delayMillis); + } catch (final InterruptedException e) { + Thread.currentThread().interrupt(); + return false; // test shutdown + } + } + respond( + out, + next.status, + null, + "application/soap+xml;charset=UTF-8", + next.soapBody.getBytes(StandardCharsets.UTF_8) + ); + return true; + } + // --- NTLM server side ------------------------------------------------------- private static byte[] negotiateToken(final String authorization) { diff --git a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java index 23f52a7..706e453 100644 --- a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java +++ b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java @@ -34,6 +34,7 @@ import static org.metricshub.winrm.light.FakeWsmanResponses.stream; import java.nio.charset.StandardCharsets; +import java.util.Base64; import java.util.List; import java.util.Map; import org.junit.jupiter.api.AfterEach; @@ -449,6 +450,103 @@ void wrongPasswordSurfacesTheCxfAuthenticationErrorMessage() throws Exception { } } + // --- HTTP Basic --------------------------------------------------------------- + + @Test + void basicAuthenticatesEveryRequestOverPlaintextHttp() throws Exception { + // HTTP Basic is stateless: the Authorization header must repeat on EVERY request, and the + // payload is plaintext SOAP (no message protection). The fake server enforces the header and + // serves plaintext responses, so a successful round trip proves both. + final String expectedHeader = "Basic " + Base64.getEncoder().encodeToString( + (DOMAIN + "\\" + USER + ":" + PASSWORD).getBytes(StandardCharsets.UTF_8) + ); + server.withBasicAuth(expectedHeader); + server + .enqueue( + 200, + envelope( + "" + + "uuid:CTX-1" + + "" + + service("Spooler", "Running") + + "" + + "" + ) + ) + .enqueue( + 200, + envelope( + "" + + "" + + service("WinRM", "Running") + + "" + + "" + + "" + ) + ); + + final WinRMEndpoint endpoint = new WinRMEndpoint( + WinRMHttpProtocolEnum.HTTP, + "127.0.0.1", + server.port(), + DOMAIN + "\\" + USER, + PASSWORD.toCharArray(), + null + ); + try ( + LightWinRMService service = LightWinRMService + .createInstance(endpoint, TIMEOUT, null, List.of(AuthenticationEnum.BASIC))) { + final List> rows = service.executeWql("SELECT Name,State FROM Win32_Service", TIMEOUT); + + assertEquals(2, rows.size()); + assertEquals("Spooler", rows.get(0).get("Name")); + assertEquals("WinRM", rows.get(1).get("Name")); + } + + // The header must have been repeated on BOTH the Enumerate and the Pull (stateless Basic). + final List authorizations = server.requestAuthorizations(); + assertEquals(2, authorizations.size(), () -> String.join("\n", authorizations)); + assertEquals(expectedHeader, authorizations.get(0)); + assertEquals(expectedHeader, authorizations.get(1)); + } + + @Test + void basicWithWrongCredentialSurfacesTheCxfAuthenticationErrorMessage() throws Exception { + // The server expects a different Basic credential than the client presents, so it rejects the + // request with 401; the client must surface the standard (CXF-parity) authentication error. + server.withBasicAuth("Basic " + Base64.getEncoder().encodeToString("user:wrong".getBytes(StandardCharsets.UTF_8))); + server.enqueue(200, envelope("")); + + final WinRMEndpoint endpoint = new WinRMEndpoint( + WinRMHttpProtocolEnum.HTTP, + "127.0.0.1", + server.port(), + DOMAIN + "\\" + USER, + PASSWORD.toCharArray(), + null + ); + try ( + LightWinRMService service = LightWinRMService + .createInstance(endpoint, TIMEOUT, null, List.of(AuthenticationEnum.BASIC))) { + final WinRMException e = assertThrows( + WinRMException.class, + () -> service.executeWql("SELECT Name FROM Win32_Service", TIMEOUT) + ); + assertEquals( + "Authentication error on http://127.0.0.1:" + server.port() + "/wsman with user name \"FAKE\\user\"", + e.getMessage() + ); + } + } + // --- response body builders ----------------------------------------------------- private static String service(final String name, final String state) { diff --git a/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java b/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java index 13083ff..18fc3d0 100644 --- a/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java +++ b/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java @@ -93,6 +93,45 @@ void kerberosOverHttpsAccepted() throws Exception { } } + @Test + void basicAcceptedOverHttpAndHttps() throws Exception { + // Basic is stateless and rides the Authorization header, so it is supported over both + // transports. Constructing the executor opens no connection, so this stays offline. + try ( + final WindowsRemoteExecutor executor = WinRMExecutorFactory.createInstance( + endpoint(WinRMHttpProtocolEnum.HTTP), + 30000L, + null, + List.of(AuthenticationEnum.BASIC) + )) { + assertInstanceOf(LightWinRMService.class, executor); + } + try ( + final WindowsRemoteExecutor executor = WinRMExecutorFactory.createInstance( + endpoint(WinRMHttpProtocolEnum.HTTPS), + 30000L, + null, + List.of(AuthenticationEnum.BASIC) + )) { + assertInstanceOf(LightWinRMService.class, executor); + } + } + + @Test + void basicFallsBackWithOtherSchemesOverHttp() throws Exception { + // Ordered fallback: [BASIC, NTLM] over HTTP is a valid candidate list (both are supported + // over HTTP), so it constructs successfully. + try ( + final WindowsRemoteExecutor executor = WinRMExecutorFactory.createInstance( + endpoint(WinRMHttpProtocolEnum.HTTP), + 30000L, + null, + List.of(AuthenticationEnum.BASIC, AuthenticationEnum.NTLM) + )) { + assertInstanceOf(LightWinRMService.class, executor); + } + } + @Test void closedLightExecutorRejectsOperations() throws Exception { // close() must release the executor for good: a later operation is rejected, not silently served diff --git a/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java b/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java index 8dddcaa..6e3262b 100644 --- a/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java +++ b/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java @@ -4,6 +4,7 @@ import static java.util.Optional.of; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.metricshub.winrm.Utils.EMPTY; +import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.BASIC; import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.KERBEROS; import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.NTLM; import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.getValueOf; @@ -23,5 +24,8 @@ void testGetValueOf() { assertEquals(of(KERBEROS), getValueOf(" kerberos ")); assertEquals(of(KERBEROS), getValueOf(" Kerberos ")); assertEquals(of(KERBEROS), getValueOf(" KERBEROS ")); + assertEquals(of(BASIC), getValueOf(" basic ")); + assertEquals(of(BASIC), getValueOf(" Basic ")); + assertEquals(of(BASIC), getValueOf(" BASIC ")); } } From 672727ef75870c4d287fbe1efab4279b355d368c Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Mon, 31 Aug 2026 21:34:44 +0200 Subject: [PATCH 2/9] Address review: forward Basic header in fallback, wipe derived credential, 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. --- CHANGELOG.md | 8 +- README.md | 8 +- .../winrm/light/BasicAuthScheme.java | 88 ++++++++++++++----- .../winrm/light/FallbackAuthScheme.java | 7 ++ .../winrm/light/PlaintextSoapAuthScheme.java | 2 +- .../client/auth/AuthenticationEnum.java | 7 +- src/site/markdown/authentication.md | 5 +- src/site/markdown/migrating-from-winrm4j.md | 9 +- src/site/markdown/preparing-the-host.md | 7 +- .../winrm/light/BasicAuthSchemeTest.java | 43 +++++++-- .../winrm/light/FallbackAuthSchemeTest.java | 38 ++++++++ .../client/auth/AuthenticationEnumTest.java | 17 ++++ 12 files changed, 193 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb02b5a..c6f8ef9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,11 +11,11 @@ Kerberos: * `WinRMClient.Builder.authentication(AuthScheme.BASIC)` and the CLI's `--basic` option select it. * Basic is stateless: the credential rides the `Authorization` header of **every** request, and - there is no message protection — the payload travels as plaintext SOAP. It therefore requires - **HTTPS** in practice, where TLS protects the credential and the payload (Basic over plain HTTP - is possible but must never be used, since everything is sent in the clear). + there is no message protection — the payload travels as plaintext SOAP. The scheme is accepted + over both transports, but it must be used over **HTTPS** in practice, where TLS protects the + credential and the payload (over plain HTTP both travel in the clear). * A domain-qualified user name (`DOMAIN\user`) keeps its domain prefix on the wire; the server - must have `AllowBasicAuth` enabled on the WinRM service. + must have the `Basic` setting enabled on the WinRM service (`winrm/config/service/auth`). * `BASIC` joins `AuthenticationEnum` (legacy API) and participates in the ordered-fallback list like the other schemes. diff --git a/README.md b/README.md index a3047fd..4e08d3e 100644 --- a/README.md +++ b/README.md @@ -54,9 +54,11 @@ WinRM must be enabled on the targeted Windows host, and the account must have su Non-administrator accounts need an explicit grant on the WinRM listener (`RootSDDL`), plus — only if they run WQL queries — WMI grants (`WinRMRemoteWMIUsers__` and namespace rights). An account that only runs commands never reaches WMI and needs nothing there. -* `AllowUnencrypted`, `Basic`, `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. +* `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 + ([Preparing the Windows Host](https://metricshub.org/winrm-java/preparing-the-host.html)). The full prerequisites — enabling WinRM over HTTP or HTTPS, Group Policy, firewall rules, the privileges each operation requires, configuring a non-administrator account, host quotas, and a diff --git a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java index 63a56b0..d5b6126 100644 --- a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java @@ -4,7 +4,7 @@ * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ * WinRM Java Client * ჻჻჻჻჻჻ - * Copyright 2023 - 2026 MetricsHub + * Copyright (C) 2023 - 2026 MetricsHub * ჻჻჻჻჻჻ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,7 +20,12 @@ * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ */ +import java.nio.ByteBuffer; +import java.nio.CharBuffer; +import java.nio.charset.CoderResult; +import java.nio.charset.CharsetEncoder; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.Base64; /** @@ -30,44 +35,78 @@ * transport: over HTTPS (TLS) the credential and the SOAP are protected; over plain HTTP they are * sent in the clear and must not be used. *

- * The credential is computed from the caller's {@code char[]} password and held as an immutable - * header value; the password array itself is never retained, so it can be wiped by the caller after - * closing the client. + * The caller's {@code char[]} password is kept only as a reference (never copied into a + * {@code String}), exactly like the NTLM scheme, so the caller remains the single owner of the + * secret and can wipe it after {@code close()}. The derived {@code Authorization} header is held + * as a wipeable {@code byte[]}, because Base64 is reversible: it is erased on + * {@link #reset()} — which {@code close()} always runs — and re-derived from the caller's still + * live array if the connection is (re)established before the caller wipes it. */ final class BasicAuthScheme extends PlaintextSoapAuthScheme { - private final String authorizationHeader; + // The full "Basic " header, held wipeable: it is a reversible copy of the credential, + // so it must not outlive the caller's own password array (erased in reset()). + private byte[] authorization; + private final String username; + private final char[] password; /** - * @param username the account name (without any {@code DOMAIN\} prefix) + * @param username the account name exactly as the caller gave it (a domain-qualified name + * keeps its domain prefix, which is how the server locates the account) * @param password the account password, kept as {@code char[]} so the caller owns the single * wipeable copy of the secret */ BasicAuthScheme(final String username, final char[] password) { - this.authorizationHeader = "Basic " + Base64.getEncoder().encodeToString( - buildCredential(username, password) - ); + this.username = username; + this.password = password; + this.authorization = buildAuthorizationHeader(username, password); } /** - * Encode {@code user:password} to UTF-8 straight from the caller's {@code char[]} password, - * never forming a {@code String} copy of the secret (the credentials contract: the caller owns - * the single wipeable array, and no live copy of the password may outlive {@code close()}). + * Build the full {@code Authorization} header value, encoding the password to UTF-8 + * straight from the caller's {@code char[]} (a {@link CharBuffer} view) — the secret is + * never copied into a {@code String}, per the credentials contract. + * + * @param username the account name (may be domain-qualified) + * @param password the account password + * @return the ASCII bytes of {@code Basic } */ - private static byte[] buildCredential(final String username, final char[] password) { - final StringBuilder user = new StringBuilder(username.length() + 1 + password.length); - user.append(username); - user.append(':'); - for (final char c : password) { - user.append(c); + private static byte[] buildAuthorizationHeader(final String username, final char[] password) { + final byte[] user = username.getBytes(StandardCharsets.UTF_8); + final byte[] secret = new byte[password.length * 3]; // UTF-8 never exceeds 3 bytes/char + final ByteBuffer secretBuffer = ByteBuffer.wrap(secret); + final CharsetEncoder encoder = StandardCharsets.UTF_8.newEncoder(); + final CoderResult result = encoder.encode(CharBuffer.wrap(password), secretBuffer, true); + // A reported error can only be a malformed-input one: an unpaired surrogate in the password. + if (result.isError()) { + throw new IllegalArgumentException("The password contains unpaired surrogate characters"); } - return user.toString().getBytes(StandardCharsets.UTF_8); + encoder.flush(secretBuffer); + final int secretLength = secretBuffer.position(); + final byte[] raw = new byte[user.length + 1 + secretLength]; + System.arraycopy(user, 0, raw, 0, user.length); + raw[user.length] = (byte) ':'; + System.arraycopy(secret, 0, raw, user.length + 1, secretLength); + Arrays.fill(secret, (byte) 0); + final byte[] base64 = Base64.getEncoder().encode(raw); + Arrays.fill(raw, (byte) 0); + final byte[] header = new byte["Basic ".length() + base64.length]; + System.arraycopy("Basic ".getBytes(StandardCharsets.US_ASCII), 0, header, 0, "Basic ".length()); + System.arraycopy(base64, 0, header, "Basic ".length(), base64.length); + Arrays.fill(base64, (byte) 0); + return header; } @Override public String authenticate(final HttpTransport transport) throws Exception { // No handshake: Basic has no server challenge. Mark the connection authenticated so the // client proceeds straight to the first real request (which carries the Authorization header). + // A reset() when the connection dropped may have erased the credential, so re-derive it + // from the caller's still-live password array (the same behavior as the NTLM scheme, which + // keeps the password by reference to re-handshake a dropped connection). + if (authorization == null) { + authorization = buildAuthorizationHeader(username, password); + } authenticated = true; return null; } @@ -75,11 +114,20 @@ public String authenticate(final HttpTransport transport) throws Exception { @Override public String requestAuthorization() { // Stateless: the same header repeats on every request, not just the first. - return authorizationHeader; + if (authorization == null) { + throw new IllegalStateException("The Basic credential was erased before the connection was re-authenticated"); + } + return new String(authorization, StandardCharsets.US_ASCII); } @Override 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. authenticated = false; + if (authorization != null) { + Arrays.fill(authorization, (byte) 0); + authorization = null; + } } } diff --git a/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java b/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java index ac3b7e0..cc2262e 100644 --- a/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java @@ -75,6 +75,13 @@ public String authenticate(final HttpTransport transport) throws Exception { ); } + @Override + public String requestAuthorization() { + // A stateless active candidate (Basic) repeats its header on EVERY request, so the wrapper + // must forward it; the interface default of null would drop the header entirely. + return active != null ? active.requestAuthorization() : null; + } + @Override public boolean isAuthenticated() { return active != null && active.isAuthenticated(); diff --git a/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java b/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java index 91f6000..22e8918 100644 --- a/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/PlaintextSoapAuthScheme.java @@ -4,7 +4,7 @@ * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ * WinRM Java Client * ჻჻჻჻჻჻ - * Copyright 2023 - 2026 MetricsHub + * Copyright (C) 2023 - 2026 MetricsHub * ჻჻჻჻჻჻ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java b/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java index 17849d9..faf71af 100644 --- a/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java +++ b/src/main/java/org/metricshub/winrm/service/client/auth/AuthenticationEnum.java @@ -20,6 +20,7 @@ * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ */ +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.function.Function; @@ -42,6 +43,10 @@ public enum AuthenticationEnum { * @return An optional with the enum value if found empty otherwise */ public static Optional getValueOf(final String name) { - return name != null ? Optional.ofNullable(VALUES_OF.get(name.trim().toUpperCase())) : Optional.empty(); + // Locale.ROOT: the default-locale toUpperCase() mangles some names (Turkish: "basic" → + // "BASİC"), which would silently fail to resolve in those environments. + return name != null + ? Optional.ofNullable(VALUES_OF.get(name.trim().toUpperCase(Locale.ROOT))) + : Optional.empty(); } } diff --git a/src/site/markdown/authentication.md b/src/site/markdown/authentication.md index 1c9b99e..b133526 100644 --- a/src/site/markdown/authentication.md +++ b/src/site/markdown/authentication.md @@ -111,8 +111,9 @@ try (WinRMClient client = WinRMClient.builder("server.example.com") } ``` -The server must have Basic authentication enabled -(`winrm set winrm/config/service @{AllowBasicAuth=true}`); see +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 [Preparing the Windows Host](preparing-the-host.html). ## Authentication failures diff --git a/src/site/markdown/migrating-from-winrm4j.md b/src/site/markdown/migrating-from-winrm4j.md index 4d413b2..66b2b4b 100644 --- a/src/site/markdown/migrating-from-winrm4j.md +++ b/src/site/markdown/migrating-from-winrm4j.md @@ -134,10 +134,11 @@ the switch: final 0.12.x releases. Here, HTTP always uses NTLM message encryption — there is no unencrypted mode and nothing to configure, and hosts that require encryption (`AllowUnencrypted=false`, the Windows default) work out of the box. -* **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 - `AllowBasicAuth` enabled on the WinRM service and be reachable over HTTPS +* **Basic needs HTTPS, but the client does not enforce it.** winrm4j offers `AuthSchemes.BASIC`, + which here maps to `authentication(AuthScheme.BASIC)`. This client has no Basic message + protection: it accepts the scheme over both transports, so **use `https()` with it** — without + TLS, the credential and payload travel in the clear and the client will not stop you. The host + must have the `Basic` setting enabled on the WinRM service and be reachable over HTTPS ([Preparing the Windows Host](preparing-the-host.html)). NTLM remains the recommended scheme. * **Kerberos requires HTTPS.** winrm4j runs Kerberos over plain HTTP; this client refuses at `build()`, because it does not implement Kerberos message encryption — without TLS the payload diff --git a/src/site/markdown/preparing-the-host.md b/src/site/markdown/preparing-the-host.md index fe98ae4..3fe17e9 100644 --- a/src/site/markdown/preparing-the-host.md +++ b/src/site/markdown/preparing-the-host.md @@ -23,7 +23,7 @@ when it is already on, how to turn it on, and how to get the privileges right. | A listener | HTTP on port **5985**, or HTTPS on port **5986**. See [TLS / HTTPS](tls.html). | | Firewall open on that port | Inbound, from the machine running the client. | | `Negotiate` authentication enabled on the service | **`True` by default.** This is what carries NTLM; `Kerberos` (also `True` by default) carries Kerberos. | -| `AllowBasicAuth` enabled on the service | **Only for HTTP Basic** (not for NTLM or Kerberos). See below. | +| The `Basic` setting enabled on the service (`winrm/config/service/auth`) | **Only for HTTP Basic** (not for NTLM or Kerberos). See below. | | An account with the right privileges | See [Privileges](#Privileges) below. | Just as important, a few settings that other WinRM guides tell you to change are **not** needed @@ -34,8 +34,9 @@ here: traffic is satisfied. (Exception: HTTP Basic has no message protection, so Basic requires HTTPS, where TLS provides the confidentiality — see [Authentication](authentication.html).) -* **`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 +* **The service's `Basic` and `CredSSP` settings stay `False` unless you use HTTP Basic.** NTLM + and Kerberos need neither. To use HTTP Basic, enable the `Basic` setting under the service's + `auth` section — `winrm set winrm/config/service/auth @{Basic=true}` — and connect over HTTPS ([Authentication](authentication.html)). * **`TrustedHosts` is irrelevant.** That is a setting on the *Windows* WinRM **client**, consulted by the `winrs` command-line tool. A Java client never reads it, so you do not need to add anything diff --git a/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java index d48576d..7570947 100644 --- a/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java +++ b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.charset.StandardCharsets; @@ -39,16 +40,28 @@ void headerIsStatelessAndIdenticalOnEveryRequest() throws Exception { } @Test - void resetReturnsToUnauthenticatedState() throws Exception { - final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); + void resetErasesTheDerivedCredential() throws Exception { + // close() always runs reset(), and the Base64 value is a reversible copy of the credential, + // so the reset must leave no live copy of the password in the scheme. + final char[] password = "password".toCharArray(); + final BasicAuthScheme scheme = new BasicAuthScheme("user", password); scheme.authenticate(new HttpTransport("localhost", 1, 1000)); assertTrue(scheme.isAuthenticated()); + assertEquals( + "Basic " + Base64.getEncoder().encodeToString( + "user:password".getBytes( + StandardCharsets.UTF_8 + ) + ), + scheme.requestAuthorization() + ); scheme.reset(); assertFalse(scheme.isAuthenticated()); - // The credential header is still available: Basic re-sends it on the next request, so reset - // only clears the authenticated flag, not the derived header. - assertTrue(scheme.requestAuthorization().startsWith("Basic ")); + // With the caller's password wiped, the erased credential cannot be re-derived — the next + // request must fail loudly rather than silently send a header of zeros. + java.util.Arrays.fill(password, '\0'); + assertThrows(Exception.class, scheme::requestAuthorization); } @Test @@ -62,14 +75,28 @@ void wrapAndUnwrapArePlaintextPassThrough() { } @Test - void passwordIsNeverRetainedAfterConstruction() { - // The scheme must encode the credential at construction and not hold the caller's char[]: - // wiping the array afterward must not change the (already-derived) header. + void thePasswordArrayIsNotCopiedIntoAnImmutableString() { + // The scheme keeps the caller's char[] only as a reference (like the NTLM scheme) and + // derives the header on demand, so the caller remains the single owner of the secret and + // can wipe it: a header derived before the wipe still works, and the scheme never builds a + // String copy of the password. final char[] password = "s3cret".toCharArray(); final BasicAuthScheme scheme = new BasicAuthScheme("user", password); final String header = scheme.requestAuthorization(); + assertEquals( + "Basic " + Base64.getEncoder().encodeToString( + "user:s3cret".getBytes( + StandardCharsets.UTF_8 + ) + ), + header + ); java.util.Arrays.fill(password, '\0'); + // The already-derived (and not yet reset) header is still servable... assertEquals(header, scheme.requestAuthorization()); + // ...and after reset() the wiped array cannot be re-derived — loud failure, no zero header. + scheme.reset(); + assertThrows(Exception.class, scheme::requestAuthorization); } } diff --git a/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java b/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java index f1641e9..7568ad3 100644 --- a/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java +++ b/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -16,12 +17,23 @@ private static final class FakeScheme implements AuthScheme { private final String name; private final int failFromCall; // fail on this 1-based authenticate() call onward; MAX_VALUE = never + private final String requestAuthorization; // non-null mimics a stateless scheme (e.g. Basic) private boolean authenticated; private int authenticateCalls; FakeScheme(final String name, final int failFromCall) { + this(name, failFromCall, null); + } + + FakeScheme(final String name, final int failFromCall, final String requestAuthorization) { this.name = name; this.failFromCall = failFromCall; + this.requestAuthorization = requestAuthorization; + } + + @Override + public String requestAuthorization() { + return requestAuthorization; } @Override @@ -118,4 +130,30 @@ void allSchemesFailingThrows() { ); assertThrows(IllegalStateException.class, () -> fallback.authenticate(dummyTransport())); } + + @Test + void requestAuthorizationForwardsToTheActiveStatelessScheme() throws Exception { + // A stateless candidate (e.g. Basic) repeats its Authorization header on EVERY request, so + // the wrapper must forward it — the interface default of null would drop the header and + // every request would 401. Here Basic is the first (active) candidate of a fallback list. + final FakeScheme basic = new FakeScheme("basic", NEVER, "Basic dXNlcjpwYXNz"); + final FakeScheme ntlm = new FakeScheme("ntlm", NEVER); + final FallbackAuthScheme fallback = new FallbackAuthScheme(List.of(basic, ntlm)); + + fallback.authenticate(dummyTransport()); // Basic wins the fallback and becomes active + assertEquals("Basic dXNlcjpwYXNz", fallback.requestAuthorization()); + } + + @Test + void requestAuthorizationIsNullWhileUnauthenticated() throws Exception { + // Before any handshake there is no active scheme, and the header must be null (the + // client then runs the handshake, which for Basic marks the connection authenticated). + final FallbackAuthScheme fallback = new FallbackAuthScheme( + List.of(new FakeScheme("basic", NEVER, "Basic dXNlcjpwYXNz")) + ); + assertNull(fallback.requestAuthorization()); + + fallback.authenticate(dummyTransport()); + assertEquals("Basic dXNlcjpwYXNz", fallback.requestAuthorization()); + } } diff --git a/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java b/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java index 6e3262b..6ff6abf 100644 --- a/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java +++ b/src/test/java/org/metricshub/winrm/service/client/auth/AuthenticationEnumTest.java @@ -9,6 +9,7 @@ import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.NTLM; import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.getValueOf; +import java.util.Locale; import org.junit.jupiter.api.Test; class AuthenticationEnumTest { @@ -28,4 +29,20 @@ void testGetValueOf() { assertEquals(of(BASIC), getValueOf(" Basic ")); assertEquals(of(BASIC), getValueOf(" BASIC ")); } + + @Test + void testGetValueOfIsInsensitiveToTheDefaultLocale() { + // With the default locale set to Turkish, the locale-sensitive toUpperCase() mangles "basic" + // into "BASİC" (dotted capital I), which does not match any enum name. The lookup must + // resolve regardless of the JVM default locale. + final Locale original = Locale.getDefault(); + Locale.setDefault(new Locale("tr")); + try { + assertEquals(of(BASIC), getValueOf("basic")); + assertEquals(of(KERBEROS), getValueOf("kerberos")); + assertEquals(of(NTLM), getValueOf("ntlm")); + } finally { + Locale.setDefault(original); + } + } } From e307dcdad51704ada58aa2f6105fbe6091aa9b99 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Tue, 1 Sep 2026 00:14:03 +0200 Subject: [PATCH 3/9] Address second review round: reset all fallback candidates, document 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. --- README.md | 5 ++-- .../winrm/light/FallbackAuthScheme.java | 10 ++++--- src/site/markdown/authentication.md | 5 +++- src/site/markdown/preparing-the-host.md | 7 ++--- .../winrm/light/FallbackAuthSchemeTest.java | 27 +++++++++++++++++++ 5 files changed, 44 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 4e08d3e..ed78563 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ See **[Project Documentation](https://metricshub.org/winrm-java)** and the [Javadoc](https://metricshub.org/winrm-java/apidocs) for more information on how to use this library in your code. The Windows Remote Management (WinRM) Java Client is a library that enables to: -* Connect to a remote Windows server using one of the two authentication types (NTLM, KERBEROS) +* Connect to a remote Windows server using one of three authentication types (NTLM, Kerberos, or Basic) * Execute WMI Query Language (WQL) queries which uses HTTP/HTTPS protocols. > ## ⚠️ Upgrading from 1.x @@ -203,7 +203,8 @@ The pre-existing static helpers (`WinRMWqlExecutor.executeWql(...)`, The client has **zero runtime dependencies** (no Apache CXF / JAX-WS / JAXB, no BouncyCastle, no SLF4J — problems are reported through exceptions only) and is immune by construction to JAXP `ServiceLoader` conflicts (it uses the JDK-default XML factories). It supports **NTLM over HTTP -(with message encryption) and HTTPS** and **Kerberos (SPNEGO) over HTTPS**. +(with message encryption) and HTTPS**, **Kerberos (SPNEGO) over HTTPS**, and **HTTP Basic over +HTTPS**. Files passed to `upload(...)` (or `localFileToCopyList` in the legacy API) are copied to the remote host **through the WinRM channel itself** (chunked base64 through the command shell, diff --git a/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java b/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java index cc2262e..d98c6ca 100644 --- a/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/FallbackAuthScheme.java @@ -89,10 +89,12 @@ public boolean isAuthenticated() { @Override public void reset() { - // A dropped connection: clear the active scheme's session but keep it selected so the next - // authenticate() re-handshakes with the same (already-accepted) scheme. - if (active != null) { - active.reset(); + // Clear EVERY candidate's session state, not just the active one: a dropped connection or + // close() must erase the derived, reversible secrets of every scheme — including a Basic + // credential held by a scheme that never became active — while startIndex keeps the fallback + // order intact, so the next authenticate() still retries the last-accepted scheme first. + for (final AuthScheme candidate : candidates) { + candidate.reset(); } } diff --git a/src/site/markdown/authentication.md b/src/site/markdown/authentication.md index b133526..a8d1bf0 100644 --- a/src/site/markdown/authentication.md +++ b/src/site/markdown/authentication.md @@ -114,7 +114,10 @@ try (WinRMClient client = WinRMClient.builder("server.example.com") 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 -[Preparing the Windows Host](preparing-the-host.html). +[Preparing the Windows Host](preparing-the-host.html). Over HTTPS that is all that is needed, since +TLS provides the confidentiality. Over plain HTTP — which, as noted, should not be used — the +service would additionally have to set `AllowUnencrypted=true` (otherwise it refuses the unprotected +SOAP), which is exactly what the HTTPS recommendation exists to avoid. ## Authentication failures diff --git a/src/site/markdown/preparing-the-host.md b/src/site/markdown/preparing-the-host.md index 3fe17e9..e0fdbe3 100644 --- a/src/site/markdown/preparing-the-host.md +++ b/src/site/markdown/preparing-the-host.md @@ -31,9 +31,10 @@ here: * **`AllowUnencrypted` stays `False` for NTLM and Kerberos.** Over plain HTTP the client protects the payload with **NTLM message encryption**, so the service's default refusal of unencrypted - traffic is satisfied. (Exception: HTTP Basic has no message protection, so Basic requires - HTTPS, where TLS provides the confidentiality — see - [Authentication](authentication.html).) + traffic is satisfied. (Exception: HTTP Basic has no message protection, so it belongs on HTTPS, + where TLS provides the confidentiality and no extra setting is needed. If — contrary to that — + you run Basic over plain HTTP, the service must also set `AllowUnencrypted=true`, since otherwise + it refuses the unprotected SOAP — see [Authentication](authentication.html).) * **The service's `Basic` and `CredSSP` settings stay `False` unless you use HTTP Basic.** NTLM and Kerberos need neither. To use HTTP Basic, enable the `Basic` setting under the service's `auth` section — `winrm set winrm/config/service/auth @{Basic=true}` — and connect over HTTPS diff --git a/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java b/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java index 7568ad3..c334346 100644 --- a/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java +++ b/src/test/java/org/metricshub/winrm/light/FallbackAuthSchemeTest.java @@ -20,6 +20,7 @@ private static final class FakeScheme implements AuthScheme { private final String requestAuthorization; // non-null mimics a stateless scheme (e.g. Basic) private boolean authenticated; private int authenticateCalls; + private int resetCalls; FakeScheme(final String name, final int failFromCall) { this(name, failFromCall, null); @@ -55,6 +56,7 @@ public boolean isAuthenticated() { @Override public void reset() { authenticated = false; + resetCalls++; } @Override @@ -62,6 +64,10 @@ public byte[] wrap(final byte[] soapUtf8) { return soapUtf8; } + int resets() { + return resetCalls; + } + @Override public String wrapContentType() { return "application/soap+xml;charset=UTF-8"; @@ -156,4 +162,25 @@ void requestAuthorizationIsNullWhileUnauthenticated() throws Exception { fallback.authenticate(dummyTransport()); assertEquals("Basic dXNlcjpwYXNz", fallback.requestAuthorization()); } + + @Test + void resetClearsEveryCandidateNotJustTheActiveOne() throws Exception { + // close() and a dropped connection must erase the derived, reversible secrets of EVERY + // scheme in the list — including a stateless one (Basic) that never became active. The + // fallback order must survive: the next authenticate() still retries the first scheme. + final FakeScheme ntlm = new FakeScheme("ntlm", NEVER); + final FakeScheme basic = new FakeScheme("basic", NEVER, "Basic dXNlcjpwYXNz"); + final FallbackAuthScheme fallback = new FallbackAuthScheme(List.of(ntlm, basic)); + + fallback.authenticate(dummyTransport()); // ntlm becomes active, basic stays inactive + fallback.reset(); + + assertEquals(1, ntlm.resets()); + assertEquals(1, basic.resets()); // the inactive candidate is erased too + + // The fallback order is intact: the same (first) scheme is retried on re-authentication. + assertEquals("Negotiate ntlm", fallback.authenticate(dummyTransport())); + assertEquals(2, ntlm.authenticateCalls); // initial handshake + retry after the reset + assertEquals(0, basic.authenticateCalls); + } } From 84c884b86680708cee89a227929aaebedb27a496 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Tue, 1 Sep 2026 04:01:48 +0200 Subject: [PATCH 4/9] Address third review round: erase Basic credential when close races an 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. --- .../winrm/light/BasicAuthScheme.java | 31 ++++-- .../winrm/light/KerberosAuthScheme.java | 10 +- .../winrm/light/LightWinRMService.java | 5 +- .../metricshub/winrm/light/WsmanClient.java | 44 +++++++-- .../winrm/service/WinRMEndpoint.java | 12 ++- .../winrm/light/BasicAuthCloseRaceTest.java | 99 +++++++++++++++++++ .../winrm/light/BasicAuthSchemeTest.java | 11 +++ .../winrm/light/WsmanProtocolTest.java | 51 ++++++++++ .../winrm/service/WinRMEndpointTest.java | 3 + 9 files changed, 243 insertions(+), 23 deletions(-) create mode 100644 src/test/java/org/metricshub/winrm/light/BasicAuthCloseRaceTest.java diff --git a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java index d5b6126..fda8158 100644 --- a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java @@ -39,20 +39,25 @@ * {@code String}), exactly like the NTLM scheme, so the caller remains the single owner of the * secret and can wipe it after {@code close()}. The derived {@code Authorization} header is held * as a wipeable {@code byte[]}, because Base64 is reversible: it is erased on - * {@link #reset()} — which {@code close()} always runs — and re-derived from the caller's still - * live array if the connection is (re)established before the caller wipes it. + * {@link #reset()}. {@code close()} erases it directly when it can acquire the connection permit; + * when an in-flight operation or streaming handle still holds the permit (a timed-out worker, for + * example) the last operation to release the connection erases it instead, so the credential + * cannot survive the close either way. */ final class BasicAuthScheme extends PlaintextSoapAuthScheme { // The full "Basic " header, held wipeable: it is a reversible copy of the credential, - // so it must not outlive the caller's own password array (erased in reset()). - private byte[] authorization; + // so it must not outlive the caller's own password array (erased in reset()). volatile: reset() + // can be called from another thread (close() or a worker releasing the connection) while a + // worker is reading the header. + private volatile byte[] authorization; private final String username; private final char[] password; /** - * @param username the account name exactly as the caller gave it (a domain-qualified name - * keeps its domain prefix, which is how the server locates the account) + * @param username the account name (whitespace already stripped by the endpoint; a + * domain-qualified name keeps its domain prefix, which is how the server locates the + * account) * @param password the account password, kept as {@code char[]} so the caller owns the single * wipeable copy of the secret */ @@ -122,11 +127,17 @@ public String requestAuthorization() { @Override 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. + // Erase the Base64 credential — a reversible copy of the password — so that wiping the + // caller's char[] afterward leaves no live copy of the password. Called by close() when the + // connection permit is acquirable, and by WsmanClient when the last in-flight operation + // releases the connection after close() could not acquire the permit (a timed-out worker). + // The two callers are different threads with no shared lock, so this must be re-entrant and + // idempotent: capture the reference in a local so a concurrent reset() cannot null the field + // between the null-check and the fill (filling a twice-erased array is harmless). authenticated = false; - if (authorization != null) { - Arrays.fill(authorization, (byte) 0); + final byte[] header = authorization; + if (header != null) { + Arrays.fill(header, (byte) 0); authorization = null; } } diff --git a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java index a62ff9d..2d65e3f 100644 --- a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java @@ -112,9 +112,15 @@ public String authenticate(final HttpTransport transport) throws Exception { @Override public void reset() { - if (context != null) { + // The wipe can come from two threads at once (close() and the last in-flight operation + // releasing the connection) with no shared lock, so claim the context in a local before + // 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; + if (ctx != null) { try { - context.dispose(); + ctx.dispose(); } catch (final GSSException ignored) { // disposing a dead context is best-effort } diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index 30fe87f..d2e6c8d 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -278,8 +278,9 @@ private static AuthScheme resolveAuthScheme( // else: Kerberos is unavailable over plain HTTP — leave it out of the candidate list. } 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. + // over both transports. The credential is the (whitespace-stripped) raw username: a + // domain-qualified name keeps its domain prefix, which is how the server locates the + // account — the same normalized account the NTLM/Kerberos candidates use. schemes.add(new BasicAuthScheme(winRMEndpoint.getRawUsername(), password)); } else { throw new WinRMException( diff --git a/src/main/java/org/metricshub/winrm/light/WsmanClient.java b/src/main/java/org/metricshub/winrm/light/WsmanClient.java index 17f6d93..af3a616 100644 --- a/src/main/java/org/metricshub/winrm/light/WsmanClient.java +++ b/src/main/java/org/metricshub/winrm/light/WsmanClient.java @@ -132,11 +132,41 @@ final class WsmanClient implements AutoCloseable { private void lockAbortably() throws InterruptedException { connectionPermit.acquire(); if (Thread.interrupted()) { - connectionPermit.release(); + releaseConnection(); throw new InterruptedException("Operation abandoned: cancelled while waiting for the connection."); } } + /** + * Release the connection permit, disposing the authentication state when the client has been + * closed. {@link #close()} only disposes the state when it can acquire the permit itself; when it + * 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 + * the last one in flight: nothing else is mid-read, and resetting the scheme is race-free. This + * is what erases the connection-bound secrets the instant the connection is gone — the Basic + * credential in particular, which is a reversible copy of the password — rather than leaving + * them in a closed client that stays referenced. + */ + private void releaseConnection() { + // Wipe while still holding the permit: the connection is a serial channel (one permit), so + // this operation is the last one in flight and no other operation is reading or writing the + // auth state — the wipe is race-free. This covers the common case where close() has already + // run (transport closed, permit unacquirable by close, so close's own reset was skipped). + if (closed) { + auth.reset(); + } + connectionPermit.release(); + // Best-effort final sweep. close() may set 'closed' and fail its tryAcquire AFTER the check + // above but BEFORE this release: then close's own reset was skipped (it never got the permit) + // and the check above missed it. Resetting here, without the permit, is safe unconditionally: + // reset() only writes fixed reset values to its volatile fields and is idempotent, so even a + // concurrent reset (from close(), or from a worker that already released earlier) is harmless. + if (closed) { + auth.reset(); + } + } + /** * Abort between protocol steps when this task has been cancelled. A classic socket read does * not observe the interrupt the timeout path delivers: a worker blocked in (say) the Create @@ -291,7 +321,7 @@ WqlEnumeration openWql( return enumeration; } finally { if (!opened) { - connectionPermit.release(); + releaseConnection(); } } } @@ -370,7 +400,7 @@ private Map advance() throws Exception { while (cursor >= page.size()) { if (endOfSequence) { finished = true; - connectionPermit.release(); + releaseConnection(); return null; } // Stop pulling once the caller has been told the operation timed out. @@ -409,7 +439,7 @@ public void close() { } } } finally { - connectionPermit.release(); + releaseConnection(); } } } @@ -544,7 +574,7 @@ RemoteCommand startCommand( return new RemoteCommand(commandId, operationTimeoutMs, failOnQuietTimeout); } finally { if (!opened) { - connectionPermit.release(); + releaseConnection(); } } } @@ -846,7 +876,7 @@ private void finish() throws Exception { } } } finally { - connectionPermit.release(); + releaseConnection(); } } @@ -864,7 +894,7 @@ private void finishBounded(final long budgetMs) { terminateCompleted(budgetMs); } } finally { - connectionPermit.release(); + releaseConnection(); } } diff --git a/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java b/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java index 213bd04..0b1c9ac 100644 --- a/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java +++ b/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java @@ -74,11 +74,14 @@ public WinRMEndpoint( this.hostname = hostname.replaceAll("\\s", Utils.EMPTY); this.password = password; - rawUsername = username; this.namespace = buildNamespace(namespace); final String user = username.replaceAll("\\s", Utils.EMPTY); + // 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; if (user.contains("\\")) { final String[] array = user.split("\\\\"); domain = array[0]; @@ -110,7 +113,12 @@ public String getDomain() { return domain; } - /** get the username as indicated in the constructor (could be in domain\\user form) */ + /** + * Get the username, whitespace-stripped, in the form it was given in the constructor (could be + * in domain\\user form). The whitespace is stripped exactly like the rest of the account parts, + * so a protocol that uses this value (Basic) sends the same normalized account as the protocols + * that use the {@link #getDomain()}/{@link #getUsername()} pair. + */ public String getRawUsername() { return rawUsername; } diff --git a/src/test/java/org/metricshub/winrm/light/BasicAuthCloseRaceTest.java b/src/test/java/org/metricshub/winrm/light/BasicAuthCloseRaceTest.java new file mode 100644 index 0000000..9e21dcc --- /dev/null +++ b/src/test/java/org/metricshub/winrm/light/BasicAuthCloseRaceTest.java @@ -0,0 +1,99 @@ +package org.metricshub.winrm.light; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * The close-vs-in-flight-operation race for the Basic scheme (review round 3): when {@code close()} + * cannot acquire the connection permit because an operation still holds it (a timed-out worker + * blocked on a socket read), the LAST operation to release the connection must still erase the + * Base64 credential, so a wiped password can never be re-derived from a closed client. + */ +class BasicAuthCloseRaceTest { + + private static final String USERNAME = "domain\\user"; + private static final char[] PASSWORD = "s3cret-Passw0rd".toCharArray(); + private static final long TIMEOUT = 30_000L; + + private FakeWsmanServer server; + + @BeforeEach + void startServer() throws Exception { + server = new FakeWsmanServer("domain", "user", new String(PASSWORD)); + } + + @AfterEach + void stopServer() { + server.close(); + } + + @Test + void closeWhileOperationInFlightStillErasesTheBasicCredential() throws Exception { + // The Basic credential is a reversible Base64 copy of the password. The client sends it on + // every request, so the fake server expects it. The single scripted response is DELAYED, + // so the worker thread is still blocked on the socket read — and still holding the + // connection permit — when close() runs. + final BasicAuthScheme scheme = new BasicAuthScheme(USERNAME, PASSWORD); + final WsmanClient client = new WsmanClient( + "127.0.0.1", + server.port(), + TIMEOUT, + null, + false, + scheme, + USERNAME, + 65001, + 0, + 0L + ); + server.withBasicAuth(headerFor(USERNAME)); + // One delayed response: the worker blocks on the Enumerate's socket read (holding the + // permit) and, once it is served, returns an OPEN enumeration that keeps the permit until + // close() runs — long after the worker is parked. + server.enqueueDelayed(200, plaintextEnvelope("enumerate"), 1500L); + + final Thread worker = new Thread( + () -> { + try { + client.openWql("ROOT/CIMV2", "SELECT Name FROM Win32_Service", TIMEOUT, 1000, 0, false); + } catch (final Exception ignored) { + // close() hard-cuts the read (or the server 401s/500s): the outcome is irrelevant — + // what matters is that the operation ran to the end and released the connection. + } + }, + "basic-close-race-worker" + ); + worker.start(); + + // Wait until the worker has authenticated and sent its first request, so it is now blocked + // on the delayed read and still holds the connection permit. Seeing the Authorization header + // on the server is the proof — no fixed sleep, so the test is not sensitive to scheduling. + for (int i = 0; i < 1000 && server.requestAuthorizations().isEmpty(); i++) { + Thread.sleep(5L); + } + client.close(); + worker.join(30_000L); + assertFalse(worker.isAlive(), "the worker should have completed once the delayed read ended"); + + // Whichever path erased it (close() acquiring the permit, or the worker's release), the + // credential is gone: no live header, no authenticated flag. + assertFalse(scheme.isAuthenticated()); + assertThrows(IllegalStateException.class, scheme::requestAuthorization); + } + + private static String headerFor(final String username) { + return "Basic " + + java.util.Base64.getEncoder().encodeToString( + (username + ":" + new String(PASSWORD)).getBytes(java.nio.charset.StandardCharsets.UTF_8) + ); + } + + private static String plaintextEnvelope(final String marker) { + return "" + marker + + ""; + } +} diff --git a/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java index 7570947..50086cc 100644 --- a/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java +++ b/src/test/java/org/metricshub/winrm/light/BasicAuthSchemeTest.java @@ -64,6 +64,17 @@ void resetErasesTheDerivedCredential() throws Exception { assertThrows(Exception.class, scheme::requestAuthorization); } + @Test + void resetIsIdempotentAndSafeToCallTwice() { + // close() and the last in-flight operation can both run the wipe on different threads, so + // reset() must be re-entrant: a second call (finding the already-erased field) must be a no-op. + final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); + scheme.reset(); + scheme.reset(); + assertFalse(scheme.isAuthenticated()); + assertThrows(IllegalStateException.class, scheme::requestAuthorization); + } + @Test void wrapAndUnwrapArePlaintextPassThrough() { final BasicAuthScheme scheme = new BasicAuthScheme("user", "password".toCharArray()); diff --git a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java index 706e453..313d169 100644 --- a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java +++ b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java @@ -547,8 +547,59 @@ void basicWithWrongCredentialSurfacesTheCxfAuthenticationErrorMessage() throws E } } + @Test + void basicSendsTheWhitespaceNormalizedAccountOnTheWire() throws Exception { + // The endpoint strips whitespace from the account parts (domain / user) for the + // domain-splitting protocols; Basic must send the SAME normalized account (the whole + // whitespace-stripped username, still domain-qualified) rather than the padded string. + server.withBasicAuth(expectedNormalizedHeader()); + server.enqueue( + 200, + envelope( + "uuid:1" + ) + ); + server.enqueue( + 200, + envelope( + "" + ) + ); + + // The caller passes the account heavily padded with whitespace, exactly the form that + // previously leaked through unnormalized: the endpoint must strip it to the plain account. + final WinRMEndpoint endpoint = new WinRMEndpoint( + WinRMHttpProtocolEnum.HTTP, + "127.0.0.1", + server.port(), + " \t\r\n " + DOMAIN + " \t\r\n \\ \t\r\n " + USER + " \t\r\n ", + PASSWORD.toCharArray(), + null + ); + // The endpoint's own normalization is what the wire credential must match. + assertEquals(DOMAIN + "\\" + USER, endpoint.getRawUsername()); + try ( + LightWinRMService service = LightWinRMService + .createInstance(endpoint, TIMEOUT, null, List.of(AuthenticationEnum.BASIC))) { + final List> rows = service.executeWql("SELECT Name FROM Win32_Service", TIMEOUT); + assertEquals(0, rows.size()); + } + + // The server would have 401'd on any non-normalized header, so a successful round trip + // proves the client sent the normalized account, exactly as the other protocols do. + assertEquals(expectedNormalizedHeader(), server.requestAuthorizations().get(0)); + } + // --- response body builders ----------------------------------------------------- + private static String expectedNormalizedHeader() { + return "Basic " + Base64.getEncoder().encodeToString( + (DOMAIN + "\\" + USER + ":" + PASSWORD).getBytes(StandardCharsets.UTF_8) + ); + } + private static String service(final String name, final String state) { return instance("Win32_Service", "Name", name, "State", state); } diff --git a/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java b/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java index 80d913e..bdd5390 100644 --- a/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java +++ b/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java @@ -44,6 +44,9 @@ void testWinRMEndpoint() { assertEquals("http://host:5985/wsman", winRMEndpoint.getEndpoint()); assertEquals("domain", winRMEndpoint.getDomain()); assertEquals(USER, winRMEndpoint.getUsername()); + // The raw account is whitespace-stripped too, so a protocol that uses it (Basic) + // sends the same normalized account as the domain/username pair above. + assertEquals("domain\\user", winRMEndpoint.getRawUsername()); assertArrayEquals(PASSWORD, winRMEndpoint.getPassword()); assertEquals("ROOT/CIMV2", winRMEndpoint.getNamespace()); assertEquals(HTTP, winRMEndpoint.getProtocol()); From cb63bf27dfbeb654f52b45fe478c0bb3d9d6f4af Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Tue, 1 Sep 2026 04:46:09 +0200 Subject: [PATCH 5/9] Address fourth review round: keep rawUsername verbatim, normalize the 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). --- README.md | 8 ++++---- .../java/org/metricshub/winrm/WinRMClient.java | 7 +++++-- .../metricshub/winrm/light/BasicAuthScheme.java | 7 ++++--- .../metricshub/winrm/light/LightWinRMService.java | 10 ++++++---- .../metricshub/winrm/service/WinRMEndpoint.java | 14 ++++++-------- .../metricshub/winrm/light/WsmanProtocolTest.java | 5 +++-- .../winrm/service/WinRMEndpointTest.java | 6 +++--- 7 files changed, 31 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index ed78563..bb9ac95 100644 --- a/README.md +++ b/README.md @@ -54,10 +54,10 @@ WinRM must be enabled on the targeted Windows host, and the account must have su Non-administrator accounts need an explicit grant on the WinRM listener (`RootSDDL`), plus — only if they run WQL queries — WMI grants (`WinRMRemoteWMIUsers__` and namespace rights). An account that only runs commands never reaches WMI and needs nothing there. -* `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 +* With NTLM, `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 HTTP Basic scheme is the exception: + enable the service's `Basic` setting, and — over plain HTTP only — also `AllowUnencrypted=true` ([Preparing the Windows Host](https://metricshub.org/winrm-java/preparing-the-host.html)). The full prerequisites — enabling WinRM over HTTP or HTTPS, Group Policy, firewall rules, the diff --git a/src/main/java/org/metricshub/winrm/WinRMClient.java b/src/main/java/org/metricshub/winrm/WinRMClient.java index 0fb2137..6f2738a 100644 --- a/src/main/java/org/metricshub/winrm/WinRMClient.java +++ b/src/main/java/org/metricshub/winrm/WinRMClient.java @@ -308,8 +308,11 @@ public Builder https() { } /** - * Connect over HTTP (port 5985 unless {@link #port(int)} is set) — the default. The SOAP - * messages are NTLM-encrypted on the wire. + * Connect over HTTP (port 5985 unless {@link #port(int)} is set) — the default. With NTLM + * (the default scheme) the SOAP messages are NTLM-encrypted on the wire, so plaintext HTTP + * is still protected. Other schemes change that guarantee: HTTP Basic sends both the + * credential and the SOAP in cleartext, so use {@link #https()} with Basic (Kerberos over + * plain HTTP is rejected, since it cannot be protected). * * @return this builder */ diff --git a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java index fda8158..625fd3b 100644 --- a/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/BasicAuthScheme.java @@ -55,9 +55,10 @@ final class BasicAuthScheme extends PlaintextSoapAuthScheme { private final char[] password; /** - * @param username the account name (whitespace already stripped by the endpoint; a - * domain-qualified name keeps its domain prefix, which is how the server locates the - * account) + * @param username the account name (a domain-qualified name keeps its domain prefix, which is + * how the server locates the account). The caller passes an already whitespace-stripped + * account (the service rebuilds it from the endpoint's normalized domain/username parts), + * so it is used verbatim here. * @param password the account password, kept as {@code char[]} so the caller owns the single * wipeable copy of the secret */ diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index d2e6c8d..4ad8e04 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -278,10 +278,12 @@ private static AuthScheme resolveAuthScheme( // else: Kerberos is unavailable over plain HTTP — leave it out of the candidate list. } 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 - // domain-qualified name keeps its domain prefix, which is how the server locates the - // account — the same normalized account the NTLM/Kerberos candidates use. - schemes.add(new BasicAuthScheme(winRMEndpoint.getRawUsername(), password)); + // over both transports. Rebuild the account from the whitespace-normalized + // domain/username parts (the same account NTLM/Kerberos use), domain-qualified when + // the endpoint was given one — the endpoint's raw username is kept verbatim for + // public-API stability and is intentionally NOT used here. + final String basicAccount = domain != null ? domain + "\\" + username : username; + schemes.add(new BasicAuthScheme(basicAccount, password)); } else { throw new WinRMException( "The light WinRM backend supports only NTLM, Kerberos, and Basic (requested: " + diff --git a/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java b/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java index 0b1c9ac..42a15cc 100644 --- a/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java +++ b/src/main/java/org/metricshub/winrm/service/WinRMEndpoint.java @@ -74,14 +74,13 @@ public WinRMEndpoint( this.hostname = hostname.replaceAll("\\s", Utils.EMPTY); this.password = password; + // Keep the caller's raw username verbatim (a domain-qualified account keeps its prefix) so + // the public API is stable — the domain/username split below already normalizes whitespace. + rawUsername = username; this.namespace = buildNamespace(namespace); final String user = username.replaceAll("\\s", Utils.EMPTY); - // 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; if (user.contains("\\")) { final String[] array = user.split("\\\\"); domain = array[0]; @@ -114,10 +113,9 @@ public String getDomain() { } /** - * Get the username, whitespace-stripped, in the form it was given in the constructor (could be - * in domain\\user form). The whitespace is stripped exactly like the rest of the account parts, - * so a protocol that uses this value (Basic) sends the same normalized account as the protocols - * that use the {@link #getDomain()}/{@link #getUsername()} pair. + * Get the username exactly as supplied in the constructor (could be in domain\\user form, and + * may contain the whitespace the caller typed). The {@link #getDomain()} / {@link #getUsername()} + * parts are the whitespace-normalized split of this value, which is what the protocols send. */ public String getRawUsername() { return rawUsername; diff --git a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java index 313d169..1ffc664 100644 --- a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java +++ b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java @@ -578,8 +578,9 @@ void basicSendsTheWhitespaceNormalizedAccountOnTheWire() throws Exception { PASSWORD.toCharArray(), null ); - // The endpoint's own normalization is what the wire credential must match. - assertEquals(DOMAIN + "\\" + USER, endpoint.getRawUsername()); + // The endpoint keeps the raw username verbatim (stable public API), but the wire credential + // must be the normalized account the service rebuilds from domain/username. + assertEquals(" \t\r\n " + DOMAIN + " \t\r\n \\ \t\r\n " + USER + " \t\r\n ", endpoint.getRawUsername()); try ( LightWinRMService service = LightWinRMService .createInstance(endpoint, TIMEOUT, null, List.of(AuthenticationEnum.BASIC))) { diff --git a/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java b/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java index bdd5390..c7e3748 100644 --- a/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java +++ b/src/test/java/org/metricshub/winrm/service/WinRMEndpointTest.java @@ -44,9 +44,9 @@ void testWinRMEndpoint() { assertEquals("http://host:5985/wsman", winRMEndpoint.getEndpoint()); assertEquals("domain", winRMEndpoint.getDomain()); assertEquals(USER, winRMEndpoint.getUsername()); - // The raw account is whitespace-stripped too, so a protocol that uses it (Basic) - // sends the same normalized account as the domain/username pair above. - assertEquals("domain\\user", winRMEndpoint.getRawUsername()); + // The raw username is preserved VERBATIM (whitespace included) — it is the stable public + // API used by equals()/hashCode(); the domain/username pair above is the normalized form. + assertEquals(" \t\r\n domain \t\r\n \\ \t\r\n user \t\r\n ", winRMEndpoint.getRawUsername()); assertArrayEquals(PASSWORD, winRMEndpoint.getPassword()); assertEquals("ROOT/CIMV2", winRMEndpoint.getNamespace()); assertEquals(HTTP, winRMEndpoint.getProtocol()); From ba037c62174caaf9ef07771c76f0ceb2ab68d5bb Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 4 Sep 2026 00:51:31 +0200 Subject: [PATCH 6/9] Fix auth-state disposal and Kerberos-over-HTTP contract (PR review round 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. --- .../org/metricshub/winrm/WinRMClient.java | 6 +- .../winrm/light/KerberosAuthScheme.java | 32 ++++++---- .../winrm/light/LightWinRMService.java | 25 ++++---- .../metricshub/winrm/light/WsmanClient.java | 60 ++++++++----------- .../service/WinRMExecutorFactoryTest.java | 17 +++--- 5 files changed, 71 insertions(+), 69 deletions(-) diff --git a/src/main/java/org/metricshub/winrm/WinRMClient.java b/src/main/java/org/metricshub/winrm/WinRMClient.java index 6f2738a..159ffbb 100644 --- a/src/main/java/org/metricshub/winrm/WinRMClient.java +++ b/src/main/java/org/metricshub/winrm/WinRMClient.java @@ -311,8 +311,10 @@ public Builder https() { * Connect over HTTP (port 5985 unless {@link #port(int)} is set) — the default. With NTLM * (the default scheme) the SOAP messages are NTLM-encrypted on the wire, so plaintext HTTP * is still protected. Other schemes change that guarantee: HTTP Basic sends both the - * credential and the SOAP in cleartext, so use {@link #https()} with Basic (Kerberos over - * plain HTTP is rejected, since it cannot be protected). + * credential and the SOAP in cleartext, so use {@link #https()} with Basic. Kerberos + * requires HTTPS; it is rejected fail-closed at {@link #build()} for ANY scheme list that + * contains it — including an ordered fallback such as {@code (KERBEROS, NTLM)} — rather + * than being silently dropped and downgraded to another scheme. * * @return this builder */ diff --git a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java index 2d65e3f..be49d81 100644 --- a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java @@ -26,6 +26,7 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -65,7 +66,12 @@ final class KerberosAuthScheme extends PlaintextSoapAuthScheme { private final char[] password; private final Path ticketCache; - private GSSContext context; + // The SPNEGO context, claimed atomically on disposal: reset() can run from two threads at once + // (close() disposing the state, and the last operation releasing the connection) with no shared + // lock, and two threads must never dispose the same GSSContext. The claim is the + // getAndSet(null) in reset() — a single atomic step — so exactly one thread wins the context and + // disposes it, while any concurrent reset() sees null and skips. + private final AtomicReference context = new AtomicReference<>(); /** * @param servicePrincipalHost the host whose {@code HTTP/} SPN to target — must be the FQDN @@ -97,13 +103,15 @@ public String authenticate(final HttpTransport transport) throws Exception { final Oid spnego = new Oid(SPNEGO_OID); // NT_HOSTBASED_SERVICE "HTTP@host" maps to the SPN HTTP/host. final GSSName serverName = manager.createName("HTTP@" + servicePrincipalHost, GSSName.NT_HOSTBASED_SERVICE); - context = manager.createContext(serverName, spnego, null, GSSContext.DEFAULT_LIFETIME); - context.requestMutualAuth(true); - context.requestCredDeleg(false); + final GSSContext newContext = manager.createContext(serverName, spnego, null, GSSContext.DEFAULT_LIFETIME); + newContext.requestMutualAuth(true); + newContext.requestCredDeleg(false); // The AP-REQ is complete after the first call; the KDC issued the service ticket using the // Subject's TGT. The server validates it on the first real request (and, over HTTPS, TLS // already authenticates the server, so we do not need to process a mutual-auth reply token). - return context.initSecContext(new byte[0], 0, 0); + final byte[] token = newContext.initSecContext(new byte[0], 0, 0); + context.set(newContext); + return token; } ); authenticated = true; @@ -112,19 +120,19 @@ public String authenticate(final HttpTransport transport) throws Exception { @Override public void reset() { - // The wipe can come from two threads at once (close() and the last in-flight operation - // releasing the connection) with no shared lock, so claim the context in a local before - // 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; + // The wipe can come from two threads at once (close() disposing the state, and the last + // in-flight operation releasing the connection) with no shared lock, so CLAIM the context + // atomically before disposing: getAndSet(null) is a single atomic step, so exactly one + // concurrent reset() wins a non-null context and disposes it, while the others see null and + // skip. A plain "read then null" (even into a local) would let two threads read the same + // non-null value before either clears it, and both would dispose the same GSSContext. + final GSSContext ctx = context.getAndSet(null); if (ctx != null) { try { ctx.dispose(); } catch (final GSSException ignored) { // disposing a dead context is best-effort } - context = null; } authenticated = false; } diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index 4ad8e04..ab4370d 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -246,9 +246,11 @@ public static LightWinRMService createInstance( * Resolve the requested authentication schemes into a single {@link AuthScheme}, honoring the * caller's order. {@code null}/empty means NTLM only. A single scheme is used directly; several * become an ordered {@link FallbackAuthScheme} (e.g. Kerberos then NTLM). Kerberos requires HTTPS - * (no message encryption over plain HTTP, matching the CXF backend), so it is dropped from the - * candidate list over HTTP — a fallback list then uses its remaining schemes, and a Kerberos-only - * request over HTTP fails toward the escape hatch. + * (no message encryption over plain HTTP, matching the CXF backend) and is rejected FAIL-CLOSED + * for EVERY list that contains it over HTTP — not just a Kerberos-only request: silently dropping + * it from a fallback list (e.g. {@code [KERBEROS, NTLM]}) would downgrade the client to another + * scheme without the caller's consent, contradicting the builder's "Kerberos requested over HTTP + * is rejected" contract. */ private static AuthScheme resolveAuthScheme( final WinRMEndpoint winRMEndpoint, @@ -274,8 +276,15 @@ private static AuthScheme resolveAuthScheme( if (https) { // The SPN is HTTP/, so the caller must connect by the FQDN the KDC knows. schemes.add(new KerberosAuthScheme(winRMEndpoint.getHostname(), username, password, ticketCache)); + } else { + // 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( + "Kerberos over WinRM requires HTTPS (endpoint was " + + winRMEndpoint.getEndpoint() + + "): there is no Kerberos message encryption over plain HTTP. Use https()." + ); } - // else: Kerberos is unavailable over plain HTTP — leave it out of the candidate list. } else if (auth == AuthenticationEnum.BASIC) { // Basic is stateless and rides the Authorization header of every request, so it works // over both transports. Rebuild the account from the whitespace-normalized @@ -293,14 +302,6 @@ private static AuthScheme resolveAuthScheme( } } - if (schemes.isEmpty()) { - // e.g. Kerberos requested over plain HTTP with no other scheme to fall back to. - throw new WinRMException( - "Kerberos over WinRM requires HTTPS (endpoint was " + - winRMEndpoint.getEndpoint() + - "): there is no Kerberos message encryption over plain HTTP. Use HTTPS, or add NTLM to the authentication list." - ); - } return schemes.size() == 1 ? schemes.get(0) : new FallbackAuthScheme(schemes); } diff --git a/src/main/java/org/metricshub/winrm/light/WsmanClient.java b/src/main/java/org/metricshub/winrm/light/WsmanClient.java index af3a616..4e4bf36 100644 --- a/src/main/java/org/metricshub/winrm/light/WsmanClient.java +++ b/src/main/java/org/metricshub/winrm/light/WsmanClient.java @@ -138,30 +138,18 @@ private void lockAbortably() throws InterruptedException { } /** - * Release the connection permit, disposing the authentication state when the client has been - * closed. {@link #close()} only disposes the state when it can acquire the permit itself; when it - * 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 - * the last one in flight: nothing else is mid-read, and resetting the scheme is race-free. This - * is what erases the connection-bound secrets the instant the connection is gone — the Basic - * credential in particular, which is a reversible copy of the password — rather than leaving - * them in a closed client that stays referenced. + * Release the connection permit. Authentication state is disposed by {@link #close()} + * (unconditionally, once the transport is closed); this method only releases the permit, plus a + * backstop for the one state close() cannot see: a worker that re-authenticated — installing, + * e.g., a fresh Kerberos GSSContext — AFTER close()'s own reset, which is possible because the + * Kerberos GSS exchange does no socket I/O and so is not interrupted by the transport close. + * Resetting after the release is safe: every scheme's reset() is idempotent and the Kerberos + * scheme claims its context atomically, so racing close()'s reset cannot double-dispose it. */ private void releaseConnection() { - // Wipe while still holding the permit: the connection is a serial channel (one permit), so - // this operation is the last one in flight and no other operation is reading or writing the - // auth state — the wipe is race-free. This covers the common case where close() has already - // run (transport closed, permit unacquirable by close, so close's own reset was skipped). - if (closed) { - auth.reset(); - } connectionPermit.release(); - // Best-effort final sweep. close() may set 'closed' and fail its tryAcquire AFTER the check - // above but BEFORE this release: then close's own reset was skipped (it never got the permit) - // and the check above missed it. Resetting here, without the permit, is safe unconditionally: - // reset() only writes fixed reset values to its volatile fields and is idempotent, so even a - // concurrent reset (from close(), or from a worker that already released earlier) is harmless. + // Backstop for the post-close re-authentication described above: if the client was closed + // while this operation installed fresh auth state, erase it now. if (closed) { auth.reset(); } @@ -1376,31 +1364,33 @@ public void close() { // Only attempt a graceful shell Delete if no operation is currently using the connection: a // non-blocking tryAcquire (never an acquire()) keeps close() from waiting on an abandoned, // timed-out worker — or an open streaming handle — still holding the permit while blocked on - // a socket read. When we cannot acquire the permit, or a request would otherwise race the - // worker, we skip the Delete and just hard-close the transport below — which unblocks that - // worker's read; the shell is reaped by the server IdleTimeout. + // a socket read. When we cannot acquire the permit we skip the Delete and just hard-close the + // transport — which unblocks that worker's read; the shell is reaped by the server IdleTimeout. final boolean locked = connectionPermit.tryAcquire(); try { final String shell = shellId; shellId = null; - if (locked) { - if (shell != null) { - try { - send(Envelopes.deleteShell(url, shell, timeoutMs)); - } catch (final Exception ignored) { - // best-effort shell cleanup - } + if (locked && shell != null) { + try { + send(Envelopes.deleteShell(url, shell, timeoutMs)); + } catch (final Exception ignored) { + // best-effort shell cleanup } - // Release the connection-bound auth state — notably the Kerberos GSSContext, whose only - // disposal path is reset(). Skipped when not locked: another (timed-out) worker still owns - // the auth scheme, and the transport hard-close below unblocks it. - auth.reset(); } } finally { if (locked) { connectionPermit.release(); } transport.close(); + // Dispose the auth state unconditionally, now that the transport is gone. The state (the + // NTLM session keys, the Basic credential, the Kerberos GSSContext) is bound to the + // connection, not the socket, so it needs no permit — and disposing it here, rather than + // only when we hold the permit, is what erases the credential even when an idle streaming + // handle still holds the permit and so skipped the shell Delete above. This is the single + // disposal point; it is safe to race a worker's own release sweep (releaseConnection) + // because every scheme's reset() is idempotent and the Kerberos scheme claims its context + // atomically, so the two cannot double-dispose it. + auth.reset(); } } } diff --git a/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java b/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java index 18fc3d0..8bd9311 100644 --- a/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java +++ b/src/test/java/org/metricshub/winrm/service/WinRMExecutorFactoryTest.java @@ -64,18 +64,19 @@ void kerberosOnlyOverHttpRejected() { } @Test - void mixedKerberosNtlmFallsBackToNtlmOverHttp() throws Exception { - // Ordered fallback: [KERBEROS, NTLM] over HTTP cannot use Kerberos (HTTPS-only), so it falls back - // to NTLM and constructs successfully. Building the client opens no connection, so this is offline. - try ( - final WindowsRemoteExecutor executor = WinRMExecutorFactory.createInstance( + void mixedKerberosNtlmRejectedOverHttp() { + // Kerberos requires HTTPS and is rejected FAIL-CLOSED for any list that contains it over HTTP — + // rather than being silently dropped from an ordered fallback and downgrading to NTLM, which + // would contradict the "Kerberos over HTTP is rejected" contract. Building opens no connection. + assertThrows( + WinRMException.class, + () -> WinRMExecutorFactory.createInstance( endpoint(WinRMHttpProtocolEnum.HTTP), 30000L, null, List.of(AuthenticationEnum.KERBEROS, AuthenticationEnum.NTLM) - )) { - assertInstanceOf(LightWinRMService.class, executor); - } + ) + ); } @Test From fea97440e6313e96cbc2613d4f08ab0c747d6cfe Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 4 Sep 2026 21:52:00 +0200 Subject: [PATCH 7/9] Address fifth-round review: Kerberos context disposal, test race, doc examples KerberosAuthScheme.authenticate(): dispose the GSSContext when setup fails (requestMutualAuth/requestCredDeleg/initSecContext). A failed initSecContext never published the context, so no reset()/close() could dispose it and each failed attempt (unavailable SPN/KDC) leaked GSS/native resources. The context is now published only on success and disposed in the catch on any failure. FakeWsmanServer: make basicMode/expectedBasicHeader volatile and write the expected header before enabling the mode, so the connection-serving thread cannot observe Basic mode on with a stale null header (intermittent NTLM-path or 401 in the Basic protocol tests). Docs: add .https() to the authentication.md scheme-selection example so every uncommented alternative (Basic, Kerberos, the KERBEROS+NTLM fallback) uses an HTTPS transport; qualify the README fallback note that Kerberos requires https(); and document that the Basic credential sends the whitespace-normalized account, not the exact string typed. --- README.md | 3 +- .../winrm/light/KerberosAuthScheme.java | 30 ++++++++++++++----- src/site/markdown/authentication.md | 9 ++++-- .../winrm/light/FakeWsmanServer.java | 13 ++++++-- 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index bb9ac95..a40ab5d 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,8 @@ try (WinRMClient client = WinRMClient.builder("server01.acme.com") ``` Connection-scoped options on the builder: `https()`, `port(int)`, -`authentication(AuthScheme.KERBEROS, AuthScheme.NTLM)` (ordered fallback; NTLM is the default), +`authentication(AuthScheme.KERBEROS, AuthScheme.NTLM)` (ordered fallback; NTLM is the default — +Kerberos in the list requires `https()`), `ticketCache(Path)`, `namespace(String)`, `trustAllCertificates()` (per-client alternative to the `org.metricshub.winrm.tls.insecure` system property; insecure, testing only), and `sslContext(SSLContext)` for a dedicated trust store. diff --git a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java index be49d81..84bc8bf 100644 --- a/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java +++ b/src/main/java/org/metricshub/winrm/light/KerberosAuthScheme.java @@ -104,14 +104,28 @@ public String authenticate(final HttpTransport transport) throws Exception { // NT_HOSTBASED_SERVICE "HTTP@host" maps to the SPN HTTP/host. final GSSName serverName = manager.createName("HTTP@" + servicePrincipalHost, GSSName.NT_HOSTBASED_SERVICE); final GSSContext newContext = manager.createContext(serverName, spnego, null, GSSContext.DEFAULT_LIFETIME); - newContext.requestMutualAuth(true); - newContext.requestCredDeleg(false); - // The AP-REQ is complete after the first call; the KDC issued the service ticket using the - // Subject's TGT. The server validates it on the first real request (and, over HTTPS, TLS - // already authenticates the server, so we do not need to process a mutual-auth reply token). - final byte[] token = newContext.initSecContext(new byte[0], 0, 0); - context.set(newContext); - return token; + try { + newContext.requestMutualAuth(true); + newContext.requestCredDeleg(false); + // The AP-REQ is complete after the first call; the KDC issued the service ticket using the + // Subject's TGT. The server validates it on the first real request (and, over HTTPS, TLS + // already authenticates the server, so we do not need to process a mutual-auth reply token). + final byte[] token = newContext.initSecContext(new byte[0], 0, 0); + // Publish only on success: a failed setup (below) must not leave a half-initialized + // context in `context`, and the success path is disposed by the normal reset()/close(). + context.set(newContext); + return token; + } catch (final Exception e) { + // Dispose the context on any failure before it is published to `context`: otherwise no + // reset()/close() could ever reach it, and each failed attempt (e.g. an unavailable + // service principal or KDC) would leak implementation/native GSS resources. + try { + newContext.dispose(); + } catch (final GSSException ignored) { + // disposing an already-failed context is best-effort + } + throw e; + } } ); authenticated = true; diff --git a/src/site/markdown/authentication.md b/src/site/markdown/authentication.md index a8d1bf0..8796430 100644 --- a/src/site/markdown/authentication.md +++ b/src/site/markdown/authentication.md @@ -14,6 +14,7 @@ values: import org.metricshub.winrm.AuthScheme; WinRMClient.builder("server.example.com") + .https() .credentials("DOMAIN\\Administrator", password) .authentication(AuthScheme.NTLM) // NTLM only (also the default) // .authentication(AuthScheme.KERBEROS) // Kerberos only @@ -97,9 +98,11 @@ handshake and no message protection, so the payload travels as plaintext SOAP. I transports, but over plain HTTP the credential and the data are sent **in the clear**: use Basic over HTTPS only, where TLS protects both. -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. +The credential is the user name, **with all whitespace removed** before it is sent: a +`DOMAIN\user` account is rebuilt as `DOMAIN` + `\` + the account (the backslash is where the domain +and account are split on), and a bare name is sent as-is. Windows account names contain no +whitespace, so the removal is a no-op in practice — but if you supply one, the header carries the +whitespace-stripped account, not the exact string you typed. ```java try (WinRMClient client = WinRMClient.builder("server.example.com") diff --git a/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java b/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java index 769a3ab..a44085b 100644 --- a/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java +++ b/src/test/java/org/metricshub/winrm/light/FakeWsmanServer.java @@ -116,8 +116,13 @@ static final class Scripted { // HTTP Basic mode: instead of the NTLM handshake, each request must carry the expected // Authorization header, and scripted bodies are served as PLAINTEXT SOAP (Basic has no message // protection — the payload is never encrypted by the client). - private boolean basicMode; - private String expectedBasicHeader; + // Both are volatile: withBasicAuth() writes them on the test thread, while handleConnection() + // reads them on a separately spawned server thread, and the server thread must observe the + // expected header before (or together with) the mode flag. withBasicAuth() writes the header + // FIRST and the flag SECOND, so a reader that sees basicMode == true is guaranteed to see the + // header already set (volatile write ordering / happens-before). + private volatile boolean basicMode; + private volatile String expectedBasicHeader; private final List requestAuthorizations = new CopyOnWriteArrayList<>(); /** @@ -225,8 +230,10 @@ public FakeWsmanServer withChunkedResponses() { * @return this server, for chaining */ public FakeWsmanServer withBasicAuth(final String expectedAuthorization) { - basicMode = true; + // Write the header before enabling the mode (see the field comment): the server thread must + // never observe basicMode == true with a null expected header. expectedBasicHeader = expectedAuthorization; + basicMode = true; return this; } From 481521879d36dfd9487ab8be6a7dbe25672eb212 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 4 Sep 2026 21:57:46 +0200 Subject: [PATCH 8/9] Qualify the ordered-fallback doc: a list with Kerberos requires HTTPS The 'Ordered fallback' section is the canonical KERBEROS,NTLM reference, so make its HTTPS requirement explicit there too, consistent with the scheme- selection example and the README note. --- src/site/markdown/authentication.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/site/markdown/authentication.md b/src/site/markdown/authentication.md index 8796430..a20c745 100644 --- a/src/site/markdown/authentication.md +++ b/src/site/markdown/authentication.md @@ -29,7 +29,9 @@ When `authentication(...)` is not called, **NTLM** is used. Several schemes form an **ordered fallback list**: each is tried in the given order until one succeeds. `authentication(KERBEROS, NTLM)` attempts Kerberos first and falls back to NTLM — for -example when the KDC is unreachable or the clock skew is too large. +example when the KDC is unreachable or the clock skew is too large. Because the list contains +Kerberos, it requires an HTTPS transport: Kerberos is rejected over plain HTTP rather than being +silently dropped (see [Kerberos](#kerberos-spnego) below). ## User name and domain From 5cd5b032c383e4b40313defb6e84456be7313707 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Sat, 5 Sep 2026 14:27:55 +0200 Subject: [PATCH 9/9] Document the Kerberos-over-HTTP fail-closed change in the CHANGELOG An ordered fallback list such as [KERBEROS, NTLM] over the default (HTTP) builder previously dropped the Kerberos entry and quietly fell back to the remaining schemes; it now fails at build(). This is a user-visible behavior change for existing code, so it belongs in the changelog. Kept version-agnostic (previously/now) since the drop behavior is still present on the PR base. --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6f8ef9..5144ca0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,6 +130,14 @@ Consequences: ### Changed +- **Kerberos over plain HTTP is now rejected for any authentication list that contains it.** + An ordered fallback list such as `authentication(AuthScheme.KERBEROS, AuthScheme.NTLM)` on the + default (HTTP) builder previously dropped the Kerberos entry and quietly fell back to the + remaining schemes (NTLM); it now fails at `build()` with a `WinRMClientException` + ("Kerberos over WinRM requires HTTPS …"). This makes the rejection fail-closed for every list + rather than only a Kerberos-only one, matching the builder's documented "Kerberos requested over + HTTP is rejected" contract and the CLI's `--kerberos` requiring `--https`. Existing code that + uses such a list over plain HTTP must either call `https()` or remove Kerberos from the list. - HTTPS connections validate certificates and verify hostnames by default (see the breaking change above). - The exception surface matches the pre-2.0.0 CXF backend (feature parity): authentication