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.

Sign up for GitHub

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

Jump to bottom

update serial number to a valid non-zero number in ca certificate #117791

Merged
merged 2 commits into from
May 9, 2023
Merged

update serial number to a valid non-zero number in ca certificate #117791

merged 2 commits into from
May 9, 2023

Conversation

nnmin-aws
Copy link
Contributor

@nnmin-aws nnmin-aws commented May 4, 2023 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

EKS customer complains all cluster CA certificates created when an EKS cluster is created have an empty Serial number.

Which issue(s) this PR fixes:

fixes #117790

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 4, 2023
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 4, 2023
@nnmin-aws
Copy link
Contributor Author

/retest

@nckturner
Copy link
Contributor

/cc @liggitt @mikedanese

@k8s-ci-robot k8s-ci-robot requested a review from liggitt May 5, 2023 00:29
@liggitt
Copy link
Member

liggitt commented May 5, 2023

/lgtm
/approve
/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 5, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6f729f348175d20a88bad55f7701ac72c15e50da

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2023
liggitt
liggitt reviewed May 5, 2023
View reviewed changes
staging/src/k8s.io/client-go/util/cert/cert.go Outdated
@@ -57,8 +58,12 @@ type AltNames struct {
// NewSelfSignedCACert creates a CA certificate
func NewSelfSignedCACert(cfg Config, key crypto.Signer) (*x509.Certificate, error) {
now := time.Now()
serial, err := cryptorand.Int(cryptorand.Reader, new(big.Int).SetInt64(math.MaxInt64))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, can't this still return zero?

Int returns a uniform random value in [0, max). It panics if max <= 0.

/hold

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fun!

@liggitt this snippet was copied from elsewhere in our repo :)

https://cs.k8s.io/?q=serial%2C%20err%20%3A%3D.*rand&i=nope&files=&excludeFiles=&repos=kubernetes/kubernetes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could fix the existing problem as part of this PR in a separate commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could someone help take a look at the update? if it is good, I will fix the existing problem in a separate commit with similar code.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2023
@neolit123
Copy link
Member

please change this in the PR description

#117790

to

fixes #117790

so that the related issue is closed.

@neolit123 neolit123 mentioned this pull request May 5, 2023
Closed
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023
@k8s-ci-robot k8s-ci-robot requested a review from liggitt May 8, 2023 19:53
@nnmin-aws nnmin-aws requested a review from dims May 9, 2023 03:53
neolit123
neolit123 reviewed May 9, 2023
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 35b2fa6560837e8ba82b28b940e448dbc515982d

@dims
Copy link
Member

dims commented May 9, 2023

/release-note-none
/hold cancel

@liggitt 's concern in #117791 (comment) has been fixed. (for all locations the same kind of snippet is used!)

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 9, 2023
@liggitt liggitt added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 9, 2023
@liggitt
Copy link
Member

liggitt commented May 9, 2023

lgtm

@k8s-ci-robot k8s-ci-robot merged commit e865b30 into kubernetes:master May 9, 2023
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone May 9, 2023
@cici37
Copy link
Contributor

cici37 commented May 9, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 9, 2023
@neolit123 neolit123 mentioned this pull request May 10, 2023
Closed
@bertinatto bertinatto mentioned this pull request Jun 20, 2023
Closed
This was referenced Jun 29, 2023
Merged
Merged
Merged
Merged
@champtar
Copy link
Contributor

Created the cherry pick PRs for this PR as I get a git conflict while trying to backport #118922

@cpanato
Copy link
Member

cpanato commented Jul 1, 2023

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 1, 2023
k8s-ci-robot added a commit that referenced this pull request Jul 1, 2023
@k8s-ci-robot
Merge pull request #118972 from champtar/automated-cherry-pick-of-#11…
3a755bf 
…7791-upstream-release-1.25

Automated cherry pick of #117791: update serial number to a valid non-zero number in ca
k8s-ci-robot added a commit that referenced this pull request Jul 5, 2023
@k8s-ci-robot
Merge pull request #118969 from champtar/automated-cherry-pick-of-#11…
db247e1 
…7791-upstream-release-1.27

Automated cherry pick of #117791: update serial number to a valid non-zero number in ca
k8s-ci-robot added a commit that referenced this pull request Jul 5, 2023
@k8s-ci-robot
Merge pull request #118970 from champtar/automated-cherry-pick-of-#11…
76fa65b 
…7791-upstream-release-1.26

Automated cherry pick of #117791: update serial number to a valid non-zero number in ca
rayowang pushed a commit to rayowang/kubernetes that referenced this pull request Feb 9, 2024
@nnmin-aws
update serial number to a valid non-zero number in ca certificate (ku…
62e810c 
…bernetes#117791)

* update serial number to a valid non-zero number in ca certificate

* fix the existing problem (0 SerialNumber in all certificate) as part of this PR in a separate commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@mikedanese mikedanese Awaiting requested review from mikedanese

@thockin thockin Awaiting requested review from thockin

@liggitt liggitt Awaiting requested review from liggitt

@dims dims Awaiting requested review from dims

Assignees

@neolit123 neolit123

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

all cluster CA certificates created when a cluster is created have an empty Serial number
9 participants
@nnmin-aws @nckturner @liggitt @k8s-ci-robot @neolit123 @dims @cici37 @champtar @cpanato