-
Notifications
You must be signed in to change notification settings - Fork 193
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
sig-auth review: comment on code #162
Conversation
Signed-off-by: Krzysztof Ostrowski <[email protected]>
flagset.AddGoFlagSet(klogFlags) | ||
|
||
// kube-rbac-proxy flags | ||
flagset.StringVar(&cfg.insecureListenAddress, "insecure-listen-address", "", "The address the kube-rbac-proxy HTTP server should listen on.") |
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.
going top to bottom, this is weird. Why would a process consuming tokens allow non-tls access? The kube-apiserver, kube-controller-manager, etc listen on tls and allow unauthenticated access to readyz for LBs.
TODO: pre-acceptance, eliminate the ability to use cleartext tokens.
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.
@s-urbaniak, can we remove the insecure-listen-address
in the release after next?
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.
historically this was made a separate setting in 904be98.
I do agree this is dangerous territory. insecure listening is maybe just useful for debugging. I am fine with removing it. We should make it clear though that this is a breaking change.
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.
@deads2k, we are only focused on removing an insecure listen address for kube-rbac-proxy, but do we want to continue support for an upstream H2C?
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.
@deads2k, we are only focused on removing an insecure listen address for kube-rbac-proxy, but do we want to continue support for an upstream H2C?
I'm ok allowing an http backend as long as we are not asserting identity to it. If identity is asserted, then the connection should be https.
flagset.StringVar(&cfg.tls.keyFile, "tls-private-key-file", "", "File containing the default x509 private key matching --tls-cert-file.") | ||
flagset.StringVar(&cfg.tls.minVersion, "tls-min-version", "VersionTLS12", "Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants.") | ||
flagset.StringSliceVar(&cfg.tls.cipherSuites, "tls-cipher-suites", nil, "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") | ||
flagset.DurationVar(&cfg.tls.reloadInterval, "tls-reload-interval", time.Minute, "The interval at which to watch for TLS certificate changes, by default set to 1 minute.") |
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.
minor: k8s.io/apiserver auto-reloads these now if this depended on that.
TODO: post-acceptance, this needs to be updated.
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.
No, this is for the raw go http proxy server cert/key reload and not dependent on k8s.io/apiserver.
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.
However, we can investigate if k8s.io/apiserver proxy code can be reused 👍
|
||
// Auth flags | ||
flagset.StringVar(&cfg.auth.Authentication.X509.ClientCAFile, "client-ca-file", "", "If set, any request presenting a client certificate signed by one of the authorities in the client-ca-file is authenticated with an identity corresponding to the CommonName of the client certificate.") | ||
flagset.BoolVar(&cfg.auth.Authentication.Header.Enabled, "auth-header-fields-enabled", false, "When set to true, kube-rbac-proxy adds auth-related fields to the headers of http requests sent to the upstream") |
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.
Where can I configure the client cert/key pair used to identify the kube-rbac proxy to the upstream when setting these headers?
TODO: pre-acceptance, if kube-rbac is a front-proxy, it must have a client-certificate so that the server it proxies to can confirm it is the kube-rbac-proxy.
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.
The TLS config can be specified here.
It is used here to specify tls.Config.
If no TLS cert / key is provided, is creates a self-signed one.
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.
The TLS config can be specified here.
It is used here to specify tls.Config.
If no TLS cert / key is provided, is creates a self-signed one.
Those are serving certificates, but not client certificates
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.
yes, agreed, this flag should gain additional client-cert/key parameters. My only historical guess why this is currently as is:
- upstream and kube-rbac-proxy are always running in the same pod
- upstream listens (usually) on localhost only
- hence, a rogue client would have to enter the pod's network namespace to execute rogue requests
- if a malicious client can enter pod's network namespace, it can most likely also enter the pod's mount namespace and introspect configures client certs/keys
anyways, definitely a good and necessary addition 👍
flagset.StringVar(&cfg.auth.Authentication.Header.UserFieldName, "auth-header-user-field-name", "x-remote-user", "The name of the field inside a http(2) request header to tell the upstream server about the user's name") | ||
flagset.StringVar(&cfg.auth.Authentication.Header.GroupsFieldName, "auth-header-groups-field-name", "x-remote-groups", "The name of the field inside a http(2) request header to tell the upstream server about the user's groups") | ||
flagset.StringVar(&cfg.auth.Authentication.Header.GroupSeparator, "auth-header-groups-field-separator", "|", "The separator string used for concatenating multiple group names in a groups header field's value") | ||
flagset.StringSliceVar(&cfg.auth.Authentication.Token.Audiences, "auth-token-audiences", []string{}, "Comma-separated list of token audiences to accept. By default a token does not have to have any specific audience. It is recommended to set a specific audience.") |
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 default means that a kube-apiserver token would be accepted by this proxy? This looks like a risky default.
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.
Yes, this is what it does.
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.
@deads2k do you mind to elaborate? I recall clayton also mentioned this during the sig-auth call, but I honestly don't remember 😬
flagset.DurationVar(&cfg.tls.reloadInterval, "tls-reload-interval", time.Minute, "The interval at which to watch for TLS certificate changes, by default set to 1 minute.") | ||
|
||
// Auth flags | ||
flagset.StringVar(&cfg.auth.Authentication.X509.ClientCAFile, "client-ca-file", "", "If set, any request presenting a client certificate signed by one of the authorities in the client-ca-file is authenticated with an identity corresponding to the CommonName of the client certificate.") |
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.
Will this default to the in-cluster value if unset?
TODO: answer and we'll decide
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.
No, I does not. It uses the DelegatingAuthenticatorConfig
that only verifies the client-ca, if it is given (link).
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.
That's a pain.
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.
@deads2k what would be a sensible in-cluster location?
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.
@deads2k what would be a sensible in-cluster location?
it's stored in a configmap in kube-public namespace
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.
Post acceptance, but strongly recommended.
flagset.StringVar(&cfg.upstream, "upstream", "", "The upstream URL to proxy to once requests have successfully been authenticated and authorized.") | ||
flagset.BoolVar(&cfg.upstreamForceH2C, "upstream-force-h2c", false, "Force h2c to communiate with the upstream. This is required when the upstream speaks h2c(http/2 cleartext - insecure variant of http/2) only. For example, go-grpc server in the insecure mode, such as helm's tiller w/o TLS, speaks h2c only") | ||
flagset.StringVar(&cfg.upstreamCAFile, "upstream-ca-file", "", "The CA the upstream uses for TLS connection. This is required when the upstream uses TLS and its own CA certificate") | ||
flagset.StringVar(&configFileName, "config-file", "", "Configuration file to configure kube-rbac-proxy.") |
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 is the authorization config, distinct from the authentication config?
TODO: pre-acceptance, update developer comments and command-line help
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.
Yes, the config file is authz only. Authentication is handled by flags. Might have grown organically in time.
The pre-acceptance task is to update documentation in CLI and README, not the uniform handling of configuration options, right?
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.
The pre-acceptance task is to update documentation in CLI and README, not the uniform handling of configuration options, right?
correct. Ideally with links to where I can see what format I've got here. I couldn't tell what this was without reading the code.
// Force http/2 for connections to the upstream i.e. do not start with HTTP1.1 UPGRADE req to | ||
// initialize http/2 session. | ||
// See https://github.com/golang/go/issues/14141#issuecomment-219212895 for more context | ||
proxy.Transport = &http2.Transport{ |
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.
Flag for more eyes.
} else { | ||
klog.Info("Reading certificate files") | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
r, err := rbac_proxy_tls.NewCertReloader(cfg.tls.certFile, cfg.tls.keyFile, cfg.tls.reloadInterval) |
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.
note: find this package.
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.
note: find this package.
prefer the k8s.io/apiserver package that does this.
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.
} | ||
authorizerConfig := authorizerfactory.DelegatingAuthorizerConfig{ | ||
SubjectAccessReviewClient: client, | ||
AllowCacheTTL: 5 * time.Minute, |
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.
What is the kube default allow and deny TTL.
TODO: post-acceptance, make this configurable with flags matching k8s.io/apiserver and have the defaults match.
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.
It is 5 * time.Minute
/ 30 * time.Second
: https://github.com/kubernetes/kubernetes/blob/442574f3a7321ff0f34dec9bae67f28651dbabd2/pkg/kubelet/apis/config/v1beta1/defaults.go#L87
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.
But this doesn't make that much sense to use some kubelet defaults.
This looks more appropriate: 10 * time.Second
.
userName = a.GetUser().GetName() | ||
} | ||
|
||
if isAllowed(saConfig.User.Name, userName) && |
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.
Allowing an empty username (and really any field) in the file to match all users looks like a recipe for "mispelled a value and now any user can do this!"
TODO: post-acceptance, start a path to have a safe (non-matching) default and migrate with warnings, flags, and forced changes.
explicitly using a "*" instead is safer and doesn't match valid names.
config []StaticAuthorizationConfig | ||
} | ||
|
||
func (saConfig StaticAuthorizationConfig) Equal(a authorizer.Attributes) bool { |
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.
minor: this looks more like Matches
than Equal
|
||
func NewStaticAuthorizer(config []StaticAuthorizationConfig) (*staticAuthorizer, error) { | ||
for _, c := range config { | ||
if c.ResourceRequest != (c.Path == "") { |
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.
post-acceptance, validate this in an explicit "validate user input" stage in addition to having it here. That change matches the upstream k/k structure
"k8s.io/apiserver/pkg/authorization/authorizer" | ||
) | ||
|
||
func TestStaticAuthorizer(t *testing.T) { |
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.
missing wildcard non-resource checks?
"k8s.io/apiserver/pkg/authorization/authorizer" | ||
) | ||
|
||
func TestStaticAuthorizer(t *testing.T) { |
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.
missing multiple path non-resource requests. /heathz/detailedfailure
for instance. Positive and negative.
|
||
authenticatorConfig := authenticatorfactory.DelegatingAuthenticatorConfig{ | ||
Anonymous: false, // always require authentication | ||
CacheTTL: 2 * time.Minute, |
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.
What are the k8s.io/apiserver defaults
TODO: post-acceptance, path to make defaults match.
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.
} | ||
|
||
// New creates an authenticator, an authorizer, and a matching authorizer attributes getter compatible with the kube-rbac-proxy | ||
func New(client clientset.Interface, config Config, authorizer authorizer.Authorizer, authenticator authenticator.Request) (*kubeRBACProxy, 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.
error is always nil.
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.
clientset.Interface is also not used. lol
|
||
type kubeRBACProxy struct { | ||
// authenticator identifies the user for requests to kube-rbac-proxy | ||
authenticator.Request |
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.
minor: prefer not directly embedding types.
} | ||
|
||
// Authenticate | ||
u, ok, err := h.AuthenticateRequest(req) |
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 does not appear to match kube filters.WithAuthentication
. What is different and can we use the upstream filter
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.
The authenticator.Request
interface is at the core of filters.WithAuthentication.
The biggest difference is that there are no metrics taken and that the audience check happens in the DelegatingAuthenticatorConfig
part.
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.
TODO: pre-acceptance, collapse onto upstream for consistency.
func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { | ||
ctx := req.Context() | ||
if len(h.Config.Authentication.Token.Audiences) > 0 { | ||
ctx = authenticator.WithAudiences(ctx, h.Config.Authentication.Token.Audiences) |
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.
not seeing where this is checked yet. Where is it? Can we match kube and do it during authentication.
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.
The Audiences are used for the DelegatingAuthenticatorConfig. This leverages kubernetes logic.
// GetRequestAttributes populates authorizer attributes for the requests to kube-rbac-proxy. | ||
func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http.Request) []authorizer.Attributes { | ||
apiVerb := "" | ||
switch r.Method { |
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.
Falling through with an empty verb isn't what I would expect. Why does it do this instead of Unknown_r.Method
or failing?
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.
TODO: pre-acceptance closing the empty verb hole
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.
didn't find good precedence in k8s which quickly looking but we should investigate.
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.
If the method is not specified, SAR returns allowed only if the identity is allowed to use all verbs with "*"
, but we could make it explicit by starting of with "*"
instead of ""
.
allAttrs := append(allAttrs, authorizer.AttributesRecord{ | ||
User: u, | ||
Verb: apiVerb, | ||
Namespace: n.authzConfig.ResourceAttributes.Namespace, |
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.
Do these values match what was provided in the --config from back in main.go?
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.
Yes. The configfile is unmarshaled into the Config type that has the Namespace ResourceAttributes
.
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.
But only in the particular case of n.authzConfig.Rewrites == nil
, otherwise it might be a template.
} | ||
} | ||
|
||
if len(params) == 0 { |
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 is a case where the configuration means: "forward this request without performing authorization"? I need to check the config file format, but I'm surprised that the default is "don't protect". Am I reading this incorrectly?
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.
Well, the logic is not trivial and begs to break on code changes, but it looks like so:
- We start with empty allAttr.
- The next two if-blocks are returning.
- The next two if-blocks fill the
params
. - If
params
is empty, returnallAttrs
, which is by then is still empty. - And here we fail, if the returned
allAttrs
is empty.
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.
TODO: pre-acceptance this should proactively error from this method so that we aren't succeeding by coordination in two unrelated parts.
attrs := authorizer.AttributesRecord{ | ||
User: u, | ||
Verb: apiVerb, | ||
Namespace: templateWithValue(n.authzConfig.ResourceAttributes.Namespace, param), |
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.
So this means that the same param is passed to every possible resource attribute for golang template substitution? That means that the header and query param can only logically control a single aspect without differentiating between header and query param?
Making sure I understand and that it's well described. The behavior is acceptable.
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.
Actually, this seems somewhat risky. If I specify multiple headers and get one to succeed, but the others fail, how can we be assured that the consuming application honors the same header or query param that passed?
This must be documented as an "if any of these values, controlled by the caller, pass validation, the request is allowed" case.
EDIT: re-reading from above, it actually means "everything must allow", but it forgot to get values for repeated headers?
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.
So as far as I understand it,
- We can specify values through query (multiple) and through the header (first).
- Those values get individually applied to the config.
- Where there are template markers, those get replaced.
- We append it to the
authorizer.AttributesRecord
.
Simple, non-hacky example:
https://github.com/brancz/kube-rbac-proxy/blob/master/test/e2e/static-auth/configmap-resource.yaml#L9-L14=
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.
TODO: pre-acceptance. headers are map[string][]string. This code needs to properly handle that.
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.
agreed
} | ||
|
||
// DeepCopy of Proxy Configuration | ||
func (c *Config) DeepCopy() *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.
TODO: pre-acceptance either remove or fuzz test. This is similar to the deep copy of rest.config in k8s.io/client-go to be sure that adding fields doesn't result in non-deepcopiable results or incorrect deep copies.
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.
let's remove, it seems unused.
} | ||
|
||
// http.Transport sourced from go 1.10.7 | ||
transport := &http.Transport{ |
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.
TODO: pre-acceptance diff with k/k transport defaults
proxy.Transport = &http2.Transport{ | ||
// Allow http schema. This doesn't automatically disable TLS | ||
AllowHTTP: true, | ||
// Do disable TLS. |
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.
TODO: pre-acceptance figure out why it's save to disable TLS. If it isn't safe to disable TLS (seems questionable for a proxy) stop doing it.
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.
@deads2k what is kube's stance on supporting h2c? If kube does not explicitly support it, i think this is fine to be removed.
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.
(again, a breaking change with announcement necessary if we decide that path)
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 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.
By doing some reading on h2c, the code and history digging, I found that this is meant to be used for proxy to upstream communication and not anyone else.
It seems like some folks wanted to protect tiller (which did h2c) and what not.
In combination with your comment here @deads2k, which I agree on, I would revisit the configuration / logic for identity forwarding in such cases.
for _, pathAllowed := range cfg.allowPaths { | ||
found, err = path.Match(pathAllowed, req.URL.Path) | ||
if err != nil { | ||
return |
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.
If we have an error, why not at least return a 500 to the user?
TODO: pre-acceptance either return an error (doesn't have to be the real error) or write a test proving that this produces an error.
|
||
mux := http.NewServeMux() | ||
mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
found := len(cfg.allowPaths) == 0 |
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.
personal taste: I found this logic difficult to read and would find it easier with a switch/case I think.
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 agree. It might be prone to fatal mistakes.
} | ||
|
||
mux := http.NewServeMux() | ||
mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { |
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'm not seeing a unit test for this
TODO: pre-acceptance this mux handling method must have have 100% unit test coverage.
for _, pathIgnored := range cfg.ignorePaths { | ||
ignorePathFound, err = path.Match(pathIgnored, req.URL.Path) | ||
if err != nil { | ||
return |
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.
same error comment as above.
|
||
// Handle authenticates the client and authorizes the request. | ||
// If the authn fails, a 401 error is returned. If the authz fails, a 403 error is returned | ||
func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { |
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.
TODO: pre-acceptance, explain what the bool means. I think it means, "passed authentication and authorization", but it's relied on from main.go
as simply, ok
, so it's less than clear.
} | ||
} | ||
|
||
proxy.ServeHTTP(w, req) |
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.
Flag for diff against the kube-apiserver upgradeaware proxy. I'm surprised a different proxy is being used here than the aggregated apiserver proxy. I remember issues with ugprade awareness.
if cfg.secureListenAddress != "" { | ||
srv := &http.Server{Handler: mux, TLSConfig: &tls.Config{}} | ||
|
||
if cfg.tls.certFile == "" && cfg.tls.keyFile == "" { |
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.
For a proxy you'd expect something external to trust and hand a token to, generating a self-signed one here doesn't seem particularly helpful.
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 do agree
var gr run.Group | ||
{ | ||
if cfg.secureListenAddress != "" { | ||
srv := &http.Server{Handler: mux, TLSConfig: &tls.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.
most of this actually looks like it wants k8s.io/apiserver ServingOptions for the ports, bind addresses, certs, tls ciphers, etc.
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.
the more i think about the more i believe we should strongly consider reusing k8s.io/apiserver serving logic (and options).
srv.TLSConfig.MinVersion = version | ||
srv.TLSConfig.ClientAuth = tls.RequestClientCert | ||
|
||
if err := http2.ConfigureServer(srv, nil); err != nil { |
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.
TODO, if this isn't replaced with upstream's serving library, the tls and listener logic here needs review.
} | ||
} | ||
|
||
proxy := httputil.NewSingleHostReverseProxy(upstreamURL) |
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.
The purpose of this PR was to create the issues. I would close the PR as of now. The issues are still open and reference key discussion of this PR. |
Signed-off-by: Krzysztof Ostrowski [email protected]