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 pulling requestheader CA from extension-apiserver-authentication ConfigMap without client CA #66394

Merged
merged 1 commit into from Aug 8, 2018

Conversation

@rtripat
Copy link
Contributor

commented Jul 19, 2018

This commit prevents extension API server from erroring out during bootstrap when the core
API server doesn't support certificate based authentication for it's clients i.e. client-ca isn't
present in extension-apiserver-authentication ConfigMap in kube-system.

This can happen in cluster setups where core API server uses Webhook token authentication.

Fixes: #65724

Which issue(s) this PR fixes
Fixes #65724

Special notes for your reviewer:

Release note:

Allows extension API server to dynamically discover the requestheader CA certificate when the core API server doesn't use certificate based authentication for it's clients
@DirectXMan12

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

On the subject of the PR description, please remove the duplicated sections (you don't have to follow the template to the letter if you're duplicating description. It's probably worth explicitly mentioning a real-world use case where this comes up (e.g. we use all token-based auth in our cluster).

Something like

Support pulling requestheader CA from extension configmap without client CA

This commit prevents extension API servers from erroring out during bootstrap when the core API server doesn't only adds a requestheader CA to the extension auth configmap, without a client CA.

This can happen in cluster setups where all communication is token based, and there's a desire to not support client-certificate auth (e.g. [your particular case here]).

Fixes: #65724

[release note stuff here]

More comments to come

@DirectXMan12
Copy link
Contributor

left a comment

comments inline. I think we might need a discussion around whether or not this needs a flag (I think it probably does, because you might still want to error out if you're expecting to get a client CA, but don't)

@@ -240,7 +240,8 @@ func (s *DelegatingAuthenticationOptions) getClientCA() (*ClientCertAuthenticati
return nil, err
}
if incluster == nil {
return nil, fmt.Errorf("cluster doesn't provide client-ca-file")
glog.Warningf("cluster doesn't provide client-ca-file")

This comment has been minimized.

Copy link
@DirectXMan12

DirectXMan12 Jul 19, 2018

Contributor

looks like your indentation has changed here. might want to double-check that.

Might also want to say cluster doesn't provide a client-ca file, so client certificate authentication won't work.

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 19, 2018

Author Contributor

I'll fix these in the next upload

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 24, 2018

Author Contributor

Made the warning message more explicit

@@ -240,7 +240,8 @@ func (s *DelegatingAuthenticationOptions) getClientCA() (*ClientCertAuthenticati
return nil, err
}
if incluster == nil {

This comment has been minimized.

Copy link
@DirectXMan12

DirectXMan12 Jul 19, 2018

Contributor

we should have an option like SkipInClusterClientCALookup or something, similarly to SkipInClusterLookup, or perhaps change the behavior of SkipInClusterLookup

@kubernetes/sig-api-machinery-pr-reviews WDYT?

This comment has been minimized.

Copy link
@DirectXMan12
@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

/cc @cheftako

@k8s-ci-robot k8s-ci-robot requested a review from cheftako Jul 19, 2018

@@ -240,7 +240,8 @@ func (s *DelegatingAuthenticationOptions) getClientCA() (*ClientCertAuthenticati
return nil, err
}
if incluster == nil {
return nil, fmt.Errorf("cluster doesn't provide client-ca-file")
glog.Warningf("cluster doesn't provide client-ca-file")

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 19, 2018

Member

probably worth making this warning more explicit about what it was looking for, and what usually should be done in the main apiserver to populate that configmap. also worth warning that this is continuing without the ability to use x509-based authentication

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 19, 2018

Member

it seems like getRequestHeader() should be consistent with this as well... if we successfully look up the options from the configmap, and get no info, should getRequestHeader() log a warning that we're continuing without request header auth, and continue?

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 19, 2018

Author Contributor

@liggitt I didn't make the same change for request header because as per my understanding delegated authentication requires the requestheader client CA to trust the aggregator, hence failing when it isn't present is the right behavior. @DirectXMan12 What do you think?

This comment has been minimized.

Copy link
@DirectXMan12

DirectXMan12 Jul 20, 2018

Contributor

Yeah, I think there's a lot more of an argument for "I want requestheader auth without normal client cert auth" than vice-versa (I don't see the case for auto-auth setup with only normal client cert auth).

Do agree with the warning being more explicit -- one of the reasons I actually suggested an option, so that you have to manually opt in to this behavior if you want it.

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 20, 2018

Author Contributor

I'm open to making the log message more explicit as suggested though unsure why a new flag is necessary to opt in to building the extension API server without client-ca? What's the risk in that case?

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 20, 2018

Member

I think there's a lot more of an argument for "I want requestheader auth without normal client cert auth" than vice-versa

That's probably true. Forget I said anything.

@rtripat rtripat force-pushed the rtripat:i-65724 branch 2 times, most recently from fbc4063 to ef7fe60 Jul 24, 2018

@rtripat rtripat changed the title Allow extension API server to bootstrap when core API server doesn't … Support pulling requestheader CA from extension-apiserver-authentication ConfigMap without client CA Jul 24, 2018

@@ -240,7 +240,8 @@ func (s *DelegatingAuthenticationOptions) getClientCA() (*ClientCertAuthenticati
return nil, err
}
if incluster == nil {
return nil, fmt.Errorf("cluster doesn't provide client-ca-file")
glog.Warningf("cluster doesn't provide client-ca-file in configmap/%s in %s, so client certificate authentication to extension api-server won't work.", authenticationConfigMapName, authenticationConfigMapNamespace)

This comment has been minimized.

Copy link
@sttts

sttts Jul 24, 2018

Contributor

having glog warnings in funcs with error return type is an anti-pattern. Can't we return a better error and make sure it is printed in the caller's code?

This comment has been minimized.

Copy link
@cheftako

cheftako Jul 25, 2018

Member

The code is switching an error at startup for a warning. I'm concerned that this will make debugging handshake issues with aggregated api-servers much harder. This only works if there is some external system enforcing the client-ca is kept consistent for all aggregated api-servers. I agree with @DirectXMan12, if we proceed with this it should be a deliberate decision to not look up config map rather than implied by it being empty.

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 25, 2018

Author Contributor

Are you suggesting a new flag like SkipInClusterClientCaLookup to not look up client-ca in ConfigMap when set to true?

If so, I have two follow up questions:

  1. How do administrators reason between usage of existing SkipInClusterLookup and the new SkipInClusterClientCaLookup? Won't that be confusing?
  2. Isn't the recommendation to use an altogether separate CA for aggregator layer as a client? It will be nice to understand scenario in which extension API server needs to authenticate it's client using cluster's client CA.

This comment has been minimized.

Copy link
@cheftako

cheftako Jul 25, 2018

Member

Sorry going to work on your questions out of order.

The aggregation system uses two sets of certs, 1 to authenticate incoming requests from the Kube API Server (KAS) and another as the client cert it uses to identify itself in mutual TLS when making requests to the Kube API Server (and possibly other systems such as webhooks). The cert obtained from the config map is the cert used to authenticate the requester is the KAS. The reference recommendation has to do with if the second cert could be relied on as being signed by the same CA. Currently we allow it to be different and allow the the CA being given to the KAS via API registration. Then the aggregated API server would normally load this cert from a location on disk. In the sample api server we specify this location with the flags --tls-cert-file and --tls-private-key-file.

On your first question I am not sure there should be a difference. However we passed a check for SkipInClusterLookup, so we know its not set. However I do not like the idea of the admin having configured the system to use in cluster certs but we do something else because the client ca file was missing.

@cheftako
Copy link
Member

left a comment

Details are under @sttts comments. I am a little concerned that the admin has set SkipInClusterLookup to false but that by not returning error on a missing cluster ca file we are effectively skipping the in cluster lookup.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Successful fetching of the configmap with an absent client-ca means the kube-apiserver was started without that option, right? Continuing with a warning in that case still seems like honoring the specified configuration to me, and doesn't impact the ability of the server to handle requests via the aggregator

@cheftako

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Successful fetching of the configmap with an absent client-ca means we failed to get a value for the extension-apiserver-authentication key from the config map. That probably means that the KAS was started without "--requestheader-client-ca-file" option. However than having the aggregator started with SkipInClusterLookup set to false seems like a misconfiguration across multiple servers. Having that misconfiguration cause the aggregator to try and read a local file which may or may not be correct/present for the CA seems dangerous. I also suspect that if its a misconfiguration then it won't actually work (the handshake at least) and that will be hard to debug.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

That probably means that the KAS was started without "--requestheader-client-ca-file" option.

That's not what this PR is changing. This PR is tolerating a kube-apiserver started without --client-ca, which does not affect aggregation

@cheftako

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Unless I'm missing something this PR is changing the staging/src/k8s.io/apiserver/pkg/server/options.DelegatingAuthenticationOptions::getClientCA() method. This in turned is used by staging/src/k8s.io/apiserver/pkg/server/options.RecommendedOptions which is in turn used by staging/src/k8s.io/sample-apiserver/pkg/cmd/server.WardleServerOptions. This is the example we present of how to build an aggregated api-server and I believe it is the mechanism we intend our customers to use for establishing trust on aggregated api-servers. So I believe this does affect aggregation.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

delegating authentication configures two methods of authentication:

  • getClientCA() sets up a ca for x509-authenticated requests made directly to the server
  • getRequestHeader() sets up the authenticator for requests made via the aggregator (includes a ca for mTLS with the aggregator as auth proxy)

if no client ca is configured for the kube-apiserver, that does not affect aggregation

@cheftako

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 26, 2018

@rtripat rtripat force-pushed the rtripat:i-65724 branch from ef7fe60 to acac227 Jul 26, 2018

@k8s-ci-robot k8s-ci-robot added size/S and removed lgtm size/XS labels Jul 26, 2018

@@ -394,3 +402,13 @@ func (s *DelegatingAuthenticationOptions) newTokenAccessReview() (authentication

return client.TokenReviews(), nil
}

func IgnorableError(s string, args ...interface{}) ignorableError {

This comment has been minimized.

Copy link
@sttts

sttts Jul 31, 2018

Contributor

why do we need this pubilc constructor. Would prefer to not leak it.

This comment has been minimized.

Copy link
@rtripat

rtripat Jul 31, 2018

Author Contributor

Good point, fixed

@rtripat rtripat force-pushed the rtripat:i-65724 branch from 642a97b to d96f091 Jul 31, 2018

@rtripat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@sttts Could you take a look at the latest revision again?

@@ -394,3 +402,13 @@ func (s *DelegatingAuthenticationOptions) newTokenAccessReview() (authentication

return client.TokenReviews(), nil
}

func newIgnorableError(s string, args ...interface{}) ignorableError {

This comment has been minimized.

Copy link
@sttts

sttts Aug 6, 2018

Contributor

simpler: type ignorable error. Then we don't need a constructor or any func. Just the type wrapper.

This comment has been minimized.

Copy link
@rtripat

rtripat Aug 7, 2018

Author Contributor

If I'm creating just a type wrapper, then I wasn't able to differentiate between ignorable error and non-ignorable error.
See: https://play.golang.org/p/T0hSRsyPKj9

I'm fairly new to golang so this may be due to something I'm missing.

This comment has been minimized.

Copy link
@sttts

sttts Aug 7, 2018

Contributor

Compare https://goplay.space/#C2IONvrPbiH (which does not work, as you say) and https://goplay.space/#NfoDb-hUFtS which does.

This comment has been minimized.

Copy link
@rtripat

rtripat Aug 7, 2018

Author Contributor

Done

@rtripat rtripat force-pushed the rtripat:i-65724 branch from d96f091 to 14b31a1 Aug 7, 2018

@rtripat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Failed run looks unrelated to change, re-running

/test pull-kubernetes-e2e-kops-aws

@@ -394,3 +402,5 @@ func (s *DelegatingAuthenticationOptions) newTokenAccessReview() (authentication

return client.TokenReviews(), nil
}

type ignorableError struct{ error }

This comment has been minimized.

Copy link
@sttts

sttts Aug 8, 2018

Contributor

👍

gofmt will not like this, will it?

This comment has been minimized.

Copy link
@sttts

sttts Aug 8, 2018

Contributor

strange. isn't there a space missing?

This comment has been minimized.

Copy link
@sttts

sttts Aug 8, 2018

Contributor

it likes it.

if _, ignorable := err.(ignorableError); !ignorable {
return err
} else {
glog.Warningf("%s", err)

This comment has been minimized.

Copy link
@sttts

sttts Aug 8, 2018

Contributor

Warning(err)

Support pulling requestheader CA from extension-apiserver-authenticat…
…ion ConfigMap without client CA

This commit prevents extension API server from erroring out during bootstrap when the core
API server doesn't support certificate based authentication for it's clients i.e. client-ca isn't
present in extension-apiserver-authentication ConfigMap in kube-system.

This can happen in cluster setups where core API server uses Webhook token authentication.

Fixes: #65724

@rtripat rtripat force-pushed the rtripat:i-65724 branch from 14b31a1 to db828a4 Aug 8, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 8, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, rtripat, sttts

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 446eef5 into kubernetes:master Aug 8, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation rtripat authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2018
Kubernetes Submit Queue
Merge pull request #67272 from liggitt/automated-cherry-pick-of-#6639…
…4-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #66394: Support pulling requestheader CA from

Cherry pick of #66394 on release-1.9.

#66394: Support pulling requestheader CA from
k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2018
Kubernetes Submit Queue
Merge pull request #67271 from liggitt/automated-cherry-pick-of-#6639…
…4-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #66394: Support pulling requestheader CA from

Cherry pick of #66394 on release-1.11.

#66394: Support pulling requestheader CA from
k8s-github-robot pushed a commit that referenced this pull request Aug 16, 2018
Kubernetes Submit Queue
Merge pull request #67270 from liggitt/automated-cherry-pick-of-#6639…
…4-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #66394: Support pulling requestheader CA from

Cherry pick of #66394 on release-1.10.

#66394: Support pulling requestheader CA from
k8s-github-robot pushed a commit that referenced this pull request Aug 28, 2018
Kubernetes Submit Queue
Merge pull request #67694 from sttts/sttts-kube-apiserver-always-crea…
…te-extension-apiserver-authentication

Automatic merge from submit-queue (batch tested with PRs 67694, 64973, 67902). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kube-apiserver: always create configmap/extension-apiserver-authentication

Other components (aggregated apiservers) read the configmap and fail hard if it does not exist. But they work without all fields being set (#66394). In the future, components like ctrl-manager and scheduler won't need kube-apiserver to authenticate with them at all. So, consequently we should always create the file, even if it is empty.

```release-note
Always create configmaps/extensions-apiserver-authentication from kube-apiserver.
```
@jaymccon jaymccon referenced this pull request Oct 3, 2018
1 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.