Add SNI support to the apiserver #35109

sttts · 2016-10-19T12:05:05Z

This PR adds the --tls-sni-key-cert flag to the apiserver. It can be passed multiple times in the following ways:

$ apiserver \
    --tls-sni-cert-key '*.example.com,example.com: example.key,example.crt' \
    --tls-sni-cert-key 'foo.key,foo.crt'

The first variant explicitly sets the accepted domain names, the second variant reads the common names and DNS names from the certificate itself.

If no domain name matches, the existing certificate (--tls-cert-file) is used.

    fs.Var(config.NewNamedCertKeyArray(&s.SNICertKeys), "tls-sni-cert-key", ""+
        "A pair of x509 certificate and private key file paths, optionally prefixed with a list of "+
        "domain patterns which are fully qualified domain names, possibly with prefixed wildcard "+
        "segments. 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-key-cert multiple times. "+
        "Examples: \"example.key,example.crt\" or \"*.foo.com,foo.com:foo.key,foo.crt\".")

Add SNI support to the apiserver

Pass multiple certificates and domain name patterns with `--tls-sni-cert-key` and the right certificate will be chosen depending on the url the client is using.

This change is

sttts · 2016-10-19T13:57:03Z

@k8s-bot gci gke e2e test this

liggitt · 2016-10-19T14:01:17Z

needs tests that actually start a TLS server and check that a default-cert+sni-certs config actually serves the sni cert to one of the sni hostnames, and falls back to the default cert for all other hostnames

liggitt · 2016-10-19T13:34:21Z

pkg/genericapiserver/genericapiserver.go

+			return nil, err
+		}
+		if len(nkc.Names) > 0 {
+			for _, name := range nkc.Names {


explicitly specified names should trump auto-populated names

for example, if I have the following certs:
cert1: CN=bar.com
cert2: CN=foo.com, dnsSAN=bar.com

and I specify these flags:
--tls-sni-cert-key=cert1.pem,key1.pem,bar.com --tls-sni-cert-key=cert2.pem,key2.pem

I should end up with
"bar.com": cert1
"foo.com": cert2

I think we should give auto-generation precedence, but enforce that the first cert given trumps any later one. That matches your example. WDYT?

explicitly given names should always outweigh auto-detected ones

liggitt · 2016-10-19T13:35:21Z

pkg/genericapiserver/options/server_run_options.go

+		"domain patterns which are fully qualified domain names, possibly with prefixed wildcard "+
+		"segments. If no domain patterns are provided, the common names of the certificate are "+
+		"used instead. For multiple key/certificate pairs, use the --tls-sni-key-cert multiple times. "+
+		"Examples: \"example.key,example.crt\" or \"*.foo.com,foo.com:foo.key,foo.crt\".")


maybe put the variable args at the end, and the cert first (all the go stdlib functions do cert,key)? (e.g. cert,key[,name,...])

maybe with a colon?

sure, cert:key[:name,...]?

key,cert:name,name,name

put cert first for consistency, unsure about the delimiters

I am fine with either order. Where do we have cert,key? I my head the key exists first, then the cert ;)

server.ListenAndServeTLS(cert,key)
tls.X509KeyPair(certbytes,keybytes)
tls.LoadX509KeyPair(cert,key)

That's not user facing. But cert/key is fine as well.

changed it to: cert,key:name,name,name

liggitt · 2016-10-19T13:37:26Z

pkg/util/config/namedkeycert_flag.go

+	return "namedKeyCert"
+}
+
+func (a *NamedKeyCertArray) String() string {


this is for display only, not for re-parsing, right?

It's only used for the default output right now: [].

liggitt · 2016-10-19T13:48:44Z

pkg/genericapiserver/genericapiserver.go

@@ -200,7 +206,8 @@ func (s *GenericAPIServer) Run() {
 				// Can't use SSLv3 because of POODLE and BEAST
 				// Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher
 				// Can't use TLSv1.1 because of RC4 cipher usage
-				MinVersion: tls.VersionTLS12,
+				MinVersion:        tls.VersionTLS12,
+				NameToCertificate: namedCerts,


does this need to include the names from the default cert as well?

when I did this in OpenShift, I had to use a custom GetCertificate function instead of NameToCertificate:

// Set SNI certificate func
// Do not use NameToCertificate, since that requires certificates be included in the server's tlsConfig.Certificates list,
// which we do not control when running with http.Server#ListenAndServeTLS
GetCertificate: cmdutil.GetCertificateFunc(extraCerts),

can you verify this is no longer the case?

edit: looks like this may have been fixed in go1.5

There is a default GetCertificate now.

there was a default GetCertificate in go1.4 as well, which behaved differently, and required jumping through hoops to get the right cert selected. The current implementation looks like it does what we want, but I do want tests to make sure the combination of ListenAndServeTLS(defaultCert,defaultKey) and the NameToCertificate map we're passing in selects the right cert.

sttts · 2016-10-19T14:12:51Z

@liggitt do we have any test for GenericApiServer.run() right now? I guess we don't, but we should. I vote for adding a close channel to run() and allow clean shutdown. /cc @deads2k

liggitt · 2016-10-19T14:24:29Z

I don't see a clean way to shut down a server started with ListenAndServeTLS

deads2k · 2016-10-20T14:39:21Z

@liggitt do we have any test for GenericApiServer.run() right now? I guess we don't, but we should. I vote for adding a close channel to run() and allow clean shutdown. /cc @deads2k

I'm ok adding a channel, but I doubt it will bring everything down cleanly. We had a lot of trouble with that in the distant past.

sttts · 2016-10-20T14:47:28Z

@deads2k former go versions (<= 1.3) had races when closing the http.Listener asynchronously to bring down a server. I have read this has improved and maybe this was a reason for the trouble your are mentioning.

sttts · 2016-10-21T11:25:56Z

@deads2k @liggitt added integration tests

k8s-ci-robot · 2016-10-28T09:01:02Z

Jenkins Kubemark GCE e2e failed for commit ce9830c9e1292ba79eb4a01ab0bd1fb46d2665cc. Full PR test history.

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

k8s-ci-robot · 2016-10-28T09:07:05Z

Jenkins GCI GCE e2e failed for commit ce9830c9e1292ba79eb4a01ab0bd1fb46d2665cc. Full PR test history.

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

liggitt · 2016-10-31T20:08:24Z

LGTM, squash in "address comments" commit

deads2k · 2016-10-31T20:09:34Z

examples/apiserver/apiserver.go

@@ -105,6 +106,6 @@ func Run(serverOptions *genericoptions.ServerRunOptions) error {
 	if err := s.InstallAPIGroup(&apiGroupInfo); err != nil {
 		return fmt.Errorf("Error in installing API: %v", err)
 	}
-	s.PrepareRun().Run()
+	s.PrepareRun().Run(wait.NeverStop)


This one we want to stop.

added a close channel everywhere where the example apiserver is used.

deads2k · 2016-10-31T20:11:49Z

pkg/genericapiserver/genericapiserver.go

 	InsecureServingInfo *ServingInfo

+	// numerical ports, set after listening
+	effectiveSecurePort, effectiveInsecurePort int


Just reading through, this looks weird. I'm assuming I'll see why later, but a better comment may help.

deads2k · 2016-10-31T20:16:01Z

pkg/genericapiserver/serve.go

+	go func() {
+		defer utilruntime.HandleCrash()
+
+		for {


Why are we still trying multiple times? Didn't we decide we preferred crashlooping? I'm ok as a followup.

Only after the first synchronous try which worked. That was my understanding from our small irc discussion. A follow-up is fine. I prefer a hard crash in such a case such that the outer orchestrator (e.g. systemd or kubelet) can do its job.

I can't think of a good reason to retry such a fundamental failure internally

deads2k · 2016-10-31T20:17:20Z

pkg/util/cert/cert.go

+		// Many clients fail TLS connections if they see two different certs with the same serial number
+		// from the same signer (and they have a local memory of previously seen certs from a given signer).
+		// Hence, we put a timestamp here.
+		SerialNumber: new(big.Int).SetInt64(time.Now().Unix()),


I'd rather see UID. @liggitt did you prefer timestamp?

Oh, and if it switches to uid, it needs to be positive.

yeah, random would probably be better

In fact this was an accident that this function was changed at all. It's unrelated to SNI. Will undo the change.

deads2k · 2016-10-31T20:20:49Z

minor comments, lgtm otherwise.

k8s-ci-robot · 2016-11-01T08:13:28Z

Jenkins unit/integration failed for commit 2ceca5d7d42bace0766e181b72d8984d603bd54b. 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.

sttts · 2016-11-01T10:10:38Z

Squashed.

k8s-github-robot · 2016-11-01T10:47:39Z

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

k8s-github-robot · 2016-11-01T11:25:54Z

Automatic merge from submit-queue

Automatic merge from submit-queue Restore old apiserver cert CN This patch got lost during rebase of #35109: - set `host@<unix-timestamp>` as CN in self-signed apiserver certs - skip non-domain CN in getNamedCertificateMap

sttts added kind/enhancement area/apiserver labels Oct 19, 2016

sttts added this to the v1.5 milestone Oct 19, 2016

sttts assigned sttts and liggitt Oct 19, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 19, 2016

sttts force-pushed the sttts-sni branch from 0e321be to 6db6562 Compare October 19, 2016 12:42

googlebot added the cla: yes label Oct 19, 2016

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

sttts force-pushed the sttts-sni branch from 6db6562 to a496413 Compare October 19, 2016 13:14

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

sttts removed their assignment Oct 19, 2016

sttts 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 Oct 19, 2016

liggitt requested changes Oct 19, 2016

View reviewed changes

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2016

sttts force-pushed the sttts-sni branch from bfc1e74 to 4eeed17 Compare October 21, 2016 11:24

sttts force-pushed the sttts-sni branch from 4eeed17 to 85f4409 Compare October 21, 2016 11:36

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 21, 2016

sttts force-pushed the sttts-sni branch from 85f4409 to 69b677b Compare October 21, 2016 11:41

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

sttts force-pushed the sttts-sni branch from ce9830c to 4c7fa3b Compare October 31, 2016 16:02

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

liggitt approved these changes Oct 31, 2016

View reviewed changes

deads2k reviewed Oct 31, 2016

View reviewed changes

sttts force-pushed the sttts-sni branch from ed9447e to 2ceca5d Compare November 1, 2016 07:57

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

sttts added 3 commits November 1, 2016 09:50

Make GenericApiServer.Run interruptable and fail on first listen

d0b3981

Add --tls-sni-cert-key to the apiserver for SNI support

5dc31d3

Update bazel

7c5e354

sttts force-pushed the sttts-sni branch from 2ceca5d to 7c5e354 Compare November 1, 2016 08:51

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

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

k8s-github-robot merged commit 6babfb6 into kubernetes:master Nov 1, 2016

sttts mentioned this pull request Nov 2, 2016

Restore old apiserver cert CN #36068

Merged

chentao1596 mentioned this pull request Dec 5, 2016

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

Closed

aaronlevy mentioned this pull request Jan 9, 2017

pkg/asset/internal: Introduce a Flock around the API server kubernetes-retired/bootkube#264

Merged

sttts deleted the sttts-sni branch March 1, 2017 09:45

Add SNI support to the apiserver #35109

Add SNI support to the apiserver #35109

Conversation

sttts commented Oct 19, 2016 • edited Loading

sttts commented Oct 19, 2016

liggitt commented Oct 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 19, 2016

liggitt commented Oct 19, 2016

deads2k commented Oct 20, 2016

sttts commented Oct 20, 2016

sttts commented Oct 21, 2016

k8s-ci-robot commented Oct 28, 2016

k8s-ci-robot commented Oct 28, 2016

liggitt commented Oct 31, 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

deads2k commented Oct 31, 2016

k8s-ci-robot commented Nov 1, 2016

sttts commented Nov 1, 2016

k8s-github-robot commented Nov 1, 2016

k8s-github-robot commented Nov 1, 2016

sttts commented Oct 19, 2016 •

edited

Loading