Implement AccessorID resolution and client ip propagation

[ncode]

Hi @mxab,

I've started the patch last week and would like to double check the direction it's going so far :).

My idea here was to resolve the token as early as possible to propagate down. The upstream ip address is very cheap to propagate, so I thought about always passing it so we can have cidr based validators and mutators even when not resolving the token

Addresses #18

--- Copilot summary of changes

This pull request introduces several changes to the admissionctrl and cmd/nacp packages to support token resolution and context headers. The most significant changes include adding a new resolveToken field, modifying the JobHandler to handle token resolution, and updating the mutators and validators to include context headers.

Token Resolution and Context Headers:

• admissionctrl/controller.go: Added a new resolveToken field to the JobHandler struct and updated the NewJobHandler function to accept this parameter. Introduced a ResolveToken method to check the resolveToken flag. [1] [2]
• cmd/nacp/nacp.go: Implemented a new getClientIP function and resolveTokenAccessor function to support token resolution. Updated the NewProxyHandler function to store context headers and resolve tokens if required. Modified the buildServer function to create mutators and validators with token resolution capabilities. [1] [2] [3] [4] [5]

Mutator and Validator Updates:

• admissionctrl/mutator/json_patch_webhook.go, admissionctrl/mutator/webhook_mutator.go, admissionctrl/validator/webhook_validator.go: Added context headers to HTTP requests in the Mutate and Validate methods. [1] [2] [3] [4] [5] [6]

Configuration Changes:

• config/config.go: Added a new ResolveToken field to the Validator and Mutator structs, and introduced a RequestContext struct to store context information. [1] [2]

Test Updates:

• cmd/nacp/nacp_test.go: Updated tests to include scenarios for token resolution and context headers. Modified existing tests to handle the new resolveToken parameter and added new tests to verify token resolution functionality. [1] [2] [3] [4] [5] [6] [7] [8] [9]

[mxab]

Hi @ncode, awesome! This goes definitely in the right direction!

Some thoughts so far but theses are things we also could stabilise later:

• the token resolution provides additional context, e.g. policies afaik, should we pass those to the webhooks as well?
• afaik X-... headers are discouraged, should we use NACP-.... ?
• passing everything as headers might get a bit messy in cases there will be more values, we could break the API so far and pass it as a combined document to the webhook ´{ job: , context : { remote_ip_addr: ..., ... }}`

I'm also currently waiting for a recommendation from the OPA communityhow to model this there

[ncode]

Hi @mxab,

Hi @ncode, awesome! This goes definitely in the right direction!

Some thoughts so far but theses are things we also could stabilise later:

• the token resolution provides additional context, e.g. policies afaik, should we pass those to the webhooks as well?

I didn't think of it but it's a pretty nice idea to pass all the available information since we've already resolved it :)

• afaik X-... headers are discouraged, should we use NACP-.... ?

Yes. Let's use NACP-...

• passing everything as headers might get a bit messy in cases there will be more values, we could break the API so far and pass it as a combined document to the webhook ´{ job: , context : { remote_ip_addr: ..., ... }}`

I agree that passing the context like you describe will make things a bit more flexible, specially with the extra metadata from the token it will be cleaner. I'm only in doubt about X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content. What do you think?

I'm also currently waiting for a recommendation from the OPA communityhow to model this there

Nice!

[mxab]

Sorry for the late response.

X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content

Ah so if I get you correctly your "validation" part in this case is solely on the Client IP Address + Accessor ID. Yeah I guess it makes sense.

Regarding the OPA part, they recommended to create an input consisting out of the context and the job. So I guess we need to update this part here and create a input struct the consists out of a caller context (client ip addr, resolved token) and the job

So for the remote hooks I think it make sense to use the same structure, we could still leave the headers as well so it works for your use case

[ncode]

Sorry for the late response.

No worries!

X-FORWARD-FOR and the NACP-Nomad-Accessor-ID, these would be useful to have available as readers without the need to parse the content

Ah so if I get you correctly your "validation" part in this case is solely on the Client IP Address + Accessor ID. Yeah I guess it makes sense.

Regarding the OPA part, they recommended to create an input consisting out of the context and the job. So I guess we need to update this part here and create a input struct the consists out of a caller context (client ip addr, resolved token) and the job

So for the remote hooks I think it make sense to use the same structure, we could still leave the headers as well so it works for your use case

Deal! I will start working on it this weekend!