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

MOD: support wildcard DNS for apiserver certSANs #76920

Merged
merged 2 commits into from Apr 29, 2019

Conversation

@sempr
Copy link
Contributor

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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.


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

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

/ok-to-test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

/assign @timothysc

@neolit123
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

/ok-to-test

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

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

@sempr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

left a comment

@sempr
please address the minor comment bellow.

looks like we are leaning towards +1 here.

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, "; "))))

This comment has been minimized.

Copy link
@neolit123

neolit123 Apr 25, 2019

Member

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....

This comment has been minimized.

Copy link
@sempr

sempr Apr 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/priority important-longterm

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@sempr

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

@timothysc timothysc removed the request for review from liztio Apr 29, 2019

@timothysc
Copy link
Member

left a comment

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

[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:

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

@neolit123
Copy link
Member

left a comment

/lgtm

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

/retest

1 similar comment
@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

/retest

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

20 checks passed

cla/linuxfoundation sempr authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.