Skip to content

Add CSR common name to SANs if no other SANs are defined in CSR - #1172

Merged
dopey merged 6 commits into
masterfrom
max/common-name-name
May 9, 2024
Merged

Add CSR common name to SANs if no other SANs are defined in CSR#1172
dopey merged 6 commits into
masterfrom
max/common-name-name

Conversation

@dopey

@dopey dopey commented May 7, 2024

Copy link
Copy Markdown
Contributor

@dopey
dopey requested review from hslatman and maraino May 7, 2024 02:28
@github-actions github-actions Bot added the needs triage Waiting for discussion / prioritization by team label May 7, 2024
Comment thread command/certificate/sign.go Outdated
Comment on lines +449 to +460
if addCNSAN {
cnInSANs := false
for _, v := range sans {
if v == cr.Subject.CommonName {
cnInSANs = true
break
}
}
if !cnInSANs {
sans = append(sans, cr.Subject.CommonName)
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we want to do this only if there are no SANs.

Comment thread command/certificate/sign.go Outdated
Comment on lines +182 to +187
cli.BoolTFlag{
Name: "add-cn-san",
Usage: `Ensure that Common Name from CSR subject is added to Subject Alternative
Name (SAN) extension of the certificate. This flag is enabled by default. To
disable default behavior pass the flag as '--add-cn-san=false'.`,
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's do something like --omit-cn-san as @hslatman suggested.

@dopey
dopey requested a review from maraino May 7, 2024 17:55
hslatman
hslatman previously approved these changes May 7, 2024

@hslatman hslatman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice 馃槃

Comment thread command/certificate/sign.go Outdated
Co-authored-by: Mariano Cano <mariano.cano@gmail.com>
@dopey
dopey requested a review from maraino May 9, 2024 17:12
@dopey
dopey merged commit e5ab833 into master May 9, 2024
@dopey
dopey deleted the max/common-name-name branch May 9, 2024 18:43
@hslatman hslatman added this to the v0.26.2 milestone May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs triage Waiting for discussion / prioritization by team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: CN not added as SAN

3 participants