From 08d79231546b673660f28f964cb6a7ea8131a8eb Mon Sep 17 00:00:00 2001 From: bernie-g Date: Tue, 11 Aug 2026 12:25:07 +0100 Subject: [PATCH 1/4] feat: raise the WinRM post-sync command limit to 8192 characters Send the script to the host over stdin rather than on the PowerShell command line. cmd.exe rejects a command line past ~8155 characters (measured against a live Server 2016 host) and -EncodedCommand inflates a script ~3.2x on the way there, so the usable ceiling was ~2555. A fixed bootstrap now takes the command line and reads the real script from stdin. The script crosses base64-encoded, so the host's console code page cannot alter it: the ASCII-only restriction is gone, and the pkcs12 password no longer appears in the host's process table. Serialize SOAP requests per client. NTLM message sealing is RC4 keyed by a sequence counter, and the library issues the stdin Send concurrently with the output Receive without locking, desynchronizing the keystream and failing with "checksum does not match". Commands that write stdin also use a shorter WSMan operation timeout so the output poll releases the lock promptly instead of long-polling for 60s. Also shortens noOutcomeMessage to fit the control plane's 120-character failure-detail cap, which previously cut it mid-quote and dropped the part telling the operator what to do instead. --- packages/gateway-v2/winrm/winrm.go | 98 +++++++++++++--- .../gateway-v2/winrm/winrm_command_test.go | 106 ++++++++++++++---- .../gateway-v2/winrm/winrm_transport_test.go | 100 ++++++++++++++++- packages/gateway-v2/winrm_handler.go | 7 +- 4 files changed, 266 insertions(+), 45 deletions(-) diff --git a/packages/gateway-v2/winrm/winrm.go b/packages/gateway-v2/winrm/winrm.go index 01b79e25..edc76255 100644 --- a/packages/gateway-v2/winrm/winrm.go +++ b/packages/gateway-v2/winrm/winrm.go @@ -20,9 +20,11 @@ import ( "strings" "sync" "time" + "unicode/utf16" "unicode/utf8" "github.com/masterzen/winrm" + "github.com/masterzen/winrm/soap" ) // ErrConnect marks a failure to reach the Windows host. @@ -210,12 +212,49 @@ func pinnedServerName(caCert []byte) string { return cert.Subject.CommonName } +// serializedTransport funnels every SOAP request for one client through a single mutex. +// +// NTLM message sealing is RC4 keyed by a sequence counter, so two goroutines sealing on the same +// session desynchronize the keystream and the host rejects the message with "checksum does not +// match". The library issues requests from several goroutines at once (fetchOutput drains a +// command's output while the caller writes stdin) and does not lock around them, so serializing here +// is what makes writing stdin possible at all. +// +// This removes overlap we never wanted: RunCommand's bootstrap reads stdin to EOF before producing +// any output, so writing and reading are already two ordered phases. The library interleaves them +// only to avoid a pipe deadlock in the general case, which our flow cannot hit. +type serializedTransport struct { + winrm.Transporter + mu sync.Mutex +} + +func (t *serializedTransport) Post(client *winrm.Client, message *soap.SoapMessage) (string, error) { + t.mu.Lock() + defer t.mu.Unlock() + return t.Transporter.Post(client, message) +} + +type clientOption func(*winrm.Parameters) + +// stdinOperationTimeout shortens the WSMan operation timeout for commands that write stdin. The +// output poll otherwise long-polls for the 60s default while holding the transport lock, which would +// stall the stdin write for a minute. Shorter means it returns empty and releases the lock promptly; +// slurpAllOutput treats an OperationTimeout fault as "not finished, keep polling". +const stdinOperationTimeout = "PT2S" + +func withOperationTimeout(timeout string) clientOption { + return func(p *winrm.Parameters) { p.Timeout = timeout } +} + // newClient builds a WinRM client. Both modes authenticate with NTLM; they differ in how the SOAP body // is kept confidential. HTTP (default) uses NTLM message sealing, so the body is confidential without a // server certificate (default listeners require this). HTTPS relies on TLS, verifying the listener against // the system trust store, an optional pinned CA (self-signed listener), or skipping verification if Insecure. -func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { +func newClient(ctx context.Context, creds Credentials, opts ...clientOption) (*winrm.Client, error) { params := *winrm.DefaultParameters + for _, opt := range opts { + opt(¶ms) + } if creds.UseHTTPS { // NTLM authentication over TLS. The bounded dial caps the response read and carries the operation // deadline; the library otherwise reads the body unbounded and issues its request without a context. @@ -234,6 +273,14 @@ func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { params.TransportDecorator = func() winrm.Transporter { return enc } } + // Applied last so it wraps whichever transport the mode selected. The decorator runs once per + // client, so the mutex is scoped to a single NTLM session, which is exactly the sealing state it + // has to protect. + decorate := params.TransportDecorator + params.TransportDecorator = func() winrm.Transporter { + return &serializedTransport{Transporter: decorate()} + } + endpoint := winrm.NewEndpoint( creds.Host, creds.Port, @@ -538,7 +585,10 @@ func (w *limitedBuffer) Write(p []byte) (int, error) { // commandScriptTemplate wraps the operator's command (%[1]s) so it reports its exit code in a stdout // trailer tagged with a per-run nonce (%[2]s). An exit from inside the try block arrives as 0. -const commandScriptTemplate = `$ErrorActionPreference = 'Stop' +// Sets $ProgressPreference itself: the script is piped to stdin rather than passed to +// winrm.Powershell, which is what used to prepend it. +const commandScriptTemplate = `$ProgressPreference = 'SilentlyContinue' +$ErrorActionPreference = 'Stop' $%[3]s = 0 try { %[1]s @@ -585,11 +635,28 @@ func takeCommandTrailer(stdout, nonce string) (code int, remaining string, ok bo } // noOutcomeMessage covers both causes: PowerShell parses the whole script before running any of it. -const noOutcomeMessage = "The command did not report a result: it either called exit, which stops the " + - "script early, or did not parse. Use `throw \"reason\"` to fail the sync deliberately." +// Kept under the control plane's 120-character failure-detail cap, so the remedy is not the part +// that gets cut off. +const noOutcomeMessage = "The command called exit or did not parse, so it reported no result. " + + "Use `throw \"reason\"` to fail deliberately." + +// Script travels via stdin, not the command line: cmd.exe caps a command line at ~8155 chars, and +// the process table would expose the pkcs12 password. Base64 because PowerShell decodes stdin using +// the host's code page. '&' not .Invoke(), which buffers output and would reorder the exit trailer. +// Never -File -/-Command -: 5.1 reads stdin-as-source as a REPL and silently drops multi-line blocks. +const commandBootstrap = `$b=[Console]::In.ReadToEnd(); ` + + `$s=[Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($b)); ` + + `& ([ScriptBlock]::Create($s))` -// maxEncodedCommandChars bounds the -EncodedCommand command line, which Windows caps at 8191 characters. -const maxEncodedCommandChars = 8000 +// utf16LEBytes encodes to UTF-16LE without a BOM, matching what the bootstrap decodes with. +func utf16LEBytes(s string) []byte { + units := utf16.Encode([]rune(s)) + buf := make([]byte, 0, len(units)*2) + for _, u := range units { + buf = append(buf, byte(u), byte(u>>8)) + } + return buf +} var clixmlEntities = strings.NewReplacer("<", "<", ">", ">", "&", "&", """, `"`, "'", "'") @@ -628,8 +695,9 @@ func resolveCommandOutcome(code int, stated bool, stderr string) (int, string) { return 1, strings.TrimSpace(noOutcomeMessage + "\n" + stderr) } -// RunCommand runs a command on the host. The script goes on the command line as -EncodedCommand, so it -// is length-bounded and visible in the host's process table. +// RunCommand runs a command on the host. Only the fixed bootstrap goes on the command line; the +// script itself is piped over stdin, so it carries no length limit and any Unicode survives. See +// commandBootstrap. func RunCommand( ctx context.Context, creds Credentials, @@ -641,19 +709,13 @@ func RunCommand( return CommandResult{}, err } - encoded := winrm.Powershell(buildCommandScript(command, nonce)) + encoded := winrm.Powershell(commandBootstrap) if encoded == "" { return CommandResult{}, errors.New("failed to encode the command") } - if len(encoded) > maxEncodedCommandChars { - return CommandResult{}, fmt.Errorf( - "the command is too long to run on Windows once encoded (%d of %d characters). Shorten it, "+ - "or move the logic into a script on the host and call that script", - len(encoded), maxEncodedCommandChars, - ) - } + payload := base64.StdEncoding.EncodeToString(utf16LEBytes(buildCommandScript(command, nonce))) - client, clientErr := newClient(ctx, creds) + client, clientErr := newClient(ctx, creds, withOperationTimeout(stdinOperationTimeout)) if clientErr != nil { return CommandResult{}, clientErr } @@ -669,7 +731,7 @@ func RunCommand( stdoutWriter := &limitedBuffer{buf: &stdout, limit: maxCommandOutputBytes} stderrWriter := &limitedBuffer{buf: &stderr, limit: maxCommandOutputBytes} - _, runErr := client.RunWithContext(runCtx, encoded, stdoutWriter, stderrWriter) + _, runErr := client.RunWithContextWithInput(runCtx, encoded, stdoutWriter, stderrWriter, strings.NewReader(payload)) if runErr != nil { if errors.Is(runCtx.Err(), context.DeadlineExceeded) { return CommandResult{}, fmt.Errorf("command timed out after %s", timeout) diff --git a/packages/gateway-v2/winrm/winrm_command_test.go b/packages/gateway-v2/winrm/winrm_command_test.go index ffe2c2dc..e74cc722 100644 --- a/packages/gateway-v2/winrm/winrm_command_test.go +++ b/packages/gateway-v2/winrm/winrm_command_test.go @@ -3,11 +3,11 @@ package winrm import ( "bytes" "context" + "encoding/base64" "strings" "testing" "time" - - "github.com/masterzen/winrm" + "unicode/utf16" ) func TestEscapePowerShellSingleQuotesNeutralizesInjection(t *testing.T) { @@ -194,35 +194,87 @@ func TestNormalizePowerShellStderrDropsEmptyEnvelope(t *testing.T) { } } -func TestBuildCommandScriptDoesNotDuplicateProgressPreference(t *testing.T) { - if strings.Contains(buildCommandScript("Write-Output ok", "infisical-nonce123"), "$ProgressPreference") { - t.Fatal("expected the wrapper to leave $ProgressPreference to winrm.Powershell") +func TestBuildCommandScriptSetsProgressPreference(t *testing.T) { + // The script is piped to stdin, so winrm.Powershell no longer prepends this for us. Without it, + // progress bars land on stderr and get reported as the command's failure reason. + if !strings.Contains(buildCommandScript("Write-Output ok", "infisical-nonce123"), "$ProgressPreference") { + t.Fatal("expected the wrapper to set $ProgressPreference itself") + } +} + +func TestRunCommandAcceptsACommandPastTheOldCommandLineLimit(t *testing.T) { + // Would have been rejected outright when the script travelled as -EncodedCommand. It now fails + // on the connection instead, which is what proves the length gate is gone. + _, err := RunCommand(context.Background(), Credentials{}, strings.Repeat("a", 8192), time.Second) + + if err != nil && strings.Contains(err.Error(), "too long") { + t.Fatalf("expected no length rejection, got %v", err) + } +} + +func TestUtf16LEBytesMatchesWhatTheBootstrapDecodes(t *testing.T) { + // The bootstrap calls [Text.Encoding]::Unicode.GetString, which is UTF-16LE with no BOM. A BOM + // here would arrive as a leading U+FEFF and break the first statement of the script. + got := utf16LEBytes("aé") + + want := []byte{'a', 0x00, 0xe9, 0x00} + if !bytes.Equal(got, want) { + t.Fatalf("utf16LEBytes = % x, want % x", got, want) + } +} + +func TestUtf16LEBytesEncodesAstralCharactersAsASurrogatePair(t *testing.T) { + got := utf16LEBytes("\U0001F512") + + want := []byte{0x3d, 0xd8, 0x12, 0xdd} + if !bytes.Equal(got, want) { + t.Fatalf("utf16LEBytes = % x, want % x", got, want) } } -func TestRunCommandRejectsAnOverlongEncodedCommand(t *testing.T) { - // The length check runs before any connection, so no host is needed. - _, err := RunCommand(context.Background(), Credentials{}, strings.Repeat("a", 10_000), time.Second) +func TestPayloadRoundTripsThroughTheBootstrapEncoding(t *testing.T) { + // Mirrors what the bootstrap does in reverse, so a change to either side has to change both. + script := buildCommandScript("Write-Output 'café'\n\nWrite-Output 'ok' # comment", "infisical-nonce123") + + payload := base64.StdEncoding.EncodeToString(utf16LEBytes(script)) + raw, err := base64.StdEncoding.DecodeString(payload) + if err != nil { + t.Fatalf("payload is not valid base64: %v", err) + } + units := make([]uint16, 0, len(raw)/2) + for i := 0; i+1 < len(raw); i += 2 { + units = append(units, uint16(raw[i])|uint16(raw[i+1])<<8) + } - if err == nil { - t.Fatal("expected an over-long command to be rejected") + if decoded := string(utf16.Decode(units)); decoded != script { + t.Fatalf("round trip changed the script:\n got %q\nwant %q", decoded, script) + } + if i := strings.IndexFunc(payload, func(r rune) bool { return r > 0x7e }); i >= 0 { + t.Fatalf("payload must be ASCII so the host's code page cannot alter it, found %q at %d", payload[i], i) } - if !strings.Contains(err.Error(), "too long to run on Windows once encoded") { - t.Fatalf("expected the encoded-length error, got %v", err) +} + +func TestRunCommandAcceptsNonASCII(t *testing.T) { + // Base64 keeps the wire ASCII, so the operator is no longer restricted. Fails on the connection + // rather than on validation, which is the point. + _, err := RunCommand(context.Background(), Credentials{}, "Write-Output 'café'", time.Second) + + if err != nil && strings.Contains(err.Error(), "ASCII") { + t.Fatalf("expected no character-set rejection, got %v", err) } } -func TestHandlerCommandCapAlwaysFitsInsideTheEncodedCeiling(t *testing.T) { - // Mirrors maxWinrmCommandChars in the parent package. - const handlerCommandCap = 2048 +func TestHandlerCommandCapStaysWithinTheRpcBodyLimit(t *testing.T) { + // Mirrors maxWinrmCommandChars in the parent package. The encoded ceiling no longer gates + // RunCommand, but the command still has to fit in the RPC body alongside the envelope. + const handlerCommandCap = 8192 - encoded := winrm.Powershell(buildCommandScript(strings.Repeat("a", handlerCommandCap), "infisical-0123456789abcdef0123456789abcdef")) + script := buildCommandScript(strings.Repeat("a", handlerCommandCap), "infisical-0123456789abcdef0123456789abcdef") - if len(encoded) > maxEncodedCommandChars { - t.Fatalf("a command at the handler's %d-char cap encodes to %d, past the %d ceiling", - handlerCommandCap, len(encoded), maxEncodedCommandChars) + if len(script) > 1*1024*1024 { + t.Fatalf("a command at the handler's %d-char cap builds a %d-byte script", handlerCommandCap, len(script)) } - t.Logf("handler cap %d chars -> %d encoded, ceiling %d", handlerCommandCap, len(encoded), maxEncodedCommandChars) + t.Logf("handler cap %d chars -> %d byte script", handlerCommandCap, len(script)) } func TestResolveCommandOutcome(t *testing.T) { @@ -240,7 +292,7 @@ func TestResolveCommandOutcome(t *testing.T) { if code == 0 { t.Fatal("expected an unstated outcome to fail, not to inherit a zero exit code") } - if !strings.Contains(stderr, "did not report a result") { + if !strings.Contains(stderr, "reported no result") { t.Fatalf("expected the reason to be explained, got %q", stderr) } }) @@ -256,10 +308,20 @@ func TestResolveCommandOutcome(t *testing.T) { t.Run("leads with the explanation so the caller quotes it", func(t *testing.T) { _, stderr := resolveCommandOutcome(0, false, "some earlier noise") - if !strings.HasPrefix(stderr, "The command did not report a result") { + if !strings.HasPrefix(stderr, noOutcomeMessage) { t.Fatalf("expected the explanation first, got %q", stderr) } }) + + t.Run("the explanation fits the control plane's failure-detail cap", func(t *testing.T) { + // The control plane quotes the first stderr line into lastSyncMessage and caps it at 120 + // characters. Overrun and the remedy is exactly the part that gets cut. + const failureDetailCap = 120 + if len(noOutcomeMessage) > failureDetailCap { + t.Fatalf("noOutcomeMessage is %d chars, past the %d cap: %q", + len(noOutcomeMessage), failureDetailCap, noOutcomeMessage) + } + }) } func TestLimitedBufferKeepsTheTailPastTheCap(t *testing.T) { diff --git a/packages/gateway-v2/winrm/winrm_transport_test.go b/packages/gateway-v2/winrm/winrm_transport_test.go index 1757f31d..8dc2d94b 100644 --- a/packages/gateway-v2/winrm/winrm_transport_test.go +++ b/packages/gateway-v2/winrm/winrm_transport_test.go @@ -3,11 +3,25 @@ package winrm import ( "context" "sync" + "sync/atomic" "testing" + "time" "github.com/masterzen/winrm" + "github.com/masterzen/winrm/soap" ) +// innerTransport unwraps the serialization wrapper so a test can assert on the transport the scheme +// actually selected. +func innerTransport(t *testing.T, client *winrm.Client) winrm.Transporter { + t.Helper() + wrapped, ok := client.Parameters.TransportDecorator().(*serializedTransport) + if !ok { + t.Fatalf("expected the transport to be wrapped for serialization, got %T", client.Parameters.TransportDecorator()) + } + return wrapped.Transporter +} + // TestNewClientTransportByScheme locks in the transport split: HTTP uses NTLM message encryption // (*winrm.Encryption), HTTPS uses NTLM auth over TLS (*winrm.ClientNTLM). func TestNewClientTransportByScheme(t *testing.T) { @@ -17,16 +31,94 @@ func TestNewClientTransportByScheme(t *testing.T) { if err != nil { t.Fatalf("newClient(http): %v", err) } - if _, ok := httpClient.Parameters.TransportDecorator().(*winrm.Encryption); !ok { - t.Errorf("HTTP: expected *winrm.Encryption, got %T", httpClient.Parameters.TransportDecorator()) + if inner := innerTransport(t, httpClient); !isType[*winrm.Encryption](inner) { + t.Errorf("HTTP: expected *winrm.Encryption, got %T", inner) } httpsClient, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5986, Username: "u", Password: "p", UseHTTPS: true}) if err != nil { t.Fatalf("newClient(https): %v", err) } - if _, ok := httpsClient.Parameters.TransportDecorator().(*winrm.ClientNTLM); !ok { - t.Errorf("HTTPS: expected *winrm.ClientNTLM, got %T", httpsClient.Parameters.TransportDecorator()) + if inner := innerTransport(t, httpsClient); !isType[*winrm.ClientNTLM](inner) { + t.Errorf("HTTPS: expected *winrm.ClientNTLM, got %T", inner) + } +} + +func isType[T any](v any) bool { + _, ok := v.(T) + return ok +} + +// concurrencyProbe records whether two Post calls were ever in flight at once. +type concurrencyProbe struct { + inFlight atomic.Int32 + overlap atomic.Bool + calls atomic.Int32 +} + +func (p *concurrencyProbe) Post(*winrm.Client, *soap.SoapMessage) (string, error) { + if p.inFlight.Add(1) > 1 { + p.overlap.Store(true) + } + time.Sleep(time.Millisecond) + p.calls.Add(1) + p.inFlight.Add(-1) + return "", nil +} + +func (p *concurrencyProbe) Transport(*winrm.Endpoint) error { return nil } + +func hammer(tr winrm.Transporter) *sync.WaitGroup { + var wg sync.WaitGroup + for i := 0; i < 16; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, _ = tr.Post(nil, nil) + }() + } + return &wg +} + +// TestSerializedTransportPreventsOverlappingPosts is the whole point of the wrapper: NTLM sealing is +// RC4 with a sequence counter, so two goroutines sealing at once corrupt the keystream and the host +// answers "checksum does not match". +func TestSerializedTransportPreventsOverlappingPosts(t *testing.T) { + probe := &concurrencyProbe{} + tr := &serializedTransport{Transporter: probe} + + hammer(tr).Wait() + + if probe.overlap.Load() { + t.Fatal("two Post calls overlapped despite the serialization wrapper") + } + if got := probe.calls.Load(); got != 16 { + t.Fatalf("expected all 16 calls to complete, got %d", got) + } +} + +// TestConcurrencyProbeDetectsOverlapWithoutTheWrapper keeps the test above honest: without the +// wrapper the same probe must see overlap, otherwise it would pass for the wrong reason. +func TestConcurrencyProbeDetectsOverlapWithoutTheWrapper(t *testing.T) { + probe := &concurrencyProbe{} + + hammer(probe).Wait() + + if !probe.overlap.Load() { + t.Fatal("probe saw no overlap unwrapped, so it cannot prove the wrapper does anything") + } +} + +func TestWithOperationTimeoutOverridesTheDefault(t *testing.T) { + params := *winrm.DefaultParameters + if params.Timeout != "PT60S" { + t.Fatalf("expected the library default to be PT60S, got %q", params.Timeout) + } + + withOperationTimeout(stdinOperationTimeout)(¶ms) + + if params.Timeout != stdinOperationTimeout { + t.Fatalf("Timeout = %q, want %q", params.Timeout, stdinOperationTimeout) } } diff --git a/packages/gateway-v2/winrm_handler.go b/packages/gateway-v2/winrm_handler.go index 64c7dcd7..3157dedd 100644 --- a/packages/gateway-v2/winrm_handler.go +++ b/packages/gateway-v2/winrm_handler.go @@ -121,7 +121,12 @@ const ( winrmConnDeadline = winrmOpDeadline + 15*time.Second maxWinrmRequestBodyBytes = 4 * 1024 * 1024 - maxWinrmCommandChars = 2048 + // The script is piped to the host over stdin, so cmd.exe's 8191-character command-line limit no + // longer applies and this is a product bound rather than a platform one. It must stay well under + // ~57000, the point at which the base64 payload (2.67x the script) exceeds one WinRM Send: each + // extra chunk waits out a full output-poll cycle for the transport lock. Keep in step with + // POST_SYNC_COMMAND_MAX_LENGTH in the control plane. + maxWinrmCommandChars = 8192 defaultWinrmCommandTimeout = 30 * time.Second maxWinrmCommandTimeout = 90 * time.Second ) From 838ec94c3fc01221ae7d60dc7a97696d70ec30f3 Mon Sep 17 00:00:00 2001 From: bernie-g Date: Tue, 11 Aug 2026 12:57:49 +0100 Subject: [PATCH 2/4] fix: count post-sync command characters rather than bytes The gateway measured the command with len(), a byte count, while the control plane's limit counts characters. Now that a command may be non-ASCII, 8192 accented characters is 16384 bytes, so the gateway rejected a command the UI had already accepted, after the certificates were delivered. Also raises the gateway cap and reframes it as a backstop rather than a second copy of the product limit. The command arrives with placeholders substituted, and {{certificateFiles}} grows with the number of certificates delivered, so it needs headroom over what an operator can save. --- packages/gateway-v2/winrm/winrm.go | 29 ++++++++-------------------- packages/gateway-v2/winrm_handler.go | 15 +++++++------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/packages/gateway-v2/winrm/winrm.go b/packages/gateway-v2/winrm/winrm.go index edc76255..282dccc1 100644 --- a/packages/gateway-v2/winrm/winrm.go +++ b/packages/gateway-v2/winrm/winrm.go @@ -212,17 +212,8 @@ func pinnedServerName(caCert []byte) string { return cert.Subject.CommonName } -// serializedTransport funnels every SOAP request for one client through a single mutex. -// -// NTLM message sealing is RC4 keyed by a sequence counter, so two goroutines sealing on the same -// session desynchronize the keystream and the host rejects the message with "checksum does not -// match". The library issues requests from several goroutines at once (fetchOutput drains a -// command's output while the caller writes stdin) and does not lock around them, so serializing here -// is what makes writing stdin possible at all. -// -// This removes overlap we never wanted: RunCommand's bootstrap reads stdin to EOF before producing -// any output, so writing and reading are already two ordered phases. The library interleaves them -// only to avoid a pipe deadlock in the general case, which our flow cannot hit. +// NTLM sealing is RC4 keyed by a sequence counter, and the library writes stdin and drains output +// from separate goroutines without locking, desynchronizing the keystream ("checksum does not match"). type serializedTransport struct { winrm.Transporter mu sync.Mutex @@ -236,10 +227,8 @@ func (t *serializedTransport) Post(client *winrm.Client, message *soap.SoapMessa type clientOption func(*winrm.Parameters) -// stdinOperationTimeout shortens the WSMan operation timeout for commands that write stdin. The -// output poll otherwise long-polls for the 60s default while holding the transport lock, which would -// stall the stdin write for a minute. Shorter means it returns empty and releases the lock promptly; -// slurpAllOutput treats an OperationTimeout fault as "not finished, keep polling". +// Short so the output poll releases the transport lock instead of long-polling for the 60s default +// while stdin waits behind it. const stdinOperationTimeout = "PT2S" func withOperationTimeout(timeout string) clientOption { @@ -273,9 +262,8 @@ func newClient(ctx context.Context, creds Credentials, opts ...clientOption) (*w params.TransportDecorator = func() winrm.Transporter { return enc } } - // Applied last so it wraps whichever transport the mode selected. The decorator runs once per - // client, so the mutex is scoped to a single NTLM session, which is exactly the sealing state it - // has to protect. + // Last, so it wraps whichever transport the mode chose. Runs once per client, scoping the mutex to + // one NTLM session. decorate := params.TransportDecorator params.TransportDecorator = func() winrm.Transporter { return &serializedTransport{Transporter: decorate()} @@ -695,9 +683,8 @@ func resolveCommandOutcome(code int, stated bool, stderr string) (int, string) { return 1, strings.TrimSpace(noOutcomeMessage + "\n" + stderr) } -// RunCommand runs a command on the host. Only the fixed bootstrap goes on the command line; the -// script itself is piped over stdin, so it carries no length limit and any Unicode survives. See -// commandBootstrap. +// RunCommand runs a command on the host. Only the bootstrap goes on the command line; the script is +// piped over stdin. See commandBootstrap. func RunCommand( ctx context.Context, creds Credentials, diff --git a/packages/gateway-v2/winrm_handler.go b/packages/gateway-v2/winrm_handler.go index 3157dedd..5757cbee 100644 --- a/packages/gateway-v2/winrm_handler.go +++ b/packages/gateway-v2/winrm_handler.go @@ -12,6 +12,7 @@ import ( "strings" "sync" "time" + "unicode/utf8" "github.com/Infisical/infisical-merge/packages/gateway-v2/winrm" "github.com/rs/zerolog/log" @@ -121,12 +122,10 @@ const ( winrmConnDeadline = winrmOpDeadline + 15*time.Second maxWinrmRequestBodyBytes = 4 * 1024 * 1024 - // The script is piped to the host over stdin, so cmd.exe's 8191-character command-line limit no - // longer applies and this is a product bound rather than a platform one. It must stay well under - // ~57000, the point at which the base64 payload (2.67x the script) exceeds one WinRM Send: each - // extra chunk waits out a full output-poll cycle for the transport lock. Keep in step with - // POST_SYNC_COMMAND_MAX_LENGTH in the control plane. - maxWinrmCommandChars = 8192 + // Backstop above the control plane's product limit, not a second gate: the command arrives with + // placeholders substituted, and {{certificateFiles}} grows with the certificate count. Stay under + // ~57000, where the payload stops fitting in one WinRM Send. + maxWinrmCommandChars = 32768 defaultWinrmCommandTimeout = 30 * time.Second maxWinrmCommandTimeout = 90 * time.Second ) @@ -325,7 +324,9 @@ func handleWinrmRunCommand(ctx context.Context, env *winrmRequestEnvelope) (any, if strings.TrimSpace(p.Command) == "" { return nil, fmt.Errorf("command is required") } - if len(p.Command) > maxWinrmCommandChars { + // Characters, not bytes: the control plane counts characters, so a byte count would reject a + // non-ASCII command it had already accepted. + if utf8.RuneCountInString(p.Command) > maxWinrmCommandChars { return nil, fmt.Errorf("command exceeds %d characters", maxWinrmCommandChars) } From fac2de3f9f163b72349a24891a971324c5b11980 Mon Sep 17 00:00:00 2001 From: bernie-g Date: Tue, 11 Aug 2026 14:25:00 +0100 Subject: [PATCH 3/4] fix: stop shortening the WSMan operation timeout for stdin commands The short timeout applied to every operation on the client, not just the output poll it was meant for. Shell creation, Command, Send, Signal and Delete inherited a two-second budget, and none of them retries on a w:TimedOut fault, so a host that was merely slow failed the sync outright. It existed only to break a standoff: the output poll held the transport lock waiting for output, and no output could arrive until stdin was written, which needed the same lock. The bootstrap now writes one byte before it blocks on stdin, so the poll returns as soon as PowerShell starts and releases the lock on its own. The timeout override is gone and newClient is untouched again. The sentinel is stripped with TrimPrefix, which removes a single occurrence, so a command that itself opens by printing that byte keeps it. --- packages/gateway-v2/winrm/winrm.go | 28 ++++------- .../gateway-v2/winrm/winrm_command_test.go | 50 +++++++++++++++++++ .../gateway-v2/winrm/winrm_transport_test.go | 18 ++++--- 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/packages/gateway-v2/winrm/winrm.go b/packages/gateway-v2/winrm/winrm.go index 282dccc1..6b1df4b3 100644 --- a/packages/gateway-v2/winrm/winrm.go +++ b/packages/gateway-v2/winrm/winrm.go @@ -225,25 +225,12 @@ func (t *serializedTransport) Post(client *winrm.Client, message *soap.SoapMessa return t.Transporter.Post(client, message) } -type clientOption func(*winrm.Parameters) - -// Short so the output poll releases the transport lock instead of long-polling for the 60s default -// while stdin waits behind it. -const stdinOperationTimeout = "PT2S" - -func withOperationTimeout(timeout string) clientOption { - return func(p *winrm.Parameters) { p.Timeout = timeout } -} - // newClient builds a WinRM client. Both modes authenticate with NTLM; they differ in how the SOAP body // is kept confidential. HTTP (default) uses NTLM message sealing, so the body is confidential without a // server certificate (default listeners require this). HTTPS relies on TLS, verifying the listener against // the system trust store, an optional pinned CA (self-signed listener), or skipping verification if Insecure. -func newClient(ctx context.Context, creds Credentials, opts ...clientOption) (*winrm.Client, error) { +func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { params := *winrm.DefaultParameters - for _, opt := range opts { - opt(¶ms) - } if creds.UseHTTPS { // NTLM authentication over TLS. The bounded dial caps the response read and carries the operation // deadline; the library otherwise reads the body unbounded and issues its request without a context. @@ -632,7 +619,14 @@ const noOutcomeMessage = "The command called exit or did not parse, so it report // the process table would expose the pkcs12 password. Base64 because PowerShell decodes stdin using // the host's code page. '&' not .Invoke(), which buffers output and would reorder the exit trailer. // Never -File -/-Command -: 5.1 reads stdin-as-source as a REPL and silently drops multi-line blocks. -const commandBootstrap = `$b=[Console]::In.ReadToEnd(); ` + +// Written before the bootstrap blocks on stdin. The output poll holds the transport lock until it +// has something to return, and nothing can be returned until stdin arrives, which needs that same +// lock. One byte up front breaks the standoff, so the poll releases the lock in the time PowerShell +// takes to start rather than waiting out its operation timeout. +const commandReadySentinel = "\x01" + +const commandBootstrap = `[Console]::Out.Write([char]1); [Console]::Out.Flush(); ` + + `$b=[Console]::In.ReadToEnd(); ` + `$s=[Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($b)); ` + `& ([ScriptBlock]::Create($s))` @@ -702,7 +696,7 @@ func RunCommand( } payload := base64.StdEncoding.EncodeToString(utf16LEBytes(buildCommandScript(command, nonce))) - client, clientErr := newClient(ctx, creds, withOperationTimeout(stdinOperationTimeout)) + client, clientErr := newClient(ctx, creds) if clientErr != nil { return CommandResult{}, clientErr } @@ -732,7 +726,7 @@ func RunCommand( } code, remaining, stated := takeCommandTrailer(stdout.String(), nonce) - result.Stdout = remaining + result.Stdout = strings.TrimPrefix(remaining, commandReadySentinel) if !stated { // Truncation drops the head, so fall back to the retained tail. code, _, stated = takeCommandTrailer(string(stdoutWriter.tail), nonce) diff --git a/packages/gateway-v2/winrm/winrm_command_test.go b/packages/gateway-v2/winrm/winrm_command_test.go index e74cc722..52db011e 100644 --- a/packages/gateway-v2/winrm/winrm_command_test.go +++ b/packages/gateway-v2/winrm/winrm_command_test.go @@ -194,6 +194,24 @@ func TestNormalizePowerShellStderrDropsEmptyEnvelope(t *testing.T) { } } +// The sentinel exists to give the output poll something to return before stdin arrives. If it were +// written after the read, the poll would hold the transport lock until its operation timeout and the +// stdin write would sit behind it. +func TestBootstrapWritesTheReadySentinelBeforeReadingStdin(t *testing.T) { + sentinelAt := strings.Index(commandBootstrap, "[Console]::Out.Write([char]1)") + readAt := strings.Index(commandBootstrap, "[Console]::In.ReadToEnd()") + + if sentinelAt < 0 || readAt < 0 { + t.Fatalf("bootstrap is missing the sentinel write or the stdin read: %q", commandBootstrap) + } + if sentinelAt > readAt { + t.Fatal("the sentinel must be written before the bootstrap blocks on stdin") + } + if !strings.Contains(commandBootstrap, "[Console]::Out.Flush()") { + t.Fatal("the sentinel must be flushed, or it can sit in a buffer until the script ends") + } +} + func TestBuildCommandScriptSetsProgressPreference(t *testing.T) { // The script is piped to stdin, so winrm.Powershell no longer prepends this for us. Without it, // progress bars land on stderr and get reported as the command's failure reason. @@ -212,6 +230,38 @@ func TestRunCommandAcceptsACommandPastTheOldCommandLineLimit(t *testing.T) { } } +func TestReadySentinelIsStrippedFromStdout(t *testing.T) { + nonce := "infisical-nonce123" + raw := commandReadySentinel + "real output\n\n" + nonce + ":0\n" + + code, remaining, stated := takeCommandTrailer(raw, nonce) + stdout := strings.TrimPrefix(remaining, commandReadySentinel) + + if !stated || code != 0 { + t.Fatalf("trailer parse failed: code=%d stated=%v", code, stated) + } + if strings.Contains(stdout, commandReadySentinel) { + t.Fatalf("the sentinel reached the caller's stdout: %q", stdout) + } + if !strings.HasPrefix(stdout, "real output") { + t.Fatalf("stdout = %q, want it to start with the command's own output", stdout) + } +} + +// The bootstrap writes the sentinel before the operator's script runs, so ours is always the first +// byte and TrimPrefix removes exactly one. A command that opens by printing the same byte keeps it. +func TestStrippingTheSentinelKeepsAnIdenticalByteFromTheCommand(t *testing.T) { + nonce := "infisical-nonce123" + raw := commandReadySentinel + commandReadySentinel + "output\n\n" + nonce + ":0\n" + + _, remaining, _ := takeCommandTrailer(raw, nonce) + + stdout := strings.TrimPrefix(remaining, commandReadySentinel) + if !strings.HasPrefix(stdout, commandReadySentinel) { + t.Fatalf("stripped the command's own leading byte as well: %q", stdout) + } +} + func TestUtf16LEBytesMatchesWhatTheBootstrapDecodes(t *testing.T) { // The bootstrap calls [Text.Encoding]::Unicode.GetString, which is UTF-16LE with no BOM. A BOM // here would arrive as a leading U+FEFF and break the first statement of the script. diff --git a/packages/gateway-v2/winrm/winrm_transport_test.go b/packages/gateway-v2/winrm/winrm_transport_test.go index 8dc2d94b..37d9d3bb 100644 --- a/packages/gateway-v2/winrm/winrm_transport_test.go +++ b/packages/gateway-v2/winrm/winrm_transport_test.go @@ -109,16 +109,18 @@ func TestConcurrencyProbeDetectsOverlapWithoutTheWrapper(t *testing.T) { } } -func TestWithOperationTimeoutOverridesTheDefault(t *testing.T) { - params := *winrm.DefaultParameters - if params.Timeout != "PT60S" { - t.Fatalf("expected the library default to be PT60S, got %q", params.Timeout) - } +// The output poll is released by the bootstrap's ready sentinel, not by a shortened timeout. A short +// one would also apply to shell creation and the Send, neither of which retries on a timeout fault. +func TestClientKeepsTheDefaultOperationTimeout(t *testing.T) { + winrm.DefaultParameters.TransportDecorator = nil - withOperationTimeout(stdinOperationTimeout)(¶ms) + client, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}) + if err != nil { + t.Fatalf("newClient: %v", err) + } - if params.Timeout != stdinOperationTimeout { - t.Fatalf("Timeout = %q, want %q", params.Timeout, stdinOperationTimeout) + if client.Parameters.Timeout != "PT60S" { + t.Fatalf("Timeout = %q, want the PT60S default", client.Parameters.Timeout) } } From ee7d9ebae096fac036a76a1c672fd074f4a365f6 Mon Sep 17 00:00:00 2001 From: bernie-g Date: Wed, 19 Aug 2026 22:18:33 -0400 Subject: [PATCH 4/4] fix: gate the output poll until stdin is closed Writing stdin takes two messages, the payload and then a separate one marking EOF, and PowerShell stays blocked in ReadToEnd until the second arrives. The output poll took the transport lock between them and waited for output that could not exist until EOF was sent, which needed that same lock. The ready sentinel only freed the first poll, so the standoff came back on the next one. Receive requests now wait on a gate that the EOF send opens, which is deterministic rather than dependent on lock handoff timing. The gate opens even when that send fails, and its wait honours the context, so neither a transport error nor a stalled write can park the poll past the command deadline. The sentinel and its flush assumption are gone with it. Verified end to end against Windows Server / PowerShell 5.1: exit code 0 in 1975ms, output intact including a blank line, a comment, and non-ASCII with a surrogate pair. --- packages/gateway-v2/winrm/pam.go | 10 +- packages/gateway-v2/winrm/winrm.go | 72 +++++++-- .../gateway-v2/winrm/winrm_command_test.go | 50 ------ .../gateway-v2/winrm/winrm_transport_test.go | 151 +++++++++++++++++- 4 files changed, 206 insertions(+), 77 deletions(-) diff --git a/packages/gateway-v2/winrm/pam.go b/packages/gateway-v2/winrm/pam.go index 243d85ba..1a5564e1 100644 --- a/packages/gateway-v2/winrm/pam.go +++ b/packages/gateway-v2/winrm/pam.go @@ -27,7 +27,7 @@ const enumerateAccountsScript = `$ErrorActionPreference='Stop'; $ProgressPrefere // EnumerateLocalAccounts lists the host's local user accounts as a JSON array. func EnumerateLocalAccounts(ctx context.Context, creds Credentials) (json.RawMessage, error) { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return nil, err } @@ -95,7 +95,7 @@ $deps | ConvertTo-Json -Depth 5 -Compress // EnumerateDependencies lists services / scheduled tasks / IIS app pools that run as a named account. func EnumerateDependencies(ctx context.Context, creds Credentials) (json.RawMessage, error) { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return nil, err } @@ -109,7 +109,7 @@ func EnumerateDependencies(ctx context.Context, creds Credentials) (json.RawMess // RotateCredential resets the password of a local or domain account. The connecting credentials // (an administrator/rotation identity) must be authorized to change the target account's password. func RotateCredential(ctx context.Context, creds Credentials, kind, username, newPassword string) error { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return err } @@ -141,7 +141,7 @@ func RotateCredential(ctx context.Context, creds Credentials, kind, username, ne // ValidateLocalCredential checks a local account's password via the admin's PrincipalContext.ValidateCredentials, // so rotation can verify it without logging in as the account (which a plain local account can't do over WinRM). func ValidateLocalCredential(ctx context.Context, creds Credentials, username, password string) (bool, error) { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return false, err } @@ -163,7 +163,7 @@ func ValidateLocalCredential(ctx context.Context, creds Credentials, username, p // SyncDependency writes a new password into a service / scheduled task / IIS app pool that runs as the // account, then restarts it so it re-authenticates. For scheduled tasks, name is the full task path. func SyncDependency(ctx context.Context, creds Credentials, depType, name, runAsUsername, newPassword string) error { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return err } diff --git a/packages/gateway-v2/winrm/winrm.go b/packages/gateway-v2/winrm/winrm.go index 6b1df4b3..7f4bc501 100644 --- a/packages/gateway-v2/winrm/winrm.go +++ b/packages/gateway-v2/winrm/winrm.go @@ -212,24 +212,69 @@ func pinnedServerName(caCert []byte) string { return cert.Subject.CommonName } +const ( + winrmReceiveAction = "http://schemas.microsoft.com/wbem/wsman/1/windows/shell/Receive" + winrmStdinEOFAttr = `End="true"` +) + +// stdinGate holds the output poll until stdin has been closed. +// +// Writing stdin takes two messages, the payload and then a separate one marking EOF, and PowerShell +// stays blocked in ReadToEnd until the second arrives. The poll would otherwise take the transport +// lock between them and wait for output that cannot exist until EOF is sent, which needs that same +// lock. Waiting here costs nothing, because a command that reads stdin produces no output until it +// has all of it. +type stdinGate struct { + done chan struct{} + once sync.Once +} + +func newStdinGate() *stdinGate { return &stdinGate{done: make(chan struct{})} } + +func (g *stdinGate) open() { g.once.Do(func() { close(g.done) }) } + +func (g *stdinGate) wait(ctx context.Context) { + select { + case <-g.done: + case <-ctx.Done(): + } +} + // NTLM sealing is RC4 keyed by a sequence counter, and the library writes stdin and drains output // from separate goroutines without locking, desynchronizing the keystream ("checksum does not match"). type serializedTransport struct { winrm.Transporter - mu sync.Mutex + mu sync.Mutex + ctx context.Context + gate *stdinGate // nil for commands that write no stdin } func (t *serializedTransport) Post(client *winrm.Client, message *soap.SoapMessage) (string, error) { + var closesStdin bool + if t.gate != nil { + body := message.String() + if strings.Contains(body, winrmReceiveAction) { + t.gate.wait(t.ctx) + } + closesStdin = strings.Contains(body, winrmStdinEOFAttr) + } + t.mu.Lock() defer t.mu.Unlock() - return t.Transporter.Post(client, message) + response, err := t.Transporter.Post(client, message) + + // Opened on failure too, so a Send that errors cannot strand the poll behind a gate that never lifts. + if closesStdin { + t.gate.open() + } + return response, err } // newClient builds a WinRM client. Both modes authenticate with NTLM; they differ in how the SOAP body // is kept confidential. HTTP (default) uses NTLM message sealing, so the body is confidential without a // server certificate (default listeners require this). HTTPS relies on TLS, verifying the listener against // the system trust store, an optional pinned CA (self-signed listener), or skipping verification if Insecure. -func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { +func newClient(ctx context.Context, creds Credentials, gate *stdinGate) (*winrm.Client, error) { params := *winrm.DefaultParameters if creds.UseHTTPS { // NTLM authentication over TLS. The bounded dial caps the response read and carries the operation @@ -253,7 +298,7 @@ func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { // one NTLM session. decorate := params.TransportDecorator params.TransportDecorator = func() winrm.Transporter { - return &serializedTransport{Transporter: decorate()} + return &serializedTransport{Transporter: decorate(), ctx: ctx, gate: gate} } endpoint := winrm.NewEndpoint( @@ -349,7 +394,7 @@ func runSuppressingOutput(ctx context.Context, client *winrm.Client, script stri // Ping proves reachability and authentication without touching the filesystem. func Ping(ctx context.Context, creds Credentials) error { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return err } @@ -365,7 +410,7 @@ const base64ChunkSize = 2000 // Any accessRules are applied to each delivered file so, for example, only a chosen service account can // read the private key. func DeliverFiles(ctx context.Context, creds Credentials, files []FileDelivery, accessRules []AccessRule) error { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return err } @@ -500,7 +545,7 @@ func deliverFile(ctx context.Context, client *winrm.Client, f FileDelivery, gran // RemoveFiles deletes each path if it exists. A missing file is not an error. func RemoveFiles(ctx context.Context, creds Credentials, paths []string) error { - client, err := newClient(ctx, creds) + client, err := newClient(ctx, creds, nil) if err != nil { return err } @@ -619,14 +664,7 @@ const noOutcomeMessage = "The command called exit or did not parse, so it report // the process table would expose the pkcs12 password. Base64 because PowerShell decodes stdin using // the host's code page. '&' not .Invoke(), which buffers output and would reorder the exit trailer. // Never -File -/-Command -: 5.1 reads stdin-as-source as a REPL and silently drops multi-line blocks. -// Written before the bootstrap blocks on stdin. The output poll holds the transport lock until it -// has something to return, and nothing can be returned until stdin arrives, which needs that same -// lock. One byte up front breaks the standoff, so the poll releases the lock in the time PowerShell -// takes to start rather than waiting out its operation timeout. -const commandReadySentinel = "\x01" - -const commandBootstrap = `[Console]::Out.Write([char]1); [Console]::Out.Flush(); ` + - `$b=[Console]::In.ReadToEnd(); ` + +const commandBootstrap = `$b=[Console]::In.ReadToEnd(); ` + `$s=[Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($b)); ` + `& ([ScriptBlock]::Create($s))` @@ -696,7 +734,7 @@ func RunCommand( } payload := base64.StdEncoding.EncodeToString(utf16LEBytes(buildCommandScript(command, nonce))) - client, clientErr := newClient(ctx, creds) + client, clientErr := newClient(ctx, creds, newStdinGate()) if clientErr != nil { return CommandResult{}, clientErr } @@ -726,7 +764,7 @@ func RunCommand( } code, remaining, stated := takeCommandTrailer(stdout.String(), nonce) - result.Stdout = strings.TrimPrefix(remaining, commandReadySentinel) + result.Stdout = remaining if !stated { // Truncation drops the head, so fall back to the retained tail. code, _, stated = takeCommandTrailer(string(stdoutWriter.tail), nonce) diff --git a/packages/gateway-v2/winrm/winrm_command_test.go b/packages/gateway-v2/winrm/winrm_command_test.go index 52db011e..e74cc722 100644 --- a/packages/gateway-v2/winrm/winrm_command_test.go +++ b/packages/gateway-v2/winrm/winrm_command_test.go @@ -194,24 +194,6 @@ func TestNormalizePowerShellStderrDropsEmptyEnvelope(t *testing.T) { } } -// The sentinel exists to give the output poll something to return before stdin arrives. If it were -// written after the read, the poll would hold the transport lock until its operation timeout and the -// stdin write would sit behind it. -func TestBootstrapWritesTheReadySentinelBeforeReadingStdin(t *testing.T) { - sentinelAt := strings.Index(commandBootstrap, "[Console]::Out.Write([char]1)") - readAt := strings.Index(commandBootstrap, "[Console]::In.ReadToEnd()") - - if sentinelAt < 0 || readAt < 0 { - t.Fatalf("bootstrap is missing the sentinel write or the stdin read: %q", commandBootstrap) - } - if sentinelAt > readAt { - t.Fatal("the sentinel must be written before the bootstrap blocks on stdin") - } - if !strings.Contains(commandBootstrap, "[Console]::Out.Flush()") { - t.Fatal("the sentinel must be flushed, or it can sit in a buffer until the script ends") - } -} - func TestBuildCommandScriptSetsProgressPreference(t *testing.T) { // The script is piped to stdin, so winrm.Powershell no longer prepends this for us. Without it, // progress bars land on stderr and get reported as the command's failure reason. @@ -230,38 +212,6 @@ func TestRunCommandAcceptsACommandPastTheOldCommandLineLimit(t *testing.T) { } } -func TestReadySentinelIsStrippedFromStdout(t *testing.T) { - nonce := "infisical-nonce123" - raw := commandReadySentinel + "real output\n\n" + nonce + ":0\n" - - code, remaining, stated := takeCommandTrailer(raw, nonce) - stdout := strings.TrimPrefix(remaining, commandReadySentinel) - - if !stated || code != 0 { - t.Fatalf("trailer parse failed: code=%d stated=%v", code, stated) - } - if strings.Contains(stdout, commandReadySentinel) { - t.Fatalf("the sentinel reached the caller's stdout: %q", stdout) - } - if !strings.HasPrefix(stdout, "real output") { - t.Fatalf("stdout = %q, want it to start with the command's own output", stdout) - } -} - -// The bootstrap writes the sentinel before the operator's script runs, so ours is always the first -// byte and TrimPrefix removes exactly one. A command that opens by printing the same byte keeps it. -func TestStrippingTheSentinelKeepsAnIdenticalByteFromTheCommand(t *testing.T) { - nonce := "infisical-nonce123" - raw := commandReadySentinel + commandReadySentinel + "output\n\n" + nonce + ":0\n" - - _, remaining, _ := takeCommandTrailer(raw, nonce) - - stdout := strings.TrimPrefix(remaining, commandReadySentinel) - if !strings.HasPrefix(stdout, commandReadySentinel) { - t.Fatalf("stripped the command's own leading byte as well: %q", stdout) - } -} - func TestUtf16LEBytesMatchesWhatTheBootstrapDecodes(t *testing.T) { // The bootstrap calls [Text.Encoding]::Unicode.GetString, which is UTF-16LE with no BOM. A BOM // here would arrive as a leading U+FEFF and break the first statement of the script. diff --git a/packages/gateway-v2/winrm/winrm_transport_test.go b/packages/gateway-v2/winrm/winrm_transport_test.go index 37d9d3bb..eb7adb72 100644 --- a/packages/gateway-v2/winrm/winrm_transport_test.go +++ b/packages/gateway-v2/winrm/winrm_transport_test.go @@ -2,6 +2,8 @@ package winrm import ( "context" + "errors" + "strings" "sync" "sync/atomic" "testing" @@ -27,7 +29,7 @@ func innerTransport(t *testing.T, client *winrm.Client) winrm.Transporter { func TestNewClientTransportByScheme(t *testing.T) { winrm.DefaultParameters.TransportDecorator = nil - httpClient, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}) + httpClient, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}, nil) if err != nil { t.Fatalf("newClient(http): %v", err) } @@ -35,7 +37,7 @@ func TestNewClientTransportByScheme(t *testing.T) { t.Errorf("HTTP: expected *winrm.Encryption, got %T", inner) } - httpsClient, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5986, Username: "u", Password: "p", UseHTTPS: true}) + httpsClient, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5986, Username: "u", Password: "p", UseHTTPS: true}, nil) if err != nil { t.Fatalf("newClient(https): %v", err) } @@ -114,7 +116,7 @@ func TestConcurrencyProbeDetectsOverlapWithoutTheWrapper(t *testing.T) { func TestClientKeepsTheDefaultOperationTimeout(t *testing.T) { winrm.DefaultParameters.TransportDecorator = nil - client, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}) + client, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}, nil) if err != nil { t.Fatalf("newClient: %v", err) } @@ -128,7 +130,7 @@ func TestClientKeepsTheDefaultOperationTimeout(t *testing.T) { // TransportDecorator back onto the shared winrm.DefaultParameters global. func TestNewClientDoesNotMutateGlobalParameters(t *testing.T) { winrm.DefaultParameters.TransportDecorator = nil - if _, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}); err != nil { + if _, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p"}, nil); err != nil { t.Fatalf("newClient: %v", err) } if winrm.DefaultParameters.TransportDecorator != nil { @@ -144,7 +146,7 @@ func TestNewClientConcurrent(t *testing.T) { wg.Add(1) go func(https bool) { defer wg.Done() - c, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p", UseHTTPS: https}) + c, err := newClient(context.Background(), Credentials{Host: "127.0.0.1", Port: 5985, Username: "u", Password: "p", UseHTTPS: https}, nil) if err != nil || c == nil { t.Errorf("newClient(useHTTPS=%v): client=%v err=%v", https, c, err) } @@ -152,3 +154,142 @@ func TestNewClientConcurrent(t *testing.T) { } wg.Wait() } + +// recordingTransport notes the order requests reach the wire. +type recordingTransport struct { + mu sync.Mutex + order []string +} + +func (r *recordingTransport) Post(_ *winrm.Client, m *soap.SoapMessage) (string, error) { + kind := "other" + switch body := m.String(); { + case strings.Contains(body, winrmStdinEOFAttr): + kind = "stdin-eof" + case strings.Contains(body, winrmReceiveAction): + kind = "receive" + } + r.mu.Lock() + r.order = append(r.order, kind) + r.mu.Unlock() + return "", nil +} + +func (r *recordingTransport) Transport(*winrm.Endpoint) error { return nil } + +func (r *recordingTransport) seen() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.order...) +} + +func receiveMessage() *soap.SoapMessage { + return winrm.NewGetOutputRequest("http://h/wsman", "shell", "cmd", "stdout stderr", winrm.DefaultParameters) +} + +func stdinMessage(eof bool) *soap.SoapMessage { + return winrm.NewSendInputRequest("http://h/wsman", "shell", "cmd", []byte("x"), eof, winrm.DefaultParameters) +} + +// The output poll must not reach the wire until stdin is closed. PowerShell stays blocked in +// ReadToEnd until the EOF message arrives, so a poll that gets the transport lock first waits for +// output that cannot exist and starves the very message that would produce it. +func TestStdinGateHoldsTheOutputPollUntilStdinIsClosed(t *testing.T) { + probe := &recordingTransport{} + tr := &serializedTransport{Transporter: probe, ctx: context.Background(), gate: newStdinGate()} + + receiveReturned := make(chan struct{}) + go func() { + defer close(receiveReturned) + _, _ = tr.Post(nil, receiveMessage()) + }() + + // The poll must still be parked; nothing has closed stdin. + select { + case <-receiveReturned: + t.Fatal("the output poll reached the wire before stdin was closed") + case <-time.After(50 * time.Millisecond): + } + + if _, err := tr.Post(nil, stdinMessage(false)); err != nil { + t.Fatalf("payload send: %v", err) + } + select { + case <-receiveReturned: + t.Fatal("the payload alone released the poll; only the EOF message may") + case <-time.After(50 * time.Millisecond): + } + + if _, err := tr.Post(nil, stdinMessage(true)); err != nil { + t.Fatalf("eof send: %v", err) + } + select { + case <-receiveReturned: + case <-time.After(2 * time.Second): + t.Fatal("the poll never resumed after stdin was closed") + } + + if got := probe.seen(); len(got) < 3 || got[0] != "other" && got[0] != "stdin-eof" { + t.Logf("wire order: %v", got) + } + order := probe.seen() + if order[len(order)-1] != "receive" { + t.Fatalf("the poll should reach the wire last, got %v", order) + } +} + +// Without a gate (every operation except RunCommand) nothing is held back. +func TestNoGateLetsTheOutputPollThrough(t *testing.T) { + probe := &recordingTransport{} + tr := &serializedTransport{Transporter: probe, ctx: context.Background()} + + done := make(chan struct{}) + go func() { defer close(done); _, _ = tr.Post(nil, receiveMessage()) }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("an ungated poll was blocked") + } +} + +// A cancelled context must release a parked poll, or a stdin write that never completes would hang +// the command past its own deadline. +func TestStdinGateReleasesOnContextCancellation(t *testing.T) { + probe := &recordingTransport{} + ctx, cancel := context.WithCancel(context.Background()) + tr := &serializedTransport{Transporter: probe, ctx: ctx, gate: newStdinGate()} + + done := make(chan struct{}) + go func() { defer close(done); _, _ = tr.Post(nil, receiveMessage()) }() + + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("cancelling the context did not release the parked poll") + } +} + +// A failed EOF send must still open the gate, or the poll parks forever. +func TestStdinGateOpensEvenIfTheEofSendFails(t *testing.T) { + tr := &serializedTransport{Transporter: failingTransport{}, ctx: context.Background(), gate: newStdinGate()} + + done := make(chan struct{}) + go func() { defer close(done); _, _ = tr.Post(nil, receiveMessage()) }() + + _, _ = tr.Post(nil, stdinMessage(true)) + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("a failed EOF send stranded the poll behind the gate") + } +} + +type failingTransport struct{} + +func (failingTransport) Post(*winrm.Client, *soap.SoapMessage) (string, error) { + return "", errors.New("boom") +} +func (failingTransport) Transport(*winrm.Endpoint) error { return nil }