Refactor ignoring of provisioner flag to not rely on package globals

hslatman · hslatman · commit 064866f86c64 · 2025-03-27T19:50:31.000+01:00
diff --git a/cmd/step/main.go b/cmd/step/main.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/smallstep/cli/command/version"
 	"github.com/smallstep/cli/internal/plugin"
+	"github.com/smallstep/cli/internal/provisionerflag"
 	"github.com/smallstep/cli/utils"
 
 	// Enabled cas interfaces.
@@ -126,11 +127,17 @@ func newApp(stdout, stderr io.Writer) *cli.App {
 	app.Copyright = fmt.Sprintf("(c) 2018-%d Smallstep Labs, Inc.", time.Now().Year())
 
 	// Flag of custom configuration flag
-	app.Flags = append(app.Flags, cli.StringFlag{
+	app.Flags = append(app.Flags, cli.StringFlag{ //nolint:gocritic // intentionally split for documentation
 		Name:  "config",
 		Usage: "path to the config file to use for CLI flags",
 	})
 
+	// add a hidden flag that can be used to signal that the provisioner
+	// flag should be ignored in certain commands. By defining it on the
+	// app level it can be ignored in multiple (sub)commands without having
+	// to specify the flag in each command.
+	app.Flags = append(app.Flags, provisionerflag.DisabledSentinelFlag)
+
 	// Action runs on `step` or `step <command>` if the command is not enabled.
 	app.Action = func(ctx *cli.Context) error {
 		args := ctx.Args()
diff --git a/cmd/step/main_test.go b/cmd/step/main_test.go
@@ -3,9 +3,13 @@ package main
 import (
 	"bytes"
 	"regexp"
+	"slices"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli"
+
+	"github.com/smallstep/cli/internal/provisionerflag"
 )
 
 func TestAppHasAllCommands(t *testing.T) {
@@ -44,3 +48,15 @@ func TestAppRuns(t *testing.T) {
 	output := ansiRegex.ReplaceAllString(stdout.String(), "")
 	require.Contains(t, output, "step -- plumbing for distributed systems")
 }
+
+func TestAppHasSentinelFlagForIgnoringProvisionersFlag(t *testing.T) {
+	app := newApp(nil, nil)
+	require.NotNil(t, app)
+
+	// this test only checks if the flag is present when an app is created
+	// through [getApp]. This is sufficient for now to proof that the flag
+	// exists in the actual released CLI binary.
+	require.True(t, slices.ContainsFunc(app.Flags, func(f cli.Flag) bool {
+		return f.GetName() == provisionerflag.DisabledSentinelFlagName()
+	}))
+}
diff --git a/command/ca/policy/acme/acme.go b/command/ca/policy/acme/acme.go
@@ -8,7 +8,6 @@ import (
 	"github.com/smallstep/cli/command/ca/policy/actions"
 	"github.com/smallstep/cli/command/ca/policy/policycontext"
 	"github.com/smallstep/cli/command/ca/policy/x509"
-	"github.com/smallstep/cli/internal/provisionerflag"
 )
 
 // Command returns the ACME account policy subcommand.
@@ -28,9 +27,5 @@ Please note that certificate issuance policies for ACME accounts are currently o
 			actions.RemoveCommand(ctx),
 			x509.Command(ctx),
 		},
-		Before: func(ctx *cli.Context) error {
-			provisionerflag.Ignore()
-			return nil
-		},
 	}
 }
diff --git a/command/ca/policy/actions/cn.go b/command/ca/policy/actions/cn.go
@@ -76,6 +76,8 @@ $ step ca policy authority x509 deny cn "My Bad CA Name"
 }
 
 func commonNamesAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/dns.go b/command/ca/policy/actions/dns.go
@@ -94,6 +94,8 @@ $ step ca policy authority ssh host allow dns "badsshhost.local"
 }
 
 func dnsAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/emails.go b/command/ca/policy/actions/emails.go
@@ -81,6 +81,8 @@ $ step ca policy provisioner ssh user deny email @example.com --provisioner my_p
 }
 
 func emailAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/ips.go b/command/ca/policy/actions/ips.go
@@ -114,6 +114,8 @@ $ step ca policy authority ssh host deny ip 192.168.0.40
 }
 
 func ipAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/policy.go b/command/ca/policy/actions/policy.go
@@ -16,13 +16,26 @@ import (
 
 	"github.com/smallstep/cli/command/ca/policy/policycontext"
 	"github.com/smallstep/cli/internal/command"
+	"github.com/smallstep/cli/internal/provisionerflag"
 )
 
 var provisionerFilterFlag = cli.StringFlag{
 	Name:  "provisioner",
 	Usage: `The provisioner <name>`,
 }
 
+// ignoreProvisionerFlagIfRequired is a helper function that marks the provisioner
+// flag to be ignored when managing a provisioner or ACME account level policy. In
+// those cases the provisioner flag is used to filter which provisioner the policy
+// applies to, as opposed to its normal usage, where it can be used to select the
+// (admin) provisioner to use for authentication.
+func ignoreProvisionerFlagIfRequired(ctx context.Context) {
+	clictx := command.CLIContextFromContext(ctx)
+	if policycontext.IsProvisionerPolicyLevel(ctx) || policycontext.IsACMEPolicyLevel(ctx) {
+		provisionerflag.Ignore(clictx)
+	}
+}
+
 func retrieveAndInitializePolicy(ctx context.Context, client *ca.AdminClient) (*linkedca.Policy, error) {
 	var (
 		policy *linkedca.Policy
diff --git a/command/ca/policy/actions/principals.go b/command/ca/policy/actions/principals.go
@@ -76,6 +76,8 @@ $ step ca policy provisioner ssh host deny principal root --provisioner my_ssh_u
 }
 
 func principalAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/remove.go b/command/ca/policy/actions/remove.go
@@ -71,6 +71,8 @@ $ step ca policy acme remove --provisioner my_acme_provisioner --eab-key-id "lUO
 }
 
 func removeAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 	provisioner := clictx.String("provisioner")
 	reference := clictx.String("eab-key-reference")
diff --git a/command/ca/policy/actions/uris.go b/command/ca/policy/actions/uris.go
@@ -71,6 +71,8 @@ $ step ca policy provisioner x509 allow uri "*.example.com" --provisioner my_pro
 }
 
 func uriAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	args := clictx.Args()
diff --git a/command/ca/policy/actions/view.go b/command/ca/policy/actions/view.go
@@ -72,6 +72,8 @@ $ step ca policy acme view --provisioner my_acme_provisioner --eab-key-id "lUOTG
 }
 
 func viewAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 	provisioner := clictx.String("provisioner")
 	reference := clictx.String("eab-key-reference")
diff --git a/command/ca/policy/actions/wildcards.go b/command/ca/policy/actions/wildcards.go
@@ -10,6 +10,8 @@ import (
 
 // AllowWildcardsAction updates the policy to allow wildcard names.
 func AllowWildcardsAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	client, err := cautils.NewAdminClient(clictx)
@@ -34,6 +36,8 @@ func AllowWildcardsAction(ctx context.Context) (err error) {
 
 // DenyWildcardsAction updates the policy to deny wildcard names.
 func DenyWildcardsAction(ctx context.Context) (err error) {
+	ignoreProvisionerFlagIfRequired(ctx)
+
 	clictx := command.CLIContextFromContext(ctx)
 
 	client, err := cautils.NewAdminClient(clictx)
diff --git a/command/ca/policy/provisioner/provisioner.go b/command/ca/policy/provisioner/provisioner.go
@@ -9,7 +9,6 @@ import (
 	"github.com/smallstep/cli/command/ca/policy/policycontext"
 	"github.com/smallstep/cli/command/ca/policy/ssh"
 	"github.com/smallstep/cli/command/ca/policy/x509"
-	"github.com/smallstep/cli/internal/provisionerflag"
 )
 
 // Command returns the policy subcommand.
@@ -30,9 +29,5 @@ Please note that certificate issuance policies on the provisioner level are curr
 			x509.Command(ctx),
 			ssh.Command(ctx),
 		},
-		Before: func(ctx *cli.Context) error {
-			provisionerflag.Ignore()
-			return nil
-		},
 	}
 }
diff --git a/internal/provisionerflag/ignore.go b/internal/provisionerflag/ignore.go
@@ -1,17 +1,58 @@
+// this package is used for ignoring the provisioner flag in specific
+// cli commands.
 package provisionerflag
 
 import (
-	"sync/atomic"
+	"github.com/urfave/cli"
 )
 
-var disabled atomic.Bool
+var disabledSentinel = "/x-disable-provisioner-flag"
 
-// Ignore marks the provisionerflag to be ignored
-func Ignore() {
-	disabled.Store(true)
+// DisabledSentinelFlagName returns the name of the sentinel flag
+// that can be used to ignore the provisioner flag in specific cli commands.
+func DisabledSentinelFlagName() string {
+	return disabledSentinel
 }
 
-// ShouldBeIgnored returns true if the provisioner flag should be ignored
-func ShouldBeIgnored() bool {
-	return disabled.Load()
+// DisabledSentinelFlag is a sentinel flag that can be used to ignore
+// the provisioner flag in specific cli commands.
+var DisabledSentinelFlag = cli.BoolFlag{
+	Name:   disabledSentinel,
+	Hidden: true,
+}
+
+// Ignore marks the provisioner flag to be ignored. If an error occurs it
+// will traverse the [cli.Context] recursively until setting the value
+// succeeds or the root context is reached. If the value is not set along
+// the way, it will panic.
+func Ignore(ctx *cli.Context) {
+	if ctx == nil {
+		panic("context is nil")
+	}
+
+	err := ctx.Set(disabledSentinel, "true")
+	switch {
+	case err == nil:
+		return
+	case ctx.Parent() != nil:
+		Ignore(ctx.Parent())
+	default:
+		panic(err)
+	}
+}
+
+// ShouldBeIgnored returns whether the provisioner flag should be ignored.
+// If the [cli.Context] does not contain the sentinel flag value, it will
+// recursively look for it up to the root context.
+func ShouldBeIgnored(ctx *cli.Context) bool {
+	if ctx.IsSet(disabledSentinel) && ctx.String(disabledSentinel) == "true" {
+		return true
+	}
+
+	// recursively look for the sentinel value in the parent context
+	if ctx.Parent() != nil {
+		return ShouldBeIgnored(ctx.Parent())
+	}
+
+	return false
 }
diff --git a/internal/provisionerflag/ignore_test.go b/internal/provisionerflag/ignore_test.go
@@ -0,0 +1,90 @@
+// this package is used for ignoring the provisioner flag in specific
+// cli commands.
+package provisionerflag_test
+
+import (
+	"flag"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli"
+
+	"github.com/smallstep/cli/internal/provisionerflag"
+)
+
+func TestProvisionerFlagCanBeIgnored(t *testing.T) {
+	t.Parallel()
+
+	app := cli.NewApp()
+
+	t.Run("not-ignored", func(t *testing.T) {
+		t.Parallel()
+
+		parentFlags := flag.NewFlagSet("parent", 0)
+		parentFlags.String(provisionerflag.DisabledSentinelFlagName(), "", "")
+
+		parent := cli.NewContext(app, parentFlags, nil)
+		ctx := cli.NewContext(app, flag.NewFlagSet("test", 0), parent)
+
+		require.False(t, provisionerflag.ShouldBeIgnored(ctx))
+	})
+
+	t.Run("child", func(t *testing.T) {
+		t.Parallel()
+
+		parent := cli.NewContext(app, flag.NewFlagSet("parent", 0), nil)
+
+		childFlags := flag.NewFlagSet("test", 0)
+		childFlags.String(provisionerflag.DisabledSentinelFlagName(), "", "")
+		ctx := cli.NewContext(app, childFlags, parent)
+
+		provisionerflag.Ignore(ctx)
+
+		require.True(t, provisionerflag.ShouldBeIgnored(ctx))
+	})
+
+	t.Run("parent", func(t *testing.T) {
+		t.Parallel()
+
+		parentFlags := flag.NewFlagSet("parent", 0)
+		parentFlags.String(provisionerflag.DisabledSentinelFlagName(), "", "")
+
+		parent := cli.NewContext(app, parentFlags, nil)
+		ctx := cli.NewContext(app, flag.NewFlagSet("test", 0), parent)
+
+		provisionerflag.Ignore(ctx)
+
+		require.True(t, provisionerflag.ShouldBeIgnored(ctx))
+	})
+
+	t.Run("chain", func(t *testing.T) {
+		t.Parallel()
+
+		parentFlags := flag.NewFlagSet("parent", 0)
+		parentFlags.String(provisionerflag.DisabledSentinelFlagName(), "", "")
+
+		parent := cli.NewContext(app, parentFlags, nil)
+		ctx := cli.NewContext(app, flag.NewFlagSet("test-1", 0), parent)
+		ctx = cli.NewContext(app, flag.NewFlagSet("test-2", 0), ctx)
+		ctx = cli.NewContext(app, flag.NewFlagSet("test-3", 0), ctx)
+
+		provisionerflag.Ignore(ctx)
+
+		require.True(t, provisionerflag.ShouldBeIgnored(ctx))
+	})
+
+	t.Run("nil-context", func(t *testing.T) {
+		t.Parallel()
+
+		require.Panics(t, func() { provisionerflag.Ignore(nil) })
+	})
+
+	t.Run("flag-undefined", func(t *testing.T) {
+		t.Parallel()
+
+		parent := cli.NewContext(app, flag.NewFlagSet("parent", 0), nil)
+		ctx := cli.NewContext(app, flag.NewFlagSet("test", 0), parent)
+
+		require.Panics(t, func() { provisionerflag.Ignore(ctx) })
+	})
+}
diff --git a/utils/cautils/token_flow.go b/utils/cautils/token_flow.go
@@ -352,7 +352,7 @@ func provisionerPrompt(ctx *cli.Context, provisioners provisioner.List) (provisi
 	// Filter by provisioner / issuer (provisioner name)
 	if provisionerName, flag := flags.FirstStringOf(ctx, "provisioner", "issuer"); provisionerName != "" {
 		provisioners = provisionerFilter(provisioners, func(p provisioner.Interface) bool {
-			if provisionerflag.ShouldBeIgnored() {
+			if provisionerflag.ShouldBeIgnored(ctx) {
 				return true // fake match; effectively skipping provisioner flag value for provisioner-dependent policy commands
 			}
 
diff --git a/utils/cautils/token_flow_test.go b/utils/cautils/token_flow_test.go

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,6 @@ import (`
`8`	`8`	`"github.com/smallstep/cli/command/ca/policy/actions"`
`9`	`9`	`"github.com/smallstep/cli/command/ca/policy/policycontext"`
`10`	`10`	`"github.com/smallstep/cli/command/ca/policy/x509"`
`11`		`- "github.com/smallstep/cli/internal/provisionerflag"`
`12`	`11`	`)`
`13`	`12`
`14`	`13`	`// Command returns the ACME account policy subcommand.`
`@@ -28,9 +27,5 @@ Please note that certificate issuance policies for ACME accounts are currently o`
`28`	`27`	`actions.RemoveCommand(ctx),`
`29`	`28`	`x509.Command(ctx),`
`30`	`29`	`},`
`31`		`- Before: func(ctx *cli.Context) error {`
`32`		`- provisionerflag.Ignore()`
`33`		`- return nil`
`34`		`- },`
`35`	`30`	`}`
`36`	`31`	`}`
Original file line number	Diff line number	Diff line change
`@@ -76,6 +76,8 @@ $ step ca policy authority x509 deny cn "My Bad CA Name"`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`func commonNamesAction(ctx context.Context) (err error) {`
	`79`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`80`	`+`
`79`	`81`	`clictx := command.CLIContextFromContext(ctx)`
`80`	`82`
`81`	`83`	`args := clictx.Args()`
Original file line number	Diff line number	Diff line change
`@@ -94,6 +94,8 @@ $ step ca policy authority ssh host allow dns "badsshhost.local"`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`func dnsAction(ctx context.Context) (err error) {`
	`97`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`98`	`+`
`97`	`99`	`clictx := command.CLIContextFromContext(ctx)`
`98`	`100`
`99`	`101`	`args := clictx.Args()`
Original file line number	Diff line number	Diff line change
`@@ -81,6 +81,8 @@ $ step ca policy provisioner ssh user deny email @example.com --provisioner my_p`
`81`	`81`	`}`
`82`	`82`
`83`	`83`	`func emailAction(ctx context.Context) (err error) {`
	`84`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`85`	`+`
`84`	`86`	`clictx := command.CLIContextFromContext(ctx)`
`85`	`87`
`86`	`88`	`args := clictx.Args()`
Original file line number	Diff line number	Diff line change
`@@ -114,6 +114,8 @@ $ step ca policy authority ssh host deny ip 192.168.0.40`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`func ipAction(ctx context.Context) (err error) {`
	`117`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`118`	`+`
`117`	`119`	`clictx := command.CLIContextFromContext(ctx)`
`118`	`120`
`119`	`121`	`args := clictx.Args()`
Original file line number	Diff line number	Diff line change
`@@ -76,6 +76,8 @@ $ step ca policy provisioner ssh host deny principal root --provisioner my_ssh_u`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`func principalAction(ctx context.Context) (err error) {`
	`79`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`80`	`+`
`79`	`81`	`clictx := command.CLIContextFromContext(ctx)`
`80`	`82`
`81`	`83`	`args := clictx.Args()`
Original file line number	Diff line number	Diff line change
`@@ -71,6 +71,8 @@ $ step ca policy acme remove --provisioner my_acme_provisioner --eab-key-id "lUO`
`71`	`71`	`}`
`72`	`72`
`73`	`73`	`func removeAction(ctx context.Context) (err error) {`
	`74`	`+ ignoreProvisionerFlagIfRequired(ctx)`
	`75`	`+`
`74`	`76`	`clictx := command.CLIContextFromContext(ctx)`
`75`	`77`	`provisioner := clictx.String("provisioner")`
`76`	`78`	`reference := clictx.String("eab-key-reference")`