MOD: support wildcard DNS for apiserver certSANs #76920

sempr · 2019-04-23T02:32:06Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

It support wildcard certSANs for apiServer, the same as the normal https certificates.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE

kubeadm: support sub-domain wildcards in certificate SANs

k8s-ci-robot · 2019-04-23T02:32:09Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2019-04-23T02:32:13Z

Hi @sempr. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-04-23T08:23:40Z

@sempr: you cannot LGTM your own PR.

In response to this:

/lgtm

test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sempr · 2019-04-23T08:24:51Z

/ok-to-test

k8s-ci-robot · 2019-04-23T08:25:05Z

@sempr: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sempr · 2019-04-23T11:20:39Z

/assign @timothysc

neolit123

hi, i don't think we want to support wildcards.
the security model of kubeadm is explicitly designed to allow you to only specify a hardcoded list of addresses and domains for a component such as the api server.

see here:
https://godoc.org/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1

apiServer:
...
  certSANs:
  - "10.100.1.1"
  - "ec2-10-100-0-1.compute-1.amazonaws.com"

@kubernetes/sig-cluster-lifecycle-pr-reviews
/hold

timothysc · 2019-04-23T15:12:07Z

/ok-to-test

timothysc · 2019-04-23T15:12:52Z

@sempr - Can you outline a user story for this? What's the use case?

sempr · 2019-04-24T02:07:30Z

@sempr - Can you outline a user story for this? What's the use case?

@timothysc as is known, a https certs can use wildcard dns names, as an http(2) server, kube-apiserver should support it. let's encrypt also supported wildcard domain names on version 2.

A reasonable user case is like this, i have 3 kubernetes masters, m1.masters.k8s.lan, m2.masters.k8s.lan m3.masters.k8s.lan, if one day later i want to add other 2 masters, for example, m4.masters.k8s.lan m5.masters.k8s.lan, i can easily add the wildcard names *.masters.k8s.lan when the cluster setup, without change anything.

timothysc · 2019-04-24T16:28:55Z

@liggitt ^ could I get a hot take on this request from a security perspective?

liggitt · 2019-04-24T20:00:56Z

Is a single certificate/key valid for all the specified SANs already being handed out to all the apiservers? if so, this is not worse, for the set of servers specified.

It does have a couple implications users would have to be aware of:

a lost/stolen cert means you potentially have to burn an ensure DNS suffix, instead of just the specific N servers named
the servers would be given a cert valid for any other non-k8s/etcd servers under the same DNS suffix and would be able to be serve TLS traffic for those

detiber · 2019-04-24T20:50:19Z

@liggitt currently only SANs for the local system are baked into the certificate (unless explicitly specified by the user though the kubeadm config).

imo, since this is explicitly only enabling users to pass in a wildcard as an additional SAN, while it may not be recommended in most cases, I don't see any reason why we should completely disallow it.

neolit123

@sempr
please address the minor comment bellow.

looks like we are leaning towards +1 here.

neolit123 · 2019-04-25T16:07:52Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

-				allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label: %s", strings.Join(errs, "; "))))
+			if errs2 := validation.IsWildcardDNS1123Subdomain(altname); len(errs2) != 0 {
+				if net.ParseIP(altname) == nil {
+					allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label or Wildcard DNS label: %s; %s", strings.Join(errs, "; "), strings.Join(errs2, "; "))))


please change:
...a valid IP address or DNS label or Wildcard DNS label....
to
...not a valid IP address, DNS label or a DNS label with subdomain wildcards....

done

Suggested change

allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label or Wildcard DNS label: %s; %s", strings.Join(errs, "; "), strings.Join(errs2, "; "))))

allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label or Wildcard DNS label: %s; %s", strings.Join(errs, "; "), strings.Join(errs2, "; "))))

neolit123 · 2019-04-25T16:09:02Z

/priority important-longterm

neolit123 · 2019-04-28T19:21:18Z

@sempr

please add a release note instead of NONE:
kubeadm: support sub-domain wildcards in certificate SANs
thanks

timothysc

/approve

k8s-ci-robot · 2019-04-29T16:09:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sempr, timothysc

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:

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

neolit123

/lgtm

neolit123 · 2019-04-29T18:05:16Z

/retest

neolit123 · 2019-04-29T19:16:18Z

/retest

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 23, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 23, 2019

k8s-ci-robot requested review from liztio and timothysc April 23, 2019 02:32

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 23, 2019

MOD: support Wildcard DNS for apiserver certSANs

0c7d872

sempr force-pushed the master branch from a0631c8 to 0c7d872 Compare April 23, 2019 02:48

k8s-ci-robot assigned timothysc Apr 23, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2019

neolit123 reviewed Apr 23, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 23, 2019

neolit123 reviewed Apr 25, 2019

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 25, 2019

MOD: minor comment update for error message

81e896e

timothysc removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2019

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 29, 2019

timothysc removed the request for review from liztio April 29, 2019 16:08

timothysc approved these changes Apr 29, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2019

neolit123 reviewed Apr 29, 2019

View reviewed changes

k8s-ci-robot assigned neolit123 Apr 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2019

k8s-ci-robot merged commit 35b2784 into kubernetes:master Apr 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MOD: support wildcard DNS for apiserver certSANs #76920

MOD: support wildcard DNS for apiserver certSANs #76920

sempr commented Apr 23, 2019 •

edited

k8s-ci-robot commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

sempr commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

sempr commented Apr 23, 2019

neolit123 left a comment

timothysc commented Apr 23, 2019

timothysc commented Apr 23, 2019

sempr commented Apr 24, 2019

timothysc commented Apr 24, 2019

liggitt commented Apr 24, 2019

detiber commented Apr 24, 2019

neolit123 left a comment

neolit123 Apr 25, 2019

sempr Apr 26, 2019

neolit123 commented Apr 25, 2019

neolit123 commented Apr 28, 2019

timothysc left a comment

k8s-ci-robot commented Apr 29, 2019

neolit123 left a comment

neolit123 commented Apr 29, 2019

neolit123 commented Apr 29, 2019

	allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label or Wildcard DNS label: %s; %s", strings.Join(errs, "; "), strings.Join(errs2, "; "))))
	allErrs = append(allErrs, field.Invalid(fldPath, altname, fmt.Sprintf("altname is not a valid IP address or DNS label or Wildcard DNS label: %s; %s", strings.Join(errs, "; "), strings.Join(errs2, "; "))))

MOD: support wildcard DNS for apiserver certSANs #76920

MOD: support wildcard DNS for apiserver certSANs #76920

Conversation

sempr commented Apr 23, 2019 • edited

k8s-ci-robot commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

sempr commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019

sempr commented Apr 23, 2019

neolit123 left a comment

Choose a reason for hiding this comment

timothysc commented Apr 23, 2019

timothysc commented Apr 23, 2019

sempr commented Apr 24, 2019

timothysc commented Apr 24, 2019

liggitt commented Apr 24, 2019

detiber commented Apr 24, 2019

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Apr 25, 2019

Choose a reason for hiding this comment

sempr Apr 26, 2019

Choose a reason for hiding this comment

neolit123 commented Apr 25, 2019

neolit123 commented Apr 28, 2019

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 29, 2019

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Apr 29, 2019

neolit123 commented Apr 29, 2019

sempr commented Apr 23, 2019 •

edited