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

@rtripat rtripat 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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 19, 2018
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2018
@DirectXMan12
Copy link
Contributor

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

Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix these in the next upload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the warning message more explicit

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe @kubernetes/sig-auth-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jul 19, 2018
@fedebongio
Copy link
Contributor

/cc @cheftako

@k8s-ci-robot k8s-ci-robot requested a review from cheftako July 19, 2018 20:13
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 i-65724 branch 2 times, most recently from fbc4063 to ef7fe60 Compare July 24, 2018 01:16
@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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rtripat rtripat Jul 25, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@cheftako cheftako left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

liggitt 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
Copy link
Member

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
Copy link
Member

liggitt 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
Copy link
Member

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
Copy link
Member

liggitt 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
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 26, 2018
@rtripat
Copy link
Contributor Author

rtripat commented Jul 30, 2018

/test pull-kubernetes-integration

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

return client.TokenReviews(), nil
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed

@rtripat
Copy link
Contributor Author

rtripat 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sttts sttts Aug 7, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rtripat
Copy link
Contributor Author

rtripat 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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

gofmt will not like this, will it?

Copy link
Contributor

Choose a reason for hiding this comment

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

strange. isn't there a space missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

it likes it.

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

Choose a reason for hiding this comment

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

Warning(err)

…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: kubernetes#65724
@sttts
Copy link
Contributor

sttts commented Aug 8, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2018
@k8s-ci-robot
Copy link
Contributor

[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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2018
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

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
k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2018
…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
…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
…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
…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.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow extension API server to read requestheader-client-ca from extension-apiserver-authentication ConfigMap
8 participants