fix(cli): no redirects on the management client, race-safe token refresh, verbose Basic-fallback note (review 2026-10-21 05/U1, 05/P1 L1/L2/L3/L4)
- CreateDefaultHttpClient sets AllowAutoRedirect=false so a 307/308 cannot re-POST the /auth/token credential form elsewhere. - A forced re-exchange after a 401 carries the refused token; inside the gate it reuses a token another caller already re-exchanged instead of binding again. The BasicOnly switch is written under the same gate. - SCADABRIDGE_VERBOSE=1 prints a stderr note when a fresh token is refused and the CLI falls back to Basic. - debug stream's AccessTokenProvider retries the exchange once and documents the null-token degradation (the hub refuses, automatic reconnect retries). - Tests: concurrent refusal reuses the peer refresh (2 exchanges, not 3); real-socket redirect is not followed.
This commit is contained in:
@@ -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 <token>`; 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
|
||||
|
||||
@@ -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
|
||||
{
|
||||
|
||||
@@ -39,6 +39,13 @@ public class ManagementHttpClient : IDisposable
|
||||
/// <summary>Environment variable selecting the auth mode: <c>token</c> (default) or <c>basic</c>.</summary>
|
||||
public const string AuthModeEnvVar = "SCADABRIDGE_AUTH_MODE";
|
||||
|
||||
/// <summary>
|
||||
/// Environment variable that, when <c>1</c>/<c>true</c>, 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.
|
||||
/// </summary>
|
||||
public const string VerboseEnvVar = "SCADABRIDGE_VERBOSE";
|
||||
|
||||
/// <summary>Re-exchange a token this long before its stated expiry.</summary>
|
||||
internal static readonly TimeSpan TokenRefreshMargin = TimeSpan.FromSeconds(60);
|
||||
|
||||
@@ -117,15 +124,25 @@ public class ManagementHttpClient : IDisposable
|
||||
/// <param name="password">The password for HTTP Basic authentication.</param>
|
||||
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)
|
||||
{
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds the production <see cref="HttpClient"/>: infinite overall timeout (each call
|
||||
/// carries its own deadline), connect bounded by <see cref="ResolveConnectTimeout"/>, and
|
||||
/// redirects NOT followed — a 307/308 from a proxy would otherwise re-POST the
|
||||
/// <c>/auth/token</c> form, i.e. the credentials, to wherever the redirect points.
|
||||
/// </summary>
|
||||
/// <returns>The configured client.</returns>
|
||||
internal static HttpClient CreateDefaultHttpClient() =>
|
||||
new(new SocketsHttpHandler { ConnectTimeout = ResolveConnectTimeout(), AllowAutoRedirect = false })
|
||||
{
|
||||
Timeout = Timeout.InfiniteTimeSpan
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// True when <see cref="AuthModeEnvVar"/> is <c>basic</c>, which disables the token
|
||||
/// exchange for the process.
|
||||
@@ -179,7 +196,7 @@ public class ManagementHttpClient : IDisposable
|
||||
/// <returns>The token, a fall-back-to-Basic signal, or a failure to report.</returns>
|
||||
internal async Task<BearerResult> 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 <see cref="AuthChoice.Failure"/> is the exchange's own
|
||||
/// 401/429 response, to be surfaced to the caller as-is.
|
||||
/// </summary>
|
||||
private async Task<AuthChoice> AcquireAuthAsync(bool forceExchange, CancellationToken ct)
|
||||
/// <param name="forceExchange">Exchange even though a cached token looks usable (the server refused it).</param>
|
||||
/// <param name="refusedToken">
|
||||
/// 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.
|
||||
/// </param>
|
||||
/// <param name="ct">Cancellation token.</param>
|
||||
private async Task<AuthChoice> 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<HttpResponseMessage> SendAuthenticatedAsync(
|
||||
Func<HttpRequestMessage> 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<HttpResponseMessage> SendWithAsync(
|
||||
Func<HttpRequestMessage> buildRequest, AuthenticationHeaderValue header, HttpCompletionOption completion, CancellationToken ct)
|
||||
{
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -21,6 +21,7 @@ public class ManagementHttpClientTokenTests
|
||||
public List<(string Path, string? Auth)> Requests { get; } = [];
|
||||
public Func<HttpResponseMessage>? TokenResponse { get; set; }
|
||||
public Func<string?, HttpResponseMessage>? ManagementResponse { get; set; }
|
||||
public Func<string?, Task>? 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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Two callers refused the same token: the second must reuse the token the first already
|
||||
/// re-exchanged instead of binding again (review L3).
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>The production HttpClient never follows redirects, so credentials cannot be replayed (review L2).</summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[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<string>();
|
||||
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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user