Add support for passing file path to certificate for certificate create command

[rdoherty-fastly]

It is common (and easier) to pass a file path into a CLI tool vs trying to pass in file contents. This change allows the --cert-blob argument to take a file path. It is backwards compatible via the argparser.Content function, which determines if the flag value is a file path or file contents.

This follows the existing behavior in pkg/commands/vcl/snippet/create.go and a few others. I've tested with a file path and raw cert contents with the --debug flag and it does send the correct cert data both ways.

Questions:

• Not sure if it's a good idea to overload the --cert-blob field, but seems benign?
• It feels weird to use the argparser.OptionalString type for the certBlob field that is required, but I couldn't find anything else that might work but could have missed it.
• How should this be tested? I'm new to Go and don't see existing tests for this functionality. Is there anything I can use as a guide?
• Is this considered backwards compatible? I think so, existing tooling would be passing in the full cert contents and keep working, this would allow users to use a file path if they like.

I'm fairly new to Go, any feedback is appreciated!

[Integralist]

Thanks @rdoherty-fastly for opening this PR! Much appreciated 🙂

I've made some small suggested edits for you to review.

In answer to your questions...

• Not sure if it's a good idea to overload the --cert-blob field, but seems benign?

  • I think this is fine. It's backwards compatible and we've used a similar approach of overloading a flag elsewhere in the CLI.

• It feels weird to use the argparser.OptionalString type for the certBlob field that is required, but I couldn't find anything else that might work but could have missed it.

  • I made suggested edits to revert it back to the original type of a string. I'm not sure why you needed to change the type, but I presume (as you're new to Go) that maybe you simply made a mistake somewhere earlier and it just evolved into using OptionalString. But you're instinct is correct, and we shouldn't use it for a 'required' flag.

• How should this be tested? I'm new to Go and don't see existing tests for this functionality. Is there anything I can use as a guide?

  • In Go the tests always exist in a file alongside the code file itself, and is always suffixed with _test.go as the filename. So you'll see in the same directory https://github.com/fastly/cli/blob/2d6c936cda7d2acdbd7709ec7c67c11758fdf894/pkg/commands/tls/custom/certificate/certificate_test.go

  I would suggest making a copy of the error/success test cases

  cli/pkg/commands/tls/custom/certificate/certificate_test.go

  Lines 34 to 54 in 2d6c936

  	Name: validateAPIError,
  	API: mock.API{
  	CreateCustomTLSCertificateFn: func(_ *fastly.CreateCustomTLSCertificateInput) (*fastly.CustomTLSCertificate, error) {
  	return nil, testutil.Err
  	},
  	},
  	Args: args("tls-custom certificate create --cert-blob example"),
  	WantError: testutil.Err.Error(),
  	},
  	{
  	Name: validateAPISuccess,
  	API: mock.API{
  	CreateCustomTLSCertificateFn: func(_ *fastly.CreateCustomTLSCertificateInput) (*fastly.CustomTLSCertificate, error) {
  	return &fastly.CustomTLSCertificate{
  	ID: mockResponseID,
  	}, nil
  	},
  	},
  	Args: args("tls-custom certificate create --cert-blob example"),
  	WantOutput: fmt.Sprintf("Created TLS Certificate '%s'", mockResponseID),
  	},

  and modify the copies so that you're passing a relative file path instead of example (which in the current tests is standing in for the actual PEM contents, which we mock the API call to succeed). I think you should look at adding a test file into a ./testdata/ directory inside of this same directory (e.g. add pkg/commands/tls/custom/certificate/testdata/certificate.crt) and making sure you get the appropriate error response if passing in a nonsensical path like ./I/dont/exist.pem vs getting a success message when passing ./testdata/certificate.crt).

  IMPORTANT: Use filepath.Join() from the standard library for the paths (e.g. testCert := filepath.Join("I", "dont", "exist") because these tests are run across multiple OS' in CI, including Windows which will error if you use *nix forward slash for a path. The filepath.Join() internally will use the right path separator for the OS the binary is compiled for.

  In Go, you use testdata directories for holding fixture data...

  Test files that declare a package with the suffix "_test" will be compiled as a
  separate package, and then linked and run with the main test binary. The go tool will ignore a directory named "testdata", making it available
  to hold ancillary data needed by the tests. -- https://pkg.go.dev/cmd/go/internal/test

• Is this considered backwards compatible? I think so, existing tooling would be passing in the full cert contents and keep working, this would allow users to use a file path if they like.

  • Yes. I think this is backwards compatible. It shouldn't break for a user who passes a blob instead of a file path.

Thanks!

[rdoherty-fastly]

Thanks @Integralist ! Will work on these things today or tmrw!

[rdoherty-fastly]

Thanks for the feedback @Integralist! I've updated the PR with your changes.

[rdoherty-fastly]

Oops I need to update tests.

[rdoherty-fastly]

Tests added @Integralist , would love a review of them, thanks!

[Integralist]

I'm not sure how this is passing because from what I can tell there isn't a file at pkg/config/testdata/certificate.crt.

[rdoherty-fastly]

Uh yep that is not good. I forgot to commit that file, but it should fail in CI.

[Integralist]

Yeah let's make sure ci fails first then commit the file to see if it passes. You can of course run the tests locally to help debug things

[rdoherty-fastly]

It's succeeding b/c if argparser.Content can't find the file it thinks the argument is the content. So the cert-blob being uploaded is pkg/config/testdata/certificate.crt . A filename passed in that doesn't exist becomes the raw cert blob contents. Will think about how to handle this.

[rdoherty-fastly]

cli/pkg/argparser/flags.go

Line 276 in 46fefb5

// Content determines if the given flag value is a file path, and if so read

[rdoherty-fastly]

So the question is, do we do more heuristics or validation on the cert-blob before uploading to check if we're uploading a cert? Or do we do some heuristics on if the cert-blob looks like a file path?

My concern is if someone gives an invalid path, the client will upload a 'cert' that's invalid. They then get this error:

➜  cli git:(rdoherty/certificate-create-accept-file-path) ✗ ./fastly tls-custom certificate create --cert-blob=bad/path

ERROR: the Fastly API returned 400 Bad Request: Invalid value for cert_blob (The contents provided do not appear to be a valid X.509 certificate.).

Which isn't too helpful. Only if they use --debug-mode would they see that the 'contents' are the file path. We could use pem.Decode() and if it fails return an error. Thoughts?

[Integralist]

OK, I think we need to start handling the error case in argparser.Content...

https://github.com/fastly/cli/blob/main/pkg/argparser/flags.go#L281C43-L281C43

...so this ^^ should change to != nil rather than the happy path == nil.

e.g.

func Content(flagval string) (string, error) {
	content := flagval
	if path, err := filepath.Abs(flagval); err == nil {
		if _, err := os.Stat(path); err != nil {
			return "", fmt.Errorf("error looking up provided path '%s': %q", path, err)
		}
		if data, err := os.ReadFile(path); err == nil /* #nosec */ {
			content = string(data)
		}
	}
	return content, nil
}

Notice the function signature has changed to returning two arguments now (string, error).

This will mean updating the callers to handle an error now as well. I think it's only called in like 5 places in the codebase though so shouldn't be a massive amount of work.

Well done spotting this bug 👍🏻 🙂

[rdoherty-fastly]

Thanks for the tips! I have tried to implement the change to handled the error case, but I am getting errors when passing in a cert blob directly via args("tls-custom certificate create --cert-blob "-----BEGIN CERTIFICATE-----
MIIDnDC..."). The line if _, err := os.Stat(path); err != nil {` gets an error because it's trying to find a file with the filename being the cert blob.

I think the problem is that in Content, if a full cert blob is passed in, the line if path, err := filepath.Abs(flagval); err == nil will treat flagval as a path no matter what, and then that gets passed to os.Stat(path); which fails. This could be fine if we catch that error in certificate/create.go and assume that if no file is found, the raw arg passed in is a cert blob, but what if a customer typo'd their filename?

Options I can think of:

1. Check if the --cert-blob arg contains 'BEGIN CERTIFICATE', if it does, treat as raw. If not, treat as filename. Simple, but problematic if users try to do a cat(filename) in their CLI command to pipe the cert and typo the name or command.
2. Do some basic sanity checking of the --cert-blob after reading the file OR if it starts with BEGIN CERTIFICATE. Parse via https://pkg.go.dev/crypto/x509#ParseCertificate? Pro is we can give a better error message, on is we're doing more in the client and if the client's implementation of parsing differs from the server's, users may not be able to upload a valid cert.
3. Abort this and create a new argument for this command for passing in a filename, --cert-file. The CLI would have to make passing in --cert-blob OR --cert-file required, but not both. Makes it clear to users and the CLI what is being passed in via args, making error handling simpler.

At this point I think #3 is the best experience for a user and simpler. Thoughts? Other options? I might be missing something with the error or how we could handle this.

I'll push up a WIP commit to this PR to show what's happening.

[Integralist]

Thanks for your patience @rdoherty-fastly

So yes, you're correct, I was able to replicate here the issue with the absolute path:
https://go.dev/play/p/JYULtb5RZuG

I didn't expect filepath.Abs() to succeed if given a cert blob, but I was wrong 😬

So I would say, yes we should probably just go with option 3.

[Integralist]

We should handle the returned error.

[Integralist]

Thanks for your patience @rdoherty-fastly

So yes, you're correct, I was able to replicate here the issue with the absolute path:
https://go.dev/play/p/JYULtb5RZuG

I didn't expect filepath.Abs() to succeed if given a cert blob, but I was wrong 😬

So I would say, yes we should probably just go with option 3.

[rdoherty-fastly]

I'm closing this PR in favor of opening up a new one that will add a separate flag (--cert-file?) that is mutually exclusive to --cert-blob. One of the two will be required.

[Integralist]

Thanks @rdoherty-fastly