Add user-agent header in requests to Venafi API #6865

wallrj · 2024-03-20T10:10:09Z

TLDR

Purpose: Improve Venafi API interactions by adding a custom User-Agent header to requests from cert-manager.
Issue Addressed: Allows Venafi TPP server administrators to identify cert-manager as the source of API requests, aiding in troubleshooting and bug reporting.
Implementation: The User-Agent string mirrors that used for the ACME issuer and includes cert-manager version details.
Testing: Local and E2E tests conducted, including verification of User-Agent header presence in TPP server logs.

Details

As an administrator of a Venafi TPP server or of Venafi Cloud , I want to know which software is being used to connect to the REST API.

Right now the vcert SDK will use the default Go HTTP client User-Agent header: Go-http-client/1.1 and the requests from cert-manager will be indistinguishable with those from the vcert cli or any other vcert-sdk using software.

For example, if the TPP API server is being overwhelmed by a rapid repeated requests, I would like to be able to look at the IIS server logs, find the User-Agent of the offending requests and learn that the requests may be being generated by cert-manager.

As a maintainer of the cert-manager Venafi Issuer, I want it to send User-Agent: cert-manager/vX.Y.Z so that the administrator of a Venafi TPP server or Venafi Cloud service, can know which software is being used to connect to the REST API. Why? Because if there is a bug in the cert-manager Venafi issuer, which causes it to not back off in the event of a failed API request, the Venafi server administrator can quickly know that cert-manager is the culprit and quickly report the bug so that it be quickly fixed.

In this PR I've used the same User-Agent that is being used for the ACME issuer since: #4773.

The User-Agent headers will be:

Health checks from the Venafi Issuer controller:

cert-manager-certificaterequests-issuer-venafi/v1.14.0-beta.0-198-gef068a59008f6e+(linux/amd64)+cert-manager/ef068a59008f6ed919b98a7177921ddc9e297200

Signing and retrieve requests from the Venafi CertificateRequest and CertificateSigningRequest controllers:

cert-manager-certificaterequests-issuer-venafi/v1.14.0-beta.0-198-gef068a59008f6e+(linux/amd64)+cert-manager/ef068a59008f6ed919b98a7177921ddc9e297200

Fixes: #5803

Set the User-Agent for cert-manager including version #5803

Testing

Local testing

Run mitmproxy:

mitmproxy

Run the cert-manager controller locally:

make e2e-setup
kubectl scale -n cert-manager deployment cert-manager --replicas 0
HTTPS_PROXY=localhost:8080 go run ./cmd/controller --kubeconfig ~/.kube/config  --cluster-resource-namespace cert-manager

# example.yaml
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: venafi
spec:
  venafi:
    zone: fake-zone
    tpp:
      url: https://httpbin.org
      caBundle: ${MITMPROXY_CA}
      credentialsRef:
        name: venafi-credentials
---
apiVersion: v1
kind: Secret
metadata:
  name: venafi-credentials
  namespace: cert-manager
stringData:
  username: fake-username
  password: fake-password

export MITMPROXY_CA=$(base64 -w 0 ~/.mitmproxy/mitmproxy-ca-cert.pem)
envsubst < example.yaml | kubectl apply -f -

E2E tests

I triggered the Venafi TPP E2E tests by commenting on this PR with: /test pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp in
Logged into the cert-manager TPP server using RDP and examined the IIS logs of the TPP server.

Before:
(logs created by periodic tests using the existing cert-manager code)

#Software: Microsoft Internet Information Services 10.0
#Version: 1.0
#Date: 2024-03-20 04:47:53
#Fields: date time s-ip cs-method cs-uri-stem cs-uri-query s-port cs-username c-ip cs(User-Agent) cs(Referer) sc-status sc-substatus sc-win32-status time-taken

2024-03-20 04:47:54 10.132.0.2 POST /vedsdk/certificates/checkpolicy - 443 - 10.8.0.52 Go-http-client/1.1 - 200 0 0 12
2024-03-20 04:47:54 10.132.0.2 POST /vedsdk/certificates/retrieve - 443 - 10.8.0.52 Go-http-client/1.1 - 202 0 0 9

After:
(logs created by cert-manager built from this branch)

https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/cert-manager_cert-manager/6865/pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp/1770457571052752896

2024-03-20 14:34:02 10.132.0.2 POST /vedsdk/certificates/checkpolicy - 443 - 10.8.0.8 cert-manager-certificaterequests-issuer-venafi/v1.14.0-beta.0-198-gef068a59008f6e+(linux/amd64)+cert-manager/ef068a59008f6ed919b98a7177921ddc9e297200 - 200 0 0 11
2024-03-20 14:34:02 10.132.0.2 POST /vedsdk/certificates/retrieve - 443 - 10.8.0.8 cert-manager-certificaterequests-issuer-venafi/v1.14.0-beta.0-198-gef068a59008f6e+(linux/amd64)+cert-manager/ef068a59008f6ed919b98a7177921ddc9e297200 - 202 0 0 6

Venafi Issuer now sends a cert-manager HTTP User-Agent header in all Venafi Rest API requests.
For example: `cert-manager-certificaterequests-issuer-venafi/v1.15.0+(linux/amd64)+cert-manager/ef068a59008f6ed919b98a7177921ddc9e297200`.

wallrj · 2024-03-20T10:12:38Z

/test pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp
/test pull-cert-manager-master-e2e-v1-28-issuers-venafi-cloud

…uests This code existed in cert-manager once before and I'm reviving it. Here's the history: * Added: cert-manager#422 * Moved: cert-manager#432 * Obsoleted: cert-manager#797 * Deleted: cert-manager#966 Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj · 2024-03-20T10:36:22Z

/retest

Warning	FailedScheduling	19m	default-scheduler	0/4 nodes are available: 1 node(s) had untolerated taint {node.cloudprovider.kubernetes.io/uninitialized: true}, 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }, 2 Insufficient cpu, 2 Insufficient memory. preemption: 0/4 nodes are available: 2 No preemption victims found for incoming pod, 2 Preemption is not helpful for scheduling..

https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/cert-manager_cert-manager/6865/pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp/1770393015253209088

Signed-off-by: Richard Wall <richard.wall@venafi.com>

…for Venafi Cloud Signed-off-by: Richard Wall <richard.wall@venafi.com>

…t-manager code Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj · 2024-03-20T14:29:10Z

/test pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp
/test pull-cert-manager-master-e2e-v1-28-issuers-venafi-cloud

wallrj · 2024-03-20T14:39:53Z

/retest

  - lastProbeTime: null
    lastTransitionTime: "2024-03-20T14:32:16Z"
    message: 'GCPControllerManager: node no longer exists'
    reason: DeletionByGCPControllerManager
    status: "True"
    type: DisruptionTarget

https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/cert-manager_cert-manager/6865/pull-cert-manager-master-e2e-v1-28-issuers-venafi-cloud/1770457571128250368

wallrj · 2024-03-20T15:16:17Z

pkg/issuer/venafi/client/venaficlient.go

+//     REST API endpoints.
+//  3. The vcert package does not currently provide an easier way to change those
+//     settings. See:
+//     * https://github.com/Venafi/vcert/issues/437


Set the User-Agent HTTP header in requests to Venafi API server Venafi/vcert#437

wallrj · 2024-03-20T15:16:29Z

pkg/issuer/venafi/client/venaficlient.go

+//  3. The vcert package does not currently provide an easier way to change those
+//     settings. See:
+//     * https://github.com/Venafi/vcert/issues/437
+//     * https://github.com/Venafi/vcert/issues/438


Allow user to set the vcert client TLS renegotiation setting when accessing TPP servers Venafi/vcert#438

wallrj · 2024-03-20T15:44:13Z

/test pull-cert-manager-master-e2e-v1-28-issuers-venafi-tpp

wallrj · 2024-03-20T17:12:56Z

pkg/issuer/venafi/client/venaficlient.go

@@ -183,16 +190,44 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.Se
 			Credentials: &endpoint.Authentication{
 				APIKey: apiKey,
 			},
+			Client: httpClientForVcert(&httpClientForVcertOptions{
+				UserAgent: ptr.To(userAgent),
+			}),


Previously we did not supply a custom http.Client when connecting to Venafi Cloud,
instead relying on the default created by the vcert-sdk.

Here I've refactored and re-used the httpClientForVcert function which had previously only existed for the purpose of configuring the TLS renegotation setting when connecting to TPP.

Now it also allows us to configure the User-Agent header.

wallrj · 2024-03-20T17:39:20Z

pkg/util/useragent.go

+func (u userAgentRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	req.Header.Set("User-Agent", u.userAgent)
+	return u.inner.RoundTrip(req)
+}


This code existed in cert-manager once before and I'm reviving it.
Here's the history:

Added: Add a meaningful User-Agent. #422

Moved: Plumb a user-agent through pretty much everywhere #432

Obsoleted: Refactor ACME client construction into dedicated ACME package #797

Deleted: Remove dead code and add extra comments #966

The only alteration I made was to use req.Header.Set rather than req.Header.Add,
because there should only be a single User-Agent header.

Other examples of this pattern:

https://github.com/jetstack/venafi-connection-lib/pull/124

https://github.com/opentofu/opentofu/blob/230fc89a28dfed5a4b0d6eed43dc8d9fc639d24b/internal/httpclient/useragent.go#L22-L34

The RoundTripper documentation says:

// RoundTrip should not modify the request, except for
// consuming and closing the Request's Body. RoundTrip may
// read fields of the request in a separate goroutine. Callers
// should not mutate or reuse the request until the Response's
// Body has been closed.

http: RoundTripper mutating *Request golang/go#2146

This implementation does mutate the request, but I don't think it will cause any problems.
And ultimately the vcert library may provide a way to set the User-Agent and their implementation will hopefully not use this RoundTripper pattern and instead add the header when they generate each request.
Or similar to how Go ACME package implemented it:

x/crypto/acme: Set a meaningful user-agent golang/go#24496

https://go-review.googlesource.com/c/crypto/+/183267

hawksight

From my stand point this is a great enhancement to get in. The output you provided is far superior IMO than the current default user-agent set.

Can't comment on the code but it does look throughtly implemented. I've be happy to test an alpha release with his feature in.

inteon · 2024-03-24T09:12:30Z

pkg/issuer/venafi/client/venaficlient.go

+// Why is it necessary to create our own HTTP client for vcert?
+//
+//  1. We need to customize the client TLS renegotiation setting when connecting
+//     to certain TPP servers.


Suggested change

// to certain TPP servers.

// to TPP servers that accept mTLS authentication.

I think this was the reason some TPP servers require this option, right?

EDIT: I found that it is explained below, maybe we can add a reference [1] or something similar, so it is clear that the explanation follows.

inteon · 2024-03-24T09:18:07Z

pkg/issuer/venafi/client/venaficlient.go

@@ -217,16 +252,19 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.Se
 //     because cert-manager establishes a new HTTPS connection for each API
 //     request and therefore should only ever need to renegotiate once in this
 //     scenario.
-//  5. But overriding the HTTP client causes vcert to ignore the
+//
+// Why do we supply CA bundle in the HTTP client **and** in the vcert.Config?


Do we still have to pass the cabundle through vcert.Config?
If we always use a custom http client, I think we don't have to pass the CAs through vcert.Config anymore.

EDIT: this comment explains why we pass it twice: https://github.com/cert-manager/cert-manager/pull/6865/files#diff-5f8cbfd3b75b6d8e0f899a92377a3aa5b5fa1867e18d19fcda51f8e8db2793b0L149-L156
Maybe we can refer to that comment here or add some extra info?

inteon · 2024-03-25T10:32:49Z

/approve
/lgtm
/hold In case you want to still fix the comments I added.

jetstack-bot · 2024-03-25T10:32:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawksight, inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wallrj · 2024-03-27T14:19:37Z

Thanks both for your reviews.

@inteon I'll update the comment when I next visit that code....perhaps if Venafi/vcert#443 merges when vcert is next released and I can remove the RoundTripper

/unhold

wallrj · 2024-03-27T14:29:56Z

/kind feature

wallrj added 2 commits March 20, 2024 11:02

Set the User-Agent header in all Venafi API requests

04ee7fe

Signed-off-by: Richard Wall <richard.wall@venafi.com>

Supply User-Agent string to the Venafi controllers

cca333d

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj force-pushed the 5803-cert-manager-user-agent-venafi-issuer branch from 09125e7 to cca333d Compare March 20, 2024 12:11

wallrj added 3 commits March 20, 2024 12:21

Supply tests with a non-nil REST config in controller.Context

95a347c

Signed-off-by: Richard Wall <richard.wall@venafi.com>

Refactor the httpClientForVcert function so that it can also be used …

dd0762e

…for Venafi Cloud Signed-off-by: Richard Wall <richard.wall@venafi.com>

Link to upstream vcert issues that would allow us to simplify the cer…

30db9e2

…t-manager code Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj force-pushed the 5803-cert-manager-user-agent-venafi-issuer branch from b6a52d1 to dd0762e Compare March 20, 2024 14:22

wallrj mentioned this pull request Mar 20, 2024

Set the User-Agent HTTP header in requests to Venafi API server Venafi/vcert#437

Closed

wallrj commented Mar 20, 2024

View reviewed changes

wallrj changed the title ~~WIP: Add user-agent header to Venafi API client~~ Add user-agent header to Venafi API client Mar 20, 2024

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2024

wallrj changed the title ~~Add user-agent header to Venafi API client~~ Add user-agent header in requests to Venafi API Mar 20, 2024

wallrj requested a review from hawksight March 20, 2024 17:49

hawksight approved these changes Mar 22, 2024

View reviewed changes

inteon reviewed Mar 24, 2024

View reviewed changes

jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 25, 2024

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2024

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024

jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Mar 27, 2024

jetstack-bot merged commit b61de55 into cert-manager:master Mar 27, 2024
7 checks passed

wallrj deleted the 5803-cert-manager-user-agent-venafi-issuer branch March 27, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-agent header in requests to Venafi API #6865

Add user-agent header in requests to Venafi API #6865

wallrj commented Mar 20, 2024 •

edited

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024 •

edited

wallrj Mar 20, 2024

wallrj Mar 20, 2024

wallrj commented Mar 20, 2024

wallrj Mar 20, 2024

wallrj Mar 20, 2024

hawksight left a comment

inteon Mar 24, 2024

inteon Mar 24, 2024

inteon Mar 24, 2024 •

edited

inteon Mar 24, 2024

inteon commented Mar 25, 2024

jetstack-bot commented Mar 25, 2024

wallrj commented Mar 27, 2024

wallrj commented Mar 27, 2024

	// to certain TPP servers.
	// to TPP servers that accept mTLS authentication.

Add user-agent header in requests to Venafi API #6865

Add user-agent header in requests to Venafi API #6865

Conversation

wallrj commented Mar 20, 2024 • edited

TLDR

Details

Testing

Local testing

E2E tests

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024

wallrj commented Mar 20, 2024 • edited

wallrj Mar 20, 2024

Choose a reason for hiding this comment

wallrj Mar 20, 2024

Choose a reason for hiding this comment

wallrj commented Mar 20, 2024

wallrj Mar 20, 2024

Choose a reason for hiding this comment

wallrj Mar 20, 2024

Choose a reason for hiding this comment

hawksight left a comment

Choose a reason for hiding this comment

inteon Mar 24, 2024

Choose a reason for hiding this comment

inteon Mar 24, 2024

Choose a reason for hiding this comment

inteon Mar 24, 2024 • edited

Choose a reason for hiding this comment

inteon Mar 24, 2024

Choose a reason for hiding this comment

inteon commented Mar 25, 2024

jetstack-bot commented Mar 25, 2024

wallrj commented Mar 27, 2024

wallrj commented Mar 27, 2024

wallrj commented Mar 20, 2024 •

edited

wallrj commented Mar 20, 2024 •

edited

inteon Mar 24, 2024 •

edited