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

Ensure defaultGeneratedCert.main as Subject's CN #10581

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

Lamatte
Copy link
Contributor

@Lamatte Lamatte commented Apr 9, 2024

What does this PR do?

This PR aims at fixing the issue #10580, by removing the sort of domain names that makes it impossible to specify the Common Name of an Acme certificate.

I can't see why a sort would be necessary here, and I don't think removing it may impact something else.

Motivation

I'm willing to use Acme, and I have a strong requirement with the CN used within the certificate.
I cannot depend on alphabetical sort to make sure which domain will be the "main" one.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

N/A

@traefiker traefiker added this to the 2.11 milestone Apr 9, 2024
@Lamatte Lamatte force-pushed the defaultGeneratedCertCommonName branch 2 times, most recently from 32850ed to 954d995 Compare April 9, 2024 12:07
@Lamatte Lamatte changed the title fix(acme): honor defaultGenaratedCert CN fix(acme): honor defaultGeneratedCert CN Apr 9, 2024
@Lamatte Lamatte force-pushed the defaultGeneratedCertCommonName branch 2 times, most recently from 7b18c92 to 82231ef Compare April 10, 2024 14:56
@Lamatte Lamatte changed the title fix(acme): honor defaultGeneratedCert CN fix(acme): ensure defaultGeneratedCert.main as Subject's CN Apr 11, 2024
@nmengin
Copy link
Contributor

nmengin commented Sep 16, 2024

Hello @Lamatte,

Thank you for your contribution, and sorry for the delay.
I move the Pr to the status need-review to allow us to start reviewing the PR.

@nmengin
Copy link
Contributor

nmengin commented Sep 16, 2024

I'm closing and reopening the PR to launch the CI.

@nmengin nmengin closed this Sep 16, 2024
@nmengin nmengin reopened this Sep 16, 2024
@Lamatte Lamatte force-pushed the defaultGeneratedCertCommonName branch from bd40bd8 to 77490f4 Compare September 18, 2024 09:22
@kevinpollet kevinpollet changed the title fix(acme): ensure defaultGeneratedCert.main as Subject's CN Ensure defaultGeneratedCert.main as Subject's CN Sep 18, 2024
@Lamatte Lamatte force-pushed the defaultGeneratedCertCommonName branch 4 times, most recently from 1f9d09e to 174839c Compare September 24, 2024 13:18
@rtribotte rtribotte added the kind/bug/fix a bug fix label Sep 26, 2024
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks @Lamatte for the latest changes.
I think we don't care using the sortedDomains in place of domains after the domainKey has been generated, and it is preferable to delimitate the usage scope of the sortedDomains to the key generation.

pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
pkg/provider/acme/provider.go Outdated Show resolved Hide resolved
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 4d6cb6a into traefik:v2.11 Sep 30, 2024
39 checks passed
@nmengin
Copy link
Contributor

nmengin commented Oct 8, 2024

@Lamatte

Thank you for your contribution.
We have released Traefik v2.11.9 and Traefik v3.1.5 with your fix 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants