Merge pull request #6865 from wallrj/5803-cert-manager-user-agent-ven…

…afi-issuer

Add user-agent header in requests to Venafi API

pkg/controller/certificaterequests/venafi/venafi.go

@@ -54,6 +54,9 @@ type Venafi struct {
  clientBuilder venaficlient.VenafiClientBuilder
 
  metrics *metrics.Metrics
+
+ // userAgent is the string used as the UserAgent when making HTTP calls.
+ userAgent string
 }
 
 func init() {
@@ -73,14 +76,15 @@ func NewVenafi(ctx *controllerpkg.Context) certificaterequests.Issuer {
  clientBuilder: venaficlient.New,
  metrics: ctx.Metrics,
  cmClient: ctx.CMClient,
+ userAgent: ctx.RESTConfig.UserAgent,
  }
 }
 
 func (v *Venafi) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerObj cmapi.GenericIssuer) (*issuerpkg.IssueResponse, error) {
  log := logf.FromContext(ctx, "sign")
  log = logf.WithRelatedResource(log, issuerObj)
 
- client, err := v.clientBuilder(v.issuerOptions.ResourceNamespace(issuerObj), v.secretsLister, issuerObj, v.metrics, log)
+ client, err := v.clientBuilder(v.issuerOptions.ResourceNamespace(issuerObj), v.secretsLister, issuerObj, v.metrics, log, v.userAgent)
  if k8sErrors.IsNotFound(err) {
  message := "Required secret resource not found"
 

pkg/controller/certificaterequests/venafi/venafi_test.go

@@ -813,7 +813,7 @@ type testT struct {
 
 func runTest(t *testing.T, test testT) {
  test.builder.T = t
- test.builder.Init()
+ test.builder.InitWithRESTConfig()
  defer test.builder.Stop()
 
  v := NewVenafi(test.builder.Context).(*Venafi)
@@ -824,7 +824,7 @@ func runTest(t *testing.T, test testT) {
 
  if test.fakeClient != nil {
  v.clientBuilder = func(namespace string, secretsLister internalinformers.SecretLister,
- issuer cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (client.Interface, error) {
+ issuer cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (client.Interface, error) {
  return test.fakeClient, nil
  }
  }

pkg/controller/certificatesigningrequests/venafi/venafi.go

@@ -63,6 +63,9 @@ type Venafi struct {
 
  // fieldManager is the manager name used for the Apply operations.
  fieldManager string
+
+ // userAgent is the string used as the UserAgent when making HTTP calls.
+ userAgent string
 }
 
 func init() {
@@ -82,6 +85,7 @@ func NewVenafi(ctx *controllerpkg.Context) certificatesigningrequests.Signer {
  clientBuilder: venaficlient.New,
  fieldManager: ctx.FieldManager,
  metrics: ctx.Metrics,
+ userAgent: ctx.RESTConfig.UserAgent,
  }
 }
 
@@ -99,7 +103,7 @@ func (v *Venafi) Sign(ctx context.Context, csr *certificatesv1.CertificateSignin
 
  resourceNamespace := v.issuerOptions.ResourceNamespace(issuerObj)
 
- client, err := v.clientBuilder(resourceNamespace, v.secretsLister, issuerObj, v.metrics, log)
+ client, err := v.clientBuilder(resourceNamespace, v.secretsLister, issuerObj, v.metrics, log, v.userAgent)
  if apierrors.IsNotFound(err) {
  message := "Required secret resource not found"
  v.recorder.Event(csr, corev1.EventTypeWarning, "SecretNotFound", message)

pkg/controller/certificatesigningrequests/venafi/venafi_test.go

@@ -164,7 +164,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return nil, apierrors.NewNotFound(schema.GroupResource{}, "test-secret")
  },
  builder: &testpkg.Builder{
@@ -206,7 +206,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return nil, errors.New("generic error")
  },
  expectedErr: true,
@@ -252,7 +252,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{}, nil
  },
  builder: &testpkg.Builder{
@@ -320,7 +320,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{}, nil
  },
  builder: &testpkg.Builder{
@@ -388,7 +388,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RequestCertificateFn: func(_ []byte, _ time.Duration, _ []venafiapi.CustomField) (string, error) {
  return "", venaficlient.ErrCustomFieldsType{Type: "test-type"}
@@ -459,7 +459,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RequestCertificateFn: func(_ []byte, _ time.Duration, _ []venafiapi.CustomField) (string, error) {
  return "", errors.New("generic error")
@@ -530,7 +530,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RequestCertificateFn: func(_ []byte, _ time.Duration, _ []venafiapi.CustomField) (string, error) {
  return "test-pickup-id", nil
@@ -592,7 +592,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RetrieveCertificateFn: func(_ string, _ []byte, _ time.Duration, _ []venafiapi.CustomField) ([]byte, error) {
  return nil, endpoint.ErrCertificatePending{}
@@ -643,7 +643,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RetrieveCertificateFn: func(_ string, _ []byte, _ time.Duration, _ []venafiapi.CustomField) ([]byte, error) {
  return nil, endpoint.ErrRetrieveCertificateTimeout{}
@@ -694,7 +694,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RetrieveCertificateFn: func(_ string, _ []byte, _ time.Duration, _ []venafiapi.CustomField) ([]byte, error) {
  return nil, errors.New("generic error")
@@ -745,7 +745,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RetrieveCertificateFn: func(_ string, _ []byte, _ time.Duration, _ []venafiapi.CustomField) ([]byte, error) {
  return []byte("garbage"), nil
@@ -818,7 +818,7 @@ func TestProcessItem(t *testing.T) {
  Status: corev1.ConditionTrue,
  }),
  ),
- clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger) (venaficlient.Interface, error) {
+ clientBuilder: func(_ string, _ internalinformers.SecretLister, _ cmapi.GenericIssuer, _ *metrics.Metrics, _ logr.Logger, _ string) (venaficlient.Interface, error) {
  return &fakevenaficlient.Venafi{
  RetrieveCertificateFn: func(_ string, _ []byte, _ time.Duration, _ []venafiapi.CustomField) ([]byte, error) {
  return []byte(fmt.Sprintf("%s%s", certBundle.ChainPEM, certBundle.CAPEM)), nil
@@ -884,7 +884,7 @@ func TestProcessItem(t *testing.T) {
  fixedClock.SetTime(fixedClockStart)
  test.builder.Clock = fixedClock
  test.builder.T = t
- test.builder.Init()
+ test.builder.InitWithRESTConfig()
 
  // Always return true for SubjectAccessReviews in tests
  test.builder.FakeKubeClient().PrependReactor("create", "*", func(action coretesting.Action) (bool, runtime.Object, error) {

pkg/issuer/venafi/client/venaficlient.go

@@ -30,11 +30,13 @@ import (
  "github.com/Venafi/vcert/v5/pkg/venafi/cloud"
  "github.com/Venafi/vcert/v5/pkg/venafi/tpp"
  "github.com/go-logr/logr"
+ "k8s.io/utils/ptr"
 
  internalinformers "github.com/cert-manager/cert-manager/internal/informers"
  cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
  "github.com/cert-manager/cert-manager/pkg/issuer/venafi/client/api"
  "github.com/cert-manager/cert-manager/pkg/metrics"
+ "github.com/cert-manager/cert-manager/pkg/util"
 )
 
 const (
@@ -46,7 +48,7 @@ const (
 )
 
 type VenafiClientBuilder func(namespace string, secretsLister internalinformers.SecretLister,
- issuer cmapi.GenericIssuer, metrics *metrics.Metrics, logger logr.Logger) (Interface, error)
+ issuer cmapi.GenericIssuer, metrics *metrics.Metrics, logger logr.Logger, userAgent string) (Interface, error)
 
 // Interface implements a Venafi client
 type Interface interface {
@@ -85,8 +87,8 @@ type connector interface {
 
 // New constructs a Venafi client Interface. Errors may be network errors and
 // should be considered for retrying.
-func New(namespace string, secretsLister internalinformers.SecretLister, issuer cmapi.GenericIssuer, metrics *metrics.Metrics, logger logr.Logger) (Interface, error) {
- cfg, err := configForIssuer(issuer, secretsLister, namespace)
+func New(namespace string, secretsLister internalinformers.SecretLister, issuer cmapi.GenericIssuer, metrics *metrics.Metrics, logger logr.Logger, userAgent string) (Interface, error) {
+ cfg, err := configForIssuer(issuer, secretsLister, namespace, userAgent)
  if err != nil {
  return nil, err
  }
@@ -126,8 +128,9 @@ func New(namespace string, secretsLister internalinformers.SecretLister, issuer
 
 // configForIssuer will convert a cert-manager Venafi issuer into a vcert.Config
 // that can be used to instantiate an API client.
-func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.SecretLister, namespace string) (*vcert.Config, error) {
+func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.SecretLister, namespace string, userAgent string) (*vcert.Config, error) {
  venCfg := iss.GetSpec().Venafi
+
  switch {
  case venCfg.TPP != nil:
  tpp := venCfg.TPP
@@ -159,7 +162,11 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.Se
  Password: password,
  AccessToken: accessToken,
  },
- Client: httpClientForVcertTPP(tpp.CABundle),
+ Client: httpClientForVcert(&httpClientForVcertOptions{
+ UserAgent: ptr.To(userAgent),
+ CABundle: tpp.CABundle,
+ TLSRenegotiationSupport: ptr.To(tls.RenegotiateOnceAsClient),
+ }),
  }, nil
  case venCfg.Cloud != nil:
  cloud := venCfg.Cloud
@@ -183,16 +190,44 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister internalinformers.Se
  Credentials: &endpoint.Authentication{
  APIKey: apiKey,
  },
+ Client: httpClientForVcert(&httpClientForVcertOptions{
+ UserAgent: ptr.To(userAgent),
+ }),
  }, nil
  }
  // API validation in webhook and in the ClusterIssuer and Issuer controller
  // Sync functions should make this unreachable in production.
  return nil, fmt.Errorf("neither Venafi Cloud or TPP configuration found")
 }
 
-// httpClientForVcertTPP creates an HTTP client and customises it to allow client TLS renegotiation.
+// httpClientForVcertOptions contains options for `httpClientForVcert`, to allow
+// you to customize the HTTP client.
+type httpClientForVcertOptions struct {
+ // UserAgent will add a User-Agent header to all HTTP requests.
+ UserAgent *string
+ // CABundle will override the CA certificates used to verify server
+ // certificates.
+ CABundle []byte
+ // TLSRenegotiationSupport will override the TLSRenegotiationSupport setting
+ // of the client.
+ TLSRenegotiationSupport *tls.RenegotiationSupport
+}
+
+// httpClientForVcert creates an HTTP client which matches the default HTTP client of vcert,
+// but allows you to customize client TLS renegotiation, and User-Agent.
+//
+// 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.
+// 2. We need to customize the User-Agent header for all HTTP requests to Venafi
+// 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
+// * https://github.com/Venafi/vcert/issues/438
 //
-// Here's why:
+// Why is it necessary to customize the client TLS renegotiation?
 //
 // 1. The TPP API server is served by Microsoft Windows Server and IIS.
 // 2. IIS uses TLS-1.2 by default[1] and it uses a
@@ -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?
+//
+// 1. Overriding the HTTP client causes vcert to ignore the
 // `vcert.Config.ConnectionTrust` field, so we also have to set up the root
 // CA trust pool ourselves.
-// 6. And the value of RootCAs MUST be nil unless the user has supplied a
+// 2. And the value of RootCAs MUST be nil unless the user has supplied a
 // custom CA, because a nil value causes the Go HTTP client to load the
 // system default root CAs.
 //
 // [1] TLS protocol version support in Microsoft Windows: https://learn.microsoft.com/en-us/windows/win32/secauthn/protocols-in-tls-ssl--schannel-ssp-#tls-protocol-version-support
 // [2] Should I use SSL/TLS renegotiation?: https://security.stackexchange.com/a/24569
-func httpClientForVcertTPP(caBundle []byte) *http.Client {
+func httpClientForVcert(options *httpClientForVcertOptions) *http.Client {
  // Copy vcert's default HTTP transport, which is mostly identical to the
  // http.DefaultTransport settings in Go's stdlib.
  // https://github.com/Venafi/vcert/blob/89645a7710a7b529765274cb60dc5e28066217a1/pkg/venafi/tpp/tpp.go#L481-L513
@@ -250,20 +288,26 @@ func httpClientForVcertTPP(caBundle []byte) *http.Client {
  if tlsClientConfig == nil {
  tlsClientConfig = &tls.Config{}
  }
- if len(caBundle) > 0 {
+ if len(options.CABundle) > 0 {
  rootCAs := x509.NewCertPool()
- rootCAs.AppendCertsFromPEM(caBundle)
+ rootCAs.AppendCertsFromPEM(options.CABundle)
  tlsClientConfig.RootCAs = rootCAs
  }
  transport.TLSClientConfig = tlsClientConfig
 
- // Enable TLS 1.2 renegotiation (see earlier comment for justification).
- transport.TLSClientConfig.Renegotiation = tls.RenegotiateOnceAsClient
+ if options.TLSRenegotiationSupport != nil {
+ transport.TLSClientConfig.Renegotiation = *options.TLSRenegotiationSupport
+ }
+
+ var roundTripper http.RoundTripper = transport
+ if options.UserAgent != nil {
+ roundTripper = util.UserAgentRoundTripper(transport, *options.UserAgent)
+ }
 
  // Copy vcert's initialization of the HTTP client, which overrides the default timeout.
  // https://github.com/Venafi/vcert/blob/89645a7710a7b529765274cb60dc5e28066217a1/pkg/venafi/tpp/tpp.go#L481-L513
  return &http.Client{
- Transport: transport,
+ Transport: roundTripper,
  Timeout: time.Second * 30,
  }
 }

pkg/issuer/venafi/client/venaficlient_test.go

@@ -223,7 +223,7 @@ type testConfigForIssuerT struct {
 }
 
 func (c *testConfigForIssuerT) runTest(t *testing.T) {
- resp, err := configForIssuer(c.iss, c.secretsLister, "test-namespace")
+ resp, err := configForIssuer(c.iss, c.secretsLister, "test-namespace", "cert-manager/v0.0.0")
  if err != nil && !c.expectedErr {
  t.Errorf("expected to not get an error, but got: %v", err)
  }

pkg/issuer/venafi/setup.go

@@ -38,7 +38,7 @@ func (v *Venafi) Setup(ctx context.Context) (err error) {
  }
  }()
 
- client, err := v.clientBuilder(v.resourceNamespace, v.secretsLister, v.issuer, v.Metrics, v.log)
+ client, err := v.clientBuilder(v.resourceNamespace, v.secretsLister, v.issuer, v.Metrics, v.log, v.userAgent)
  if err != nil {
  return fmt.Errorf("error building client: %v", err)
  }