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

Fix wildcard match to ACME domains in cluster mode #3080

Merged
merged 3 commits into from
Mar 27, 2018

Conversation

oldmantaiter
Copy link
Contributor

What does this PR do?

  • Allows matching of hosts like x.traefik.wtf to ACME wildcard
    certs *.traefik.wtf

Motivation

  • Tested wildcard acme v2 and noticed that it was not properly selecting my wildcard domains

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

@emilevauge
Copy link
Member

emilevauge commented Mar 26, 2018

@oldmantaiter could you explain more in details why you added those lines in getCertificateForDomain() ?

if strings.HasPrefix(domain, "*.") {
	selector := "^" + strings.Replace(domain, "*.", "[^\\.]*\\.", -1) + "$"
	if domainCheck, _ := regexp.MatchString(selector, domainToFind); domainCheck {
		return domainsCertificate, true
	}
}

@ldez ldez added status/1-needs-design-review kind/enhancement a new or improved feature. kind/bug/fix a bug fix and removed status/0-needs-triage kind/enhancement a new or improved feature. labels Mar 26, 2018
@oldmantaiter
Copy link
Contributor Author

oldmantaiter commented Mar 26, 2018

@emilevauge for sure.

I was testing the new ACMEv2 merge and created a certificate for (as an example) *.foo.bar and have backends that route based on Host:x.foo.bar or Host:y.foo.bar. These were not properly getting the stored ACME wildcard cert because this function only looked for an exact string match

@dtomcej
Copy link
Contributor

dtomcej commented Mar 26, 2018

@emilevauge This code snippet has been repeated in the acme package 3x already, and in server.go, do you think that we should refactor this to be a more unified function, to check for matches better? rather than re-duplicating code?

@oldmantaiter
Copy link
Contributor Author

@dtomcej I noticed the same, I can combine if that is easier

@nmengin
Copy link
Contributor

nmengin commented Mar 27, 2018

I am agree with @dtomcej .

I guess we can transform the list of DomainCertificate into a map[string]*tls.Certificate instead of a []string in the loop instruction (by using the metod Domain.ToStrArray as keys and the DomainCertificate.Certificate as values).

Then, we can use acme.searchProvidedCertificateForDomains method instead of copy/paste the regexp matching.

@oldmantaiter Do you think you can do it? Or do you need some help?

@nmengin
Copy link
Contributor

nmengin commented Mar 27, 2018

@oldmantaiter

Many thanks for your PR.
Because this fix is very important, I did the pattern matching refacto in the way to generate quickly a new Release Candidate.

@oldmantaiter
Copy link
Contributor Author

@nmengin Thanks. There is one more place I saw this pattern matching code (provider/acme/provider.go) so I was going to put the function inside provider/acme and reference it from the acme package.

@nmengin
Copy link
Contributor

nmengin commented Mar 27, 2018

@oldmantaiter I saw it too and I am currently analyzing a solution to integrate all of them.

I think it will be in an other PR.

Many thanks for your feedback 👍

@nmengin nmengin force-pushed the acme-match-wildcard branch 2 times, most recently from d2c95e7 to 91ed6f9 Compare March 27, 2018 13:04
oldmantaiter and others added 2 commits March 27, 2018 15:42
- Allows matching of hosts like `x.traefik.wtf` to ACME wildcard
  certs `*.traefik.wtf`
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM 👏

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

Copy link
Member

@juliens juliens 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 f1a05ab into traefik:v1.6 Mar 27, 2018
@nmengin nmengin changed the title Add wildcard match to acme domains Fix wildcard match to ACME domains in cluster mode Mar 27, 2018
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.

8 participants