kubeadm: remove duplicate DNS names and IP addresses from generated certificates #92753

QianChenglong · 2020-07-02T12:12:36Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix duplicate altnames in cert

Which issue(s) this PR fixes:

Fixes #92751

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Release note:

kubeadm: remove duplicate DNS names and IP addresses from generated certificates

k8s-ci-robot · 2020-07-02T12:12:44Z

Hi @QianChenglong. 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.

QianChenglong · 2020-07-02T12:22:06Z

/kind bug

SataQiu

Thanks @QianChenglong !
Add some comments.

SataQiu · 2020-07-02T16:02:33Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

@@ -427,8 +431,13 @@ func getAltNames(cfg *kubeadmapi.InitConfiguration, certName string) (*certutil.

 	// create AltNames with defaults DNSNames/IPs
 	altNames := &certutil.AltNames{
-		DNSNames: []string{cfg.NodeRegistration.Name, "localhost"},


"localhost" lost?

Thanks for pointing out the error, leave it as it is.

SataQiu · 2020-07-02T16:09:19Z

staging/src/k8s.io/client-go/util/cert/cert.go

+		return
+	}
+
+	dnsNamesKeys := make(map[string]struct{})


FYI: https://github.com/kubernetes/apimachinery/blob/master/pkg/util/sets
There are some utils you can consider to use.

sets.NewString(altNames.DNSNames...).List()

SataQiu · 2020-07-02T16:13:10Z

cmd/kubeadm/app/phases/certs/certlist.go

@@ -21,7 +21,6 @@ import (
 	"crypto/x509"

 	"github.com/pkg/errors"
-


Why delete the blank line? We prefer to use a blank line to distinguish different import blocks.

Sorry for bringing our programming style, I would like to ask what tool is used here to automatically control imports？

neolit123 · 2020-07-03T00:17:20Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

@@ -383,6 +381,12 @@ func GetAPIServerAltNames(cfg *kubeadmapi.InitConfiguration) (*certutil.AltNames
 			advertiseAddress,
 		},
 	}
+	// cfg.NodeRegistration.Name may use ip!


thank you for the PR,
i don't think i agree with this change and the similar one in getAltNames() . the comment that "node names can be IPs" is wrong.

the Node name is supposed to be a string "that can be used as a DNS subdomain name as defined in RFC 1123"
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

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

Line 107 in 47a9952

for _, err := range validation.IsDNS1123Subdomain(nro.Name) {

however, this function is not perfectly following RFC 1123 and it allows the strings to start with a number, which is wrong.

the node name should be a hostname, not an IP address and we should not parse it as an IP address.
if you want to add the same IP address in altNames you should do so explicitly using the extraSANs kubeadm feature.

/hold

Thanks for pointing out the error, leave it as it is.

neolit123 · 2020-07-03T00:17:57Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

@@ -427,8 +431,13 @@ func getAltNames(cfg *kubeadmapi.InitConfiguration, certName string) (*certutil.

 	// create AltNames with defaults DNSNames/IPs
 	altNames := &certutil.AltNames{
-		DNSNames: []string{cfg.NodeRegistration.Name, "localhost"},
-		IPs:      []net.IP{advertiseAddress, net.IPv4(127, 0, 0, 1), net.IPv6loopback},


losing the above two lines is a regression.

neolit123 · 2020-07-03T00:20:04Z

staging/src/k8s.io/client-go/util/cert/cert.go

@@ -53,6 +53,33 @@ type AltNames struct {
 	IPs      []net.IP
 }

+// CleanAltNames cleans duplicate items in DNSNames and IPs.
+func CleanAltNames(altNames *AltNames) {


in general i like the idea of cleaning up duplicate entries in the altNames.
but please name this function RemoveDuplicateAltNames.

also kubeadm supports wildcards such as 127.0.*.* do we care to remove 127.0.0.1 if 127.0.*.* is in the list already?
possibly not.

RemoveDuplicateAltNames is indeed more clear.
For the time being, only the repetition of the character form is removed, and logical repetition may be necessary under observation.

QianChenglong · 2020-07-03T13:41:30Z

/retest

neolit123 · 2020-07-03T13:52:27Z

this looks fine to me.
/retest

do we have comments by others? cc @kubernetes/sig-cluster-lifecycle-pr-reviews

QianChenglong · 2020-07-03T15:09:29Z

/retest

neolit123 · 2020-07-03T20:13:07Z

@QianChenglong please instead of NONE in the PR description add:

kubeadm: remove duplicate DNS names and IP addresses from generated certificates

QianChenglong · 2020-07-06T01:11:03Z

@QianChenglong please instead of NONE in the PR description add:

kubeadm: remove duplicate DNS names and IP addresses from generated certificates

ok

dixudx

/lgtm

neolit123 · 2020-07-06T15:18:26Z

/approve
/retest

k8s-ci-robot · 2020-07-06T15:19:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, QianChenglong

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~~ [neolit123]

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

fejta-bot · 2020-07-06T20:30:48Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

neolit123 · 2020-07-06T20:55:53Z

/retitle kubeadm: remove duplicate DNS names and IP addresses from generated certificates

QianChenglong · 2020-07-07T01:20:02Z

/retest

fejta-bot · 2020-07-07T04:33:48Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-07-08T06:27:47Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-07-08T12:45:48Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

neolit123 · 2020-07-08T20:37:11Z

/test all

fejta-bot · 2020-07-09T04:51:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-07-09T08:42:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-07-09T12:33:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from caesarxuchao and SataQiu July 2, 2020 12:13

QianChenglong force-pushed the fix/kubeadm-altnames branch from 63c22e6 to 9c138c4 Compare July 2, 2020 12:20

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 2, 2020

QianChenglong force-pushed the fix/kubeadm-altnames branch from 9c138c4 to a950038 Compare July 2, 2020 12:21

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 2, 2020

SataQiu reviewed Jul 2, 2020

View reviewed changes

neolit123 suggested changes Jul 3, 2020

View reviewed changes

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

QianChenglong force-pushed the fix/kubeadm-altnames branch from a950038 to f57a1f9 Compare July 3, 2020 01:37

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 3, 2020

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 Jul 6, 2020

dixudx approved these changes Jul 6, 2020

View reviewed changes

k8s-ci-robot assigned dixudx Jul 6, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2020

k8s-ci-robot changed the title ~~fix duplicate altnames in cert, close #92751~~ kubeadm: remove duplicate DNS names and IP addresses from generated certificates Jul 6, 2020

spiffxp added this to the v1.19 milestone Jul 9, 2020

k8s-ci-robot merged commit 82baa26 into kubernetes:master Jul 9, 2020

neolit123 mentioned this pull request Aug 26, 2020

Duplicated SANs in k8s-apiserver certificates kubernetes/kubeadm#2260

Closed

atsikham mentioned this pull request Aug 26, 2020

[BUG] Duplicated SANs for K8s apiserver certificate hitachienergy/epiphany#1587

Closed

10 tasks

przemyslavic mentioned this pull request Aug 26, 2020

[BUG] Kubectl client outside of HA/multi-master Epiphany cluster fails to connect to server with invalid certificate hitachienergy/epiphany#1520

Closed

atsikham mentioned this pull request Feb 2, 2022

Duplicated SANs are possible for kubeadm certs generate-csr kubernetes/kubeadm#2648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: remove duplicate DNS names and IP addresses from generated certificates #92753

kubeadm: remove duplicate DNS names and IP addresses from generated certificates #92753

QianChenglong commented Jul 2, 2020 •

edited

k8s-ci-robot commented Jul 2, 2020

QianChenglong commented Jul 2, 2020

SataQiu left a comment

SataQiu Jul 2, 2020

QianChenglong Jul 3, 2020

SataQiu Jul 2, 2020

QianChenglong Jul 3, 2020

SataQiu Jul 2, 2020

QianChenglong Jul 3, 2020

neolit123 Jul 3, 2020

QianChenglong Jul 3, 2020

neolit123 Jul 3, 2020

neolit123 Jul 3, 2020

QianChenglong Jul 3, 2020

QianChenglong commented Jul 3, 2020

neolit123 commented Jul 3, 2020

QianChenglong commented Jul 3, 2020

neolit123 commented Jul 3, 2020

QianChenglong commented Jul 6, 2020

dixudx left a comment

neolit123 commented Jul 6, 2020

k8s-ci-robot commented Jul 6, 2020

fejta-bot commented Jul 6, 2020

neolit123 commented Jul 6, 2020

QianChenglong commented Jul 7, 2020

fejta-bot commented Jul 7, 2020

fejta-bot commented Jul 8, 2020

fejta-bot commented Jul 8, 2020

neolit123 commented Jul 8, 2020

fejta-bot commented Jul 9, 2020

fejta-bot commented Jul 9, 2020

fejta-bot commented Jul 9, 2020

		@@ -21,7 +21,6 @@ import (
		"crypto/x509"

		"github.com/pkg/errors"

kubeadm: remove duplicate DNS names and IP addresses from generated certificates #92753

kubeadm: remove duplicate DNS names and IP addresses from generated certificates #92753

Conversation

QianChenglong commented Jul 2, 2020 • edited

k8s-ci-robot commented Jul 2, 2020

QianChenglong commented Jul 2, 2020

SataQiu 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

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

QianChenglong commented Jul 3, 2020

neolit123 commented Jul 3, 2020

QianChenglong commented Jul 3, 2020

neolit123 commented Jul 3, 2020

QianChenglong commented Jul 6, 2020

dixudx left a comment

Choose a reason for hiding this comment

neolit123 commented Jul 6, 2020

k8s-ci-robot commented Jul 6, 2020

fejta-bot commented Jul 6, 2020

neolit123 commented Jul 6, 2020

QianChenglong commented Jul 7, 2020

fejta-bot commented Jul 7, 2020

fejta-bot commented Jul 8, 2020

fejta-bot commented Jul 8, 2020

neolit123 commented Jul 8, 2020

fejta-bot commented Jul 9, 2020

fejta-bot commented Jul 9, 2020

fejta-bot commented Jul 9, 2020

QianChenglong commented Jul 2, 2020 •

edited