diff --git a/docs/requirements/Component-CLI.md b/docs/requirements/Component-CLI.md index c77ca163..10991605 100644 --- a/docs/requirements/Component-CLI.md +++ b/docs/requirements/Component-CLI.md @@ -32,7 +32,7 @@ The CLI authenticates with a bearer token exchanged once per process (review 202 3. Every later call in the process — `POST /management`, the audit REST endpoints, and the `debug stream` hub (including its automatic reconnects) — sends `Authorization: Bearer `; the server validates it locally with no LDAP bind. A token near expiry is re-exchanged. 4. A bearer call answered `401` re-exchanges once (unless the token was minted for that very call) and, if still refused, retries with HTTP Basic and stays on Basic for the rest of the process. 5. **Basic fallback.** A server with no `/auth/token` (404/405), a login-disabled deployment (404), or an older server whose token response lacks `management_bearer: true` (its tokens are accepted nowhere) is remembered as Basic-only for the process: each call carries a Basic header and the server binds LDAP per request. A transient exchange error (5xx) uses Basic for that call only. Bad credentials (`401`) or a throttle lockout (`429`) from the exchange are reported as-is and are **not** retried with Basic (that would only add a second failed bind against the login throttle). -6. The token lives **in memory only** — never written to disk, logged or echoed — and dies with the process; each CLI invocation needs credentials. `SCADABRIDGE_AUTH_MODE=basic` disables the exchange (Basic on every call). +6. The token lives **in memory only** — never written to disk, logged or echoed — and dies with the process; each CLI invocation needs credentials. `SCADABRIDGE_AUTH_MODE=basic` disables the exchange (Basic on every call); `SCADABRIDGE_VERBOSE=1` prints a stderr note when a freshly issued token is refused and the CLI falls back to Basic. The client never follows HTTP redirects, so a 307/308 cannot replay the credential form elsewhere. 7. The CLI never talks to LDAP directly. ## Connection diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DebugCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DebugCommands.cs index dec16f64..0f25b6d5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DebugCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DebugCommands.cs @@ -120,8 +120,13 @@ public static class DebugCommands { if (bearer.Token is not null) { + // Re-invoked on every (re)connect. A null token means the exchange fell back + // (transient error, or the server dropped bearer support mid-stream): retry + // once, and if it is still null connect without a token — the hub refuses the + // handshake and WithAutomaticReconnect tries again on its backoff schedule. options.AccessTokenProvider = async () => - (await authClient.TryGetBearerTokenAsync(CancellationToken.None)).Token; + (await authClient.TryGetBearerTokenAsync(CancellationToken.None)).Token + ?? (await authClient.TryGetBearerTokenAsync(CancellationToken.None)).Token; } else { diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs index 0d70fa01..f215dc1c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs @@ -39,6 +39,13 @@ public class ManagementHttpClient : IDisposable /// Environment variable selecting the auth mode: token (default) or basic. public const string AuthModeEnvVar = "SCADABRIDGE_AUTH_MODE"; + /// + /// Environment variable that, when 1/true, makes the client note on stderr + /// when it abandons a refused bearer token for HTTP Basic (e.g. central nodes configured + /// with different signing keys). The CLI has no other verbose switch. + /// + public const string VerboseEnvVar = "SCADABRIDGE_VERBOSE"; + /// Re-exchange a token this long before its stated expiry. internal static readonly TimeSpan TokenRefreshMargin = TimeSpan.FromSeconds(60); @@ -117,15 +124,25 @@ public class ManagementHttpClient : IDisposable /// The password for HTTP Basic authentication. public ManagementHttpClient(string baseUrl, string username, string password) : this( - new HttpClient(new SocketsHttpHandler { ConnectTimeout = ResolveConnectTimeout() }) - { - Timeout = Timeout.InfiniteTimeSpan - }, + CreateDefaultHttpClient(), baseUrl, username, password, IsBasicAuthModeForced() ? null : CliTokenStore.Shared) { } + /// + /// Builds the production : infinite overall timeout (each call + /// carries its own deadline), connect bounded by , and + /// redirects NOT followed — a 307/308 from a proxy would otherwise re-POST the + /// /auth/token form, i.e. the credentials, to wherever the redirect points. + /// + /// The configured client. + internal static HttpClient CreateDefaultHttpClient() => + new(new SocketsHttpHandler { ConnectTimeout = ResolveConnectTimeout(), AllowAutoRedirect = false }) + { + Timeout = Timeout.InfiniteTimeSpan + }; + /// /// True when is basic, which disables the token /// exchange for the process. @@ -179,7 +196,7 @@ public class ManagementHttpClient : IDisposable /// The token, a fall-back-to-Basic signal, or a failure to report. internal async Task TryGetBearerTokenAsync(CancellationToken cancellationToken) { - var auth = await AcquireAuthAsync(forceExchange: false, cancellationToken).ConfigureAwait(false); + var auth = await AcquireAuthAsync(forceExchange: false, refusedToken: null, cancellationToken).ConfigureAwait(false); if (auth.Failure is not null) { var failure = await ToManagementResponseAsync(auth.Failure, cancellationToken).ConfigureAwait(false); @@ -197,7 +214,13 @@ public class ManagementHttpClient : IDisposable /// exchanged one, or Basic. A non-null is the exchange's own /// 401/429 response, to be surfaced to the caller as-is. /// - private async Task AcquireAuthAsync(bool forceExchange, CancellationToken ct) + /// Exchange even though a cached token looks usable (the server refused it). + /// + /// The token the server just refused. If another caller has already replaced it by the time + /// the gate is acquired, that replacement is returned instead of exchanging again. + /// + /// Cancellation token. + private async Task AcquireAuthAsync(bool forceExchange, string? refusedToken, CancellationToken ct) { var session = _session; if (session is null || session.BasicOnly) @@ -219,6 +242,13 @@ public class ManagementHttpClient : IDisposable return new AuthChoice(_basic, false, null); if (!forceExchange && session.HasUsableToken(DateTimeOffset.UtcNow)) return new AuthChoice(new AuthenticationHeaderValue("Bearer", session.Token), false, null); + if (forceExchange && session.HasUsableToken(DateTimeOffset.UtcNow) + && !string.Equals(session.Token, refusedToken, StringComparison.Ordinal)) + { + // A concurrent caller already re-exchanged: use its token (as freshly minted, so a + // second refusal falls back to Basic rather than exchanging yet again). + return new AuthChoice(new AuthenticationHeaderValue("Bearer", session.Token), true, null); + } session.Token = null; using var form = new FormUrlEncodedContent( @@ -296,7 +326,7 @@ public class ManagementHttpClient : IDisposable private async Task SendAuthenticatedAsync( Func buildRequest, HttpCompletionOption completion, CancellationToken ct) { - var auth = await AcquireAuthAsync(forceExchange: false, ct).ConfigureAwait(false); + var auth = await AcquireAuthAsync(forceExchange: false, refusedToken: null, ct).ConfigureAwait(false); if (auth.Failure is not null) return auth.Failure; @@ -309,7 +339,7 @@ public class ManagementHttpClient : IDisposable if (!auth.FreshlyMinted) { response.Dispose(); - auth = await AcquireAuthAsync(forceExchange: true, ct).ConfigureAwait(false); + auth = await AcquireAuthAsync(forceExchange: true, refusedToken: auth.Header.Parameter, ct).ConfigureAwait(false); if (auth.Failure is not null) return auth.Failure; response = await SendWithAsync(buildRequest, auth.Header, completion, ct).ConfigureAwait(false); @@ -322,10 +352,40 @@ public class ManagementHttpClient : IDisposable response.Dispose(); response = await SendWithAsync(buildRequest, _basic, completion, ct).ConfigureAwait(false); if (response.StatusCode != HttpStatusCode.Unauthorized && _session is not null) - _session.BasicOnly = true; + { + await MarkBasicOnlyAsync(_session, ct).ConfigureAwait(false); + if (IsVerbose()) + { + Console.Error.WriteLine( + "note: the server refused a freshly issued bearer token; using HTTP Basic for the rest of this run " + + "(check that both central nodes share ScadaBridge:Security:JwtSigningKey)."); + } + } + return response; } + // Written under the session gate so it cannot interleave with an in-flight exchange. + private static async Task MarkBasicOnlyAsync(CliTokenStore.Session session, CancellationToken ct) + { + await session.Gate.WaitAsync(ct).ConfigureAwait(false); + try + { + session.BasicOnly = true; + session.Token = null; + } + finally + { + session.Gate.Release(); + } + } + + private static bool IsVerbose() + { + var value = Environment.GetEnvironmentVariable(VerboseEnvVar)?.Trim(); + return value == "1" || string.Equals(value, "true", StringComparison.OrdinalIgnoreCase); + } + private Task SendWithAsync( Func buildRequest, AuthenticationHeaderValue header, HttpCompletionOption completion, CancellationToken ct) { diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index f13cbfa7..c798227b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -2690,6 +2690,7 @@ The CLI resolves `--username` / `--password` (or `SCADABRIDGE_USERNAME` / `SCADA - A server without bearer support — no `/auth/token`, a login-disabled deployment, or an older server whose token response lacks `management_bearer: true` — is used with HTTP Basic for the rest of the process (an LDAP bind per call, as before). - Bad credentials or a login-throttle lockout at the exchange are reported directly (exit 1) and not retried with Basic. - Set `SCADABRIDGE_AUTH_MODE=basic` to skip the exchange entirely. +- Set `SCADABRIDGE_VERBOSE=1` to get a stderr note when the server refuses a freshly issued token and the CLI falls back to Basic (usually central nodes with different `JwtSigningKey` values). ## Issues & Missing Features diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTokenTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTokenTests.cs index 6125f62a..f14b3665 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTokenTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTokenTests.cs @@ -21,6 +21,7 @@ public class ManagementHttpClientTokenTests public List<(string Path, string? Auth)> Requests { get; } = []; public Func? TokenResponse { get; set; } public Func? ManagementResponse { get; set; } + public Func? BeforeManagementResponse { get; set; } public HttpResponseMessage IssueToken(int expiresIn = 900) { @@ -42,6 +43,8 @@ public class ManagementHttpClientTokenTests return TokenResponse?.Invoke() ?? IssueToken(); } + if (BeforeManagementResponse is not null) + await BeforeManagementResponse(auth); return ManagementResponse?.Invoke(auth) ?? Json(HttpStatusCode.OK, "{\"ok\":true}"); } } @@ -225,4 +228,81 @@ public class ManagementHttpClientTokenTests Assert.Equal(new[] { ("/management", (string?)BasicHeader) }, server.Requests); } + + /// + /// Two callers refused the same token: the second must reuse the token the first already + /// re-exchanged instead of binding again (review L3). + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ConcurrentRefusal_ReusesPeerRefresh_InsteadOfSecondExchange() + { + var server = new FakeServer(); + var store = new CliTokenStore(); + using var a = Client(server, store); + using var b = Client(server, store); + await a.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); // tok-1 cached + + server.ManagementResponse = auth => auth == "Bearer tok-1" + ? Json(HttpStatusCode.Unauthorized, "{\"error\":\"expired\"}") + : Json(HttpStatusCode.OK, "{\"ok\":true}"); + var nested = false; + server.BeforeManagementResponse = async auth => + { + // While B's tok-1 request is in flight, A hits the same refusal and refreshes. + if (auth == "Bearer tok-1" && !nested) + { + nested = true; + Assert.Equal(200, (await a.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5))).StatusCode); + } + }; + + var response = await b.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + + Assert.Equal(200, response.StatusCode); + Assert.Equal(2, server.Requests.Count(r => r.Path == "/auth/token")); + Assert.Equal("Bearer tok-2", server.Requests[^1].Auth); + } + + /// The production HttpClient never follows redirects, so credentials cannot be replayed (review L2). + /// A task that represents the asynchronous operation. + [Fact] + public async Task TokenExchange_DoesNotFollowRedirects() + { + var listener = new System.Net.Sockets.TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + var requestLines = new List(); + var serve = Task.Run(async () => + { + for (var i = 0; i < 2; i++) + { + using var tcp = await listener.AcceptTcpClientAsync(); + await using var stream = tcp.GetStream(); + var buffer = new byte[8192]; + var read = await stream.ReadAsync(buffer); + requestLines.Add(Encoding.ASCII.GetString(buffer, 0, read).Split("\r\n")[0]); + var reply = i == 0 + ? $"HTTP/1.1 307 Temporary Redirect\r\nLocation: http://127.0.0.1:{port}/stolen\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + : "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{}"; + await stream.WriteAsync(Encoding.ASCII.GetBytes(reply)); + } + }); + + try + { + using var client = new ManagementHttpClient( + ManagementHttpClient.CreateDefaultHttpClient(), $"http://127.0.0.1:{port}", "user", "pass", new CliTokenStore()); + await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromSeconds(5)); + } + finally + { + await serve.WaitAsync(TimeSpan.FromSeconds(5)); + listener.Stop(); + } + + Assert.StartsWith("POST /auth/token", requestLines[0]); + Assert.DoesNotContain(requestLines, l => l.Contains("/stolen", StringComparison.Ordinal)); + Assert.StartsWith("POST /management", requestLines[1]); // 307 is a non-success → Basic for that call + } }