specify custom ca file to verify the keystone server #35488

dixudx · 2016-10-25T03:33:17Z

What this PR does / why we need it:

Sometimes the keystone server's certificate is self-signed, mainly used for internal development, testing and etc.

For this kind of ca, we need a way to verify the keystone server.

Otherwise, below error will occur.

x509: certificate signed by unknown authority

This patch provide a way to pass in a ca file to verify the keystone server when starting kube-apiserver.

Which issue this PR fixes : fixes #22695, #24984

Special notes for your reviewer:

Release note:

This change is

k8s-ci-robot · 2016-10-25T03:33:17Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

liggitt · 2016-10-25T03:39:55Z

pkg/genericapiserver/options/server_run_options.go

@@ -375,6 +376,11 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.KeystoneURL, "experimental-keystone-url", s.KeystoneURL,
 		"If passed, activates the keystone authentication plugin.")

+	fs.StringVar(&s.KeystoneCAFile, "experimental-keystone-ca-file", s.KeystoneCAFile, ""+


Use the OIDCCAFile flag description as a starting point instead

Already fix this.

liggitt · 2016-10-25T03:41:10Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

 // NewKeystoneAuthenticator returns a password authenticator that validates credentials using openstack keystone
-func NewKeystoneAuthenticator(authURL string) (*KeystoneAuthenticator, error) {
+func NewKeystoneAuthenticator(authURL string, caFile string) (*KeystoneAuthenticator, error) {
 	if !strings.HasPrefix(authURL, "https") {
 		return nil, errors.New("Auth URL should be secure and start with https")
 	}
 	if authURL == "" {
 		return nil, errors.New("Auth URL is empty")
 	}



Load the cert pool from the given file here if cafile is set, rather than loading it for every request

liggitt · 2016-10-25T03:44:11Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		return nil, err
+	}
+
+	config := &tls.Config{}


Skip this whole block if the capool is nil?

certutil.NewPool will return an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates.

So I think it is okay to remain the codes here unchanged.

I mean, leave client.HTTPClient.Transport untouched unless we want to customize the ca pool

liggitt · 2016-10-25T03:44:40Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		config.RootCAs = roots
+	}
+	client.HTTPClient.Transport = &http.Transport{TLSClientConfig: config, }
+


return openstack.Authenticate(…)

I did not get the actual use of this return.

the function already returns client, err, so just return the result directly, instead of if err != nil ...

liggitt · 2016-10-25T03:45:13Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		}
+		config.RootCAs = roots
+	}
+	client.HTTPClient.Transport = &http.Transport{TLSClientConfig: config, }


Needs gofmt

is this equivalent to the transport set up by default by openstack.AuthenticatedClient (with respect to proxy settings, dial settings, timeouts, etc)?

The client created by default openstack.AuthenticatedClient is *gophercloud.ProviderClient.

return &gophercloud.ProviderClient{ IdentityBase: base, IdentityEndpoint: "", }, nil

The default HTTPClient.Transport is nil, and DefaultTransport is used instead.

var DefaultTransport RoundTripper = &Transport{ Proxy: ProxyFromEnvironment, Dial: (&net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, }).Dial, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, }

The client.HTTPClient.Transport I used here is quite a new Transport with only TLSClientConfig configured.

Do you suggest that we should merge these two together ?

util/net#SetOldTransportDefaults(&http.Transport{TLSClientConfig: config}) will set the default dialer, proxy, and timeouts

liggitt · 2016-11-03T14:16:28Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		roots, err := certutil.NewPool(caFile)
+		if err != nil {
+			return nil, err
+		}


build the transport once here and save it

liggitt · 2016-11-03T14:16:49Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		}
+		config := &tls.Config{}
+		config.RootCAs = roots
+		return &KeystoneAuthenticator{authURL, config, caFile}, nil


no need to store config or caFile in the struct, just the custom transport

liggitt · 2016-11-03T14:17:17Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		return nil, err
+	}
+
+	if keystoneAuthenticator.caFile != "" {


if keystoneAuthenticator.transport != nil { client.HTTPClient.Transport = keystoneAuthenticator.transport }

Fixed this in the new commit. Thanks.

liggitt

squash and rebase as well

liggitt · 2016-11-03T15:09:05Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

 )

 // KeystoneAuthenticator contacts openstack keystone to validate user's credentials passed in the request.
 // The keystone endpoint is passed during apiserver startup
 type KeystoneAuthenticator struct {
-	authURL string
+	authURL   string
+	transport *http.Transport


http.RoundTripper

liggitt · 2016-11-03T15:09:25Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+	}
+
+	if keystoneAuthenticator.transport != "" {
+		client.HTTPClient.Transport = netutil.SetOldTransportDefaults(keystoneAuthenticator.transport)


client.HTTPClient.Transport = keystoneAuthenticator.transport

liggitt · 2016-11-03T15:09:39Z

plugin/pkg/auth/authenticator/password/keystone/keystone.go

+		}
+		config := &tls.Config{}
+		config.RootCAs = roots
+		transport := &http.Transport{TLSClientConfig: config}


netutil.SetOldTransportDefaults(&http.Transport{TLSClientConfig: config})

Thanks. Fixed.

k8s-ci-robot · 2016-11-03T15:43:36Z

Jenkins unit/integration failed for commit f00026bcf202bc2b50b26081b4ba707492f4f48f. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

liggitt · 2016-11-03T16:01:55Z

hack/verify-flags/known-flags.txt

@@ -189,6 +189,8 @@ experimental-allowed-unsafe-sysctls
 experimental-bootstrap-kubeconfig
 experimental-keystone-url
 experimental-mounter-path
+experimental-mounter-rootfs-path


this looks unrelated... might need to rebase or remove this

I don't know where it comes from. Seems rebase from previous commits. I did not add this. Quite weird.

Already removed this. Fixed.

k8s-ci-robot · 2016-11-03T16:03:35Z

Jenkins verification failed for commit f00026bcf202bc2b50b26081b4ba707492f4f48f. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

liggitt · 2016-11-03T16:15:26Z

also needs

I1103 08:44:17.892] Run ./hack/update-bazel.sh
I1103 08:44:17.893] FAILED   hack/make-rules/../../hack/verify-bazel.sh 22s

dixudx · 2016-11-03T16:52:23Z

@k8s-bot verify test this

dixudx · 2016-11-04T02:18:14Z

@k8s-bot verify test this

dixudx · 2016-11-04T07:55:12Z

Hi @liggitt I retest this patch on my local env. All works well.

And the corresponding doc PR has also been submitted.

Need lgtm label again. Thanks.

saad-ali · 2016-11-08T05:55:02Z

Release-czar approved post-code freeze merge--This was LGTMed and in the merge-queue at code freeze time for 1.5. Adding 1.5 milestone to let it gets merged after code freeze.

k8s-github-robot · 2016-11-08T20:34:42Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-08T21:12:59Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 40932, 41896, 41815, 41309, 41628) Add custom CA file to openstack cloud provider config **What this PR does / why we need it**: Adds ability to specify custom CA bundle file to verify OpenStack endpoint against. Useful in tests and PoC deployments. Similar to what #35488 did for authentication. **Which issue this PR fixes**: None **Special notes for your reviewer**: Based on #35488 which added support for custom CA file for authentication. **Release note**:

Automatic merge from submit-queue (batch tested with PRs 40932, 41896, 41815, 41309, 41628) Add custom CA file to openstack cloud provider config **What this PR does / why we need it**: Adds ability to specify custom CA bundle file to verify OpenStack endpoint against. Useful in tests and PoC deployments. Similar to what kubernetes/kubernetes#35488 did for authentication. **Which issue this PR fixes**: None **Special notes for your reviewer**: Based on kubernetes/kubernetes#35488 which added support for custom CA file for authentication. **Release note**:

googlebot added the cla: yes label Oct 25, 2016

dixudx mentioned this pull request Oct 25, 2016

support skipping the validity check of keystone server's certificate #35280

Closed

liggitt self-assigned this Oct 25, 2016

liggitt reviewed Oct 25, 2016

View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 25, 2016

liggitt requested changes Oct 25, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2016

dixudx force-pushed the keystone-ca-cert branch from dd60dc8 to 87bd6be Compare October 27, 2016 02:05

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 27, 2016

dixudx force-pushed the keystone-ca-cert branch from 87bd6be to cd3a6c1 Compare October 27, 2016 07:27

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 27, 2016

dixudx force-pushed the keystone-ca-cert branch from cd3a6c1 to 63349fe Compare October 30, 2016 02:30

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 30, 2016

dixudx force-pushed the keystone-ca-cert branch from 63349fe to c3e42e1 Compare November 1, 2016 03:03

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 1, 2016

liggitt reviewed Nov 3, 2016

View reviewed changes

dixudx force-pushed the keystone-ca-cert branch from c3e42e1 to a888386 Compare November 3, 2016 14:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2016

liggitt requested changes Nov 3, 2016

View reviewed changes

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 3, 2016

dixudx force-pushed the keystone-ca-cert branch from 3807c37 to f00026b Compare November 3, 2016 15:26

dixudx force-pushed the keystone-ca-cert branch from f00026b to d8b7893 Compare November 3, 2016 15:47

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2016

liggitt reviewed Nov 3, 2016

View reviewed changes

dixudx force-pushed the keystone-ca-cert branch from d8b7893 to 68d40dc Compare November 3, 2016 16:09

dixudx force-pushed the keystone-ca-cert branch from 68d40dc to aa3d33a Compare November 3, 2016 16:20

dixudx mentioned this pull request Nov 4, 2016

add doc for new flag experimental-keystone-ca-file kubernetes/website#1620

Merged

specify custom ca file to verify the keystone server

dd6c980

dixudx force-pushed the keystone-ca-cert branch from aa3d33a to dd6c980 Compare November 4, 2016 07:13

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016

saad-ali added this to the v1.5 milestone Nov 8, 2016

k8s-github-robot merged commit 860cae0 into kubernetes:master Nov 8, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

lvjiangzhao mentioned this pull request Dec 12, 2016

Remove https check from keystone authentication since this is not essential #38464

Closed

kars7e mentioned this pull request Feb 13, 2017

Add custom CA file to openstack cloud provider config #41309

Merged

dixudx deleted the keystone-ca-cert branch September 9, 2017 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

specify custom ca file to verify the keystone server #35488

specify custom ca file to verify the keystone server #35488

dixudx commented Oct 25, 2016 •

edited by k8s-oncall

Loading

k8s-ci-robot commented Oct 25, 2016

liggitt Oct 25, 2016

dixudx Nov 3, 2016

liggitt Oct 25, 2016

liggitt Oct 25, 2016

dixudx Oct 25, 2016

liggitt Oct 25, 2016

liggitt Oct 25, 2016

dixudx Oct 25, 2016

liggitt Oct 25, 2016

liggitt Oct 25, 2016

liggitt Oct 25, 2016

dixudx Oct 25, 2016

liggitt Oct 25, 2016

liggitt Nov 3, 2016

liggitt Nov 3, 2016

liggitt Nov 3, 2016

dixudx Nov 3, 2016

liggitt left a comment

liggitt Nov 3, 2016

liggitt Nov 3, 2016

liggitt Nov 3, 2016

dixudx Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

liggitt Nov 3, 2016

dixudx Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

liggitt commented Nov 3, 2016

dixudx commented Nov 3, 2016

dixudx commented Nov 4, 2016

dixudx commented Nov 4, 2016

saad-ali commented Nov 8, 2016

k8s-github-robot commented Nov 8, 2016

k8s-github-robot commented Nov 8, 2016

specify custom ca file to verify the keystone server #35488

specify custom ca file to verify the keystone server #35488

Conversation

dixudx commented Oct 25, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2016

liggitt commented Nov 3, 2016

dixudx commented Nov 3, 2016

dixudx commented Nov 4, 2016

dixudx commented Nov 4, 2016

saad-ali commented Nov 8, 2016

k8s-github-robot commented Nov 8, 2016

k8s-github-robot commented Nov 8, 2016

dixudx commented Oct 25, 2016 •

edited by k8s-oncall

Loading