-
Notifications
You must be signed in to change notification settings - Fork 188
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
Use upstream delegating authn/z logic #215
Conversation
05bb58b
to
e48a719
Compare
39d7104
to
2d62e6b
Compare
f8d06a1
to
fbb8af9
Compare
fbb8af9
to
101fdc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing effort! 🎉
pkg/proxy/proxy.go
Outdated
|
||
var allAttrs []authorizer.Attributes | ||
if len(proxyAttrs) == 0 { | ||
return authorizer.DecisionDeny, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing.
@@ -49,36 +49,56 @@ Secure serving flags: | |||
--tls-private-key-file string File containing the default x509 private key matching --tls-cert-file. | |||
--tls-sni-cert-key namedCertKey A pair of x509 certificate and private key file paths, optionally suffixed with a list of domain patterns which are fully qualified domain names, possibly with prefixed wildcard segments. The domain patterns also allow IP addresses, but IPs should only be used if the apiserver has visibility to the IP address requested by a client. If no domain patterns are provided, the names of the certificate are extracted. Non-wildcard matches trump over wildcard matches, explicit domain patterns trump over extracted names. For multiple key/certificate pairs, use the --tls-sni-cert-key multiple times. Examples: "example.crt,example.key" or "foo.crt,foo.key:*.foo.com,foo.com". (default []) | |||
|
|||
Delegating authentication flags: | |||
|
|||
--authentication-kubeconfig string kubeconfig file pointing at the 'core' kubernetes server with enough rights to create tokenreviews.authentication.k8s.io. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to create an issue for a migration doc... the flags changed quite dramatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
pkg/filters/path.go
Outdated
) | ||
|
||
func WithAllowPaths(allowPaths []string, handler http.HandlerFunc) http.HandlerFunc { | ||
func NewAllowPathAuthorizer(allowPaths []string) authorizer.Authorizer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the "lets return an authorizer.Authorizer
logic", so that it fits better into the authz-fuzz... and we can do that, but the implementation is pretty heavy. I am not completely convinced... maybe a huge comment above the functions would help.
Is there an upstream equivalent? I think something like NewPassthroughPathAuthorizer or BlockAllTheOthers... 😆 name would fit better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's this, which was a huge inspiration: https://github.com/openshift/kubernetes/blob/7dab57f2302ec0f94b648ccb48b0fa67c2befbb3/staging/src/k8s.io/apiserver/pkg/authorization/path/path.go#L30-L31
@@ -369,3 +312,28 @@ func secureServerRunner( | |||
|
|||
return runner, interrupter | |||
} | |||
|
|||
func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverconfig.AuthorizationInfo) (authorizer.Authorizer, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we could move such code to the authz pkg.
pkg/proxy/proxy.go
Outdated
var _ authorizer.Authorizer = &krpAuthorizer{} | ||
|
||
const kubeRBACProxyParamsKey = iota | ||
|
||
// Config holds proxy authorization and authentication settings | ||
type Config struct { | ||
Authentication *authn.AuthnConfig | ||
Authorization *authz.Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like in the wrong place, but not part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed as a part of the re-packaging cleanup
pkg/proxy/proxy.go
Outdated
@@ -143,3 +160,43 @@ func templateWithValue(templateString, value string) string { | |||
} | |||
return out.String() | |||
} | |||
|
|||
func WithKubeRBACProxyParamsHandler(handler http.Handler, authzConfig *authz.Config) http.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, another filter. Strengthens my opinion to move it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one belongs close to the authorizer
04d7a34
to
c9f4b71
Compare
c9f4b71
to
6605c76
Compare
@@ -224,6 +233,7 @@ func createRequest(queryParams, headers map[string][]string) *http.Request { | |||
} | |||
} | |||
r.URL.RawQuery = q.Encode() | |||
r.URL, _ = url.Parse(r.URL.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid mutating the original *url.URL
, it would make sense to copy it above the r.URL.RawQuery = q.Encdoe()
.
This brings behavior changes: - ignore-paths now only allows astersisk to denote prefix matching - when a user fails to authenticate, the returned status is going to be 403 instead of 401 as they will appear as system:anonymous to the proxy
The upstream logic only allows prefix-matching for wildcard patterns but the proxy originally allowed shell-like filepath matching.
This option actually exists in the new options, so these two were shadowing. Also don't add the client-ca as request-header-client-ca, these each serve a different purpose.
6605c76
to
4a034c4
Compare
This PR brings the upstream logic for delegated authentication and a bit of reworked authz bits to fit the upstream
WithAuthorization
http handling.It's built on top of #213 so that one needs to merge first.
We may want to consider doing a minor release before merging this PR because these changes might change the behavior quite a bit (automatic in-cluster client CA retrieval is a good example that I can think of from the top of my head).
Related to #169