Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,9 @@ go.sum text eol=lf
# generated keeps it out of GitHub language stats and collapses it
# in PR diffs by default.
pkg/generated/** linguist-generated

# Golden render snapshots are regenerated mechanically via
# `TALM_UPDATE_GOLDEN=1 go test ./pkg/engine/ -run TestGoldenRender`,
# not hand-edited. Marking them generated keeps the rendered YAML dumps
# out of language stats and collapses them in PR diffs by default.
pkg/engine/testdata/golden/** linguist-generated
22 changes: 22 additions & 0 deletions docs/manual-test-plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,28 @@ Expected (per node): `node 192.0.2.10: talm: drift verification unavailable on m

Regression anchor: a refactor that always-prefixes (`node : talm: ...` on single-node) is a UX regression. The `nodePrefix("")` helper must collapse to empty for the bare-line single-node case.

### D4. TLS `--skip-verify` — native honored, wrapped passthrough warned

`--skip-verify` skips server-certificate verification while preserving client-certificate authentication, for connecting to a node whose IP is absent from the server cert SANs. It is reimplemented in talm (`WithClientSkipVerify` in `pkg/commands/root.go`) now that the cozystack/talos fork is dropped, and its coverage differs by command kind.

Native command (apply / template / upgrade / rotate-ca) — the flag is honored. Run against a node reachable by an IP that is NOT in the server cert SANs:

```bash
talm apply --dry-run --skip-verify --nodes 192.0.2.10 -f nodes/node0.yaml
```

Expected: the connection succeeds (no `x509: certificate is valid for … not 192.0.2.10` TLS SAN error) and the dry-run render proceeds.

Wrapped talosctl passthrough command (health / get / dashboard / …) — the flag is NOT supported, and the command says so instead of failing opaquely:

```bash
talm health --skip-verify --nodes 192.0.2.10
```

Expected (stderr): `Warning: --skip-verify is not supported for the wrapped talosctl command "health"; it applies only to talm-native commands (apply, template, upgrade, rotate-ca). Connecting with full TLS verification.` — the command then connects with full verification (and fails against an out-of-SAN IP, as it would without the flag).

Regression anchor: `pkg/commands/skip_verify_test.go` pins the TLS config (skip server verify + client cert preserved), the native routing (`WithClientNoNodes` / `WithClient` reach `WithClientSkipVerify`), the `--cluster` and dial-option threading, and the passthrough warning. A change that silently no-ops `--skip-verify` on native commands, or drops the passthrough warning, is a regression.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
## E. Upgrade

### E1. Stage an upgrade to the same image
Expand Down
274 changes: 128 additions & 146 deletions go.mod

Large diffs are not rendered by default.

554 changes: 265 additions & 289 deletions go.sum

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func registerRootFlags(cmd *cobra.Command) {
cmd.PersistentFlags().StringSliceVar(&commands.GlobalArgs.Nodes, "nodes", []string{}, "target the specified nodes")
cmd.PersistentFlags().StringSliceVarP(&commands.GlobalArgs.Endpoints, "endpoints", "e", []string{}, "override default endpoints in Talos configuration")
cmd.PersistentFlags().StringVar(&commands.GlobalArgs.Cluster, "cluster", "", "Cluster to connect to if a proxy endpoint is used.")
cmd.PersistentFlags().BoolVar(&commands.GlobalArgs.SkipVerify, "skip-verify", false, "skip TLS certificate verification (keeps client authentication)")
cmd.PersistentFlags().BoolVar(&commands.SkipVerify, "skip-verify", false, "skip TLS certificate verification (keeps client authentication)")
cmd.PersistentFlags().Bool("version", false, "Print the version number of the application")
// No backticks in this usage string: pflag's UnquoteUsage treats the
// first backtick-quoted word as the flag's value-placeholder name, which
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ func withApplyClientBare(action func(ctx context.Context, c *client.Client) erro
return WithClientMaintenance(applyCmdFlags.certFingerprints, action)
}

if GlobalArgs.SkipVerify {
if SkipVerify {
return WithClientSkipVerify(action)
}

Expand Down
157 changes: 147 additions & 10 deletions pkg/commands/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@ package commands

import (
"context"
"crypto/tls"
"encoding/base64"
"os"
"os/signal"
"path/filepath"
"syscall"
"time"

"github.com/cockroachdb/errors"
Expand All @@ -27,13 +31,67 @@ import (

"github.com/siderolabs/talos/cmd/talosctl/pkg/talos/global"
"github.com/siderolabs/talos/pkg/machinery/client"
clientconfig "github.com/siderolabs/talos/pkg/machinery/client/config"
)

// GlobalArgs is the common arguments for the root command.
//
//nolint:gochecknoglobals // cobra CLI architecture: persistent flags bind to package-level state shared across all subcommands; refactoring out the global would require threading state through every command's RunE.
var GlobalArgs global.Args

// SkipVerify, when set via --skip-verify, disables TLS certificate verification
// while preserving client-certificate authentication. Upstream global.Args has no
// such field (the cozystack/talos fork added it for siderolabs/talos#12652, which
// upstream declined); it is reimplemented here so talm can drop the fork and track
// stock upstream Talos.
//
//nolint:gochecknoglobals // cobra CLI architecture: persistent flag binds to package-level state.
var SkipVerify bool

// errContextNotFound is returned when the requested context is absent from the
// talosconfig.
var errContextNotFound = errors.New("context not found in talosconfig")

// signalContext returns a context cancelled on SIGINT/SIGTERM so a --skip-verify
// client connection can be interrupted cleanly, mirroring talosctl's own wrappers.
func signalContext() (context.Context, context.CancelFunc) {
return signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
}

// skipVerifyTLSConfig builds a TLS config that skips server-certificate
// verification while preserving client-certificate authentication taken from the
// talosconfig context.
func skipVerifyTLSConfig(configContext *clientconfig.Context) (*tls.Config, error) {
// InsecureSkipVerify is the whole point of --skip-verify (connect to nodes
// whose IP is absent from the cert SANs); client-cert auth is preserved below.
tlsConfig := &tls.Config{
InsecureSkipVerify: true, //nolint:gosec // intentional: --skip-verify
}

if configContext.Crt == "" || configContext.Key == "" {
return tlsConfig, nil
}
Comment on lines +71 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Inconsistent handling of a partial cert/key pair.

If only one of Crt/Key is set, this silently returns a TLS config with no client certificate instead of erroring — unlike the invalid-base64 and mismatched-keypair cases just below, which fail loudly. A talosconfig context with a typo'd/half-populated cert pair would silently lose client-cert auth and surface as an opaque downstream authorization failure instead of a clear config error.

🛡️ Proposed fix: treat "exactly one present" as an error too
-	if configContext.Crt == "" || configContext.Key == "" {
+	if configContext.Crt == "" && configContext.Key == "" {
 		return tlsConfig, nil
 	}
+
+	if configContext.Crt == "" || configContext.Key == "" {
+		return nil, errors.New("talosconfig context has only one of client certificate/key set")
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if configContext.Crt == "" || configContext.Key == "" {
return tlsConfig, nil
}
if configContext.Crt == "" && configContext.Key == "" {
return tlsConfig, nil
}
if configContext.Crt == "" || configContext.Key == "" {
return nil, errors.New("talosconfig context has only one of client certificate/key set")
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/commands/root.go` around lines 71 - 73, Update the TLS configuration
logic around configContext.Crt and configContext.Key to distinguish an entirely
absent certificate/key pair from a partial pair. Return the existing empty TLS
configuration only when both are unset; when exactly one is set, return a clear
configuration error before the invalid-base64 and key-pair validation paths.


crtBytes, err := base64.StdEncoding.DecodeString(configContext.Crt)
if err != nil {
return nil, errors.Wrap(err, "decoding client certificate from talosconfig context")
}

keyBytes, err := base64.StdEncoding.DecodeString(configContext.Key)
if err != nil {
return nil, errors.Wrap(err, "decoding client key from talosconfig context")
}

cert, err := tls.X509KeyPair(crtBytes, keyBytes)
if err != nil {
return nil, errors.Wrap(err, "loading client key pair from talosconfig context")
}

tlsConfig.Certificates = []tls.Certificate{cert}

return tlsConfig, nil
}

// Config is the package-level configuration populated from Chart.yaml and
// CLI persistent flags. Mirrors GlobalArgs for project-root-relative path
// resolution across every subcommand.
Expand Down Expand Up @@ -84,7 +142,18 @@ var Config struct {
// WithClientNoNodes wraps common code to initialize Talos client and provide cancellable context.
//
// WithClientNoNodes doesn't set any node information on the request context.
//
// This is the single choke point every talm-native command funnels through
// (directly or via WithClient), so routing --skip-verify here restores the
// coverage the dropped cozystack/talos fork used to provide at the library
// level for the whole CLI. Wrapped talosctl passthrough commands run upstream
// RunE code that never reaches this function, so they are handled separately
// (and cannot honor --skip-verify without the fork — see talosctl_wrapper.go).
func WithClientNoNodes(action func(context.Context, *client.Client) error, dialOptions ...grpc.DialOption) error {
if SkipVerify {
return WithClientSkipVerify(action, dialOptions...)
}

//nolint:wrapcheck // thin pass-through to talos global.Args; error already carries Talos context
return GlobalArgs.WithClientNoNodes(action, dialOptions...)
}
Expand Down Expand Up @@ -119,22 +188,90 @@ func WithClientMaintenance(enforceFingerprints []string, action func(context.Con
return GlobalArgs.WithClientMaintenance(enforceFingerprints, action)
}

// skipVerifyClientOptions assembles the client options for a --skip-verify
// connection. It mirrors upstream global.Args.WithClientNoNodes so the skip
// path does not silently lose behavior the normal path has: it pins the
// already-resolved config context (so client.GetConfigContext honors
// --talosconfig / --context instead of falling back to the default config),
// forwards caller dial options, threads the --cluster proxy override when set,
// and selects endpoints from flags or the talosconfig context — all carrying
// the verification-skipping TLS config built from that context.
func skipVerifyClientOptions(configContext *clientconfig.Context, tlsConfig *tls.Config, dialOptions []grpc.DialOption) []client.OptionFunc {
opts := []client.OptionFunc{
client.WithConfigContext(configContext),
client.WithTLSConfig(tlsConfig),
client.WithDefaultGRPCDialOptions(),
// Kept for structural parity with upstream WithClientNoNodes. It is
// inert on the skip-verify path — getConn short-circuits on the explicit
// TLS config before the SideroV1 interceptor — so it never actually
// consumes the keys dir here; skip-verify + Omni SaaS-key auth is not a
// real combination.
client.WithSideroV1KeysDir(clientconfig.CustomSideroV1KeysDirPath(GlobalArgs.SideroV1KeysDir)),
}

if len(dialOptions) > 0 {
opts = append(opts, client.WithGRPCDialOptions(dialOptions...))
}

// Preserve the --cluster proxy header the non-skip path sets, so
// `--skip-verify --cluster X` still reaches nodes behind an Omni/Sidero proxy.
if GlobalArgs.Cluster != "" {
opts = append(opts, client.WithCluster(GlobalArgs.Cluster))
}

if len(GlobalArgs.Endpoints) > 0 {
opts = append(opts, client.WithEndpoints(GlobalArgs.Endpoints...))
} else if len(configContext.Endpoints) > 0 {
opts = append(opts, client.WithEndpoints(configContext.Endpoints...))
}

return opts
}

// WithClientSkipVerify wraps common code to initialize Talos client with TLS verification disabled
// but with client certificate authentication preserved.
// This is useful when connecting to nodes via IP addresses not listed in the server certificate's SANs.
func WithClientSkipVerify(action func(context.Context, *client.Client) error) error {
//nolint:wrapcheck // thin pass-through to talos global.Args; error already carries Talos context
return GlobalArgs.WithClientSkipVerify(action)
}
func WithClientSkipVerify(action func(context.Context, *client.Client) error, dialOptions ...grpc.DialOption) error {
ctx, stop := signalContext()
defer stop()

// WithClientAuto automatically selects the appropriate client wrapper based on GlobalArgs.SkipVerify.
// If SkipVerify is true, uses WithClientSkipVerify, otherwise uses WithClientNoNodes.
func WithClientAuto(action func(context.Context, *client.Client) error) error {
if GlobalArgs.SkipVerify {
return WithClientSkipVerify(action)
cfg, err := clientconfig.Open(GlobalArgs.Talosconfig)
if err != nil {
return errors.Wrapf(err, "opening talosconfig %q", GlobalArgs.Talosconfig)
}

contextName := GlobalArgs.CmdContext
if contextName == "" {
contextName = cfg.Context
}

configContext, ok := cfg.Contexts[contextName]
if !ok {
//nolint:wrapcheck // cockroachdb/errors.WithHint at boundary.
return errors.WithHint(
errors.Wrapf(errContextNotFound, "%q", contextName),
"verify the context name against `talosctl config contexts`",
)
}

tlsConfig, err := skipVerifyTLSConfig(configContext)
if err != nil {
return err
}

return WithClientNoNodes(action)
c, err := client.New(ctx, skipVerifyClientOptions(configContext, tlsConfig, dialOptions)...)
if err != nil {
return errors.Wrap(err, "constructing Talos client")
}
defer func() { _ = c.Close() }()

// Deliberately no client.WithNodes here: this is the skip-verify backing
// for the no-nodes constructors (WithClientNoNodes, withApplyClientBare),
// mirroring upstream where WithClientNoNodes never sets node metadata.
// Callers that want nodes (WithClient, the per-node apply loop) inject
// them in their own wrapper layer. Injecting here would attach a plural
// `nodes` key that apid's director rejects for COSI reads (e.g. rotate-ca).
return action(ctx, c)
}

// Commands is a list of commands published by the package.
Expand Down
3 changes: 1 addition & 2 deletions pkg/commands/rotate_ca_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ func discoverClusterNodes() (controlPlane, workers []string, err error) {
func getKubeconfigFromTalos() ([]byte, error) {
var kubeconfigData []byte

err := GlobalArgs.WithClient(func(ctx context.Context, c *client.Client) error {
err := WithClient(func(ctx context.Context, c *client.Client) error {
var err error

kubeconfigData, err = c.Kubeconfig(ctx)
Expand All @@ -323,7 +323,6 @@ func getKubeconfigFromTalos() ([]byte, error) {
return nil
})

//nolint:wrapcheck // forwarding talos/cobra error verbatim per the wrapper contract.
return kubeconfigData, err
}

Expand Down
Loading
Loading