search

Venafi / vcert

Go client SDK and command line utility designed to simplify integrations by automating key generation and certificate enrollment using Venafi machine identity services.
https://support.venafi.com/hc/en-us/articles/217991528
Apache License 2.0
88 stars 64 forks source link

WIP: [VC-31275] Allow user to configure the User-Agent HTTP header in vcert CLI #440

Closed wallrj closed 5 months ago

wallrj commented 5 months ago

Depends on: #443

After discussing with @maelvls I have spun out the SDK changes to #443. Because this vcert --user-agent feature is a nice-to-have and is not required for VC-31275.

I've added a --user-agent flag which allows you to override the User-Agent header that is added to requests made by the vcert CLI. That can also be configured by a new environment variable VCERT_USER_AGENT. vcert CLI will now use User-Agent: vCert/v5.6.0 when using a released binary, where the version has been embedded (or vCert/Unknown when compiled locally).

Testing

Validation of --user-agent

go run ./cmd/vcert renew \
  -k foo --no-prompt -id foo --trust-bundle ~/.mitmproxy/mitmproxy-ca.pem \
  --user-agent richard.$'\x7F'.wall 
vCert: 2024/03/22 17:27:17 Invalid user-agent value: "richard.\x7f.wall"
exit status 1

With mitmproxy

maelvls commented 5 months ago

Hey, really glad to see that this long lasting issue will be solved soon!

Why would one have to set --user-agent or VCERT_USER_AGENT? In my mind, the CLI should have a sane default such as User-Agent: VCert/v5.6.0 and the user shouldn't have to use some flag to enable that. Are there reasons (such as not breaking existing flows) that would support the decision of keeping the old behavior by default?

Also, nit: is the official spelling vCert or VCert? I've only seen vcert (when speaking about the CLI binary) and VCert (when speaking about the product) in this repository and in https://support.venafi.com/hc/en-us/articles/217991528-Introducing-VCert-API-Abstraction-for-DevOps.

rvelaVenafi commented 5 months ago

@maelvls So far, the official documentation (readme file) has VCert as the official name. This info pre-dates me as maintainer of the repo, so thats how much I know.

Adding default user agent would be a good idea

rvelaVenafi commented 5 months ago

@wallrj First of all, thanks a lot for this contribution, we at the Open Source team really appreciate it.

Now, I apologize for the inconvenience, but there are some big changes coming to VCert this week. We are adding support for service account authentication in TLSPC (VaaS) connector. These changes cover a wide range of files across the SDK, CLI and playbook packages.

We are also updating the go version to 1.21.

So, I would recommend to wait after those changes are merged and then rebase your branch. Im quite sure a couple of things will need fixing (due to collisions, like flags and envars).

Once again, thank you for your contribution and I hope this mildly annoying turn of events do not discourage you from contributing in the future

wallrj commented 5 months ago

Why would one have to set --user-agent or VCERT_USER_AGENT? In my mind, the CLI should have a sane default such as User-Agent: vCert/v5.6.0 and the user shouldn't have to use some flag to enable that. Are there reasons (such as not breaking existing flows) that would support the decision of keeping the old behavior by default?

The default User-Agent header for the vcert CLI is vCert/v5.6.0. It is defined in flags.go. You can see an example of the default header in the testing section of the PR description.

You can override the default User-Agent header of the CLI using the --user-agent flag or with the VCERT_USER_AGENT environment variable. For example, if you execute vcert from a shell or Python script, you might set VCERT_USER_AGENT to help the Venafi TPP administrator distinguish your software from other utilities that might also be wrapping the vcert CLI.

I will add this explanation to the README file.

The default User-Agent header for the vcert Go module, is still Go-HTTP-1.1 and that might be a mistake. In the PR description, I argued that this is for "backwards compatibility"....but that isn't consistent with my changing the default for the CLI. Perhaps the defaults should be:

Software using the Go module (such as cert-manager) can change it by setting the client.Config.UserAgent field when instantiating a vcert.Client.

Software should also be able to configure the client to omit the User-Agent by setting the value to empty string, and for that I will need to change the field to a pointer so that I can distinguish between empty string and no preference.

Also, nit: is the official spelling vCert or VCert? I've only seen vcert (when speaking about the CLI binary) and VCert (when speaking about the product) in this repository and in https://support.venafi.com/hc/en-us/articles/217991528-Introducing-VCert-API-Abstraction-for-DevOps.

I agree that the spelling looks wrong, but it has been defined in a constant and used in logs and --help since 2018. It can be changed in a separate PR.

wallrj commented 5 months ago

After discussing with @maelvls I have spun out the SDK changes to https://github.com/Venafi/vcert/pull/443. Because this vcert --user-agent feature is a nice-to-have and is not required for VC-31275.

wallrj commented 5 months ago

So, I would recommend to wait after those changes are merged and then rebase your branch. Im quite sure a couple of things will need fixing (due to collisions, like flags and envars).

@rvelaVenafi I've reduced the scope and created a new PR: #443 Please review #443 instead.

I may revive this PR if I can convince myself that it's useful to be able to set the User-Agent when running vcert CLI...in shell scripts for example, but @maelvls talked about that and neither of us can think of an example of another CLI that allows you to do this (e.g. GitHub CLI) nor of an example where either of us have wanted to do this.