fix(#5563): sets namespace using vault client methods instead of using raw request object by zscholl · Pull Request #5589 · cert-manager/cert-manager

zscholl · 2022-11-18T19:34:48Z

Pull Request Motivation

This change fixes #5563 by changing the vault client logic to instead use the vault client's functions to set the namespace, rather than passing in a new request object with custom headers.

Why is this necessary?

The behavior of the vault client changed in vault api version v1.11.0 with this commit. The impact of this change means that any headers that are set in the request object passed in to RawRequest are wiped out if there are no headers set in the vault client object.

Before vault API v1.11.0 RawRequest respected headers that were passed in via the request argument.

The vault API dependency was upgraded in cert-manager v1.10.0: 39fa9f5 and so anyone using this version or later will see that the namespace setting does nothing and all requests to vault go to the default root namespace.

Kind

bug

Release Note

Fixed Vault namespace bug

jetstack-bot · 2022-11-18T19:34:57Z

Hi @zscholl. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jetstack-bot · 2022-11-18T19:35:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zscholl
To complete the pull request process, please assign jakexks after the PR has been reviewed.
You can assign the PR to them by writing /assign @jakexks in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

zscholl · 2022-11-18T19:44:27Z

/kind bug

zscholl · 2022-11-18T19:48:26Z

/cc inteon

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

inteon · 2022-11-19T09:35:25Z

@zscholl I made some changes to your PR to make sure the solution still works when clients are shared across threads.
Can you also please add a test to verify that this is working?
/ok-to-test

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

zscholl · 2022-11-21T18:13:06Z

/retest-required

zscholl · 2022-11-21T18:18:01Z

/retest-required

zscholl · 2022-11-21T18:27:10Z

@inteon your approach looks much better 😄. Thanks for committing that. 🙏

I threw in some tests that heavily borrow from Vault's tests for the WIthNamespace function. But these should sufficiently validate and catch any regression as they check the headers that are received on the server side to ensure that the namespace is included when it should be.

inteon · 2022-11-21T18:41:37Z

internal/vault/vault_test.go

+	issuer *cmapi.VaultIssuer
+}
+
+func testHTTPServer(t *testing.T, handler http.Handler) (*vault.Config, net.Listener) {


Can we simplify this code by replacing this function with https://pkg.go.dev/net/http/httptest#NewServer?

indeed we can! Great suggestion 👍

and you can also just call Close() on server instead of server.Listener.

Good catch, thank you!

sorry for all the nitpicking, but I would also use server.URL instead of server.Listener.Addr()

no worries at all! These are all valid and good suggestions. I appreciate your keen eye :)

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

wallrj

Thanks @zscholl and @inteon

I looked at the WithNamespace method in the vault SDK and wondered if the fix could be as simple as:

master...wallrj:fix-vault-namespace-rjw

It's not clear to me why the namespace should only be applied to RawRequest?
If that is necessary, please add some explanatory comments to the code for the benefit of future maintainers.

zscholl · 2022-11-22T14:23:09Z

It's not clear to me why the namespace should only be applied to RawRequest?

Hey @wallrj! That's a good insight. The namespace doesn't necessarily have to be applied to the RawRequest, but it does have to be applied to the client. However the issue with the approach you suggested is that all requests made with the client cannot have the namespace set. Particularly the /v1/sys/health. This is one of the root only paths that cannot have a namespace sent with it.

I tested this approach early on and Vault enterprise returns a 404 when sending the namespace header to a root only API path.

e.g.:

Namespace: mynamesapce
URL: GET https://my.vault.url.com/v1/sys/health
Code: 404. Errors:

Alternatively we could use the approach you suggested and then explicitly use WithNamespace again in the IsVaultInitializedAndUnsealed method and make that request with a locally initialized client that has no namespace header. Or we could create a local client variable in each of the functions that require a namespace using WithNamespace from the Vault SDK .

I'll add a comment to the NamespacedRawRequest method to make it clear what it's purpose is for now. Please let me know if one of the other alternatives is more desirable!

Thank you for the feedback 🙏

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

jetstack-bot · 2022-11-22T15:07:34Z

@zscholl: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-e2e-v1-25-upgrade	`91afc9b`	link	true	`/test pull-cert-manager-master-e2e-v1-25-upgrade`
pull-cert-manager-master-make-test	`91afc9b`	link	true	`/test pull-cert-manager-master-make-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wallrj · 2022-11-22T15:48:26Z

Thanks @zscholl ,

A few more thoughts, and forgive me for dragging this out.
I have been reading more about the root only paths but I am still confused.

In hashicorp/vault#14934 (comment) @ncabatoff wrote:

I'm not sure the expected behaviour is a good idea. We may change some root-only paths to be available in namespaces in the future. And if you're specifying X-Vault-Namespace, you probably authenticated into that namespace, and won't be likely to have access to the root-only paths anyway unless they're unauthenticated.
...
Also note that on HCP Vault there is no access to the root namespace, so this behaviour wouldn't make sense at all there.

If that is true, then I'd expect the existing health check in the cert-manager Vault Issuer to fail for users of HCP Vault and where their enterprise vault has not been configured to allow access to the root namespace.

I'm dwelling on HCP Vault because that is what @schmitz-chris reported using in the original issue:

cert-manager in version 1.9.x and above is not able to log into HCP Vault with approle and a namespace #5563

Another confusion for me is that that original issue reports that the namespace feature is not working since cert-manager 1.9, but the new Vault client (which changed the namespace behaviour) was introduced by @inteon in 39fa9f5 which was released in 1.10.
I'd like to know, is cert-manager 1.9 broken or not?

Refs:

wallrj · 2022-11-22T16:12:28Z

Actually, perhaps /sys/health is a special case:

In hashicorp/vault#209 (comment) @mitchellh wrote:

/sys/health is an unauthenticated endpoint, youcan hit it with anything. We'll update the docs to say such.

And in the documentation for sys/health the sample request does not include an authentication token:

https://developer.hashicorp.com/vault/api-docs/system/health#sample-request

So that is consistent with the statement that Most endpoints under /v1/sys that require authentication are not available.

zscholl · 2022-11-22T17:12:37Z

@wallrj I don't know for sure about the HCP behavior, but your reading of the docs seems sound to me. That sys/health should be available for HCP users.

I'd like to know, is cert-manager 1.9 broken or not?

We've deployed cert-manager 1.9.2 and did not observe the namespace bug. That said, we are using self-deployed Enterprise Vault so I cannot say for certain that there isn't a specific problem with 1.9 and HCP.

wallrj · 2022-11-22T17:51:09Z

internal/vault/vault.go

 type Client interface {
 	NewRequest(method, requestPath string) *vault.Request
-	RawRequest(r *vault.Request) (*vault.Response, error)
+	NamespacedRawRequest(vaultIssuer *v1.VaultIssuer, r *vault.Request) (*vault.Response, error)
 	SetToken(v string)
 	Token() string
 	Sys() *vault.Sys
 }


I dislike this change because:

before, the Client interface could be described as "the subset of the methods of the Vault SDK client which we use in cert-manager", which would be clear to someone reading the code.
Now it represents a Client which is subtly different than the vault SDK.

it muddles up two APIs which IMO should be separate; the NamespacedRawRequest now accepts a cmapi.VaultIssuer pointer, where it seems to me that it only really needs to accept a new namespace string parameter.

wallrj · 2022-11-22T17:52:29Z

internal/vault/vault.go

 	Token() string
 	Sys() *vault.Sys


Both Token and Sys are unused in the implementation and should be removed....probably in a separate PR because we need to keep this PR as small as possible so that it can be backported.

wallrj · 2022-11-22T17:53:44Z

internal/vault/vault.go

-	v.addVaultNamespaceToRequest(request)
-
-	resp, err := client.RawRequest(request)
+	resp, err := client.NamespacedRawRequest(v.issuer.GetSpec().Vault, request)


Suggested change

resp, err := client.NamespacedRawRequest(v.issuer.GetSpec().Vault, request)

resp, err := client.NamespacedRawRequest(v.issuer.GetSpec().Vault.Namespace, request)

...would be much more readable, I think.

wallrj · 2022-11-22T17:54:19Z

internal/vault/vault.go

 	healthURL := path.Join("/v1", "sys", "health")
 	healthRequest := v.client.NewRequest("GET", healthURL)
-	healthResp, err := v.client.RawRequest(healthRequest)
+	healthResp, err := v.client.NamespacedRawRequest(nil, healthRequest)


Suggested change

healthResp, err := v.client.NamespacedRawRequest(nil, healthRequest)

healthResp, err := v.client.NamespacedRawRequest("", healthRequest)

....would make it clearer that this is a non-namespaced request.

wallrj · 2022-11-22T18:03:38Z

internal/vault/vault_test.go

+			}
+		})
+	}
+}


I'm not convinced we should be testing for the presence of the X-Vault-Namespace header.
That's an implementation detail of the Vault server.
What if Hashicorp introduce a slightly different Header or some other mechanism for configuring the namespace?

The vault SDK client has a Namespace() method which tells us which namespace it's going to use, and in
#5591 I explored a slightly different way of testing this and implemented the namespace and non-namespace client following a pattern I found in Hashicorp Nomad.

Please tell me what you think of that approach and if you both prefer this PR, I'll concede and approve this PR.

I prefer your approach in #5591.

I think based on the points you raised, #5591 is clearer and more consistent. It also eliminates ambiguity around when to use or not use a namespaced request. In your implementation you just use the default client for everything except sys calls.

I also agree with your point about this test being tightly coupled to Vault's implementation. And it's better to just check that the client has the expected namespace set. I did like being able to check what was actually received on the server side, but that's a better test for the vault client itself.

I'm all for closing this PR in favor of #5591 🚀

wallrj · 2022-11-23T11:33:56Z

Closing in favour of #5591 as discussed with @zscholl and @inteon :

Set the Vault namespace using vault SDK client methods instead of using raw request object #5591

jetstack-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 18, 2022

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Nov 18, 2022

jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 18, 2022

jetstack-bot requested a review from inteon November 18, 2022 19:48

fix(#5563): set namespace using vault client methods

d3b9c63

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2022

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2022

test: adds tests for vault namespaced requests

5db9228

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 21, 2022

zscholl mentioned this pull request Nov 21, 2022

Integration test flake: various timeouts #5198

Closed

inteon reviewed Nov 21, 2022

View reviewed changes

zscholl added 2 commits November 21, 2022 12:47

test: use httptest.NewServer instead of own implementation

c13cb11

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

fix: closes the server

d31735b

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

zscholl added 3 commits November 21, 2022 13:35

style: removes blank line at start of function

8930d93

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

style: removes irrelevant comment

a49de7b

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

fix: user server.URL instead of listener.Addr()

55dec5c

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

wallrj requested changes Nov 22, 2022

View reviewed changes

style: adds explanatory comment to NamespacedRawRequest

91afc9b

Signed-off-by: Zak Scholl <zak.scholl@shopify.com>

wallrj reviewed Nov 22, 2022

View reviewed changes

wallrj mentioned this pull request Nov 23, 2022

Set the Vault namespace using vault SDK client methods instead of using raw request object #5591

Merged

wallrj closed this Nov 23, 2022

zscholl deleted the zscholl/fix-vault-namespace branch November 23, 2022 14:15

	resp, err := client.NamespacedRawRequest(v.issuer.GetSpec().Vault, request)
	resp, err := client.NamespacedRawRequest(v.issuer.GetSpec().Vault.Namespace, request)

	healthResp, err := v.client.NamespacedRawRequest(nil, healthRequest)
	healthResp, err := v.client.NamespacedRawRequest("", healthRequest)

Conversation

zscholl commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Motivation

Why is this necessary?

Kind

Release Note

Uh oh!

jetstack-bot commented Nov 18, 2022

Uh oh!

jetstack-bot commented Nov 18, 2022

Uh oh!

zscholl commented Nov 18, 2022

Uh oh!

zscholl commented Nov 18, 2022

Uh oh!

inteon commented Nov 19, 2022

Uh oh!

zscholl commented Nov 21, 2022

Uh oh!

zscholl commented Nov 21, 2022

Uh oh!

zscholl commented Nov 21, 2022

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!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

zscholl commented Nov 22, 2022

Uh oh!

jetstack-bot commented Nov 22, 2022

Uh oh!

wallrj commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallrj commented Nov 22, 2022

Uh oh!

zscholl commented Nov 22, 2022

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!

wallrj commented Nov 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zscholl commented Nov 18, 2022 •

edited

Loading

wallrj commented Nov 22, 2022 •

edited

Loading