diff --git a/cmd/hauler/cli/store.go b/cmd/hauler/cli/store.go index 8162ddc..e305ceb 100644 --- a/cmd/hauler/cli/store.go +++ b/cmd/hauler/cli/store.go @@ -3,7 +3,6 @@ package cli import ( "fmt" "os" - "strconv" "github.com/spf13/cobra" "helm.sh/helm/v4/pkg/action" @@ -78,11 +77,18 @@ func addStoreSync(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Comman if o.CaFile == "" { o.CaFile = os.Getenv(consts.CaFile) } - if o.InsecureSkipTLSVerify == nil { - if v := os.Getenv(consts.InsecureSkipTLSVerify); v != "" { - b, _ := strconv.ParseBool(v) - o.InsecureSkipTLSVerify = &b - } + + // record which precedence-carrying flags the user explicitly set, so + // the resolvers can let an explicit CLI value win over per-item/annotation + o.TlogChanged = cmd.Flags().Changed("use-tlog-verify") + o.ExcludeExtrasChanged = cmd.Flags().Changed("exclude-extras") + o.InsecureChanged = cmd.Flags().Changed("insecure-skip-tls-verify") + o.StoreChanged = cmd.Flags().Changed("store") + o.RetriesChanged = cmd.Flags().Changed("retries") + + // env var only applies when the flag wasn't set, so an explicit --insecure-skip-tls-verify=false still wins + if !o.InsecureChanged && os.Getenv(consts.InsecureSkipTLSVerify) == "true" { + o.InsecureSkipTLSVerify = true } // --dry-run requires --products @@ -124,12 +130,6 @@ func addStoreSync(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Comman } rso.BlobConcurrency = bc - // resolve *bool: nil unless the user explicitly passed the flag - if cmd.Flags().Changed("insecure-skip-tls-verify") { - v, _ := cmd.Flags().GetBool("insecure-skip-tls-verify") - o.InsecureSkipTLSVerify = &v - } - return nil }, RunE: func(cmd *cobra.Command, args []string) error { @@ -411,11 +411,9 @@ func addStoreAddImage(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Co if o.CaFile == "" { o.CaFile = os.Getenv(consts.CaFile) } - if o.InsecureSkipTLSVerify == nil { - if v := os.Getenv(consts.InsecureSkipTLSVerify); v != "" { - b, _ := strconv.ParseBool(v) - o.InsecureSkipTLSVerify = &b - } + // env var only applies when the flag wasn't set, so an explicit --insecure-skip-tls-verify=false still wins + if !cmd.Flags().Changed("insecure-skip-tls-verify") && os.Getenv(consts.InsecureSkipTLSVerify) == "true" { + o.InsecureSkipTLSVerify = true } return nil }, diff --git a/cmd/hauler/cli/store/add.go b/cmd/hauler/cli/store/add.go index 73415fb..1acec5c 100644 --- a/cmd/hauler/cli/store/add.go +++ b/cmd/hauler/cli/store/add.go @@ -59,7 +59,7 @@ func AddFileCmd(ctx context.Context, o *flags.AddFileOpts, s *store.Layout, refe cfg := v1.File{ Path: reference, CaFile: o.CaFile, - InsecureSkipTLSVerify: &o.InsecureSkipTLSVerify, + InsecureSkipTLSVerify: o.InsecureSkipTLSVerify, } if len(o.Name) > 0 { cfg.Name = o.Name @@ -83,7 +83,7 @@ func storeFile(ctx context.Context, s *store.Layout, fi v1.File, ro *flags.CliRo copts := getter.ClientOptions{ NameOverride: fi.Name, - InsecureSkipTLSVerify: derefInsecure(fi.InsecureSkipTLSVerify), + InsecureSkipTLSVerify: fi.InsecureSkipTLSVerify, CAFile: fi.CaFile, } @@ -248,7 +248,7 @@ func AddImageCmd(ctx context.Context, o *flags.AddImageOpts, s *store.Layout, re func addImageVerifyConfig(o *flags.AddImageOpts) cosign.Config { switch { case o.Key != "": - return cosign.Config{Key: o.Key, Tlog: o.Tlog, InsecureSkipTLSVerify: derefInsecure(o.InsecureSkipTLSVerify), CaFile: o.CaFile} + return cosign.Config{Key: o.Key, Tlog: o.Tlog, InsecureSkipTLSVerify: o.InsecureSkipTLSVerify, CaFile: o.CaFile} case o.CertIdentityRegexp != "" || o.CertIdentity != "": return cosign.Config{ CertIdentity: o.CertIdentity, @@ -256,7 +256,7 @@ func addImageVerifyConfig(o *flags.AddImageOpts) cosign.Config { CertOidcIssuer: o.CertOidcIssuer, CertOidcIssuerRegexp: o.CertOidcIssuerRegexp, CertGithubWorkflowRepository: o.CertGithubWorkflowRepository, - InsecureSkipTLSVerify: derefInsecure(o.InsecureSkipTLSVerify), + InsecureSkipTLSVerify: o.InsecureSkipTLSVerify, CaFile: o.CaFile, } default: @@ -480,7 +480,7 @@ func storeImage(ctx context.Context, s *store.Layout, i v1.Image, platform strin return err } - insecureSkipTLSVerify := derefInsecure(i.InsecureSkipTLSVerify) + insecureSkipTLSVerify := i.InsecureSkipTLSVerify caFile := i.CaFile log.BaseFromContext(ctx).Debugf("resolving image [%s] (verified=%t, platform=%q, excludeExtras=%t, insecureSkipTLSVerify=%t, caFile=%q, rewrite=%q, digest=%q)", i.Name, verified, platform, excludeExtras, insecureSkipTLSVerify, caFile, rewrite, pinnedDigest) @@ -777,11 +777,11 @@ type chartJob struct { // // The three precedence rules are not uniform. registry is CLI > annotation. // excludeExtras is a one-way switch that any of the three sources can flip on -// and none can flip off. platform is per-chart > CLI > annotation: an explicit -// --platform is run-time intent and outranks manifest metadata. That last rule -// must stay identical to resolveImageJobs's, or a single `hauler store sync` -// run would pull a chart's discovered images for a different platform than the -// manifest's own Images section. +// and none can flip off, since a plain bool has no unset state. platform is +// CLI > per-chart > annotation. That last rule must stay identical to +// resolveImageJobs's, or a single `hauler store sync` run would pull a +// chart's discovered images for a different platform than the manifest's own +// Images section. // // Every job allocates its own *action.ChartPathOptions. flags.AddChartOpts // holds that as a pointer, so copying the struct alone would leave sibling @@ -795,20 +795,15 @@ func resolveChartJobs(o *flags.SyncOpts, annotations map[string]string, manifest jobs := make([]chartJob, 0, len(charts)) for _, ch := range charts { - excludeExtras := o.ExcludeExtras - if !o.ExcludeExtras && annotations[consts.ImageAnnotationExcludeExtras] == "true" { - excludeExtras = true - } - if ch.ExcludeExtras { - excludeExtras = ch.ExcludeExtras - } + excludeExtras := resolveBoolFlag(ch.ExcludeExtras, annotations[consts.ImageAnnotationExcludeExtras] == "true", o.ExcludeExtras, o.ExcludeExtrasChanged) platform := o.Platform - if o.Platform == "" && annotations[consts.ImageAnnotationPlatform] != "" { - platform = annotations[consts.ImageAnnotationPlatform] - } - if ch.Platform != "" { - platform = ch.Platform + if o.Platform == "" { + if ch.Platform != "" { + platform = ch.Platform + } else if annotations[consts.ImageAnnotationPlatform] != "" { + platform = annotations[consts.ImageAnnotationPlatform] + } } var valuesFiles []string @@ -826,16 +821,13 @@ func resolveChartJobs(o *flags.SyncOpts, annotations map[string]string, manifest if caFile == "" { if ch.CaFile != "" { caFile = ch.CaFile - } else if annotations[consts.ImageAnnotationCaFile] == "true" { + } else if annotations[consts.ImageAnnotationCaFile] != "" { caFile = annotations[consts.ImageAnnotationCaFile] } } - insecureSkipTLSVerify := false - if o.CaFile == "" { - insecureSkipTLSVerify = resolveInsecure(ch.InsecureSkipTLSVerify, annotations, o.InsecureSkipTLSVerify) - } else { - } + // a CA file and skipping TLS verification are mutually exclusive: providing one forces verification on + insecureSkipTLSVerify := o.CaFile == "" && resolveBoolFlag(ch.InsecureSkipTLSVerify, annotations[consts.ImageAnnotationInsecureSkipTLSVerify] == "true", o.InsecureSkipTLSVerify, o.InsecureChanged) jobs = append(jobs, chartJob{ cfg: ch, @@ -1381,12 +1373,11 @@ func fetchChart(ctx context.Context, s *store.Layout, j chartJob, tempRoot strin // there is no separate per-discovered-image TLS knob in a chart // manifest, so the registry a chart's images live in is assumed // to share the chart repo's trust configuration. - chartInsecure := j.opts.ChartOpts.InsecureSkipTLSVerify imageJobs = append(imageJobs, imageJob{ img: v1.Image{ Name: relocated, CaFile: j.opts.ChartOpts.CaFile, - InsecureSkipTLSVerify: &chartInsecure, + InsecureSkipTLSVerify: j.opts.ChartOpts.InsecureSkipTLSVerify, }, platform: j.opts.Platform, excludeExtras: j.opts.ExcludeExtras, diff --git a/cmd/hauler/cli/store/add_test.go b/cmd/hauler/cli/store/add_test.go index 13e5309..e87ea64 100644 --- a/cmd/hauler/cli/store/add_test.go +++ b/cmd/hauler/cli/store/add_test.go @@ -1798,12 +1798,13 @@ func TestResolveChartJobs_ExcludeExtras(t *testing.T) { tests := []struct { name string cli bool + cliChanged bool annotation string perChart bool want bool }{ {name: "nothing set", want: false}, - {name: "CLI flag alone", cli: true, want: true}, + {name: "CLI flag alone", cli: true, cliChanged: true, want: true}, {name: "annotation alone", annotation: "true", want: true}, {name: "per-chart alone", perChart: true, want: true}, { @@ -1816,14 +1817,16 @@ func TestResolveChartJobs_ExcludeExtras(t *testing.T) { // --exclude-extras back off; both are one-way switches. name: "CLI flag survives an annotation that is not true", cli: true, + cliChanged: true, annotation: "false", want: true, }, { - name: "CLI flag survives a false per-chart field", - cli: true, - perChart: false, - want: true, + name: "CLI flag survives a false per-chart field", + cli: true, + cliChanged: true, + perChart: false, + want: true, }, { name: "annotation survives a false per-chart field", @@ -1831,11 +1834,20 @@ func TestResolveChartJobs_ExcludeExtras(t *testing.T) { perChart: false, want: true, }, + { + // An explicit CLI --exclude-extras=false wins outright over an + // annotation/per-chart true. + name: "explicit CLI false overrides annotation and per-chart", + cliChanged: true, + annotation: "true", + perChart: true, + want: false, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - o := &flags.SyncOpts{ExcludeExtras: tc.cli} + o := &flags.SyncOpts{ExcludeExtras: tc.cli, ExcludeExtrasChanged: tc.cliChanged} a := map[string]string{} if tc.annotation != "" { a[consts.ImageAnnotationExcludeExtras] = tc.annotation @@ -1877,10 +1889,16 @@ func TestResolveChartJobs_Platform(t *testing.T) { want: "linux/amd64", }, { - name: "per-chart wins over both", + name: "CLI flag wins over annotation and per-chart", cli: "linux/amd64", annotation: "linux/arm64", perChart: "linux/s390x", + want: "linux/amd64", + }, + { + name: "per-chart wins over annotation when CLI flag unset", + annotation: "linux/arm64", + perChart: "linux/s390x", want: "linux/s390x", }, { @@ -2155,7 +2173,6 @@ func TestResolveChartJobs_NoCharts(t *testing.T) { // TestResolveChartJobs_CredentialFields pins that every TLS/verification // field on v1.Chart reaches the job's ChartOpts unchanged. func TestResolveChartJobs_CredentialFields(t *testing.T) { - insecure := true ch := v1.Chart{ Name: "rancher", Verify: true, @@ -2164,7 +2181,7 @@ func TestResolveChartJobs_CredentialFields(t *testing.T) { CertFile: "/certs/client.crt", KeyFile: "/certs/client.key", CaFile: "/certs/ca.crt", - InsecureSkipTLSVerify: &insecure, + InsecureSkipTLSVerify: true, PlainHTTP: true, } @@ -2195,14 +2212,47 @@ func TestResolveChartJobs_CredentialFields(t *testing.T) { if opts.CaFile != ch.CaFile { t.Errorf("CaFile = %q, want %q", opts.CaFile, ch.CaFile) } - if opts.InsecureSkipTLSVerify != derefInsecure(ch.InsecureSkipTLSVerify) { - t.Errorf("InsecureSkipTLSVerify = %v, want %v", opts.InsecureSkipTLSVerify, derefInsecure(ch.InsecureSkipTLSVerify)) + if opts.InsecureSkipTLSVerify != ch.InsecureSkipTLSVerify { + t.Errorf("InsecureSkipTLSVerify = %v, want %v", opts.InsecureSkipTLSVerify, ch.InsecureSkipTLSVerify) } if opts.PlainHTTP != ch.PlainHTTP { t.Errorf("PlainHTTP = %v, want %v", opts.PlainHTTP, ch.PlainHTTP) } } +func TestResolveChartJobs_CaFilePrecedence(t *testing.T) { + tests := []struct { + name string + cli string + annotation string + perChart string + want string + }{ + {name: "annotation used when CLI and per-chart unset", annotation: "/ann/ca.crt", want: "/ann/ca.crt"}, + {name: "per-chart wins over annotation", annotation: "/ann/ca.crt", perChart: "/chart/ca.crt", want: "/chart/ca.crt"}, + {name: "CLI wins over per-chart and annotation", cli: "/cli/ca.crt", annotation: "/ann/ca.crt", perChart: "/chart/ca.crt", want: "/cli/ca.crt"}, + {name: "none set stays empty", want: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + o := &flags.SyncOpts{CaFile: tc.cli} + a := map[string]string{} + if tc.annotation != "" { + a[consts.ImageAnnotationCaFile] = tc.annotation + } + + jobs, err := resolveChartJobs(o, a, "/manifests", []v1.Chart{{Name: "rancher", CaFile: tc.perChart}}) + if err != nil { + t.Fatalf("resolveChartJobs: %v", err) + } + if got := jobs[0].opts.ChartOpts.CaFile; got != tc.want { + t.Errorf("CaFile = %q, want %q", got, tc.want) + } + }) + } +} + // TestResolveChartJobs_CredentialEnv pins that UsernameEnv/PasswordEnv are // resolved into ChartOpts.Username/Password via resolveChartCreds. func TestResolveChartJobs_CredentialEnv(t *testing.T) { @@ -2999,7 +3049,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) { t.Run("bad caFile without insecure returns error and stores nothing", func(t *testing.T) { s := newTestStore(t) insecure := false - img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure} + img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure} err := storeImage(ctx, s, img, "", false, defaultRootOpts(s.Root), defaultCliOpts(), "", "", false) if err == nil { @@ -3017,7 +3067,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) { t.Fatal(err) } insecure := false - img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: &insecure} + img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: insecure} err := storeImage(ctx, s, img, "", false, defaultRootOpts(s.Root), defaultCliOpts(), "", "", false) if err == nil { @@ -3031,7 +3081,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) { // ignored and the pull still succeeds. If caFile were read first, the // pull would error and nothing would be stored. insecure := true - img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure} + img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure} err := storeImage(ctx, s, img, "", false, defaultRootOpts(s.Root), defaultCliOpts(), "", "", false) if err != nil { @@ -3043,7 +3093,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) { t.Run("valid caFile without insecure is accepted", func(t *testing.T) { s := newTestStore(t) insecure := false - img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: &insecure} + img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: insecure} err := storeImage(ctx, s, img, "", false, defaultRootOpts(s.Root), defaultCliOpts(), "", "", false) if err != nil { diff --git a/cmd/hauler/cli/store/sync.go b/cmd/hauler/cli/store/sync.go index c92f2f1..37a4552 100644 --- a/cmd/hauler/cli/store/sync.go +++ b/cmd/hauler/cli/store/sync.go @@ -189,7 +189,7 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags if strings.HasPrefix(haulPath, "http://") || strings.HasPrefix(haulPath, "https://") { l.Debugf("detected remote manifest... starting download... [%s]", haulPath) - h := getter.NewHttp(derefInsecure(o.InsecureSkipTLSVerify), o.CaFile) + h := getter.NewHttp(o.InsecureSkipTLSVerify, o.CaFile) parsedURL, err := url.Parse(haulPath) if err != nil { return err @@ -241,7 +241,7 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags if strings.HasPrefix(haulPath, "http://") || strings.HasPrefix(haulPath, "https://") { l.Debugf("detected remote image.txt... starting download... [%s]", haulPath) - h := getter.NewHttp(derefInsecure(o.InsecureSkipTLSVerify), o.CaFile) + h := getter.NewHttp(o.InsecureSkipTLSVerify, o.CaFile) parsedURL, err := url.Parse(haulPath) if err != nil { return err @@ -287,28 +287,16 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags return nil } -// resolveInsecure applies precedence: cli > per-item > annotation. -// A non-nil per-item pointer wins outright — including an explicit false — so an -// individual file/image/chart can opt out of an insecure annotation or the global -// --insecure-skip-tls-verify flag. nil means "not set on the item", which falls -// through to the annotation, then the global flag. -func resolveInsecure(item *bool, ann map[string]string, global *bool) bool { - if global != nil { - return *global +// resolveBoolFlag applies CLI-first precedence for a plain-bool flag: an +// explicitly-set CLI flag wins outright (even when false); otherwise the flag +// is on if the resolved CLI value (e.g. from an env var), the per-item field, +// or the annotation is true. Plain bools have no unset state, so per-item and +// annotation can only turn a flag on, never force it back off. +func resolveBoolFlag(item, annTrue, global, cliChanged bool) bool { + if cliChanged { + return global } - if item != nil { - return *item - } - if ann != nil && ann[consts.ImageAnnotationInsecureSkipTLSVerify] == "true" { - return true - } - return false -} - -// derefInsecure is a nil-safe read of a *bool for logging/plumbing where a plain -// bool is needed. nil reads as false. -func derefInsecure(p *bool) bool { - return p != nil && *p + return global || item || annTrue } func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *store.Layout, rso *flags.StoreRootOpts, ro *flags.CliRootOpts, targetStores map[string]*store.Layout) error { @@ -347,11 +335,11 @@ func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *stor return err } a := cfg.GetAnnotations() - docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores) + docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores, o.StoreChanged) if err != nil { return err } - docRso, err := resolveDocRetries(a, rso) + docRso, err := resolveDocRetries(a, rso, o.RetriesChanged) if err != nil { return err } @@ -374,11 +362,11 @@ func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *stor } a := cfg.GetAnnotations() - docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores) + docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores, o.StoreChanged) if err != nil { return err } - docRso, err := resolveDocRetries(a, rso) + docRso, err := resolveDocRetries(a, rso, o.RetriesChanged) if err != nil { return err } @@ -403,11 +391,11 @@ func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *stor return err } a := cfg.GetAnnotations() - docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores) + docStore, err := resolveTargetStore(ctx, a, s, rso, ro, targetStores, o.StoreChanged) if err != nil { return err } - docRso, err := resolveDocRetries(a, rso) + docRso, err := resolveDocRetries(a, rso, o.RetriesChanged) if err != nil { return err } @@ -433,7 +421,12 @@ func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *stor // resolveTargetStore picks a doc's store based on its hauler.dev/store annotation, // falling back to def. Opens (or reuses, via targetStores) the target store otherwise. -func resolveTargetStore(ctx context.Context, a map[string]string, def *store.Layout, rso *flags.StoreRootOpts, ro *flags.CliRootOpts, targetStores map[string]*store.Layout) (*store.Layout, error) { +// An explicit CLI --store wins: cliStoreSet short-circuits to def, ignoring the annotation. +func resolveTargetStore(ctx context.Context, a map[string]string, def *store.Layout, rso *flags.StoreRootOpts, ro *flags.CliRootOpts, targetStores map[string]*store.Layout, cliStoreSet bool) (*store.Layout, error) { + if cliStoreSet { + return def, nil + } + target := a[consts.AnnotationTargetStore] if target == "" { return def, nil @@ -466,8 +459,13 @@ func resolveTargetStore(ctx context.Context, a map[string]string, def *store.Lay // resolveDocRetries returns a copy of rso with Retries overridden by a doc's // hauler.dev/retries annotation, or rso unchanged if it's not set. Copy, not -// mutation, so it can't leak into a sibling doc. -func resolveDocRetries(a map[string]string, rso *flags.StoreRootOpts) (*flags.StoreRootOpts, error) { +// mutation, so it can't leak into a sibling doc. An explicit CLI --retries +// wins: cliRetriesSet returns rso unchanged, ignoring the annotation. +func resolveDocRetries(a map[string]string, rso *flags.StoreRootOpts, cliRetriesSet bool) (*flags.StoreRootOpts, error) { + if cliRetriesSet { + return rso, nil + } + v, ok := a[consts.AnnotationRetries] if !ok || v == "" { return rso, nil @@ -604,10 +602,12 @@ type imageJob struct { certGithubWorkflowRepository string } -// resolveImageJobs applies the precedence rules (per-image > annotation > -// CLI, except registry relocation which is CLI > annotation) to every image -// in images, producing one imageJob per image. It is pure -- cosign -// verification happens later, inside the pull worker; see resolveAndVerify. +// resolveImageJobs applies the precedence rules (CLI > per-image > annotation) +// to every image in images, producing one imageJob per image. For the boolean +// flags (tlog, exclude-extras, insecure-skip-tls-verify) an explicit CLI flag +// wins outright; otherwise per-image or annotation can only turn them on (see +// resolveBoolFlag). It is pure -- cosign verification happens later, inside the +// pull worker; see resolveAndVerify. func resolveImageJobs(o *flags.SyncOpts, a map[string]string, images []v1.Image) ([]imageJob, error) { var jobs []imageJob @@ -635,11 +635,8 @@ func resolveImageJobs(o *flags.SyncOpts, a map[string]string, images []v1.Image) i.CaFile = o.CaFile } - insecureSkipTLSVerify := false - if o.CaFile == "" { - insecureSkipTLSVerify = resolveInsecure(i.InsecureSkipTLSVerify, a, o.InsecureSkipTLSVerify) - } - i.InsecureSkipTLSVerify = &insecureSkipTLSVerify + // a CA file and skipping TLS verification are mutually exclusive: providing one forces verification on + i.InsecureSkipTLSVerify = o.CaFile == "" && resolveBoolFlag(i.InsecureSkipTLSVerify, a[consts.ImageAnnotationInsecureSkipTLSVerify] == "true", o.InsecureSkipTLSVerify, o.InsecureChanged) if i.Local { needsPubKeyVerification := a[consts.ImageAnnotationKey] != "" || o.Key != "" || i.Key != "" @@ -669,71 +666,71 @@ func resolveImageJobs(o *flags.SyncOpts, a map[string]string, images []v1.Image) if needsPubKeyVerification { key := o.Key - if o.Key == "" && a[consts.ImageAnnotationKey] != "" { - expanded, err := homedir.Expand(a[consts.ImageAnnotationKey]) - if err != nil { - return nil, err + if o.Key == "" { + if i.Key != "" { + expanded, err := homedir.Expand(i.Key) + if err != nil { + return nil, err + } + key = expanded + } else if a[consts.ImageAnnotationKey] != "" { + expanded, err := homedir.Expand(a[consts.ImageAnnotationKey]) + if err != nil { + return nil, err + } + key = expanded } - key = expanded - } - if i.Key != "" { - expanded, err := homedir.Expand(i.Key) - if err != nil { - return nil, err - } - key = expanded } - tlog := o.Tlog - if !o.Tlog && a[consts.ImageAnnotationTlog] == "true" { - tlog = true - } - if i.Tlog { - tlog = i.Tlog - } + tlog := resolveBoolFlag(i.Tlog, a[consts.ImageAnnotationTlog] == "true", o.Tlog, o.TlogChanged) job.needsPubKey = true job.key = key job.tlog = tlog } else if needsKeylessVerificaton { //Keyless signature verification certIdentityRegexp := o.CertIdentityRegexp - if o.CertIdentityRegexp == "" && a[consts.ImageAnnotationCertIdentityRegexp] != "" { - certIdentityRegexp = a[consts.ImageAnnotationCertIdentityRegexp] - } - if i.CertIdentityRegexp != "" { - certIdentityRegexp = i.CertIdentityRegexp + if o.CertIdentityRegexp == "" { + if i.CertIdentityRegexp != "" { + certIdentityRegexp = i.CertIdentityRegexp + } else if a[consts.ImageAnnotationCertIdentityRegexp] != "" { + certIdentityRegexp = a[consts.ImageAnnotationCertIdentityRegexp] + } } certIdentity := o.CertIdentity - if o.CertIdentity == "" && a[consts.ImageAnnotationCertIdentity] != "" { - certIdentity = a[consts.ImageAnnotationCertIdentity] - } - if i.CertIdentity != "" { - certIdentity = i.CertIdentity + if o.CertIdentity == "" { + if i.CertIdentity != "" { + certIdentity = i.CertIdentity + } else if a[consts.ImageAnnotationCertIdentity] != "" { + certIdentity = a[consts.ImageAnnotationCertIdentity] + } } certOidcIssuer := o.CertOidcIssuer - if o.CertOidcIssuer == "" && a[consts.ImageAnnotationCertOidcIssuer] != "" { - certOidcIssuer = a[consts.ImageAnnotationCertOidcIssuer] - } - if i.CertOidcIssuer != "" { - certOidcIssuer = i.CertOidcIssuer + if o.CertOidcIssuer == "" { + if i.CertOidcIssuer != "" { + certOidcIssuer = i.CertOidcIssuer + } else if a[consts.ImageAnnotationCertOidcIssuer] != "" { + certOidcIssuer = a[consts.ImageAnnotationCertOidcIssuer] + } } certOidcIssuerRegexp := o.CertOidcIssuerRegexp - if o.CertOidcIssuerRegexp == "" && a[consts.ImageAnnotationCertOidcIssuerRegexp] != "" { - certOidcIssuerRegexp = a[consts.ImageAnnotationCertOidcIssuerRegexp] - } - if i.CertOidcIssuerRegexp != "" { - certOidcIssuerRegexp = i.CertOidcIssuerRegexp + if o.CertOidcIssuerRegexp == "" { + if i.CertOidcIssuerRegexp != "" { + certOidcIssuerRegexp = i.CertOidcIssuerRegexp + } else if a[consts.ImageAnnotationCertOidcIssuerRegexp] != "" { + certOidcIssuerRegexp = a[consts.ImageAnnotationCertOidcIssuerRegexp] + } } certGithubWorkflowRepository := o.CertGithubWorkflowRepository - if o.CertGithubWorkflowRepository == "" && a[consts.ImageAnnotationCertGithubWorkflowRepository] != "" { - certGithubWorkflowRepository = a[consts.ImageAnnotationCertGithubWorkflowRepository] - } - if i.CertGithubWorkflowRepository != "" { - certGithubWorkflowRepository = i.CertGithubWorkflowRepository + if o.CertGithubWorkflowRepository == "" { + if i.CertGithubWorkflowRepository != "" { + certGithubWorkflowRepository = i.CertGithubWorkflowRepository + } else if a[consts.ImageAnnotationCertGithubWorkflowRepository] != "" { + certGithubWorkflowRepository = a[consts.ImageAnnotationCertGithubWorkflowRepository] + } } job.needsKeyless = true @@ -745,11 +742,12 @@ func resolveImageJobs(o *flags.SyncOpts, a map[string]string, images []v1.Image) } platform := o.Platform - if o.Platform == "" && a[consts.ImageAnnotationPlatform] != "" { - platform = a[consts.ImageAnnotationPlatform] - } - if i.Platform != "" { - platform = i.Platform + if o.Platform == "" { + if i.Platform != "" { + platform = i.Platform + } else if a[consts.ImageAnnotationPlatform] != "" { + platform = a[consts.ImageAnnotationPlatform] + } } rewrite := "" @@ -757,13 +755,7 @@ func resolveImageJobs(o *flags.SyncOpts, a map[string]string, images []v1.Image) rewrite = i.Rewrite } - excludeExtras := o.ExcludeExtras - if !o.ExcludeExtras && a[consts.ImageAnnotationExcludeExtras] == "true" { - excludeExtras = true - } - if i.ExcludeExtras { - excludeExtras = i.ExcludeExtras - } + excludeExtras := resolveBoolFlag(i.ExcludeExtras, a[consts.ImageAnnotationExcludeExtras] == "true", o.ExcludeExtras, o.ExcludeExtrasChanged) job.platform = platform job.rewrite = rewrite @@ -795,7 +787,7 @@ func (j imageJob) verifyConfig() cosign.Config { return cosign.Config{ Key: j.key, Tlog: j.tlog, - InsecureSkipTLSVerify: derefInsecure(j.img.InsecureSkipTLSVerify), + InsecureSkipTLSVerify: j.img.InsecureSkipTLSVerify, CaFile: j.img.CaFile, } case j.needsKeyless: @@ -805,7 +797,7 @@ func (j imageJob) verifyConfig() cosign.Config { CertOidcIssuer: j.certOidcIssuer, CertOidcIssuerRegexp: j.certOidcIssuerRegexp, CertGithubWorkflowRepository: j.certGithubWorkflowRepository, - InsecureSkipTLSVerify: derefInsecure(j.img.InsecureSkipTLSVerify), + InsecureSkipTLSVerify: j.img.InsecureSkipTLSVerify, CaFile: j.img.CaFile, } default: @@ -1152,9 +1144,10 @@ type fileJob struct { file v1.File } -// resolveFileJobs converts every v1.File in files into a fileJob, applying -// the caFile/insecure precedence rules (per-file > annotation > global) -- -// see resolveInsecure. It is pure. +// resolveFileJobs converts every v1.File in files into a fileJob. caFile is +// CLI > per-file > annotation; insecureSkipTLSVerify is CLI-first (an explicit +// CLI flag wins, otherwise per-file or annotation can only turn it on -- see +// resolveBoolFlag). It is pure. func resolveFileJobs(o *flags.SyncOpts, a map[string]string, files []v1.File) []fileJob { jobs := make([]fileJob, 0, len(files)) for _, f := range files { @@ -1166,11 +1159,8 @@ func resolveFileJobs(o *flags.SyncOpts, a map[string]string, files []v1.File) [] f.CaFile = o.CaFile } - insecure := false - if o.CaFile == "" { - insecure = resolveInsecure(f.InsecureSkipTLSVerify, a, o.InsecureSkipTLSVerify) - } - f.InsecureSkipTLSVerify = &insecure + // a CA file and skipping TLS verification are mutually exclusive: providing one forces verification on + f.InsecureSkipTLSVerify = o.CaFile == "" && resolveBoolFlag(f.InsecureSkipTLSVerify, a[consts.ImageAnnotationInsecureSkipTLSVerify] == "true", o.InsecureSkipTLSVerify, o.InsecureChanged) jobs = append(jobs, fileJob{file: f}) } diff --git a/cmd/hauler/cli/store/sync_test.go b/cmd/hauler/cli/store/sync_test.go index 0de13d6..e7c7579 100644 --- a/cmd/hauler/cli/store/sync_test.go +++ b/cmd/hauler/cli/store/sync_test.go @@ -245,7 +245,7 @@ func TestResolveDocRetries_NoAnnotation_ReturnsRsoUnchanged(t *testing.T) { rso := defaultRootOpts(t.TempDir()) rso.Retries = 5 - got, err := resolveDocRetries(map[string]string{}, rso) + got, err := resolveDocRetries(map[string]string{}, rso, false) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -258,7 +258,7 @@ func TestResolveDocRetries_Override_ReturnsCopyNotMutation(t *testing.T) { rso := defaultRootOpts(t.TempDir()) rso.Retries = 5 - got, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "9"}, rso) + got, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "9"}, rso, false) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -273,11 +273,27 @@ func TestResolveDocRetries_Override_ReturnsCopyNotMutation(t *testing.T) { } } +func TestResolveDocRetries_CLIWinsOverAnnotation(t *testing.T) { + rso := defaultRootOpts(t.TempDir()) + rso.Retries = 5 + + got, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "9"}, rso, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != rso { + t.Fatal("expected rso unchanged when --retries was set on the CLI") + } + if got.Retries != 5 { + t.Fatalf("got Retries %d, want the CLI value 5", got.Retries) + } +} + func TestResolveDocRetries_ZeroMeansDefault(t *testing.T) { rso := defaultRootOpts(t.TempDir()) rso.Retries = 5 - got, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "0"}, rso) + got, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "0"}, rso, false) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -289,7 +305,7 @@ func TestResolveDocRetries_ZeroMeansDefault(t *testing.T) { func TestResolveDocRetries_Negative_ReturnsError(t *testing.T) { rso := defaultRootOpts(t.TempDir()) - if _, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "-1"}, rso); err == nil { + if _, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "-1"}, rso, false); err == nil { t.Fatal("expected an error for a negative hauler.dev/retries value, got nil") } } @@ -297,7 +313,7 @@ func TestResolveDocRetries_Negative_ReturnsError(t *testing.T) { func TestResolveDocRetries_NotANumber_ReturnsError(t *testing.T) { rso := defaultRootOpts(t.TempDir()) - if _, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "banana"}, rso); err == nil { + if _, err := resolveDocRetries(map[string]string{consts.AnnotationRetries: "banana"}, rso, false); err == nil { t.Fatal("expected an error for a non-numeric hauler.dev/retries value, got nil") } } @@ -943,10 +959,17 @@ func TestResolveImageJobs_KeyPrecedence(t *testing.T) { wantKey: homeKey, }, { - name: "per-image overrides annotation and CLI, expanded via homedir", + // CLI wins outright over both annotation and per-image. + name: "CLI overrides annotation and per-image", cliKey: "/cli/key.pub", annotation: "/annotation/key.pub", imageKey: "~/mykey.pub", + wantKey: "/cli/key.pub", + }, + { + name: "per-image overrides annotation when CLI key unset, expanded via homedir", + annotation: "/annotation/key.pub", + imageKey: "~/mykey.pub", wantKey: homeKey, }, } @@ -982,25 +1005,36 @@ func TestResolveImageJobs_TlogPrecedence(t *testing.T) { tests := []struct { name string cliTlog bool + cliChanged bool annotation string imageTlog bool wantTlog bool }{ { - name: "CLI true", - cliTlog: true, - wantTlog: true, + name: "CLI true", + cliTlog: true, + cliChanged: true, + wantTlog: true, }, { - name: "annotation true overrides CLI false", + name: "annotation true when CLI unset", annotation: "true", wantTlog: true, }, { - name: "per-image true overrides annotation/CLI false", + name: "per-image true when CLI unset", imageTlog: true, wantTlog: true, }, + { + // An explicit CLI flag wins outright, even false, over an + // annotation/per-image true. + name: "explicit CLI false overrides annotation and per-image", + cliChanged: true, + annotation: "true", + imageTlog: true, + wantTlog: false, + }, { name: "all false stays false", wantTlog: false, @@ -1009,7 +1043,7 @@ func TestResolveImageJobs_TlogPrecedence(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - o := &flags.SyncOpts{Key: "/cli/key.pub", Tlog: tc.cliTlog} + o := &flags.SyncOpts{Key: "/cli/key.pub", Tlog: tc.cliTlog, TlogChanged: tc.cliChanged} a := map[string]string{} if tc.annotation != "" { a[consts.ImageAnnotationTlog] = tc.annotation @@ -1042,7 +1076,9 @@ func TestResolveImageJobs_PlatformPrecedence(t *testing.T) { // Annotation only applies when the CLI platform is unset — it does not // override an explicitly-set CLI platform. {name: "annotation used when CLI platform unset", annotation: "linux/arm64", want: "linux/arm64"}, - {name: "per-image overrides annotation and CLI", cliPlatform: "linux/amd64", annotation: "linux/arm64", imagePlatform: "linux/386", want: "linux/386"}, + // CLI wins outright over both annotation and per-image. + {name: "CLI overrides annotation and per-image", cliPlatform: "linux/amd64", annotation: "linux/arm64", imagePlatform: "linux/386", want: "linux/amd64"}, + {name: "per-image overrides annotation when CLI platform unset", annotation: "linux/arm64", imagePlatform: "linux/386", want: "linux/386"}, {name: "none set stays empty", want: ""}, } @@ -1103,19 +1139,21 @@ func TestResolveImageJobs_ExcludeExtrasPrecedence(t *testing.T) { tests := []struct { name string cliExcludeExtras bool + cliChanged bool annotation string imageExclude bool want bool }{ - {name: "CLI true", cliExcludeExtras: true, want: true}, - {name: "annotation true overrides CLI false", annotation: "true", want: true}, - {name: "per-image true overrides annotation/CLI false", imageExclude: true, want: true}, + {name: "CLI true", cliExcludeExtras: true, cliChanged: true, want: true}, + {name: "annotation true when CLI unset", annotation: "true", want: true}, + {name: "per-image true when CLI unset", imageExclude: true, want: true}, + {name: "explicit CLI false overrides annotation and per-image", cliChanged: true, annotation: "true", imageExclude: true, want: false}, {name: "all false stays false", want: false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - o := &flags.SyncOpts{ExcludeExtras: tc.cliExcludeExtras} + o := &flags.SyncOpts{ExcludeExtras: tc.cliExcludeExtras, ExcludeExtrasChanged: tc.cliChanged} a := map[string]string{} if tc.annotation != "" { a[consts.ImageAnnotationExcludeExtras] = tc.annotation @@ -1136,6 +1174,46 @@ func TestResolveImageJobs_ExcludeExtrasPrecedence(t *testing.T) { } } +func TestResolveImageJobs_InsecurePrecedence(t *testing.T) { + tests := []struct { + name string + cliCaFile string + cliChanged bool + annotation string + imageIns bool + want bool + }{ + {name: "per-image true when CLI unset", imageIns: true, want: true}, + {name: "annotation true when CLI unset", annotation: "true", want: true}, + {name: "explicit CLI false overrides annotation and per-image", cliChanged: true, annotation: "true", imageIns: true, want: false}, + // a CA file forces verification on, overriding a per-image/annotation true + {name: "ca-file forces insecure off", cliCaFile: "/ca.pem", annotation: "true", imageIns: true, want: false}, + {name: "all unset stays false", want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + o := &flags.SyncOpts{CaFile: tc.cliCaFile, InsecureChanged: tc.cliChanged} + a := map[string]string{} + if tc.annotation != "" { + a[consts.ImageAnnotationInsecureSkipTLSVerify] = tc.annotation + } + images := []v1.Image{{Name: "rancher/rancher:v2.9", InsecureSkipTLSVerify: tc.imageIns}} + + jobs, err := resolveImageJobs(o, a, images) + if err != nil { + t.Fatalf("resolveImageJobs: %v", err) + } + if len(jobs) != 1 { + t.Fatalf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].img.InsecureSkipTLSVerify != tc.want { + t.Errorf("got insecure %v, want %v", jobs[0].img.InsecureSkipTLSVerify, tc.want) + } + }) + } +} + func TestResolveImageJobs_NoOptions_MinimalJob(t *testing.T) { o := &flags.SyncOpts{} images := []v1.Image{{Name: "rancher/rancher:v2.9"}} diff --git a/internal/flags/add.go b/internal/flags/add.go index a219f20..ca9ab09 100644 --- a/internal/flags/add.go +++ b/internal/flags/add.go @@ -22,7 +22,7 @@ type AddImageOpts struct { ExcludeExtras bool Local bool CaFile string - InsecureSkipTLSVerify *bool + InsecureSkipTLSVerify bool } func (o *AddImageOpts) AddFlags(cmd *cobra.Command) { @@ -39,7 +39,7 @@ func (o *AddImageOpts) AddFlags(cmd *cobra.Command) { f.BoolVar(&o.ExcludeExtras, "exclude-extras", false, "(Optional) Exclude cosign signatures, attestations, SBOMs, and OCI referrers when pulling the image") f.BoolVar(&o.Local, "local", false, "(Optional) Add image from the local Docker daemon instead of a remote registry") f.StringVar(&o.CaFile, "ca-file", "", "(Optional) Location of CA Bundle to enable certification verification") - f.Bool("insecure-skip-tls-verify", false, "(Optional) Skip TLS certificate verification") + f.BoolVar(&o.InsecureSkipTLSVerify, "insecure-skip-tls-verify", false, "(Optional) Skip TLS certificate verification") } type AddFileOpts struct { diff --git a/internal/flags/sync.go b/internal/flags/sync.go index 6f067fe..114b9f1 100644 --- a/internal/flags/sync.go +++ b/internal/flags/sync.go @@ -26,7 +26,17 @@ type SyncOpts struct { Concurrency int NoProgress bool CaFile string - InsecureSkipTLSVerify *bool + InsecureSkipTLSVerify bool + + // Whether each of these flags was explicitly set on the CLI, captured in + // sync's PreRunE. A plain bool (and a resolved store/retries value) has no + // "unset" state, so the resolvers use these markers to let an explicit CLI + // value win over per-item/annotation instead of only ever turning a flag on. + TlogChanged bool + ExcludeExtrasChanged bool + InsecureChanged bool + StoreChanged bool + RetriesChanged bool } func (o *SyncOpts) AddFlags(cmd *cobra.Command) { @@ -50,5 +60,5 @@ func (o *SyncOpts) AddFlags(cmd *cobra.Command) { f.IntVarP(&o.Concurrency, "concurrency", "j", consts.DefaultConcurrency, "(Optional) Maximum number of artifacts to fetch and store concurrently (1 = serial; also via HAULER_CONCURRENCY, explicit flag wins)") f.BoolVar(&o.NoProgress, "no-progress", false, "(Optional) Disable the live progress display") f.StringVar(&o.CaFile, "ca-file", "", "(Optional) Location of CA Bundle to enable certification verification") - f.Bool("insecure-skip-tls-verify", false, "(Optional) Skip TLS certificate verification") + f.BoolVar(&o.InsecureSkipTLSVerify, "insecure-skip-tls-verify", false, "(Optional) Skip TLS certificate verification") } diff --git a/pkg/apis/hauler.cattle.io/v1/chart.go b/pkg/apis/hauler.cattle.io/v1/chart.go index 1098dcb..cbd73a0 100644 --- a/pkg/apis/hauler.cattle.io/v1/chart.go +++ b/pkg/apis/hauler.cattle.io/v1/chart.go @@ -41,6 +41,6 @@ type Chart struct { CertFile string `json:"certFile,omitempty"` KeyFile string `json:"keyFile,omitempty"` CaFile string `json:"caFile,omitempty"` - InsecureSkipTLSVerify *bool `json:"insecureSkipTLSVerify,omitempty"` + InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"` PlainHTTP bool `json:"plainHTTP,omitempty"` } diff --git a/pkg/apis/hauler.cattle.io/v1/file.go b/pkg/apis/hauler.cattle.io/v1/file.go index b1c9cfe..216d851 100644 --- a/pkg/apis/hauler.cattle.io/v1/file.go +++ b/pkg/apis/hauler.cattle.io/v1/file.go @@ -26,5 +26,5 @@ type File struct { // TLS options for verifying the file contents for remote files. // If not specified, the default system CA bundle will be used. CaFile string `json:"ca-file"` - InsecureSkipTLSVerify *bool `json:"insecure-skip-tls-verify,omitempty"` + InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"` } diff --git a/pkg/apis/hauler.cattle.io/v1/image.go b/pkg/apis/hauler.cattle.io/v1/image.go index 45e765d..561019f 100644 --- a/pkg/apis/hauler.cattle.io/v1/image.go +++ b/pkg/apis/hauler.cattle.io/v1/image.go @@ -43,5 +43,5 @@ type Image struct { // TLS options for verifying the image signature. If not specified, the default system CA bundle will be used. CaFile string `json:"ca-file"` - InsecureSkipTLSVerify *bool `json:"insecure-skip-tls-verify,omitempty"` + InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"` }