diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 156cde747..44ad7f30f 100644 --- a/cmd/auth/login/cmd.go +++ b/cmd/auth/login/cmd.go @@ -32,10 +32,9 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop // when authentication is intentionally disabled, say if the user is offline, or in // a CI/CD environment, or in a script. if viperx.GetBool(config.SkipAuthKey) { - err := errors.New("Login has been disabled via the '--skip-auth' flag.") - log.Error(err) + log.Error(errors.New("Login has been disabled via the '--skip-auth' flag.")) - return err + return cli.ErrSilent } var url string @@ -86,7 +85,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop cmd.SilenceUsage = true - return err + return cli.ErrSilent } if key == "" { @@ -101,7 +100,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop cmd.SilenceUsage = true - return err + return cli.ErrSilent } return nil diff --git a/cmd/auth/login/cmd_test.go b/cmd/auth/login/cmd_test.go new file mode 100644 index 000000000..a81761651 --- /dev/null +++ b/cmd/auth/login/cmd_test.go @@ -0,0 +1,48 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package login + +import ( + "testing" + + "github.com/datarobot/cli/internal/cli" + "github.com/datarobot/cli/internal/config" + "github.com/datarobot/cli/internal/config/viperx" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRunE_SkipAuthReturnsErrSilent covers the --skip-auth short circuit, the one +// login failure path that needs no network or config access. RunE logs the reason +// itself, so it must return cli.ErrSilent; a plain error would have main.go print +// the same thing again (CFX-6924). +func TestRunE_SkipAuthReturnsErrSilent(t *testing.T) { + viperx.Set(config.SkipAuthKey, true) + t.Cleanup(func() { viperx.Set(config.SkipAuthKey, false) }) + + err := RunE(&cobra.Command{}, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, cli.ErrSilent, + "the reason is already logged, so the error must not be printed again") +} + +// TestCmd_SilenceErrors documents that the command still opts out of cobra's +// printing. Reporting is centralized in main.go via cmd.ReportError, which honours +// cli.ErrSilent; this flag keeps cobra from adding a line of its own. +func TestCmd_SilenceErrors(t *testing.T) { + assert.True(t, Cmd().SilenceErrors) +} diff --git a/cmd/exit.go b/cmd/exit.go index 6ce413a51..6d9fc17dc 100644 --- a/cmd/exit.go +++ b/cmd/exit.go @@ -15,10 +15,31 @@ package cmd import ( + "errors" + "fmt" + "io" "os" "time" + + "github.com/datarobot/cli/internal/cli" ) +// ReportError writes a user-facing "Error: ..." line for err to w, unless err is +// cli.ErrSilent — the sentinel meaning a command already printed its own message. +// +// RootCmd sets SilenceErrors so cobra never prints errors itself, which makes this +// the single place errors reach the user. Previously cobra was the only reporter, +// and every command setting SilenceErrors: true silently swallowed failures raised +// by the root PersistentPreRunE (config loading, TLS setup) that it never had a +// chance to print — a bad --ca-cert path exited 1 with no output at all (CFX-6924). +func ReportError(w io.Writer, err error) { + if err == nil || errors.Is(err, cli.ErrSilent) { + return + } + + fmt.Fprintln(w, "Error:", err) +} + // Exit flushes any pending telemetry events then terminates the process with // code. Call this from main (instead of os.Exit) when ExecuteContext returns // an error, so that Amplitude events are delivered even though diff --git a/cmd/exit_test.go b/cmd/exit_test.go new file mode 100644 index 000000000..2eb770a35 --- /dev/null +++ b/cmd/exit_test.go @@ -0,0 +1,84 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "bytes" + "errors" + "fmt" + "testing" + + "github.com/datarobot/cli/internal/cli" + "github.com/stretchr/testify/assert" +) + +func TestReportError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want string + }{ + { + name: "nil error prints nothing", + err: nil, + want: "", + }, + { + name: "ErrSilent prints nothing because the command already reported it", + err: cli.ErrSilent, + want: "", + }, + { + name: "an error wrapping ErrSilent is still silent", + err: fmt.Errorf("auth check: %w", cli.ErrSilent), + want: "", + }, + { + name: "an ordinary error is reported", + err: errors.New("boom"), + want: "Error: boom\n", + }, + { + name: "a TLS setup failure from the root PersistentPreRunE is reported", + err: errors.New(`apply tls options: reading CA cert "C:\nope.pem": no such file or directory`), + want: "Error: apply tls options: reading CA cert \"C:\\nope.pem\": no such file or directory\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + ReportError(&buf, tt.err) + + assert.Equal(t, tt.want, buf.String()) + }) + } +} + +// TestRootSilencesErrorsForSingleReporting locks in the contract that makes +// ReportError the only error reporter. If cobra were allowed to print too, errors +// raised by the root PersistentPreRunE would either double-print or, for commands +// setting SilenceErrors: true, disappear entirely (CFX-6924). +func TestRootSilencesErrorsForSingleReporting(t *testing.T) { + t.Parallel() + + assert.True(t, RootCmd.SilenceErrors, + "RootCmd.SilenceErrors must stay true so cmd.ReportError is the single error reporter") +} diff --git a/cmd/root.go b/cmd/root.go index 9b476fab4..6c425fbe5 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -194,12 +194,13 @@ using pre-built templates. Get from idea to production in minutes, not hours. // ExecuteContext executes the root command with the given context. // It adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. +// The error is returned unwrapped on purpose. main.go reports it verbatim through +// cmd.ReportError, so an "execute root command: ..." wrapper would surface in +// user-facing output and add nothing — this is the process boundary, and there is no +// sibling call site an extra frame could disambiguate. func ExecuteContext(ctx context.Context) error { - if err := RootCmd.ExecuteContext(ctx); err != nil { - return fmt.Errorf("execute root command: %w", err) - } - - return nil + //nolint:wrapcheck // top-level passthrough; wrapping would leak into user output + return RootCmd.ExecuteContext(ctx) } // bindUniversal binds name to viper and annotates the flag for forwarding to @@ -224,6 +225,13 @@ func init() { // Disable Cobra's default completion command since we have our own under 'self' RootCmd.CompletionOptions.DisableDefaultCmd = true + // Silence cobra's own error printing so main.go, via cmd.ReportError, is the single + // reporter. Cobra skips printing whenever the executed command sets + // SilenceErrors: true, which used to mean errors raised by the root + // PersistentPreRunE (config, TLS) vanished entirely for those commands. Centralizing + // it also removes the risk of the same error being printed twice. See CFX-6924. + RootCmd.SilenceErrors = true + // Set custom version template to match our unified format RootCmd.SetVersionTemplate(internalVersion.GetAppNameVersionText() + "\n\nTo update: dr self update\n") diff --git a/cmd/tls_help.go b/cmd/tls_help.go new file mode 100644 index 000000000..9c2f14240 --- /dev/null +++ b/cmd/tls_help.go @@ -0,0 +1,41 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "fmt" + "io" +) + +// fprintNoWindowsCertsHelp explains what to do when --export-windows-certs finds an +// empty certificate store. The export itself worked; there is simply nothing to +// export, which is a machine-configuration problem the user can resolve. +// +// Kept build-tag free so it can be unit tested on any host, although only the +// Windows build calls it. +func fprintNoWindowsCertsHelp(w io.Writer) { + fmt.Fprintln(w, "The Windows certificate store contains no Root or CA certificates to export.") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " Check what is installed:") + fmt.Fprintln(w, " Get-ChildItem Cert:\\LocalMachine\\Root") + fmt.Fprintln(w, " Get-ChildItem Cert:\\CurrentUser\\Root") + fmt.Fprintln(w, " or open certlm.msc (machine) / certmgr.msc (user).") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " If your organization's root CA is missing, import it and retry:") + fmt.Fprintln(w, " Import-Certificate -FilePath -CertStoreLocation Cert:\\CurrentUser\\Root") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " To trust a PEM bundle directly instead of the Windows store:") + fmt.Fprintln(w, " dr --ca-cert ") +} diff --git a/cmd/tls_help_test.go b/cmd/tls_help_test.go new file mode 100644 index 000000000..879ff32ec --- /dev/null +++ b/cmd/tls_help_test.go @@ -0,0 +1,49 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestFprintNoWindowsCertsHelp checks the guidance is actionable rather than merely +// restating the failure: it must name how to inspect the store, how to import a CA, +// and the --ca-cert escape hatch. +func TestFprintNoWindowsCertsHelp(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + fprintNoWindowsCertsHelp(&buf) + + out := buf.String() + + for _, want := range []string{ + "no Root or CA certificates", + `Cert:\LocalMachine\Root`, + `Cert:\CurrentUser\Root`, + "certlm.msc", + "Import-Certificate", + "dr --ca-cert", + } { + assert.Contains(t, out, want) + } + + assert.True(t, strings.HasSuffix(out, "\n"), "guidance must end with a newline") +} diff --git a/cmd/tls_windows.go b/cmd/tls_windows.go index 670ce550d..56fa0dfd4 100644 --- a/cmd/tls_windows.go +++ b/cmd/tls_windows.go @@ -17,9 +17,12 @@ package cmd import ( + "errors" "fmt" + "os" "path/filepath" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" internaltls "github.com/datarobot/cli/internal/tls" "github.com/spf13/cobra" @@ -54,6 +57,17 @@ func applyWindowsCerts(cmd *cobra.Command, caCert *string) error { } if err := internaltls.ExportWindowsCerts(dest); err != nil { + // An empty store is fixable by the user, so report it and say how. Both go to + // stderr, keeping stdout parseable under --output-format json. Reporting here + // rather than letting main.go do it keeps the guidance below its own error + // message instead of above it; ErrSilent then suppresses the second print. + if errors.Is(err, internaltls.ErrNoWindowsCerts) { + ReportError(os.Stderr, fmt.Errorf("--export-windows-certs: %w", err)) + fprintNoWindowsCertsHelp(os.Stderr) + + return cli.ErrSilent + } + return fmt.Errorf("--export-windows-certs: %w", err) } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0ed525fd2..caae3e526 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -26,6 +26,7 @@ import ( "testing" "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/log" @@ -309,11 +310,16 @@ func VerifyEnvCredentials(ctx context.Context) (*EnvCredentials, error) { // EnsureAuthenticatedE checks if valid authentication exists, and if not, // triggers the login flow automatically (see EnsureAuthenticated for the -// exceptions). Returns an error if authentication fails, suitable for use in -// Cobra PreRunE hooks. +// exceptions). Suitable for use in Cobra PreRunE hooks. +// +// On failure it returns cli.ErrSilent, not a descriptive error: EnsureAuthenticated +// has already told the user what went wrong, and main.go prints anything else. func EnsureAuthenticatedE(cmd *cobra.Command, _ []string) error { if !EnsureAuthenticated(cmd.Context()) { - return errors.New("authentication failed") + // Every path where EnsureAuthenticated reports false has already written a + // user-facing message, so signal that rather than returning a fresh error + // main.go would print a second time. + return cli.ErrSilent } return nil diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index ffe3e7109..0cdb4ec2c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -26,9 +26,11 @@ import ( "path/filepath" "testing" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/testutil" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -814,3 +816,47 @@ func TestVerifyEnvCredentials(t *testing.T) { assert.Equal(t, "valid-token", creds.Token) }) } + +// TestEnsureAuthenticatedE_ReturnsErrSilent locks in the reporting contract: +// EnsureAuthenticated writes a user-facing message on every failure path, so the +// PreRunE wrapper must return cli.ErrSilent rather than a fresh error. Returning a +// plain error would make main.go print a second line for something the user was +// already told about (CFX-6924). +func TestEnsureAuthenticatedE_ReturnsErrSilent(t *testing.T) { + server, cleanup := setupTestEnvironment(t) + defer cleanup() + + // A complete but invalid env pair fails without falling back or starting a + // login flow, and reports the reason itself. + t.Setenv("DATAROBOT_ENDPOINT", server.URL+"/api/v2") + t.Setenv("DATAROBOT_API_TOKEN", "expired-token") + + APIKeyCallbackFunc = func(_ context.Context, _ string) (string, error) { + t.Error("login flow must not start when explicit env credentials fail") + + return "", errors.New("unexpected login flow") + } + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + err := EnsureAuthenticatedE(cmd, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, cli.ErrSilent, + "authentication failures are already reported, so the error must be silent") +} + +// TestEnsureAuthenticatedE_NilOnSuccess is the companion case: a valid stored +// profile must produce no error at all. +func TestEnsureAuthenticatedE_NilOnSuccess(t *testing.T) { + _, cleanup := setupTestEnvironment(t) + defer cleanup() + + viperx.Set(config.DataRobotAPIKey, "valid-token") + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + assert.NoError(t, EnsureAuthenticatedE(cmd, nil)) +} diff --git a/internal/tls/export_output.go b/internal/tls/export_output.go new file mode 100644 index 000000000..7b7495aa3 --- /dev/null +++ b/internal/tls/export_output.go @@ -0,0 +1,70 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "fmt" + "strconv" + "strings" +) + +// certCountPrefix marks the first stdout line of the export script, which reports how +// many certificates were enumerated before any encoding was attempted. +const certCountPrefix = "CERTCOUNT=" + +// No build constraint: only the Windows build calls these, but the parsing is where a +// regression would quietly bring back the misleading "no certificates found" message, +// so it is unit tested on every host. + +// parseExportOutput splits the export script's stdout into its CERTCOUNT header and +// the PEM body. The header is what lets the caller tell an empty store apart from a +// store it could not encode. +func parseExportOutput(raw string) (pem string, count int, err error) { + trimmed := strings.TrimSpace(raw) + + header, body, _ := strings.Cut(trimmed, "\n") + header = strings.TrimSpace(header) + + if !strings.HasPrefix(header, certCountPrefix) { + return "", 0, fmt.Errorf( + "exporting Windows cert store: unexpected output from powershell.exe: %s", + truncateForError(trimmed), + ) + } + + count, convErr := strconv.Atoi(strings.TrimPrefix(header, certCountPrefix)) + if convErr != nil { + return "", 0, fmt.Errorf( + "exporting Windows cert store: unreadable certificate count %q: %w", header, convErr, + ) + } + + return strings.TrimSpace(body), count, nil +} + +// truncateForError keeps unexpected subprocess output short enough to read in a +// terminal while still showing what came back. +func truncateForError(s string) string { + const maxLen = 200 + + s = strings.ReplaceAll(s, "\r\n", " ") + s = strings.ReplaceAll(s, "\n", " ") + + if len(s) > maxLen { + return strconv.Quote(s[:maxLen] + "…") + } + + return strconv.Quote(s) +} diff --git a/internal/tls/export_output_test.go b/internal/tls/export_output_test.go new file mode 100644 index 000000000..d7d56f7ad --- /dev/null +++ b/internal/tls/export_output_test.go @@ -0,0 +1,129 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const oneCert = "-----BEGIN CERTIFICATE-----\nAAAA\n-----END CERTIFICATE-----" + +func TestParseExportOutput(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw string + wantCount int + wantPEM string + wantErr bool + }{ + { + name: "count and body", + raw: "CERTCOUNT=2\n" + oneCert, + wantCount: 2, + wantPEM: oneCert, + }, + { + name: "empty store reports zero with no body", + raw: "CERTCOUNT=0\n", + wantCount: 0, + wantPEM: "", + }, + { + name: "certificates enumerated but nothing encoded", + raw: "CERTCOUNT=7\n", + wantCount: 7, + wantPEM: "", + }, + { + name: "CRLF line endings from powershell.exe", + raw: "CERTCOUNT=1\r\n" + oneCert + "\r\n", + wantCount: 1, + wantPEM: oneCert, + }, + { + name: "surrounding whitespace is tolerated", + raw: "\n CERTCOUNT=3 \n" + oneCert + "\n\n", + wantCount: 3, + wantPEM: oneCert, + }, + { + name: "missing header is an error, not a silent zero", + raw: oneCert, + wantErr: true, + }, + { + name: "empty output is an error, not an empty store", + raw: "", + wantErr: true, + }, + { + name: "non-numeric count", + raw: "CERTCOUNT=lots\n" + oneCert, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pem, count, err := parseExportOutput(tt.raw) + + if tt.wantErr { + require.Error(t, err) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantCount, count) + assert.Equal(t, tt.wantPEM, pem) + }) + } +} + +// TestParseExportOutputDistinguishesEmptyFromUnencodable is the case that motivated +// the header: an empty store must be told apart from a populated store whose +// certificates could not be encoded, because the advice for each is opposite. +func TestParseExportOutputDistinguishesEmptyFromUnencodable(t *testing.T) { + t.Parallel() + + _, empty, err := parseExportOutput("CERTCOUNT=0\n") + require.NoError(t, err) + + pem, populated, err := parseExportOutput("CERTCOUNT=31\n") + require.NoError(t, err) + + assert.Zero(t, empty, "an empty store must report zero") + assert.Equal(t, 31, populated, "a populated store must report its count even with no PEM body") + assert.Empty(t, pem) +} + +func TestTruncateForError(t *testing.T) { + t.Parallel() + + assert.Equal(t, `"short output"`, truncateForError("short output")) + assert.Equal(t, `"a b c"`, truncateForError("a\r\nb\nc"), "newlines collapse to spaces for one-line errors") + + long := truncateForError(strings.Repeat("x", 500)) + assert.Contains(t, long, "…") + assert.Less(t, len(long), 250, "long output must be truncated") +} diff --git a/internal/tls/psmodulepath.go b/internal/tls/psmodulepath.go new file mode 100644 index 000000000..a3be779e8 --- /dev/null +++ b/internal/tls/psmodulepath.go @@ -0,0 +1,73 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import "strings" + +// psModulePathKey is the environment variable telling PowerShell where to look for +// modules. Matched case-insensitively, since Windows environment variable names are. +const psModulePathKey = "PSMODULEPATH" + +// defaultSystemRoot is the fallback when SystemRoot is absent from the environment. +const defaultSystemRoot = `C:\Windows` + +// These helpers deliberately carry no build constraint so they can be unit tested on +// any host, even though only the Windows build calls them. + +// windowsPowerShellModulePath returns the Windows PowerShell 5.1 system module +// directory under systemRoot. That directory holds Microsoft.PowerShell.Security, +// the module providing the Cert: drive — without it on PSModulePath, powershell.exe +// cannot enumerate the certificate stores at all. +// +// The path is assembled with backslashes rather than filepath.Join so the result is +// identical no matter which OS runs the tests. +func windowsPowerShellModulePath(systemRoot string) string { + if systemRoot == "" { + systemRoot = defaultSystemRoot + } + + return strings.Join([]string{ + strings.TrimRight(systemRoot, `\`), + "System32", + "WindowsPowerShell", + "v1.0", + "Modules", + }, `\`) +} + +// withPSModulePath returns env with every PSModulePath entry replaced by a single +// entry pointing at modulePath. +// +// A child powershell.exe otherwise inherits the parent's PSModulePath, and a value +// omitting the Windows PowerShell system module directory stops +// Microsoft.PowerShell.Security from autoloading. The Cert: drive then does not +// exist, the export yields nothing, and the failure surfaces as the misleading +// "no certificates found in Windows cert store" (CFX-6924). pwsh 7 sets exactly +// such a value, and CI runners frequently mangle it. +func withPSModulePath(env []string, modulePath string) []string { + out := make([]string, 0, len(env)+1) + + for _, entry := range env { + key, _, found := strings.Cut(entry, "=") + + if found && strings.EqualFold(key, psModulePathKey) { + continue + } + + out = append(out, entry) + } + + return append(out, "PSModulePath="+modulePath) +} diff --git a/internal/tls/psmodulepath_test.go b/internal/tls/psmodulepath_test.go new file mode 100644 index 000000000..f6dfba5b9 --- /dev/null +++ b/internal/tls/psmodulepath_test.go @@ -0,0 +1,134 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWindowsPowerShellModulePath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + systemRoot string + want string + }{ + { + name: "typical SystemRoot", + systemRoot: `C:\WINDOWS`, + want: `C:\WINDOWS\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "empty SystemRoot falls back to C:\\Windows", + systemRoot: "", + want: `C:\Windows\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "trailing separator is not doubled", + systemRoot: `D:\Windows\`, + want: `D:\Windows\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "relocated system root", + systemRoot: `E:\CustomWin`, + want: `E:\CustomWin\System32\WindowsPowerShell\v1.0\Modules`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, windowsPowerShellModulePath(tt.systemRoot)) + }) + } +} + +func TestWithPSModulePath(t *testing.T) { + t.Parallel() + + const pinned = `C:\WINDOWS\System32\WindowsPowerShell\v1.0\Modules` + + tests := []struct { + name string + env []string + want []string + }{ + { + name: "replaces an inherited pwsh 7 value", + env: []string{ + "PATH=C:\\bin", + `PSModulePath=C:\Program Files\PowerShell\7\Modules`, + "SystemRoot=C:\\WINDOWS", + }, + want: []string{"PATH=C:\\bin", "SystemRoot=C:\\WINDOWS", "PSModulePath=" + pinned}, + }, + { + name: "matches the key case-insensitively", + env: []string{"psmodulepath=junk", "PATH=C:\\bin"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "adds the entry when absent", + env: []string{"PATH=C:\\bin"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "drops every duplicate, not just the first", + env: []string{"PSModulePath=a", "PATH=C:\\bin", "PSMODULEPATH=b"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "empty environment still yields the pinned entry", + env: nil, + want: []string{"PSModulePath=" + pinned}, + }, + { + name: "entries without a separator are preserved", + env: []string{"MALFORMED", "PSModulePath=x"}, + want: []string{"MALFORMED", "PSModulePath=" + pinned}, + }, + { + name: "a key merely prefixed with PSModulePath is left alone", + env: []string{"PSModulePathExtra=keep", "PSModulePath=drop"}, + want: []string{"PSModulePathExtra=keep", "PSModulePath=" + pinned}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, withPSModulePath(tt.env, pinned)) + }) + } +} + +// TestWithPSModulePathDoesNotMutateInput guards against aliasing: os.Environ()'s +// slice must not be rewritten underneath the caller. +func TestWithPSModulePathDoesNotMutateInput(t *testing.T) { + t.Parallel() + + env := []string{"PATH=C:\\bin", "PSModulePath=original"} + before := make([]string, len(env)) + copy(before, env) + + withPSModulePath(env, "pinned") + + assert.Equal(t, before, env) +} diff --git a/internal/tls/tls_windows.go b/internal/tls/tls_windows.go index 5b4021080..95eed7b33 100644 --- a/internal/tls/tls_windows.go +++ b/internal/tls/tls_windows.go @@ -24,34 +24,92 @@ import ( "strings" ) +// ErrNoWindowsCerts reports that enumerating the certificate stores succeeded but +// found nothing. It is a sentinel so callers can offer guidance: an empty store is a +// machine-configuration problem the user can act on, unlike a failure to reach the +// store at all, which the script reports separately. +var ErrNoWindowsCerts = errors.New( + "no certificates found in the Windows certificate store: " + + "Root and CA are empty for both LocalMachine and CurrentUser", +) + +// ErrCertEncodeFailed reports that the stores did contain certificates but none +// could be encoded. Distinguishing this from ErrNoWindowsCerts matters: telling a +// user to import a CA when their store is already populated sends them down exactly +// the wrong path, which is how the original bug wasted so much time. +var ErrCertEncodeFailed = errors.New("could not encode any certificate from the Windows certificate store") + // ExportWindowsCerts exports Root and CA certificates from the Windows // certificate store and writes them as a PEM bundle to dest. func ExportWindowsCerts(dest string) error { script := strings.Join([]string{ + // Load the module providing the Cert: drive explicitly and fail loudly when it + // is unavailable. Relying on autoloading turned a broken PSModulePath into a + // silent empty result that reported itself as "no certificates found". + `Import-Module Microsoft.PowerShell.Security -ErrorAction Stop`, + `if (-not (Get-PSDrive -PSProvider Certificate -ErrorAction SilentlyContinue)) ` + + `{ throw 'Cert: drive unavailable - Microsoft.PowerShell.Security did not load' }`, + // A restricted language mode forbids the [Convert] call below. Without this + // check the enumeration would succeed, every encode would fail, and the empty + // result would masquerade as an empty certificate store. + `if ($ExecutionContext.SessionState.LanguageMode -ne 'FullLanguage') ` + + `{ throw "PowerShell language mode is $($ExecutionContext.SessionState.LanguageMode), ` + + `which forbids certificate encoding; FullLanguage is required" }`, `$out = @()`, + `$total = 0`, `foreach ($store in 'Root','CA') {`, ` foreach ($loc in 'LocalMachine','CurrentUser') {`, - ` Get-ChildItem -Path "Cert:\$loc\$store" -ErrorAction SilentlyContinue | ForEach-Object {`, + // SilentlyContinue is kept here on purpose: an individual store may legitimately + // be empty or absent. The precondition above distinguishes that from the whole + // certificate provider being missing. + ` $items = @(Get-ChildItem -Path "Cert:\$loc\$store" -ErrorAction SilentlyContinue)`, + ` $total += $items.Count`, + ` $items | ForEach-Object {`, ` $out += '-----BEGIN CERTIFICATE-----'`, ` $out += [Convert]::ToBase64String($_.RawData, 'InsertLineBreaks')`, ` $out += '-----END CERTIFICATE-----'`, ` }`, ` }`, `}`, + `"CERTCOUNT=$total"`, `$out -join [Environment]::NewLine`, }, "; ") - out, err := exec.Command( + cmd := exec.Command( "powershell.exe", "-NoProfile", "-NonInteractive", "-Command", script, - ).Output() + ) + + // Pin PSModulePath instead of inheriting it; see withPSModulePath for why. + cmd.Env = withPSModulePath( + os.Environ(), + windowsPowerShellModulePath(os.Getenv("SystemRoot")), + ) + + out, err := cmd.Output() if err != nil { + // cmd.Output captures the child's stderr into ExitError.Stderr. Surfacing it is + // the difference between "something went wrong" and a message naming the cause. + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { + if stderr := strings.TrimSpace(string(exitErr.Stderr)); stderr != "" { + return fmt.Errorf("exporting Windows cert store: %w: %s", err, stderr) + } + } + return fmt.Errorf("exporting Windows cert store: %w", err) } - pem := strings.TrimSpace(string(out)) + pem, count, err := parseExportOutput(string(out)) + if err != nil { + return err + } + + // Only claim the store is empty when the script actually enumerated nothing. + if count == 0 { + return ErrNoWindowsCerts + } if pem == "" { - return errors.New("no certificates found in Windows cert store") + return fmt.Errorf("%w: enumerated %d certificate(s) but encoded none", ErrCertEncodeFailed, count) } if err := os.WriteFile(dest, []byte(pem+"\n"), 0o600); err != nil { diff --git a/main.go b/main.go index d157b3f07..addac82e3 100644 --- a/main.go +++ b/main.go @@ -34,6 +34,11 @@ func main() { defer log.Stop() if err := cmd.ExecuteContext(ctx); err != nil { + // RootCmd sets SilenceErrors, so nothing has printed this yet. Report it here + // before exiting; ReportError skips cli.ErrSilent, which commands return once + // they have printed their own message. + cmd.ReportError(os.Stderr, err) + log.Stop() cmd.Exit(1) }