step should skip some checks when manipulating the ca.json

[maraino]

Description

When step manipulates the ca.json to add a new provisioner, for example, while running

step ca provisioner add registration \
  --type x5c --x5c-root x5c/intermediate_crt.pem  \
  --create --ca-config $PWD/ca-staging/config/ca.json

The cli will try to validate the full ca.json, including unrelated things like the SSH templates. This might cause errors when step uses a different context than the one we want to manipulate.

See smallstep/certificates#963 (comment) for more information.

[maraino]

If we skip the validation of the Config, this will work.

diff --git a/command/ca/provisioner/provisioner.go b/command/ca/provisioner/provisioner.go
index b910fd7..dd9333e 100644
--- a/command/ca/provisioner/provisioner.go
+++ b/command/ca/provisioner/provisioner.go
@@ -122,6 +122,7 @@ func newCRUDClient(cliCtx *cli.Context, cfgFile string) (crudClient, error) {
                        return cautils.NewAdminClient(cliCtx)
                }
                ui.Println()
+               cfg.SkipValidation = true
                return newCaConfigClient(context.Background(), cfg, cfgFile)
        default:
                return nil, errs.FileError(err, cfgFile)

@dopey, do you think this will cause any harm?

[maraino]

At the same time if we disable the validation enabling vaultcas is not required (#724)

[hslatman]

I assume cfg.SkipValidation would skip validation entirely? If validation of the command input is performed before changing the contents of ca.json and the internal representation, then it could work, but I think it would still make sense to validate the entire configuration every time it's changed, because settings may be related to each other.

[maraino]

@hslatman The code validates the config when you create the step-ca authority.

In step we reuse some of the code of that authority to add or remove provisioners, but we only need is to be able to encode the right JSON for the provisioner and save it in the right place.

Working directly on the config, has another minor "issue", once we render it to a file, the order of your ca.json might change, and things like you might have configured manually, e.g. old configurations ("_tls": {...}) or comments in json format will disappear ("tls_comment": "here add support for curl v6").

SkipValidation would not solve that issue, but it won't try to ensure the certificates, keys, templates, RA configuration, ... are all ok, when they might not exist. Imagine for example that your ca.json is in your vcs, but you don't have the keys there.

[dopey]

@maraino where is the validation actually run? Like, where will that attribute be consumed?

We're already skipping initialization. I don't see any harm in skipping validation, since we're validating the provisioners using the provisioner collection API. I just don't see where it's applied.

[maraino]

@dopey authority's func New(cfg *config.Config, opts ...Option) (*Authority, error), called in newCaConfigClien validates the config, see this:

func New(cfg *config.Config, opts ...Option) (*Authority, error) {
	err := cfg.Validate()
	if err != nil {
		return nil, err
	}

cfg.Validate() will return nil if SkipValidation is set to true.

[dopey]

@maraino any reason not to use an options func like WithSkipValidate() like is already done for WithSkipInit()?

[maraino]

@maraino any reason not to use an options func like WithSkipValidate() like is already done for WithSkipInit()?

To do that, we will need to change how the New method works. The options passed to the method act on top of the authority, not on the configuration.