BUGFIX: CertificateRequest short names must be unique. by inteon · Pull Request #6354 · cert-manager/cert-manager

inteon · 2023-09-20T12:49:29Z

The new mechanism is explained here:

cert-manager/pkg/controller/certificates/requestmanager/requestmanager_controller.go

Lines 401 to 411 in 860df22

    
           // The CertificateRequest name is limited to 253 characters, assuming the nextRevision and hyphen 
        
           // can be represented using 20 characters, we can directly accept certificate names up to 233 
        
           // characters. Certificate names that are longer than this will be hashed to a shorter name. We want 
        
           // to make crafting two Certificates with the same truncated name as difficult as possible, so we 
        
           // use a cryptographic hash function to hash the full certificate name to 64 characters. 
        
           // Finally, for Certificates with a name longer than 233 characters, we build the CertificateRequest 
        
           // name as follows: <first-168-chars-of-certificate-name>-<64-char-hash>-<19-char-nextRevision> 
        
           crName, err := apiutil.ComputeSecureUniqueDeterministicNameFromData(crt.Name, 233) 
        
           if err != nil { 
        
           	return err 
        
           }

Kind

/kind bug

Release Note

BUGFIX: fix CertificateRequest name collision bug in StableCertificateRequestName feature.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

SgtCoDFish

I've left a couple of comments; what do you think?

pkg/api/util/names_test.go

pkg/api/util/names.go

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-09-21T07:01:01Z

/cherrypick release-1.13

jetstack-bot · 2023-09-21T07:01:03Z

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.13
/cherrypick release-1.12

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.

inteon · 2023-09-21T07:01:16Z

/cherrypick release-1.12

jetstack-bot · 2023-09-21T07:01:18Z

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.12

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.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

SgtCoDFish

Couple more comments!

pkg/api/util/names.go

pkg/api/util/names_test.go

SgtCoDFish · 2023-09-22T09:13:14Z

pkg/controller/certificates/requestmanager/requestmanager_controller.go

+			// We limit the GenerateName to 52 + 1 characters to stay within the 63 - 5 character limit that
+			// is used in Kubernetes when generating names.
+			// see https://github.com/kubernetes/apiserver/blob/696768606f546f71a1e90546613be37d1aa37f64/pkg/storage/names/generate.go


Love this comment!

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

SgtCoDFish

/lgtm
/approve
/hold

This is an improvement and it fixes the issue as far as I can tell! I think it'd be helpful for another maintainer to take a look at this since it's essentially setting the format for all CertificateRequests going forwards. Maybe someone else will have thoughts on it. But I'd be happy to merge I think!

jetstack-bot · 2023-09-22T13:39:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [SgtCoDFish]

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

pkg/controller/certificates/requestmanager/requestmanager_controller.go

pkg/api/util/names_test.go

pkg/api/util/names.go

pkg/controller/certificates/requestmanager/requestmanager_controller.go

pkg/api/util/names.go

maelvls · 2023-09-25T13:47:45Z

I am happy with the new format.

Maybe bumping the name limit from a DNS label (63 chars) to a DNS subdomain (253 chars) would have solved 99% of the problems, including the original author of the issue. It seems like the hashing feature addresses a small corner case: folks with very very very large certificate names (longer than 232 chars) would still get the issue reported by the original author. But it's good that we solved this corner case anyways.

Thanks for working on this!

I'll hold it if you need to fix some of the non-blocking suggestions.

/lgtm
/hold

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-09-25T14:23:52Z

/unhold

SgtCoDFish · 2023-09-25T14:27:04Z

/lgtm

(quick lgtm as requested given this has been reviewed)

jetstack-bot · 2023-09-25T14:28:49Z

@inteon: new pull request created: #6358

Details

In response to this:

/cherrypick release-1.13

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.

jetstack-bot · 2023-09-25T14:29:27Z

@inteon: new pull request created: #6359

Details

In response to this:

/cherrypick release-1.12

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.

BUGFIX: CertificateRequest short names must be unique.

fa2d933

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the fix_name_collision_bug branch from e63fc97 to fa2d933 Compare September 20, 2023 12:51

SgtCoDFish reviewed Sep 20, 2023

View reviewed changes

pkg/api/util/names_test.go Show resolved Hide resolved

pkg/api/util/names.go Outdated Show resolved Hide resolved

add uniqueness check for names util

6006182

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

fix feedback: make hash secure

860df22

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the fix_name_collision_bug branch from 0f35a61 to 860df22 Compare September 21, 2023 11:24

SgtCoDFish reviewed Sep 22, 2023

View reviewed changes

add more test cases and fix typo

eac230f

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon mentioned this pull request Sep 22, 2023

Revisit the util Name generation logic #6352

Closed

inteon requested a review from SgtCoDFish September 22, 2023 12:00

SgtCoDFish approved these changes Sep 22, 2023

View reviewed changes

jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 22, 2023

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2023

maelvls self-requested a review September 25, 2023 11:45

maelvls reviewed Sep 25, 2023

View reviewed changes

pkg/controller/certificates/requestmanager/requestmanager_controller.go Show resolved Hide resolved

pkg/api/util/names_test.go Show resolved Hide resolved

maelvls reviewed Sep 25, 2023

View reviewed changes

pkg/api/util/names.go Show resolved Hide resolved

maelvls reviewed Sep 25, 2023

View reviewed changes

pkg/controller/certificates/requestmanager/requestmanager_controller.go Show resolved Hide resolved

maelvls reviewed Sep 25, 2023

View reviewed changes

pkg/api/util/names.go Show resolved Hide resolved

add extra comment

2dc22bc

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2023

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2023

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2023

SgtCoDFish merged commit 1c417be into cert-manager:master Sep 25, 2023

This was referenced Sep 25, 2023

[release-1.13] BUGFIX: CertificateRequest short names must be unique. #6358

Merged

[release-1.12] BUGFIX: CertificateRequest short names must be unique. #6359

Merged

inteon added kind/bug Categorizes issue or PR as related to a bug. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 25, 2023

	// The CertificateRequest name is limited to 253 characters, assuming the nextRevision and hyphen
	// can be represented using 20 characters, we can directly accept certificate names up to 233
	// characters. Certificate names that are longer than this will be hashed to a shorter name. We want
	// to make crafting two Certificates with the same truncated name as difficult as possible, so we
	// use a cryptographic hash function to hash the full certificate name to 64 characters.
	// Finally, for Certificates with a name longer than 233 characters, we build the CertificateRequest
	// name as follows: <first-168-chars-of-certificate-name>-<64-char-hash>-<19-char-nextRevision>
	crName, err := apiutil.ComputeSecureUniqueDeterministicNameFromData(crt.Name, 233)
	if err != nil {
	return err
	}

Conversation

inteon commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Kind

Release Note

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inteon commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetstack-bot commented Sep 21, 2023

Uh oh!

inteon commented Sep 21, 2023

Uh oh!

jetstack-bot commented Sep 21, 2023

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SgtCoDFish Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Sep 22, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maelvls commented Sep 25, 2023

Uh oh!

inteon commented Sep 25, 2023

Uh oh!

SgtCoDFish commented Sep 25, 2023

Uh oh!

jetstack-bot commented Sep 25, 2023

Uh oh!

jetstack-bot commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

inteon commented Sep 20, 2023 •

edited

Loading

inteon commented Sep 21, 2023 •

edited

Loading