From ac9895d91b79c70f2b602af0b841e9574765641a Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 26 May 2020 16:02:38 +0200 Subject: [PATCH] Make Venafi issuer async Signed-off-by: Maartje Eyskens --- .../certificaterequests/venafi/BUILD.bazel | 10 +- .../certificaterequests/venafi/venafi.go | 68 ++++++-- .../certificaterequests/venafi/venafi_test.go | 24 +-- pkg/internal/BUILD.bazel | 1 - pkg/internal/venafi/api/BUILD.bazel | 22 --- pkg/issuer/venafi/BUILD.bazel | 11 +- .../venafi/client}/BUILD.bazel | 18 +- .../venafi/client/customfield.go} | 2 +- .../venafi/client}/fake/BUILD.bazel | 4 +- .../venafi/client}/fake/connector.go | 0 .../venafi/client}/fake/venafi.go | 8 +- .../venafi/client/request.go} | 160 ++++++++++-------- .../venafi/client/request_test.go} | 11 +- .../venafi/client/venaficlient.go} | 54 +++--- .../venafi/client/venaficlient_test.go} | 2 +- pkg/issuer/venafi/setup_test.go | 13 +- pkg/issuer/venafi/venafi.go | 6 +- 17 files changed, 222 insertions(+), 192 deletions(-) delete mode 100644 pkg/internal/venafi/api/BUILD.bazel rename pkg/{internal/venafi => issuer/venafi/client}/BUILD.bazel (79%) rename pkg/{internal/venafi/api/api.go => issuer/venafi/client/customfield.go} (98%) rename pkg/{internal/venafi => issuer/venafi/client}/fake/BUILD.bazel (83%) rename pkg/{internal/venafi => issuer/venafi/client}/fake/connector.go (100%) rename pkg/{internal/venafi => issuer/venafi/client}/fake/venafi.go (78%) rename pkg/{internal/venafi/sign.go => issuer/venafi/client/request.go} (63%) rename pkg/{internal/venafi/sign_test.go => issuer/venafi/client/request_test.go} (94%) rename pkg/{internal/venafi/venafi.go => issuer/venafi/client/venaficlient.go} (79%) rename pkg/{internal/venafi/venafi_test.go => issuer/venafi/client/venaficlient_test.go} (99%) diff --git a/pkg/controller/certificaterequests/venafi/BUILD.bazel b/pkg/controller/certificaterequests/venafi/BUILD.bazel index cea41fcec..d29cd71b6 100644 --- a/pkg/controller/certificaterequests/venafi/BUILD.bazel +++ b/pkg/controller/certificaterequests/venafi/BUILD.bazel @@ -8,15 +8,16 @@ go_library( deps = [ "//pkg/api/util:go_default_library", "//pkg/apis/certmanager/v1alpha2:go_default_library", + "//pkg/client/clientset/versioned:go_default_library", "//pkg/controller:go_default_library", "//pkg/controller/certificaterequests:go_default_library", "//pkg/controller/certificaterequests/util:go_default_library", - "//pkg/internal/venafi:go_default_library", - "//pkg/internal/venafi/api:go_default_library", "//pkg/issuer:go_default_library", + "//pkg/issuer/venafi/client:go_default_library", "//pkg/logs:go_default_library", "@com_github_venafi_vcert//pkg/endpoint:go_default_library", "@io_k8s_apimachinery//pkg/api/errors:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_client_go//listers/core/v1:go_default_library", ], ) @@ -46,9 +47,8 @@ go_test( "//pkg/apis/meta/v1:go_default_library", "//pkg/controller/certificaterequests:go_default_library", "//pkg/controller/test:go_default_library", - "//pkg/internal/venafi:go_default_library", - "//pkg/internal/venafi/api:go_default_library", - "//pkg/internal/venafi/fake:go_default_library", + "//pkg/issuer/venafi/client:go_default_library", + "//pkg/issuer/venafi/client/fake:go_default_library", "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", "//test/unit/listers:go_default_library", diff --git a/pkg/controller/certificaterequests/venafi/venafi.go b/pkg/controller/certificaterequests/venafi/venafi.go index aff04d131..7b5cd7943 100644 --- a/pkg/controller/certificaterequests/venafi/venafi.go +++ b/pkg/controller/certificaterequests/venafi/venafi.go @@ -14,13 +14,18 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "context" "encoding/json" "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + clientset "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" + venaficlient "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" + "github.com/Venafi/vcert/pkg/endpoint" k8sErrors "k8s.io/apimachinery/pkg/api/errors" corelisters "k8s.io/client-go/listers/core/v1" @@ -30,22 +35,22 @@ import ( controllerpkg "github.com/jetstack/cert-manager/pkg/controller" "github.com/jetstack/cert-manager/pkg/controller/certificaterequests" crutil "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util" - venafiinternal "github.com/jetstack/cert-manager/pkg/internal/venafi" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" issuerpkg "github.com/jetstack/cert-manager/pkg/issuer" logf "github.com/jetstack/cert-manager/pkg/logs" ) const ( - CRControllerName = "certificaterequests-issuer-venafi" + CRControllerName = "certificaterequests-issuer-venafi" + VenafiPickupIDAnnotation = "venafi.cert-manager.io/pickupid" ) type Venafi struct { issuerOptions controllerpkg.IssuerOptions secretsLister corelisters.SecretLister reporter *crutil.Reporter + cmClient clientset.Interface - clientBuilder venafiinternal.VenafiClientBuilder + clientBuilder venaficlient.VenafiClientBuilder } func init() { @@ -62,7 +67,8 @@ func NewVenafi(ctx *controllerpkg.Context) *Venafi { issuerOptions: ctx.IssuerOptions, secretsLister: ctx.KubeSharedInformerFactory.Core().V1().Secrets().Lister(), reporter: crutil.NewReporter(ctx.Clock, ctx.Recorder), - clientBuilder: venafiinternal.New, + clientBuilder: venaficlient.New, + cmClient: ctx.CMClient, } } @@ -91,7 +97,7 @@ func (v *Venafi) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerO duration := apiutil.DefaultCertDuration(cr.Spec.Duration) - var customFields []internalvanafiapi.CustomField + var customFields []venaficlient.CustomField if annotation, exists := cr.GetAnnotations()[cmapi.VenafiCustomFieldsAnnotationKey]; exists && annotation != "" { err := json.Unmarshal([]byte(annotation), &customFields) if err != nil { @@ -104,18 +110,45 @@ func (v *Venafi) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerO } } - certPem, err := client.Sign(cr.Spec.CSRPEM, duration, customFields) + pickupID, exists := cr.ObjectMeta.Annotations[VenafiPickupIDAnnotation] - // Check some known error types + // check if the pickup ID annotation is there, if not set it up. + if !exists || pickupID == "" { + pickupID, err = client.RequestCertificate(cr.Spec.CSRPEM, duration, customFields) + // Check some known error types + if err != nil { + switch err.(type) { + + case venaficlient.ErrCustomFieldsType: + v.reporter.Failed(cr, err, "CustomFieldsError", err.Error()) + log.Error(err, err.Error()) + + return nil, nil + + default: + message := "Failed to request venafi certificate" + + v.reporter.Failed(cr, err, "RequestError", message) + log.Error(err, message) + + return nil, err + } + } + + cr.ObjectMeta.Annotations[VenafiPickupIDAnnotation] = pickupID + _, err = v.cmClient.CertmanagerV1alpha2().CertificateRequests(cr.GetNamespace()).Update(ctx, cr, metav1.UpdateOptions{}) + if err != nil { + return nil, err + } + + v.reporter.Pending(cr, err, "IssuancePending", "Venafi certificate is requested") + return nil, nil + } + + certPem, err := client.RetreiveCertificate(pickupID, cr.Spec.CSRPEM, duration, customFields) if err != nil { switch err.(type) { - case venafiinternal.ErrCustomFieldsType: - v.reporter.Failed(cr, err, "CustomFieldsError", err.Error()) - log.Error(err, err.Error()) - - return nil, nil - case endpoint.ErrCertificatePending: message := "Venafi certificate still in a pending state, the request will be retried" @@ -124,9 +157,8 @@ func (v *Venafi) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerO return nil, err case endpoint.ErrRetrieveCertificateTimeout: - message := "Timed out waiting for venafi certificate, the request will be retried" - - v.reporter.Failed(cr, err, "Timeout", message) + message := "Timed out waiting for venafi certificate, the retreive request will be retried" + v.reporter.Pending(cr, err, "Timeout", message) log.Error(err, message) return nil, nil diff --git a/pkg/controller/certificaterequests/venafi/venafi_test.go b/pkg/controller/certificaterequests/venafi/venafi_test.go index e0ce578e8..3da400e73 100644 --- a/pkg/controller/certificaterequests/venafi/venafi_test.go +++ b/pkg/controller/certificaterequests/venafi/venafi_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "context" @@ -27,6 +27,8 @@ import ( "testing" "time" + "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" + "github.com/Venafi/vcert/pkg/endpoint" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,9 +44,7 @@ import ( "github.com/jetstack/cert-manager/pkg/controller/certificaterequests" controllertest "github.com/jetstack/cert-manager/pkg/controller/test" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" - internalvenafi "github.com/jetstack/cert-manager/pkg/internal/venafi" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" - internalvenafifake "github.com/jetstack/cert-manager/pkg/internal/venafi/fake" + internalvenafifake "github.com/jetstack/cert-manager/pkg/issuer/venafi/client/fake" "github.com/jetstack/cert-manager/pkg/util/pki" "github.com/jetstack/cert-manager/test/unit/gen" testlisters "github.com/jetstack/cert-manager/test/unit/listers" @@ -184,7 +184,7 @@ func TestSign(t *testing.T) { } clientReturnsPending := &internalvenafifake.Venafi{ - SignFn: func([]byte, time.Duration, []internalvanafiapi.CustomField) ([]byte, error) { + SignFn: func([]byte, time.Duration, []client.CustomField) ([]byte, error) { return nil, endpoint.ErrCertificatePending{ CertificateID: "test-cert-id", Status: "test-status-pending", @@ -192,25 +192,25 @@ func TestSign(t *testing.T) { }, } clientReturnsTimeout := &internalvenafifake.Venafi{ - SignFn: func([]byte, time.Duration, []internalvanafiapi.CustomField) ([]byte, error) { + SignFn: func([]byte, time.Duration, []client.CustomField) ([]byte, error) { return nil, endpoint.ErrRetrieveCertificateTimeout{ CertificateID: "test-cert-id", } }, } clientReturnsGenericError := &internalvenafifake.Venafi{ - SignFn: func([]byte, time.Duration, []internalvanafiapi.CustomField) ([]byte, error) { + SignFn: func([]byte, time.Duration, []client.CustomField) ([]byte, error) { return nil, errors.New("this is an error") }, } clientReturnsCert := &internalvenafifake.Venafi{ - SignFn: func([]byte, time.Duration, []internalvanafiapi.CustomField) ([]byte, error) { + SignFn: func([]byte, time.Duration, []client.CustomField) ([]byte, error) { return certPEM, nil }, } clientReturnsCertIfCustomField := &internalvenafifake.Venafi{ - SignFn: func(csr []byte, t time.Duration, fields []internalvanafiapi.CustomField) ([]byte, error) { + SignFn: func(csr []byte, t time.Duration, fields []client.CustomField) ([]byte, error) { if len(fields) > 0 && fields[0].Name == "cert-manager-test" && fields[0].Value == "test ok" { return certPEM, nil } @@ -219,8 +219,8 @@ func TestSign(t *testing.T) { } clientReturnsInvalidCustomFieldType := &internalvenafifake.Venafi{ - SignFn: func(csr []byte, t time.Duration, fields []internalvanafiapi.CustomField) ([]byte, error) { - return nil, internalvenafi.ErrCustomFieldsType{Type: fields[0].Type} + SignFn: func(csr []byte, t time.Duration, fields []client.CustomField) ([]byte, error) { + return nil, client.ErrCustomFieldsType{Type: fields[0].Type} }, } @@ -721,7 +721,7 @@ func runTest(t *testing.T, test testT) { if test.fakeClient != nil { v.clientBuilder = func(namespace string, secretsLister corelisters.SecretLister, - issuer cmapi.GenericIssuer) (internalvenafi.Interface, error) { + issuer cmapi.GenericIssuer) (client.Interface, error) { return test.fakeClient, nil } } diff --git a/pkg/internal/BUILD.bazel b/pkg/internal/BUILD.bazel index 0a3e94ed3..8e209c44d 100644 --- a/pkg/internal/BUILD.bazel +++ b/pkg/internal/BUILD.bazel @@ -14,7 +14,6 @@ filegroup( "//pkg/internal/apis/certmanager:all-srcs", "//pkg/internal/apis/meta:all-srcs", "//pkg/internal/vault:all-srcs", - "//pkg/internal/venafi:all-srcs", ], tags = ["automanaged"], visibility = ["//visibility:public"], diff --git a/pkg/internal/venafi/api/BUILD.bazel b/pkg/internal/venafi/api/BUILD.bazel deleted file mode 100644 index 4f4e15082..000000000 --- a/pkg/internal/venafi/api/BUILD.bazel +++ /dev/null @@ -1,22 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -filegroup( - name = "package-srcs", - srcs = glob(["**"]), - tags = ["automanaged"], - visibility = ["//visibility:private"], -) - -filegroup( - name = "all-srcs", - srcs = [":package-srcs"], - tags = ["automanaged"], - visibility = ["//visibility:public"], -) - -go_library( - name = "go_default_library", - srcs = ["api.go"], - importpath = "github.com/jetstack/cert-manager/pkg/internal/venafi/api", - visibility = ["//pkg:__subpackages__"], -) diff --git a/pkg/issuer/venafi/BUILD.bazel b/pkg/issuer/venafi/BUILD.bazel index 45e347a34..82066ff06 100644 --- a/pkg/issuer/venafi/BUILD.bazel +++ b/pkg/issuer/venafi/BUILD.bazel @@ -13,8 +13,8 @@ go_library( "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/apis/meta/v1:go_default_library", "//pkg/controller:go_default_library", - "//pkg/internal/venafi:go_default_library", "//pkg/issuer:go_default_library", + "//pkg/issuer/venafi/client:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_client_go//listers/core/v1:go_default_library", "@io_k8s_klog//:go_default_library", @@ -30,7 +30,10 @@ filegroup( filegroup( name = "all-srcs", - srcs = [":package-srcs"], + srcs = [ + ":package-srcs", + "//pkg/issuer/venafi/client:all-srcs", + ], tags = ["automanaged"], visibility = ["//visibility:public"], ) @@ -43,8 +46,8 @@ go_test( "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/controller:go_default_library", "//pkg/controller/test:go_default_library", - "//pkg/internal/venafi:go_default_library", - "//pkg/internal/venafi/fake:go_default_library", + "//pkg/issuer/venafi/client:go_default_library", + "//pkg/issuer/venafi/client/fake:go_default_library", "//pkg/util:go_default_library", "//test/unit/gen:go_default_library", "@io_k8s_client_go//listers/core/v1:go_default_library", diff --git a/pkg/internal/venafi/BUILD.bazel b/pkg/issuer/venafi/client/BUILD.bazel similarity index 79% rename from pkg/internal/venafi/BUILD.bazel rename to pkg/issuer/venafi/client/BUILD.bazel index 7ab70866e..876854d67 100644 --- a/pkg/internal/venafi/BUILD.bazel +++ b/pkg/issuer/venafi/client/BUILD.bazel @@ -3,14 +3,14 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ - "sign.go", - "venafi.go", + "customfield.go", + "request.go", + "venaficlient.go", ], - importpath = "github.com/jetstack/cert-manager/pkg/internal/venafi", + importpath = "github.com/jetstack/cert-manager/pkg/issuer/venafi/client", visibility = ["//pkg:__subpackages__"], deps = [ "//pkg/apis/certmanager/v1alpha2:go_default_library", - "//pkg/internal/venafi/api:go_default_library", "//pkg/util/pki:go_default_library", "@com_github_venafi_vcert//:go_default_library", "@com_github_venafi_vcert//pkg/certificate:go_default_library", @@ -30,8 +30,7 @@ filegroup( name = "all-srcs", srcs = [ ":package-srcs", - "//pkg/internal/venafi/api:all-srcs", - "//pkg/internal/venafi/fake:all-srcs", + "//pkg/issuer/venafi/client/fake:all-srcs", ], tags = ["automanaged"], visibility = ["//visibility:public"], @@ -40,15 +39,14 @@ filegroup( go_test( name = "go_default_test", srcs = [ - "sign_test.go", - "venafi_test.go", + "request_test.go", + "venaficlient_test.go", ], embed = [":go_default_library"], deps = [ "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/apis/meta/v1:go_default_library", - "//pkg/internal/venafi/api:go_default_library", - "//pkg/internal/venafi/fake:go_default_library", + "//pkg/issuer/venafi/client/fake:go_default_library", "//pkg/util:go_default_library", "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", diff --git a/pkg/internal/venafi/api/api.go b/pkg/issuer/venafi/client/customfield.go similarity index 98% rename from pkg/internal/venafi/api/api.go rename to pkg/issuer/venafi/client/customfield.go index e2e44c661..c69f60310 100644 --- a/pkg/internal/venafi/api/api.go +++ b/pkg/issuer/venafi/client/customfield.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package api +package client type CustomFieldType string diff --git a/pkg/internal/venafi/fake/BUILD.bazel b/pkg/issuer/venafi/client/fake/BUILD.bazel similarity index 83% rename from pkg/internal/venafi/fake/BUILD.bazel rename to pkg/issuer/venafi/client/fake/BUILD.bazel index c1025b6bd..1d578ddfb 100644 --- a/pkg/internal/venafi/fake/BUILD.bazel +++ b/pkg/issuer/venafi/client/fake/BUILD.bazel @@ -6,10 +6,10 @@ go_library( "connector.go", "venafi.go", ], - importpath = "github.com/jetstack/cert-manager/pkg/internal/venafi/fake", + importpath = "github.com/jetstack/cert-manager/pkg/issuer/venafi/client/fake", visibility = ["//pkg:__subpackages__"], deps = [ - "//pkg/internal/venafi/api:go_default_library", + "//pkg/issuer/venafi/client:go_default_library", "@com_github_venafi_vcert//pkg/certificate:go_default_library", "@com_github_venafi_vcert//pkg/endpoint:go_default_library", "@com_github_venafi_vcert//pkg/venafi/fake:go_default_library", diff --git a/pkg/internal/venafi/fake/connector.go b/pkg/issuer/venafi/client/fake/connector.go similarity index 100% rename from pkg/internal/venafi/fake/connector.go rename to pkg/issuer/venafi/client/fake/connector.go diff --git a/pkg/internal/venafi/fake/venafi.go b/pkg/issuer/venafi/client/fake/venafi.go similarity index 78% rename from pkg/internal/venafi/fake/venafi.go rename to pkg/issuer/venafi/client/fake/venafi.go index f2f333c1d..e89a8c96f 100644 --- a/pkg/internal/venafi/fake/venafi.go +++ b/pkg/issuer/venafi/client/fake/venafi.go @@ -19,14 +19,14 @@ package fake import ( "time" - "github.com/Venafi/vcert/pkg/endpoint" + "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" + "github.com/Venafi/vcert/pkg/endpoint" ) type Venafi struct { PingFn func() error - SignFn func([]byte, time.Duration, []internalvanafiapi.CustomField) ([]byte, error) + SignFn func([]byte, time.Duration, []client.CustomField) ([]byte, error) ReadZoneConfigurationFn func() (*endpoint.ZoneConfiguration, error) } @@ -34,7 +34,7 @@ func (v *Venafi) Ping() error { return v.PingFn() } -func (v *Venafi) Sign(b []byte, t time.Duration, f []internalvanafiapi.CustomField) ([]byte, error) { +func (v *Venafi) Sign(b []byte, t time.Duration, f []client.CustomField) ([]byte, error) { return v.SignFn(b, t, f) } diff --git a/pkg/internal/venafi/sign.go b/pkg/issuer/venafi/client/request.go similarity index 63% rename from pkg/internal/venafi/sign.go rename to pkg/issuer/venafi/client/request.go index ee1c06bbd..21543b89b 100644 --- a/pkg/internal/venafi/sign.go +++ b/pkg/issuer/venafi/client/request.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "crypto/x509" @@ -25,13 +25,12 @@ import ( "github.com/Venafi/vcert/pkg/certificate" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" "github.com/jetstack/cert-manager/pkg/util/pki" ) // ErrCustomFieldsType provides a common error structure for an invalid Venafi custom field type type ErrCustomFieldsType struct { - Type internalvanafiapi.CustomFieldType + Type CustomFieldType } func (err ErrCustomFieldsType) Error() string { @@ -43,11 +42,45 @@ var ErrorMissingSubject = errors.New("Certificate requests submitted to Venafi i // This function sends a request to Venafi to for a signed certificate. // The CSR will be decoded to be validated against the zone configuration policy. // Upon the template being successfully defaulted and validated, the CSR will be sent, as is. -func (v *Venafi) Sign(csrPEM []byte, duration time.Duration, customFields []internalvanafiapi.CustomField) (cert []byte, err error) { +// It will return a pickup ID which can be used with RetreiveCertificate to get the certificate +func (v *Venafi) RequestCertificate(csrPEM []byte, duration time.Duration, customFields []CustomField) (string, error) { + vreq, err := v.buildVReq(csrPEM, duration, customFields) + if err != nil { + return "", err + } + // Send the certificate signing request to Venafi + requestID, err := v.vcertClient.RequestCertificate(vreq) + return requestID, err +} + +func (v *Venafi) RetreiveCertificate(pickupID string, csrPEM []byte, duration time.Duration, customFields []CustomField) ([]byte, error) { + vreq, err := v.buildVReq(csrPEM, duration, customFields) + if err != nil { + return nil, err + } + + vreq.PickupID = pickupID + // TODO: better set the timeout here. Right now, we'll block for this amount of time. + vreq.Timeout = time.Minute * 5 + + // Retrieve the certificate from request + pemCollection, err := v.vcertClient.RetrieveCertificate(vreq) + if err != nil { + return nil, err + } + + // Construct the certificate chain and return the new keypair + cs := append([]string{pemCollection.Certificate}, pemCollection.Chain...) + chain := strings.Join(cs, "\n") + + return []byte(chain), nil +} + +func (v *Venafi) buildVReq(csrPEM []byte, duration time.Duration, customFields []CustomField) (*certificate.Request, error) { // Retrieve a copy of the Venafi zone. // This contains default values and policy control info that we can apply // and check against locally. - zoneCfg, err := v.client.ReadZoneConfiguration() + zoneCfg, err := v.vcertClient.ReadZoneConfiguration() if err != nil { return nil, err } @@ -64,32 +97,12 @@ func (v *Venafi) Sign(csrPEM []byte, duration time.Duration, customFields []inte // Create a vcert Request structure vreq := newVRequest(tmpl) - // Add cert-manager origin tag - vreq.CustomFields = []certificate.CustomField{ - { - Type: certificate.CustomFieldOrigin, - Value: "Jetstack cert-manager", - }, - } - // Convert over custom fields from our struct type to venafi's - if len(customFields) > 0 { - for _, field := range customFields { - var fieldType certificate.CustomFieldType - switch field.Type { - case internalvanafiapi.CustomFieldTypePlain, "": - fieldType = certificate.CustomFieldPlain - default: - return nil, ErrCustomFieldsType{Type: field.Type} - } - - vreq.CustomFields = append(vreq.CustomFields, certificate.CustomField{ - Type: fieldType, - Name: field.Name, - Value: field.Value, - }) - } + vfields, err := convertCustomFieldsToVcert(customFields) + if err != nil { + return nil, err } + vreq.CustomFields = append(vreq.CustomFields, vfields...) // Apply default values from the Venafi zone zoneCfg.UpdateCertificateRequest(vreq) @@ -102,54 +115,45 @@ func (v *Venafi) Sign(csrPEM []byte, duration time.Duration, customFields []inte return nil, err } - vreq.SetCSR(csrPEM) + friendlyName, err := getVcertFriendlyName(tmpl) + if err != nil { + return nil, err + } + vreq.FriendlyName = friendlyName + // Set options on the request vreq.CsrOrigin = certificate.UserProvidedCSR - //// TODO: better set the timeout here. Right now, we'll block for this amount of time. - vreq.Timeout = time.Minute * 5 - - // Set the 'ObjectName' through the request friendly name. This is set in - // order of precedence CN->DNS->URI. - switch { - case len(tmpl.Subject.CommonName) > 0: - vreq.FriendlyName = tmpl.Subject.CommonName - break - case len(tmpl.DNSNames) > 0: - vreq.FriendlyName = tmpl.DNSNames[0] - break - case len(tmpl.URIs) > 0: - vreq.FriendlyName = tmpl.URIs[0].String() - break - default: - return nil, errors.New( - "certificate request contains no Common Name, DNS Name, nor URI SAN, at least one must be supplied to be used as the Venafi certificate objects name") - } // Set the request CSR with the passed value if err := vreq.SetCSR(csrPEM); err != nil { return nil, err } - // Send the certificate signing request to Venafi - requestID, err := v.client.RequestCertificate(vreq) - if err != nil { - return nil, err + return vreq, nil +} + +func convertCustomFieldsToVcert(customFields []CustomField) ([]certificate.CustomField, error) { + var out []certificate.CustomField + if len(customFields) > 0 { + for _, field := range customFields { + var fieldType certificate.CustomFieldType + switch field.Type { + case CustomFieldTypePlain, "": + fieldType = certificate.CustomFieldPlain + break + default: + return nil, ErrCustomFieldsType{Type: field.Type} + } + + out = append(out, certificate.CustomField{ + Type: fieldType, + Name: field.Name, + Value: field.Value, + }) + } } - // Set the PickupID so vcert does not have to look it up by the fingerprint - vreq.PickupID = requestID - - // Retrieve the certificate from request - pemCollection, err := v.client.RetrieveCertificate(vreq) - if err != nil { - return nil, err - } - - // Construct the certificate chain and return the new keypair - cs := append([]string{pemCollection.Certificate}, pemCollection.Chain...) - chain := strings.Join(cs, "\n") - - return []byte(chain), nil + return out, nil } func newVRequest(cert *x509.Certificate) *certificate.Request { @@ -157,5 +161,27 @@ func newVRequest(cert *x509.Certificate) *certificate.Request { // overwrite entire Subject block req.Subject = cert.Subject + // Add cert-manager origin tag + req.CustomFields = []certificate.CustomField{ + { + Type: certificate.CustomFieldOrigin, + Value: "Jetstack cert-manager", + }, + } return req } + +func getVcertFriendlyName(crt *x509.Certificate) (string, error) { + // Set the 'ObjectName' through the vcert friendly name. This is set in + // order of precedence CN->DNS->URI. + switch { + case len(crt.Subject.CommonName) > 0: + return crt.Subject.CommonName, nil + case len(crt.DNSNames) > 0: + return crt.DNSNames[0], nil + case len(crt.URIs) > 0: + return crt.URIs[0].String(), nil + default: + return "", errors.New("certificate request contains no Common Name, DNS Name, nor URI SAN, at least one must be supplied to be used as the Venafi certificate objects name") + } +} diff --git a/pkg/internal/venafi/sign_test.go b/pkg/issuer/venafi/client/request_test.go similarity index 94% rename from pkg/internal/venafi/sign_test.go rename to pkg/issuer/venafi/client/request_test.go index e20647d64..32eb9cb0b 100644 --- a/pkg/internal/venafi/sign_test.go +++ b/pkg/issuer/venafi/client/request_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "crypto" @@ -30,8 +30,7 @@ import ( "github.com/Venafi/vcert/pkg/endpoint" "github.com/Venafi/vcert/pkg/venafi/fake" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" - internalfake "github.com/jetstack/cert-manager/pkg/internal/venafi/fake" + internalfake "github.com/jetstack/cert-manager/pkg/issuer/venafi/client/fake" "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -184,7 +183,7 @@ func TestSign(t *testing.T) { }, "obtain a certificate with custom fields specified": { csrPEM: csrPEM, - customFields: []internalvanafiapi.CustomField{{Name: "test", Value: "ok"}}, + customFields: []CustomField{{Name: "test", Value: "ok"}}, client: internalfake.Connector{ RetrieveCertificateFunc: func(r *certificate.Request) (*certificate.PEMCollection, error) { // we set 1 field by default @@ -208,7 +207,7 @@ func TestSign(t *testing.T) { }, "If invalid custom field type found the error": { csrPEM: csrPEM, - customFields: []internalvanafiapi.CustomField{{Name: "test", Value: "ok", Type: "Bool"}}, + customFields: []CustomField{{Name: "test", Value: "ok", Type: "Bool"}}, checkFn: checkNoCertificateIssued, expectedErr: true, }, @@ -227,7 +226,7 @@ type testSignT struct { expectedErr bool - customFields []internalvanafiapi.CustomField + customFields []CustomField checkFn func(*testing.T, []byte, []byte) } diff --git a/pkg/internal/venafi/venafi.go b/pkg/issuer/venafi/client/venaficlient.go similarity index 79% rename from pkg/internal/venafi/venafi.go rename to pkg/issuer/venafi/client/venaficlient.go index 72692df14..ab8f6306e 100644 --- a/pkg/internal/venafi/venafi.go +++ b/pkg/issuer/venafi/client/venaficlient.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "fmt" @@ -26,7 +26,6 @@ import ( corelisters "k8s.io/client-go/listers/core/v1" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" - internalvanafiapi "github.com/jetstack/cert-manager/pkg/internal/venafi/api" ) const ( @@ -39,14 +38,16 @@ const ( type VenafiClientBuilder func(namespace string, secretsLister corelisters.SecretLister, issuer cmapi.GenericIssuer) (Interface, error) +// Interface implements a Venafi client type Interface interface { - Sign(csrPEM []byte, duration time.Duration, customFields []internalvanafiapi.CustomField) (cert []byte, err error) + RequestCertificate(csrPEM []byte, duration time.Duration, customFields []CustomField) (string, error) + RetreiveCertificate(pickupID string, csrPEM []byte, duration time.Duration, customFields []CustomField) ([]byte, error) Ping() error ReadZoneConfiguration() (*endpoint.ZoneConfiguration, error) SetClient(endpoint.Connector) } -// Venafi is a implementation of govcert library to manager certificates from TPP or Venafi Cloud +// Venafi is a implementation of vcert library to manager certificates from TPP or Venafi Cloud type Venafi struct { // Namespace in which to read resources related to this Issuer from. // For Issuers, this will be the namespace of the Issuer. @@ -54,7 +55,7 @@ type Venafi struct { namespace string secretsLister corelisters.SecretLister - client connector + vcertClient connector } // connector exposes a subset of the vcert Connector interface to make stubbing @@ -67,15 +68,13 @@ type connector interface { RenewCertificate(req *certificate.RenewalRequest) (requestID string, err error) } -func New(namespace string, secretsLister corelisters.SecretLister, - issuer cmapi.GenericIssuer) (Interface, error) { - +func New(namespace string, secretsLister corelisters.SecretLister, issuer cmapi.GenericIssuer) (Interface, error) { cfg, err := configForIssuer(issuer, secretsLister, namespace) if err != nil { return nil, err } - client, err := vcert.NewClient(cfg) + vcertClient, err := vcert.NewClient(cfg) if err != nil { return nil, fmt.Errorf("error creating Venafi client: %s", err.Error()) } @@ -83,7 +82,7 @@ func New(namespace string, secretsLister corelisters.SecretLister, return &Venafi{ namespace: namespace, secretsLister: secretsLister, - client: client, + vcertClient: vcertClient, }, nil } @@ -91,21 +90,16 @@ func New(namespace string, secretsLister corelisters.SecretLister, // that can be used to instantiate an API client. func configForIssuer(iss cmapi.GenericIssuer, secretsLister corelisters.SecretLister, namespace string) (*vcert.Config, error) { venCfg := iss.GetSpec().Venafi - switch { - case venCfg.TPP != nil: + if venCfg.TPP != nil { tpp := venCfg.TPP tppSecret, err := secretsLister.Secrets(namespace).Get(tpp.CredentialsRef.Name) if err != nil { return nil, err } - username := tppSecret.Data[tppUsernameKey] - password := tppSecret.Data[tppPasswordKey] - - caBundle := "" - if len(tpp.CABundle) > 0 { - caBundle = string(tpp.CABundle) - } + username := string(tppSecret.Data[tppUsernameKey]) + password := string(tppSecret.Data[tppPasswordKey]) + caBundle := string(tpp.CABundle) return &vcert.Config{ ConnectorType: endpoint.ConnectorTypeTPP, @@ -115,12 +109,13 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister corelisters.SecretLi LogVerbose: true, ConnectionTrust: caBundle, Credentials: &endpoint.Authentication{ - User: string(username), - Password: string(password), + User: username, + Password: password, }, }, nil + } - case venCfg.Cloud != nil: + if venCfg.Cloud != nil { cloud := venCfg.Cloud cloudSecret, err := secretsLister.Secrets(namespace).Get(cloud.APITokenSecretRef.Name) if err != nil { @@ -131,7 +126,7 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister corelisters.SecretLi if cloud.APITokenSecretRef.Key != "" { k = cloud.APITokenSecretRef.Key } - apiKey := cloudSecret.Data[k] + apiKey := string(cloudSecret.Data[k]) return &vcert.Config{ ConnectorType: endpoint.ConnectorTypeCloud, @@ -140,23 +135,22 @@ func configForIssuer(iss cmapi.GenericIssuer, secretsLister corelisters.SecretLi // always enable verbose logging for now LogVerbose: true, Credentials: &endpoint.Authentication{ - APIKey: string(apiKey), + APIKey: apiKey, }, }, nil - - default: - return nil, fmt.Errorf("neither Venafi Cloud or TPP configuration found") } + + return nil, fmt.Errorf("neither Venafi Cloud or TPP configuration found") } func (v *Venafi) Ping() error { - return v.client.Ping() + return v.vcertClient.Ping() } func (v *Venafi) ReadZoneConfiguration() (*endpoint.ZoneConfiguration, error) { - return v.client.ReadZoneConfiguration() + return v.vcertClient.ReadZoneConfiguration() } func (v *Venafi) SetClient(client endpoint.Connector) { - v.client = client + v.vcertClient = client } diff --git a/pkg/internal/venafi/venafi_test.go b/pkg/issuer/venafi/client/venaficlient_test.go similarity index 99% rename from pkg/internal/venafi/venafi_test.go rename to pkg/issuer/venafi/client/venaficlient_test.go index ec0a55819..98405a0bc 100644 --- a/pkg/internal/venafi/venafi_test.go +++ b/pkg/issuer/venafi/client/venaficlient_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package venafi +package client import ( "errors" diff --git a/pkg/issuer/venafi/setup_test.go b/pkg/issuer/venafi/setup_test.go index 29541471e..49c6eac69 100644 --- a/pkg/issuer/venafi/setup_test.go +++ b/pkg/issuer/venafi/setup_test.go @@ -21,13 +21,14 @@ import ( "errors" "testing" + "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" + corelisters "k8s.io/client-go/listers/core/v1" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" "github.com/jetstack/cert-manager/pkg/controller" controllertest "github.com/jetstack/cert-manager/pkg/controller/test" - internalvenafi "github.com/jetstack/cert-manager/pkg/internal/venafi" - internalvenafifake "github.com/jetstack/cert-manager/pkg/internal/venafi/fake" + internalvenafifake "github.com/jetstack/cert-manager/pkg/issuer/venafi/client/fake" "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/test/unit/gen" ) @@ -36,12 +37,12 @@ func TestSetup(t *testing.T) { baseIssuer := gen.Issuer("test-issuer") failingClientBuilder := func(string, corelisters.SecretLister, - cmapi.GenericIssuer) (internalvenafi.Interface, error) { + cmapi.GenericIssuer) (client.Interface, error) { return nil, errors.New("this is an error") } failingPingClient := func(string, corelisters.SecretLister, - cmapi.GenericIssuer) (internalvenafi.Interface, error) { + cmapi.GenericIssuer) (client.Interface, error) { return &internalvenafifake.Venafi{ PingFn: func() error { return errors.New("this is a ping error") @@ -50,7 +51,7 @@ func TestSetup(t *testing.T) { } pingClient := func(string, corelisters.SecretLister, - cmapi.GenericIssuer) (internalvenafi.Interface, error) { + cmapi.GenericIssuer) (client.Interface, error) { return &internalvenafifake.Venafi{ PingFn: func() error { return nil @@ -99,7 +100,7 @@ func TestSetup(t *testing.T) { } type testSetupT struct { - clientBuilder internalvenafi.VenafiClientBuilder + clientBuilder client.VenafiClientBuilder iss cmapi.GenericIssuer expectedErr bool diff --git a/pkg/issuer/venafi/venafi.go b/pkg/issuer/venafi/venafi.go index 969be3508..549eac3ac 100644 --- a/pkg/issuer/venafi/venafi.go +++ b/pkg/issuer/venafi/venafi.go @@ -17,12 +17,12 @@ limitations under the License. package venafi import ( + "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" corelisters "k8s.io/client-go/listers/core/v1" apiutil "github.com/jetstack/cert-manager/pkg/api/util" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" "github.com/jetstack/cert-manager/pkg/controller" - "github.com/jetstack/cert-manager/pkg/internal/venafi" "github.com/jetstack/cert-manager/pkg/issuer" ) @@ -38,7 +38,7 @@ type Venafi struct { // For ClusterIssuers, this will be the cluster resource namespace. resourceNamespace string - clientBuilder venafi.VenafiClientBuilder + clientBuilder client.VenafiClientBuilder } func NewVenafi(ctx *controller.Context, issuer cmapi.GenericIssuer) (issuer.Interface, error) { @@ -46,7 +46,7 @@ func NewVenafi(ctx *controller.Context, issuer cmapi.GenericIssuer) (issuer.Inte issuer: issuer, secretsLister: ctx.KubeSharedInformerFactory.Core().V1().Secrets().Lister(), resourceNamespace: ctx.IssuerOptions.ResourceNamespace(issuer), - clientBuilder: venafi.New, + clientBuilder: client.New, Context: ctx, }, nil }