Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Support reviewing audiences of token #56

Merged
merged 4 commits into from
Feb 17, 2020
Merged

Conversation

brancz
Copy link
Owner

@brancz brancz commented Dec 3, 2019

This adds the ability to verify the audience of a token, so clients can use audience scoped tokens, as opposed to essentially allowing the kube-rbac-proxy to impersonate the requesting client using its serviceaccount token.

Closes #18

@s-urbaniak @paulfantom @lilic @metalmatze

@s-urbaniak
Copy link
Collaborator

do you mind to put the vendoring into a separate commit?

@brancz
Copy link
Owner Author

brancz commented Dec 4, 2019

Done.

@@ -0,0 +1,170 @@
# non-resource-url example
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, how does this differ from examples/non-resource-url-token-request/README.md? Or is this duplication just accidentally?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whops I haven't finished this yet it seems

@@ -0,0 +1,170 @@
# non-resource-url example
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as above: how do these example differ?

@s-urbaniak
Copy link
Collaborator

@brancz looking good so far, just a couple of questions:

  • the change commit contains vendoring code, do you mind to keep it separate?
  • I think there are duplicate readmes, or not?
  • How does audiences relate to non-resource URL protection?

@brancz brancz force-pushed the token-audience-review branch 2 times, most recently from 41a4d7e to 938f07a Compare January 25, 2020 09:37
@brancz
Copy link
Owner Author

brancz commented Jan 25, 2020

the change commit contains vendoring code, do you mind to keep it separate?

Done, sorry about that.

I think there are duplicate readmes, or not?

Fixed this.

How does audiences relate to non-resource URL protection?

It doesn't really, I just wanted to provide some example showing the functionality. I added a note at the top of the example readme to make sure that's clearer:

Token audience reviews can be combined with any of the other features of kube-rbac-proxy, such as reviewing permission for specific resources, this example merely chooses to show the functionality using a non-resource-url permission.

This should be ready for another round of reviews now :)

@metalmatze
Copy link
Collaborator

Are you still waiting for reviews or something else?

@brancz
Copy link
Owner Author

brancz commented Jan 30, 2020

Waiting for reviews :)

@@ -28,18 +28,22 @@ All command line flags:
```txt
$ kube-rbac-proxy -h
Usage of _output/linux/amd64/kube-rbac-proxy:
--add_dir_header If true, adds the file directory to the header
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this is used at all?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s from klog

--stderrthreshold severity logs at or above this threshold go to stderr (default 2)
--tls-cert-file string File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert)
--tls-cipher-suites strings Comma-separated list of cipher suites for the server. Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). If omitted, the default Go cipher suites will be used
--tls-min-version string Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. (default "VersionTLS12")
--tls-private-key-file string File containing the default x509 private key matching --tls-cert-file.
--tls-reload-interval duration The interval at which to watch for TLS certificate changes, by default set to 1 minute. (default 1m0s)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 👍

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually came in as I regenerated the help text


> Token audience reviews can be combined with any of the other features of kube-rbac-proxy, such as reviewing permission for specific resources, this example merely chooses to show the functionality using a non-resource-url permission.

This examples is in essence similar to the [non-resource-url](../non-resource-url/) example, with the key difference, that this example requires the ServiceAccount token sent by a client must be scoped to the `kube-rbac-proxy.default.svc` audience. In this example the scoped ServiceAccount token is obtained via a projected volume and mounted into the client container from where it can be consumed. The reasoning here is that scroped tokens cannot be used to impersonate an entity by re-using the token to perform a request against the Kubernetes API itself.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/scroped/scoped

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@brancz
Copy link
Owner Author

brancz commented Feb 7, 2020

I split the PR into multiple commits, which should make it very easy to review now, as the actual code changes (in the second commit) are minimal and are mainly upgrading to using the v1 APIs. The rest are tests and docs (95% of this PR).

if len(authn.X509.ClientCAFile) > 0 {
p, err = dynamiccertificates.NewStaticCAContentFromFile(authn.X509.ClientCAFile)
if err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit: let's wrap this err, so context is visible in log lines.

@s-urbaniak
Copy link
Collaborator

this lgtm 🎉 I really just have one nit regarding error wrapping, else this looks great. A small code footprint, but great security improvement 👍

@s-urbaniak s-urbaniak merged commit 9538692 into master Feb 17, 2020
@s-urbaniak s-urbaniak deleted the token-audience-review branch February 17, 2020 09:18
ibihim pushed a commit to ibihim/kube-rbac-proxy that referenced this pull request Oct 20, 2023
…hift-4.12-kube-rbac-proxy

OCPBUGS-11645: Updating kube-rbac-proxy images to be consistent with ART
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support audience validation
3 participants