Vault: configurable appRole authentication path by vdesjardins · Pull Request #612 · cert-manager/cert-manager

vdesjardins · 2018-06-05T02:43:32Z

What this PR does / why we need it:
Configurable appRole authentication path in Vault.

Which issue this PR fixes
fixes #560

Special notes for your reviewer:

Release note:

Configurable Vault appRole authentication path using the attribute is spec.vault.auth.authPath in the issuer.

munnerz · 2018-06-05T09:00:20Z

/ok-to-test

munnerz · 2018-06-05T09:54:38Z

docs/tutorials/vault/creating-vault-issuers.rst

        path: pki_int/sign/example-dot-com
        server: https://vault
        auth:
+          authPath: approle


This feels a bit awkward - we have auth.authPath, which actually only applies to AppRole currently, and defaults to something that is specific to the approle auth backend.

I guess this is because token based auth does not use a mounted backend, and we support no others right now.

We can work around this either by making auth.approle.path the key to set (and in future, creating auth.kubernetes.path, auth.pki.path etc). OR we have auth.path, which becomes a required field iff the auth backend selected is not 'token'.

I'm keen to choose whatever is most obvious to users 😄

Indeed that's the reason why I put it directly under the auth struct. I have no problem moving it under the appRole and supply a specific path for other authentication mechanisms when we implement them.

s/authPath/path/g (as you changed this as part of review 😄)

munnerz · 2018-06-05T09:54:58Z

pkg/apis/certmanager/v1alpha1/types.go

 	// 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"`


As this is already a child of auth, I think we can shorten this to path

munnerz · 2018-06-05T09:56:37Z

test/util/vault/util.go


+	if authPath == "" {
+		authPath = "approle"
+	}


We're defaulting this field in multiple places - I'd rather return an error if it isn't specified than default to the same value again

munnerz · 2018-06-05T09:57:11Z

test/util/vault/util.go

-	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)


should be path.Join("v1", "auth", v.authPath, "role", v.role) (i.e. let path.Join join v1 and auth)

munnerz · 2018-06-05T09:58:49Z

test/e2e/certificate/certificate_vault.go

+	certificateName := "test-vault-certificate"
+	certificateSecretName := "test-vault-certificate"
+	vaultSecretAppRoleName := "vault-role"
+	vaultPath := fmt.Sprintf("%s/sign/%s", intermediateMount, role)


I think we can use path.Join here?

avoidik · 2018-06-07T12:50:11Z

I'd like to suggest to use the same approach like with roleId and name it roleName

munnerz · 2018-06-25T11:40:18Z

docs/tutorials/vault/creating-vault-issuers.rst

        path: pki_int/sign/example-dot-com
        server: https://vault
        auth:
+          authPath: approle


s/authPath/path/g (as you changed this as part of review 😄)

munnerz · 2018-06-25T11:40:29Z

docs/tutorials/vault/creating-vault-issuers.rst

 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


s/authPath/path

munnerz · 2018-06-25T11:40:49Z

docs/tutorials/vault/creating-vault-issuers.rst

-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*.


s/authPath/path - also, is this comment still true?

Yes if the path is empty it still default to approle.

munnerz · 2018-06-25T11:41:16Z

pkg/apis/certmanager/v1alpha1/types.go


 type VaultAppRole struct {
+	// Where the authentication path is mounted in Vault.
+	Path string `json:"path,omitempty"`


Remove omitempty

munnerz · 2018-06-25T11:41:55Z

pkg/issuer/vault/issue.go

-	url := "/v1/auth/approle/login"
+	authPath := appRole.Path
+	if authPath == "" {
+		authPath = "approle"


👍 this is okay for now. We will look at moving defaulting etc. out after the webhooks PR has merged

munnerz · 2018-06-25T11:42:57Z

pkg/issuer/vault/issue.go

 	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())


I don't think the request made here is to log in? Update error message

munnerz · 2018-06-25T11:45:06Z

pkg/issuer/vault/setup.go

 		(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 != "") {


From what I can tell, v.issuer.GetSpec().Vault.Auth.AppRole.Path might be blank (as we default it elsewhere in memory). Does this not break approle auth where the Path is not set?

Also, just taking a look over the brace above this block:

https://github.com/vdesjardins/cert-manager/blob/e3f67ecaab3a8fec4da7d4e86c78d2593cee20c3/pkg/issuer/vault/setup.go#L40-L42

Should this not be using || instead of && for the check?

Either way, can we add some comments to each of these if blocks to make it clear what we're checking for? 😄

munnerz · 2018-06-25T11:47:09Z

test/util/vault/util.go


+	if authPath == "" {
+		return nil, fmt.Errorf("Error authPath must be set")
+	}


authPath is an optional field, we needn't require it be set in our fixtures (this causes disparity between tests and docs 😄)

munnerz · 2018-07-11T12:01:01Z

Sorry for the delay looking at this again - this should land in the 0.4 release later today 😄

/lgtm
/approve

jetstack-bot · 2018-07-11T12:01:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

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

munnerz · 2018-07-11T15:51:59Z

/test chart-verify

jetstack-bot · 2018-07-11T16:03:03Z

New changes are detected. LGTM label has been removed.

munnerz · 2018-07-11T16:03:35Z

Rebased and run hack/update-reference-docs-dockerized.sh for this PR so we can get it merged for 0.4 😄

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 5, 2018

jetstack-bot requested review from kragniz and munnerz June 5, 2018 02:43

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 5, 2018

munnerz reviewed Jun 5, 2018

View reviewed changes

munnerz added the area/vault Indicates a PR directly modifies the Vault Issuer code label Jun 5, 2018

vdesjardins force-pushed the custom-approle-path branch 3 times, most recently from a833406 to e3f67ec Compare June 15, 2018 16:02

munnerz reviewed Jun 25, 2018

View reviewed changes

vdesjardins force-pushed the custom-approle-path branch from e3f67ec to 51313b8 Compare July 4, 2018 02:10

madjam002 mentioned this pull request Jul 9, 2018

Adds Kubernetes authentication type for Vault Issuer #649

Closed

jetstack-bot assigned munnerz Jul 11, 2018

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2018

vdesjardins and others added 4 commits July 11, 2018 16:00

Vault: configurable appRole authentication path

2995cc9

code review modifications

ca3b909

code review fixes

7fae0fc

Run hack/update-reference-docs.sh

cb7eaf9

munnerz force-pushed the custom-approle-path branch from 51313b8 to cb7eaf9 Compare July 11, 2018 16:03

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

munnerz added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

jetstack-bot merged commit a162a5b into cert-manager:master Jul 11, 2018

Conversation

vdesjardins commented Jun 5, 2018

Uh oh!

munnerz commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdesjardins Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avoidik commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

munnerz commented Jul 11, 2018

Uh oh!

jetstack-bot commented Jul 11, 2018

Uh oh!

munnerz commented Jul 11, 2018

Uh oh!

jetstack-bot commented Jul 11, 2018

Uh oh!

munnerz commented Jul 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdesjardins Jun 6, 2018 •

edited

Loading