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 01b79e25..7f4bc501 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,11 +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 + 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() + 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 @@ -234,6 +294,13 @@ func newClient(ctx context.Context, creds Credentials) (*winrm.Client, error) { params.TransportDecorator = func() winrm.Transporter { return enc } } + // 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(), ctx: ctx, gate: gate} + } + endpoint := winrm.NewEndpoint( creds.Host, creds.Port, @@ -327,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 } @@ -343,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 } @@ -478,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 } @@ -538,7 +605,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 +655,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 +715,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. 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 bootstrap goes on the command line; the script is +// piped over stdin. See commandBootstrap. func RunCommand( ctx context.Context, creds Credentials, @@ -641,19 +728,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, newStdinGate()) if clientErr != nil { return CommandResult{}, clientErr } @@ -669,7 +750,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..eb7adb72 100644 --- a/packages/gateway-v2/winrm/winrm_transport_test.go +++ b/packages/gateway-v2/winrm/winrm_transport_test.go @@ -2,31 +2,127 @@ package winrm import ( "context" + "errors" + "strings" "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) { 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) } - 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}) + 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) } - 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") + } +} + +// 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 + + 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) + } + + if client.Parameters.Timeout != "PT60S" { + t.Fatalf("Timeout = %q, want the PT60S default", client.Parameters.Timeout) } } @@ -34,7 +130,7 @@ func TestNewClientTransportByScheme(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 { @@ -50,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) } @@ -58,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 } diff --git a/packages/gateway-v2/winrm_handler.go b/packages/gateway-v2/winrm_handler.go index 64c7dcd7..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,7 +122,10 @@ const ( winrmConnDeadline = winrmOpDeadline + 15*time.Second maxWinrmRequestBodyBytes = 4 * 1024 * 1024 - maxWinrmCommandChars = 2048 + // 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 ) @@ -320,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) }