From 2995cc90a3073152493660f0d8a58fe5045cfa3a Mon Sep 17 00:00:00 2001
From: Vincent Desjardins <vdesjardins@gmail.com>
Date: Mon, 4 Jun 2018 01:59:48 +0000
Subject: [PATCH 1/4] Vault: configurable appRole authentication path

---
 .../vault/creating-vault-issuers.rst          |  5 +-
 pkg/apis/certmanager/v1alpha1/types.go        |  2 +
 pkg/issuer/vault/issue.go                     |  7 +-
 pkg/issuer/vault/setup.go                     |  8 +++
 test/e2e/certificate/certificate_vault.go     | 66 ++++++++++++++++++-
 test/e2e/issuer/issuer_vault.go               |  6 +-
 test/util/util.go                             |  3 +-
 test/util/vault/util.go                       | 25 ++++---
 8 files changed, 105 insertions(+), 17 deletions(-)

diff --git a/docs/tutorials/vault/creating-vault-issuers.rst b/docs/tutorials/vault/creating-vault-issuers.rst
index 78d452ed3..97f1797d4 100644
--- a/docs/tutorials/vault/creating-vault-issuers.rst
+++ b/docs/tutorials/vault/creating-vault-issuers.rst
@@ -55,6 +55,7 @@ We can now create a cluster issuer referencing this secret:
         path: pki_int/sign/example-dot-com
         server: https://vault
         auth:
+          authPath: approle
           appRole:
             roleId: "291b9d21-8ff5-..."
             secretRef:
@@ -67,7 +68,9 @@ The Vault appRole credentials are supplied as the
 Vault authentication method using the appRole created in Vault. The secretRef
 references the Kubernetes secret created previously. More specifically, the field
 *name* is the Kubernetes secret name and *key* is the name given as the
-key value that store the *secretId*.
+key value that store the *secretId*. The optional attribute *authPath* specifies
+where the AppRole authentication is mounted in Vault. The *authPath* default value
+is *approle*.
 
 Once we have created the above Issuer we can use it to obtain a certificate.
 
diff --git a/pkg/apis/certmanager/v1alpha1/types.go b/pkg/apis/certmanager/v1alpha1/types.go
index 8a50cd066..0d5f996c0 100644
--- a/pkg/apis/certmanager/v1alpha1/types.go
+++ b/pkg/apis/certmanager/v1alpha1/types.go
@@ -109,6 +109,8 @@ type VaultAuth struct {
 	TokenSecretRef SecretKeySelector `json:"tokenSecretRef,omitempty"`
 	// This Secret contains a AppRole and Secret
 	AppRole VaultAppRole `json:"appRole,omitempty"`
+	// Where the authentication path is mounted in Vault.
+	AuthPath string `json:"authPath,omitempty"`
 }
 
 type VaultAppRole struct {
diff --git a/pkg/issuer/vault/issue.go b/pkg/issuer/vault/issue.go
index 6f6730202..db48a9771 100644
--- a/pkg/issuer/vault/issue.go
+++ b/pkg/issuer/vault/issue.go
@@ -131,7 +131,12 @@ func (v *Vault) requestTokenWithAppRoleRef(client *vault.Client, appRole *v1alph
 		"secret_id": secretId,
 	}
 
-	url := "/v1/auth/approle/login"
+	authPath := v.issuer.GetSpec().Vault.Auth.AuthPath
+	if authPath == "" {
+		authPath = "approle"
+	}
+
+	url := path.Join("/v1/auth", authPath, "login")
 
 	request := client.NewRequest("POST", url)
 
diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go
index 4dbb14eb0..71be54dd9 100644
--- a/pkg/issuer/vault/setup.go
+++ b/pkg/issuer/vault/setup.go
@@ -21,6 +21,7 @@ const (
 	messageServerAndPathRequired         = "Vault server and path are required fields"
 	messsageAuthFieldsRequired           = "Vault tokenSecretRef or appRole is required"
 	messageAuthFieldRequired             = "Vault tokenSecretRef and appRole cannot be set on the same issuer"
+	messageAuthPathInvalid               = "Vault authPath cannot be used with a tokenSecretRef"
 )
 
 func (v *Vault) Setup(ctx context.Context) error {
@@ -53,6 +54,13 @@ func (v *Vault) Setup(ctx context.Context) error {
 		return fmt.Errorf(messageAuthFieldRequired)
 	}
 
+	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name != "" &&
+		v.issuer.GetSpec().Vault.Auth.AuthPath != "" {
+		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageAuthPathInvalid)
+		v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageAuthPathInvalid)
+		return fmt.Errorf(messageAuthPathInvalid)
+	}
+
 	client, err := v.initVaultClient()
 	if err != nil {
 		s := messageVaultClientInitFailed + err.Error()
diff --git a/test/e2e/certificate/certificate_vault.go b/test/e2e/certificate/certificate_vault.go
index dfd4ac46c..b2f78e58e 100644
--- a/test/e2e/certificate/certificate_vault.go
+++ b/test/e2e/certificate/certificate_vault.go
@@ -33,7 +33,7 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole)", func() {
 		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
 		Expect(err).NotTo(HaveOccurred())
 		vaultPodName := podList.Items[0].Name
-		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role)
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, "")
 		Expect(err).NotTo(HaveOccurred())
 		err = vaultInit.Setup()
 		Expect(err).NotTo(HaveOccurred())
@@ -54,7 +54,69 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole)", func() {
 	vaultURL := "http://vault.vault:8200"
 	It("should generate a new valid certificate", func() {
 		By("Creating an Issuer")
-		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName))
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Waiting for Issuer to become Ready")
+		err = util.WaitForIssuerCondition(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name),
+			issuerName,
+			v1alpha1.IssuerCondition{
+				Type:   v1alpha1.IssuerConditionReady,
+				Status: v1alpha1.ConditionTrue,
+			})
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Creating a Certificate")
+		cert, err := f.CertManagerClientSet.CertmanagerV1alpha1().Certificates(f.Namespace.Name).Create(util.NewCertManagerVaultCertificate(certificateName, certificateSecretName, issuerName, v1alpha1.IssuerKind))
+		Expect(err).NotTo(HaveOccurred())
+
+		f.WaitCertificateIssuedValid(cert)
+	})
+})
+
+var _ = framework.CertManagerDescribe("Vault Certificate (AppRole with a custom mount path)", func() {
+	f := framework.NewDefaultFramework("create-vault-certificate")
+
+	rootMount := "root-ca"
+	intermediateMount := "intermediate-ca"
+	roleMount := "custom/path"
+	role := "kubernetes-vault"
+	issuerName := "test-vault-issuer"
+	certificateName := "test-vault-certificate"
+	certificateSecretName := "test-vault-certificate"
+	vaultSecretAppRoleName := "vault-role"
+	vaultPath := fmt.Sprintf("%s/sign/%s", intermediateMount, role)
+	var vaultInit *vault.VaultInitializer
+	var roleId string
+	var secretId string
+
+	BeforeEach(func() {
+		By("Configuring the Vault server")
+		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
+		Expect(err).NotTo(HaveOccurred())
+		vaultPodName := podList.Items[0].Name
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, roleMount)
+		Expect(err).NotTo(HaveOccurred())
+		err = vaultInit.Setup()
+		Expect(err).NotTo(HaveOccurred())
+		roleId, secretId, err = vaultInit.CreateAppRole()
+		Expect(err).NotTo(HaveOccurred())
+		_, err = f.KubeClientSet.CoreV1().Secrets(f.Namespace.Name).Create(vault.NewVaultAppRoleSecret(vaultSecretAppRoleName, secretId))
+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	AfterEach(func() {
+		By("Cleaning up")
+		f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Delete(issuerName, nil)
+		f.KubeClientSet.CoreV1().Secrets(f.Namespace.Name).Delete(vaultSecretAppRoleName, nil)
+		vaultInit.CleanAppRole()
+		vaultInit.Clean()
+	})
+
+	vaultURL := "http://vault.vault:8200"
+	It("should generate a new valid certificate", func() {
+		By("Creating an Issuer")
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, roleMount))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
diff --git a/test/e2e/issuer/issuer_vault.go b/test/e2e/issuer/issuer_vault.go
index 07b22accd..c10778468 100644
--- a/test/e2e/issuer/issuer_vault.go
+++ b/test/e2e/issuer/issuer_vault.go
@@ -32,7 +32,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
 		Expect(err).NotTo(HaveOccurred())
 		vaultPodName := podList.Items[0].Name
-		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role)
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, "")
 		Expect(err).NotTo(HaveOccurred())
 		err = vaultInit.Setup()
 		Expect(err).NotTo(HaveOccurred())
@@ -56,7 +56,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Creating an Issuer")
-		_, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName))
+		_, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
@@ -71,7 +71,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 
 	It("should fail to init with missing Vault AppRole", func() {
 		By("Creating an Issuer")
-		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName))
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
diff --git a/test/util/util.go b/test/util/util.go
index 74c3df834..7e6601abb 100644
--- a/test/util/util.go
+++ b/test/util/util.go
@@ -413,7 +413,7 @@ func NewCertManagerVaultIssuerToken(name, vaultURL, vaultPath, vaultSecretToken
 	}
 }
 
-func NewCertManagerVaultIssuerAppRole(name, vaultURL, vaultPath, roleId, vaultSecretAppRole string) *v1alpha1.Issuer {
+func NewCertManagerVaultIssuerAppRole(name, vaultURL, vaultPath, roleId, vaultSecretAppRole, authPath string) *v1alpha1.Issuer {
 	return &v1alpha1.Issuer{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -424,6 +424,7 @@ func NewCertManagerVaultIssuerAppRole(name, vaultURL, vaultPath, roleId, vaultSe
 					Server: vaultURL,
 					Path:   vaultPath,
 					Auth: v1alpha1.VaultAuth{
+						AuthPath: authPath,
 						AppRole: v1alpha1.VaultAppRole{
 							RoleId: roleId,
 							SecretRef: v1alpha1.SecretKeySelector{
diff --git a/test/util/vault/util.go b/test/util/vault/util.go
index 2c3d48c90..d9e9d91d9 100644
--- a/test/util/vault/util.go
+++ b/test/util/vault/util.go
@@ -42,9 +42,10 @@ type VaultInitializer struct {
 	rootMount         string
 	intermediateMount string
 	role              string
+	authPath          string
 }
 
-func NewVaultInitializer(container, rootMount, intermediateMount, role string) (*VaultInitializer, error) {
+func NewVaultInitializer(container, rootMount, intermediateMount, role, authPath string) (*VaultInitializer, error) {
 	args := []string{"port-forward", "-n", "vault", container, "8200:8200"}
 	cmd := exec.Command("kubectl", args...)
 	err := cmd.Start()
@@ -63,12 +64,17 @@ func NewVaultInitializer(container, rootMount, intermediateMount, role string) (
 
 	client.SetToken(vaultToken)
 
+	if authPath == "" {
+		authPath = "approle"
+	}
+
 	return &VaultInitializer{
 		proxyCmd:          cmd,
 		client:            client,
 		rootMount:         rootMount,
 		intermediateMount: intermediateMount,
 		role:              role,
+		authPath:          authPath,
 	}, nil
 }
 
@@ -144,21 +150,22 @@ func (v *VaultInitializer) CreateAppRole() (string, string, error) {
 		"period":   "24h",
 		"policies": v.role,
 	}
-	url := path.Join("/v1/auth/approle/role", v.role)
-	_, err = v.callVault("POST", url, "", params)
+
+	baseUrl := path.Join("/v1/auth", v.authPath, "role", v.role)
+	_, err = v.callVault("POST", baseUrl, "", params)
 	if err != nil {
 		return "", "", fmt.Errorf("Error creating approle: %s", err.Error())
 	}
 
 	// # read the role-id
-	url = path.Join("/v1/auth/approle/role", v.role, "role-id")
+	url := path.Join(baseUrl, "role-id")
 	roleId, err := v.callVault("GET", url, "role_id", map[string]string{})
 	if err != nil {
 		return "", "", fmt.Errorf("Error reading role_id: %s", err.Error())
 	}
 
 	// # read the secret-id
-	url = path.Join("/v1/auth/approle/role", v.role, "secret-id")
+	url = path.Join(baseUrl, "secret-id")
 	secretId, err := v.callVault("POST", url, "secret_id", map[string]string{})
 	if err != nil {
 		return "", "", fmt.Errorf("Error reading secret_id: %s", err.Error())
@@ -168,7 +175,7 @@ func (v *VaultInitializer) CreateAppRole() (string, string, error) {
 }
 
 func (v *VaultInitializer) CleanAppRole() error {
-	url := path.Join("/v1/auth/approle/role", v.role)
+	url := path.Join("/v1/auth", v.authPath, "role", v.role)
 	_, err := v.callVault("DELETE", url, "", map[string]string{})
 	if err != nil {
 		return fmt.Errorf("Error deleting AppRole: %s", err.Error())
@@ -280,14 +287,14 @@ func (v *VaultInitializer) setupRole() error {
 	if err != nil {
 		return fmt.Errorf("Error fetching auth mounts: %s", err.Error())
 	}
-	if _, ok := auths["approle/"]; !ok {
+
+	if _, ok := auths[v.authPath+"/"]; !ok {
 		options := &vault.EnableAuthOptions{Type: "approle"}
-		if err := v.client.Sys().EnableAuthWithOptions("approle", options); err != nil {
+		if err := v.client.Sys().EnableAuthWithOptions(v.authPath, options); err != nil {
 			return fmt.Errorf("Error enabling approle: %s", err.Error())
 		}
 	}
 
-	// vault write intermediate-ca/roles/kubernetes-vault allow_any_name=true max_ttl="2160h"
 	params := map[string]string{
 		"allow_any_name": "true",
 		"max_ttl":        "2160h",

From ca3b909cb74c28cf748771d60f95c7643947a8df Mon Sep 17 00:00:00 2001
From: Vincent Desjardins <vdesjardins@gmail.com>
Date: Fri, 8 Jun 2018 21:30:03 +0000
Subject: [PATCH 2/4] code review modifications

---
 pkg/apis/certmanager/v1alpha1/types.go    |  5 +++--
 pkg/issuer/vault/issue.go                 | 10 +++++-----
 pkg/issuer/vault/setup.go                 | 11 ++---------
 test/e2e/certificate/certificate_vault.go | 17 +++++++++--------
 test/e2e/issuer/issuer_vault.go           | 11 ++++++-----
 test/util/util.go                         |  2 +-
 test/util/vault/util.go                   |  6 +++---
 7 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/pkg/apis/certmanager/v1alpha1/types.go b/pkg/apis/certmanager/v1alpha1/types.go
index 0d5f996c0..69a9e5144 100644
--- a/pkg/apis/certmanager/v1alpha1/types.go
+++ b/pkg/apis/certmanager/v1alpha1/types.go
@@ -109,11 +109,12 @@ type VaultAuth struct {
 	TokenSecretRef SecretKeySelector `json:"tokenSecretRef,omitempty"`
 	// This Secret contains a AppRole and Secret
 	AppRole VaultAppRole `json:"appRole,omitempty"`
-	// Where the authentication path is mounted in Vault.
-	AuthPath string `json:"authPath,omitempty"`
 }
 
 type VaultAppRole struct {
+	// Where the authentication path is mounted in Vault.
+	Path string `json:"path,omitempty"`
+
 	RoleId    string            `json:"roleId"`
 	SecretRef SecretKeySelector `json:"secretRef"`
 }
diff --git a/pkg/issuer/vault/issue.go b/pkg/issuer/vault/issue.go
index db48a9771..d41c9689c 100644
--- a/pkg/issuer/vault/issue.go
+++ b/pkg/issuer/vault/issue.go
@@ -110,7 +110,7 @@ func (v *Vault) initVaultClient() (*vault.Client, error) {
 	if appRole.RoleId != "" {
 		token, err := v.requestTokenWithAppRoleRef(client, &appRole)
 		if err != nil {
-			return nil, fmt.Errorf("error reading Vault token from secret %s/%s: %s", v.issuerResourcesNamespace, appRole.SecretRef.Name, err.Error())
+			return nil, fmt.Errorf("error reading Vault token from AppRole: %s", err.Error())
 		}
 		client.SetToken(token)
 
@@ -131,12 +131,12 @@ func (v *Vault) requestTokenWithAppRoleRef(client *vault.Client, appRole *v1alph
 		"secret_id": secretId,
 	}
 
-	authPath := v.issuer.GetSpec().Vault.Auth.AuthPath
+	authPath := appRole.Path
 	if authPath == "" {
 		authPath = "approle"
 	}
 
-	url := path.Join("/v1/auth", authPath, "login")
+	url := path.Join("/v1", "auth", authPath, "login")
 
 	request := client.NewRequest("POST", url)
 
@@ -147,7 +147,7 @@ func (v *Vault) requestTokenWithAppRoleRef(client *vault.Client, appRole *v1alph
 
 	resp, err := client.RawRequest(request)
 	if err != nil {
-		return "", fmt.Errorf("error calling Vault server: %s", err.Error())
+		return "", fmt.Errorf("error logging in to Vault server: %s", err.Error())
 	}
 
 	defer resp.Body.Close()
@@ -193,7 +193,7 @@ func (v *Vault) requestVaultCert(commonName string, altNames []string, csr []byt
 
 	resp, err := client.RawRequest(request)
 	if err != nil {
-		return nil, fmt.Errorf("error calling Vault server: %s", err.Error())
+		return nil, fmt.Errorf("error logging in to Vault server: %s", err.Error())
 	}
 
 	defer resp.Body.Close()
diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go
index 71be54dd9..eb0ce23f5 100644
--- a/pkg/issuer/vault/setup.go
+++ b/pkg/issuer/vault/setup.go
@@ -21,7 +21,6 @@ const (
 	messageServerAndPathRequired         = "Vault server and path are required fields"
 	messsageAuthFieldsRequired           = "Vault tokenSecretRef or appRole is required"
 	messageAuthFieldRequired             = "Vault tokenSecretRef and appRole cannot be set on the same issuer"
-	messageAuthPathInvalid               = "Vault authPath cannot be used with a tokenSecretRef"
 )
 
 func (v *Vault) Setup(ctx context.Context) error {
@@ -48,19 +47,13 @@ func (v *Vault) Setup(ctx context.Context) error {
 
 	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name != "" &&
 		(v.issuer.GetSpec().Vault.Auth.AppRole.RoleId != "" ||
-			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name != "") {
+			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name != "" ||
+			v.issuer.GetSpec().Vault.Auth.AppRole.Path != "") {
 		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageAuthFieldRequired)
 		v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageAuthFieldRequired)
 		return fmt.Errorf(messageAuthFieldRequired)
 	}
 
-	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name != "" &&
-		v.issuer.GetSpec().Vault.Auth.AuthPath != "" {
-		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageAuthPathInvalid)
-		v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageAuthPathInvalid)
-		return fmt.Errorf(messageAuthPathInvalid)
-	}
-
 	client, err := v.initVaultClient()
 	if err != nil {
 		s := messageVaultClientInitFailed + err.Error()
diff --git a/test/e2e/certificate/certificate_vault.go b/test/e2e/certificate/certificate_vault.go
index b2f78e58e..ecf2555ee 100644
--- a/test/e2e/certificate/certificate_vault.go
+++ b/test/e2e/certificate/certificate_vault.go
@@ -1,7 +1,7 @@
 package certificate
 
 import (
-	"fmt"
+	"path"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -23,7 +23,8 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole)", func() {
 	certificateName := "test-vault-certificate"
 	certificateSecretName := "test-vault-certificate"
 	vaultSecretAppRoleName := "vault-role"
-	vaultPath := fmt.Sprintf("%s/sign/%s", intermediateMount, role)
+	vaultPath := path.Join(intermediateMount, "sign", role)
+	authPath := "approle"
 	var vaultInit *vault.VaultInitializer
 	var roleId string
 	var secretId string
@@ -33,7 +34,7 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole)", func() {
 		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
 		Expect(err).NotTo(HaveOccurred())
 		vaultPodName := podList.Items[0].Name
-		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, "")
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, authPath)
 		Expect(err).NotTo(HaveOccurred())
 		err = vaultInit.Setup()
 		Expect(err).NotTo(HaveOccurred())
@@ -54,7 +55,7 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole)", func() {
 	vaultURL := "http://vault.vault:8200"
 	It("should generate a new valid certificate", func() {
 		By("Creating an Issuer")
-		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, authPath))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
@@ -79,13 +80,13 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole with a custom
 
 	rootMount := "root-ca"
 	intermediateMount := "intermediate-ca"
-	roleMount := "custom/path"
+	authPath := "custom/path"
 	role := "kubernetes-vault"
 	issuerName := "test-vault-issuer"
 	certificateName := "test-vault-certificate"
 	certificateSecretName := "test-vault-certificate"
 	vaultSecretAppRoleName := "vault-role"
-	vaultPath := fmt.Sprintf("%s/sign/%s", intermediateMount, role)
+	vaultPath := path.Join(intermediateMount, "sign", role)
 	var vaultInit *vault.VaultInitializer
 	var roleId string
 	var secretId string
@@ -95,7 +96,7 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole with a custom
 		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
 		Expect(err).NotTo(HaveOccurred())
 		vaultPodName := podList.Items[0].Name
-		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, roleMount)
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, authPath)
 		Expect(err).NotTo(HaveOccurred())
 		err = vaultInit.Setup()
 		Expect(err).NotTo(HaveOccurred())
@@ -116,7 +117,7 @@ var _ = framework.CertManagerDescribe("Vault Certificate (AppRole with a custom
 	vaultURL := "http://vault.vault:8200"
 	It("should generate a new valid certificate", func() {
 		By("Creating an Issuer")
-		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, roleMount))
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, authPath))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
diff --git a/test/e2e/issuer/issuer_vault.go b/test/e2e/issuer/issuer_vault.go
index c10778468..8f48b5e84 100644
--- a/test/e2e/issuer/issuer_vault.go
+++ b/test/e2e/issuer/issuer_vault.go
@@ -1,7 +1,7 @@
 package issuer
 
 import (
-	"fmt"
+	"path"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -23,7 +23,8 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 	role := "kubernetes-vault"
 	vaultSecretAppRoleName := "vault-role"
 	vaultSecretTokenName := "vault-token"
-	vaultPath := fmt.Sprintf("%s/sign/%s", intermediateMount, role)
+	vaultPath := path.Join(intermediateMount, "sign", role)
+	authPath := "approle"
 	var roleId, secretId string
 	var vaultInit *vault.VaultInitializer
 
@@ -32,7 +33,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 		podList, err := f.KubeClientSet.CoreV1().Pods("vault").List(metav1.ListOptions{})
 		Expect(err).NotTo(HaveOccurred())
 		vaultPodName := podList.Items[0].Name
-		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, "")
+		vaultInit, err = vault.NewVaultInitializer(vaultPodName, rootMount, intermediateMount, role, authPath)
 		Expect(err).NotTo(HaveOccurred())
 		err = vaultInit.Setup()
 		Expect(err).NotTo(HaveOccurred())
@@ -56,7 +57,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Creating an Issuer")
-		_, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
+		_, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, authPath))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
@@ -71,7 +72,7 @@ var _ = framework.CertManagerDescribe("Vault Issuer", func() {
 
 	It("should fail to init with missing Vault AppRole", func() {
 		By("Creating an Issuer")
-		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, ""))
+		_, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(util.NewCertManagerVaultIssuerAppRole(issuerName, vaultURL, vaultPath, roleId, vaultSecretAppRoleName, authPath))
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Waiting for Issuer to become Ready")
diff --git a/test/util/util.go b/test/util/util.go
index 7e6601abb..ab655f862 100644
--- a/test/util/util.go
+++ b/test/util/util.go
@@ -424,8 +424,8 @@ func NewCertManagerVaultIssuerAppRole(name, vaultURL, vaultPath, roleId, vaultSe
 					Server: vaultURL,
 					Path:   vaultPath,
 					Auth: v1alpha1.VaultAuth{
-						AuthPath: authPath,
 						AppRole: v1alpha1.VaultAppRole{
+							Path:   authPath,
 							RoleId: roleId,
 							SecretRef: v1alpha1.SecretKeySelector{
 								Key: "secretkey",
diff --git a/test/util/vault/util.go b/test/util/vault/util.go
index d9e9d91d9..c843f952b 100644
--- a/test/util/vault/util.go
+++ b/test/util/vault/util.go
@@ -65,7 +65,7 @@ func NewVaultInitializer(container, rootMount, intermediateMount, role, authPath
 	client.SetToken(vaultToken)
 
 	if authPath == "" {
-		authPath = "approle"
+		return nil, fmt.Errorf("Error authPath must be set")
 	}
 
 	return &VaultInitializer{
@@ -151,7 +151,7 @@ func (v *VaultInitializer) CreateAppRole() (string, string, error) {
 		"policies": v.role,
 	}
 
-	baseUrl := path.Join("/v1/auth", v.authPath, "role", v.role)
+	baseUrl := path.Join("/v1", "auth", v.authPath, "role", v.role)
 	_, err = v.callVault("POST", baseUrl, "", params)
 	if err != nil {
 		return "", "", fmt.Errorf("Error creating approle: %s", err.Error())
@@ -175,7 +175,7 @@ func (v *VaultInitializer) CreateAppRole() (string, string, error) {
 }
 
 func (v *VaultInitializer) CleanAppRole() error {
-	url := path.Join("/v1/auth", v.authPath, "role", v.role)
+	url := path.Join("/v1", "auth", v.authPath, "role", v.role)
 	_, err := v.callVault("DELETE", url, "", map[string]string{})
 	if err != nil {
 		return fmt.Errorf("Error deleting AppRole: %s", err.Error())

From 7fae0fccf11904ca0d08314a201cd735f7c153c9 Mon Sep 17 00:00:00 2001
From: Vincent Desjardins <vdesjardins@gmail.com>
Date: Wed, 4 Jul 2018 02:10:18 +0000
Subject: [PATCH 3/4] code review fixes

---
 docs/tutorials/vault/creating-vault-issuers.rst |  8 ++++----
 pkg/apis/certmanager/v1alpha1/types.go          |  2 +-
 pkg/issuer/vault/issue.go                       |  2 +-
 pkg/issuer/vault/setup.go                       | 15 +++++++++++++--
 test/util/vault/util.go                         |  2 +-
 5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/docs/tutorials/vault/creating-vault-issuers.rst b/docs/tutorials/vault/creating-vault-issuers.rst
index 97f1797d4..01e0b3db6 100644
--- a/docs/tutorials/vault/creating-vault-issuers.rst
+++ b/docs/tutorials/vault/creating-vault-issuers.rst
@@ -55,8 +55,8 @@ We can now create a cluster issuer referencing this secret:
         path: pki_int/sign/example-dot-com
         server: https://vault
         auth:
-          authPath: approle
           appRole:
+            path: approle
             roleId: "291b9d21-8ff5-..."
             secretRef:
               name: cert-manager-vault-approle
@@ -68,9 +68,9 @@ The Vault appRole credentials are supplied as the
 Vault authentication method using the appRole created in Vault. The secretRef
 references the Kubernetes secret created previously. More specifically, the field
 *name* is the Kubernetes secret name and *key* is the name given as the
-key value that store the *secretId*. The optional attribute *authPath* specifies
-where the AppRole authentication is mounted in Vault. The *authPath* default value
-is *approle*.
+key value that store the *secretId*. The optional attribute *path* specifies
+where the AppRole authentication is mounted in Vault. The attribute *path* default
+value is *approle*.
 
 Once we have created the above Issuer we can use it to obtain a certificate.
 
diff --git a/pkg/apis/certmanager/v1alpha1/types.go b/pkg/apis/certmanager/v1alpha1/types.go
index 69a9e5144..578e79992 100644
--- a/pkg/apis/certmanager/v1alpha1/types.go
+++ b/pkg/apis/certmanager/v1alpha1/types.go
@@ -113,7 +113,7 @@ type VaultAuth struct {
 
 type VaultAppRole struct {
 	// Where the authentication path is mounted in Vault.
-	Path string `json:"path,omitempty"`
+	Path string `json:"path"`
 
 	RoleId    string            `json:"roleId"`
 	SecretRef SecretKeySelector `json:"secretRef"`
diff --git a/pkg/issuer/vault/issue.go b/pkg/issuer/vault/issue.go
index d41c9689c..8f554519f 100644
--- a/pkg/issuer/vault/issue.go
+++ b/pkg/issuer/vault/issue.go
@@ -193,7 +193,7 @@ func (v *Vault) requestVaultCert(commonName string, altNames []string, csr []byt
 
 	resp, err := client.RawRequest(request)
 	if err != nil {
-		return nil, fmt.Errorf("error logging in to Vault server: %s", err.Error())
+		return nil, fmt.Errorf("error signing certificate in Vault: %s", err.Error())
 	}
 
 	defer resp.Body.Close()
diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go
index eb0ce23f5..4a89ff3ef 100644
--- a/pkg/issuer/vault/setup.go
+++ b/pkg/issuer/vault/setup.go
@@ -30,6 +30,7 @@ func (v *Vault) Setup(ctx context.Context) error {
 		return fmt.Errorf(messageVaultConfigRequired)
 	}
 
+	// check if Vault server info is specified.
 	if v.issuer.GetSpec().Vault.Server == "" ||
 		v.issuer.GetSpec().Vault.Path == "" {
 		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageServerAndPathRequired)
@@ -37,6 +38,7 @@ func (v *Vault) Setup(ctx context.Context) error {
 		return fmt.Errorf(messageVaultConfigRequired)
 	}
 
+	// check if at least one auth method is specified.
 	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name == "" &&
 		v.issuer.GetSpec().Vault.Auth.AppRole.RoleId == "" &&
 		v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name == "" {
@@ -45,10 +47,19 @@ func (v *Vault) Setup(ctx context.Context) error {
 		return fmt.Errorf(messsageAuthFieldsRequired)
 	}
 
+	// check if only token auth method is set.
 	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name != "" &&
 		(v.issuer.GetSpec().Vault.Auth.AppRole.RoleId != "" ||
-			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name != "" ||
-			v.issuer.GetSpec().Vault.Auth.AppRole.Path != "") {
+			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name != "") {
+		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageAuthFieldRequired)
+		v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageAuthFieldRequired)
+		return fmt.Errorf(messageAuthFieldRequired)
+	}
+
+	// check if all mandatory Vault appRole fields are set.
+	if v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name == "" &&
+		(v.issuer.GetSpec().Vault.Auth.AppRole.RoleId == "" ||
+			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name == "") {
 		glog.Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageAuthFieldRequired)
 		v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageAuthFieldRequired)
 		return fmt.Errorf(messageAuthFieldRequired)
diff --git a/test/util/vault/util.go b/test/util/vault/util.go
index c843f952b..7d5a194f5 100644
--- a/test/util/vault/util.go
+++ b/test/util/vault/util.go
@@ -65,7 +65,7 @@ func NewVaultInitializer(container, rootMount, intermediateMount, role, authPath
 	client.SetToken(vaultToken)
 
 	if authPath == "" {
-		return nil, fmt.Errorf("Error authPath must be set")
+		authPath = "approle"
 	}
 
 	return &VaultInitializer{

From cb7eaf986f49a66d92ddbc3934aa529af1de0db5 Mon Sep 17 00:00:00 2001
From: "test@test.com" <test@test.com>
Date: Wed, 11 Jul 2018 16:02:23 +0000
Subject: [PATCH 4/4] Run hack/update-reference-docs.sh

---
 docs/generated/reference/output/reference/api-docs/index.html | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/generated/reference/output/reference/api-docs/index.html b/docs/generated/reference/output/reference/api-docs/index.html
index fab686174..9981bb4d1 100755
--- a/docs/generated/reference/output/reference/api-docs/index.html
+++ b/docs/generated/reference/output/reference/api-docs/index.html
@@ -1865,6 +1865,10 @@ Appears In:
 </thead>
 <tbody>
 <tr>
+<td><code>path</code><br /> <em>string</em></td>
+<td>Where the authentication path is mounted in Vault.</td>
+</tr>
+<tr>
 <td><code>roleId</code><br /> <em>string</em></td>
 <td></td>
 </tr>