Added IgnoreFilter for ACME domain cert generation #1260

CyrilPeponnet · 2017-03-08T19:04:10Z

This Pull Request fixes the following:

Avoid to generate ACME certs if a wildcard cert is present
Always return the ACME cert if present and fallback to wildcard if present

And add the a new feature ignoreFilters to filter out domains for ACME certificate generation.

The static declaration of domains in KV/TOML will override the filters.

Test cases:

case 1: I have wildcard certificate for domain.tld and I want acme to generate acme certs for only a subset of it.
Use the ignoreFilters with a regular expression like .*\.?[^f][^o][^o]\.domain\.tld (negative lookahead are not supported by golang), to generate acme cert for domains except those like *.foo.domain.tld.

case 2: I have wildcard for domain.tld but I want acme to take precedence over it for statically defined domains.
Ignore the entire domain with a filter like .*\.domain\.tld, and define your domains in ACME configuration (don't forget to restart traefik as this part is not reloaded dynamicaly).

case 3: I have wildcard certificate for domain.tld and I want it to be used instead of already defined acme certs.
You will need to revoke and clean the acme cert from your configuration (if from KV, you will need to base64decode it, strip the cert part you want to remove, reencode, update it and restart traefik).

Let me know if I forgot something.

Fixes #1197

dtomcej · 2017-03-08T20:09:17Z

acme/acme.go

 	for k := range a.TLSConfig.NameToCertificate {
 		selector := "^" + strings.Replace(k, "*.", "[^\\.]*\\.?", -1) + "$"
 		match, _ := regexp.MatchString(selector, domain)
 		if match {
-			return a.TLSConfig.NameToCertificate[k], nil
+			log.Debugf("Found a wildcard certificate to use as fallback: %v", k)
+			wildcardCertificate = a.TLSConfig.NameToCertificate[k]


If this is the only time wildcardCertificate is used, why not just return it directly? Why store it, and check if it is empty later?

Seems like more code execution unnecessarily.

You are right. Will move this part.

dtomcej · 2017-03-08T20:13:07Z

acme/acme.go

 			return
 		}
+
+		// Check if our domains are matching our ingoreFilters
+		if len(a.IgnoreFilters) > 0 {


is loadCertificateOnDemand the correct location for this code?

This means that the certificate will be requested from acme, but not returned correct?

Wouldn't this be better served in getCertificate before a potential cert request?

In case we are in a 'onDemand' for now there is no check with ignoreFilters, this can be added if needed.

@dtomcej please advise.

Should be good.

w3blogfr · 2017-03-23T22:21:49Z

This feature will be really usefull.

emilevauge

@CyrilPeponnet Few comments but, overall, looks good to me :)
Do you think you could add some tests on this?

emilevauge · 2017-04-06T09:14:02Z

acme/acme.go

-	}
+
+	// Check for existing challenge cert
+	log.Debugf("Checking ACME challenge for %v", domain)


I would change this debug log to Getting ACME certificate for %s

emilevauge · 2017-04-06T09:55:41Z

acme/acme.go

 			return
 		}
+
+		// Check if our domains are matching our ingoreFilters
+		if len(a.IgnoreFilters) > 0 {


Is it possible to put all the IgnoreFilters code into a function? It seems to look like https://github.com/containous/traefik/pull/1260/files#diff-faa8f9dd50dafacfcb04a8203ce3deebR542 :)

vdemeester

SGTM 👼
/cc @containous/traefik for reviews

dtomcej · 2017-04-17T15:09:25Z

acme/acme.go

+
+	// Check if we have a wildcard cert into TLSConfig that could be used
+	log.Debugf("Checking wildcard certificate matching for %v", domain)
+	for k := range a.TLSConfig.NameToCertificate {


By having this block here, it will never get executed on-demand, since the on demand domain will have already been requested from LE.

I think that this should be modified to check IF match AND not blacklisted.

Well it will be executed if the ondemand failed to get a cert (due to a filter for instance)

@dtomcej ping...

@CyrilPeponnet, the inversion of certificates checkings you want do has a real interest for the use cases described into the PR but it seems to break the backward compatibily.

If it becomes the default behavior, all users will have to specify a IgnoreFilters value.
That's why it may be good to invers the behaviour ONLY IF IgnoreFilters is specified, so users who will not fill the field will keep the current behavior.

In the way to do this, the wildcard certificate checking could be done in a new function, called at the beginning or the end of getCertificate in function of the size of the IgnoreFilters list.

The code should look like this :

func (a *ACME) getWildCardCertificate(domain string) (*tls.Certificate, nool) { // Check if we have a wildcard cert into TLSConfig that could be used log.Debugf("Checking wildcard certificate matching for %v", domain) for k := range a.TLSConfig.NameToCertificate { selector := "^" + strings.Replace(k, "*.", "[^\\.]*\\.?", -1) + "$" match, _ := regexp.MatchString(selector, domain) if match { log.Debugf("Found a wildcard certificate to use as fallback: %v", k) return a.TLSConfig.NameToCertificate[k], true } } return nil, false } func (a *ACME) getCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) { domain := types.CanonicalDomain(clientHello.ServerName) account := a.store.Get().(*Account) if a.IgnoreFilters == nil || a.IgnoreFilters.len == 0 { if wildcardCert, ok := a.getWildCardCertificate(domain); ok { return wildcardCert, nil } } // Check for existing challenge cert log.Debugf("Checking ACME challenge for %s", domain) if challengeCert, ok := a.challengeProvider.getCertificate(domain); ok { log.Debugf("Returning ACME challenge cert for %s", domain) return challengeCert, nil } // Check for existing ACME domain cert log.Debugf("Checking ACME domain cert for %v", domain) if domainCert, ok := account.DomainsCertificate.getCertificateForDomain(domain); ok { log.Debugf("Returning ACME domain cert for %s", domain) return domainCert.tlsCert, nil } if a.OnDemand { if a.checkOnDemandDomain != nil && !a.checkOnDemandDomain(domain) { return nil, nil } cert, err := a.loadCertificateOnDemand(clientHello) if cert != nil && err == nil { return cert, nil } else if err != nil { log.Errorf("On demand certificate retrieval failed for %v due to %v", domain, err) } } if a.IgnoreFilters != nil && a.IgnoreFilters.len > 0 { if wildcardCert, ok := a.getWildCardCertificate(domain); ok { return wildcardCert, nil } } log.Debugf("No certificate found to return for %s", domain) return nil, nil }

What is your mind about this suggestion?

cc @dtomcej

I'm not sure about the use case... Sorry it as been a while... Can you please explain?

@CyrilPeponnet, any thoughts on this ?

Sorry I've been busy this week. Will try to get a better look later on the day.

Well the issue I tried to address with this PR was also the fact that if I have both a self signed wildcard cert and an ACME cert, only the wildcard will be returned (case 2 and 3 in PR description).

The purpose of the IgnoreFilters was to filter out some (sub)domains for LE certificate generation. (case 1 in PR description).

So to sum up I'm trying to address:

1 - if LE and wildcard are present, return LE
2 - the IgnoreFilters can be used to filter the LE cert generation

I don't think the part 2 is changing any behavior. Part 1 is.

Now I can see 2 uses cases for Part 1.

Current behavior, always return wildcard if any

if I have a signed wildcard, it makes sense to use it instead of LE cert (but the LE cert will still exists, hidden, and will be renewed and not used).

New behavior, return LE over wildcard,

make sense if I use a self signed wildcard cert.

I'm not sure on how to address those two cases as they can both exists in the same traefik instance.

Many thanks for you reply.

As you just said, the Part1 of your response changes deeply the current behavior if the IgnoreFilter field is used to filter domains checked by LE.

But I believe the suggestion we did (use IgnoreFilter to filter (sub)domains checked by a wildcard certificate) allows to respect the 2 use cases you described :

I have a signed wildcard and I want to use it instead of LE certificate : I don't use IgnoreFilter field, the behavior will remain the same as now.

I have a self-signed wildcard and I prefer to use LE for any (all?) (sub)domains : I add the list of (sub)domains which haven't to be checked by the wildcard certificate into the IgnoreFilter field and the LE certificate will be used.

@CyrilPeponnet Are you agree with this?

cc/ @dtomcej @emilevauge @juliens @ldez

I'm wondering if by doing this it will conflict with the IgnoreFilter used to allow or not LE certificate requests.

Example:

I have *.domain.tld which is self signed. I want fourme.domain.tld to use LE.

If I set the IgnoreFilter to fourme.domain.tld, given your statement:

it will not use the self-signed cert.

it will not request a LE as per LoadCertificateForDomains func.

The only way to make it works is to define the domain inside the toml configuration (which is not filtered AFAIK).

The main usage for IgnoreFilter was meant to filter out domains for which I don't want any LE generation.

Am I missing something?

emilevauge

LGTM
Thanks @CyrilPeponnet
Could rebase one more time before merge?

emilevauge · 2017-04-20T13:39:59Z

ping @containous/traefik

otherwise fallback to wildcard if any.

emilevauge · 2017-06-06T15:27:44Z

@nmengin @juliens @ldez

juliens · 2017-06-07T07:47:22Z

acme/acme.go

 			return
 		}
+
+		// Check if our domains are matching our ingoreFilters


ingoreFilters => ignoreFilters

juliens · 2017-06-07T07:48:03Z

acme/acme.go

@@ -496,11 +538,18 @@ func (a *ACME) loadCertificateOnDemand(clientHello *tls.ClientHelloInfo) (*tls.C
 	if certificateResource, ok := account.DomainsCertificate.getCertificateForDomain(domain); ok {
 		return certificateResource.tlsCert, nil
 	}
+
+	// Check if our domain is matching our ingoreFilters


ingoreFilters => ignoreFilters

juliens · 2017-06-07T08:03:05Z

traefik.sample.toml

+# Optional
+#
+# Example to ignore certificate generation for domains that are not like *.foo.domain.tld
+# IngnoreFilters = ['.*\.?[^foo]\.domain\.tld']


IngnoreFilters => IgnoreFilters

Done, thanks for spotting those.

ldez · 2017-06-07T17:14:28Z

Due to SemaphoreCI, I close and reopen the PR.

nmengin · 2017-06-07T20:43:27Z

acme/acme.go

+		if cert != nil && err == nil {
+			return cert, nil
+		}
+		log.Errorf("On demand certicate retrival failed for %v due to %v", domain, err)


certicate retrival => certificate retrieval

…eponnet/traefik into acme_fix_and_improvements

juliens

LGTM

nmengin · 2017-06-09T17:46:50Z

acme/acme.go

+		if cert != nil && err == nil {
+			return cert, nil
+		}
+		log.Errorf("On demand certificate retrieval failed for %v due to %v", domain, err)


An error log is created even if err == nil, it should be nice to add a test before to insert the log.

You are right, but, cert can be nil if not cert found without raising an error. Will try to improve the logging.

nmengin · 2017-07-12T13:01:44Z

ping @CyrilPeponnet

nmengin · 2017-07-12T16:21:53Z

As discussed with @CyrilPeponnet on Slack, the PR is closed for the moment.
A new PR will be open if necessary.

emilevauge added status/1-needs-design-review status/2-needs-review labels Mar 8, 2017

dtomcej suggested changes Mar 8, 2017

View reviewed changes

CyrilPeponnet force-pushed the acme_fix_and_improvements branch 2 times, most recently from 1f075a0 to 1797ab9 Compare March 9, 2017 16:52

CyrilPeponnet force-pushed the acme_fix_and_improvements branch from 0f60a6b to f5c05b6 Compare March 17, 2017 16:07

emilevauge reviewed Apr 6, 2017

View reviewed changes

emilevauge added the contributor/waiting-for-corrections label Apr 6, 2017

vdemeester reviewed Apr 17, 2017

View reviewed changes

dtomcej approved these changes Apr 17, 2017

View reviewed changes

dtomcej suggested changes Apr 17, 2017

View reviewed changes

emilevauge approved these changes Apr 20, 2017

View reviewed changes

emilevauge added contributor/needs-resolve-conflicts and removed contributor/waiting-for-corrections labels Apr 20, 2017

CyrilPeponnet force-pushed the acme_fix_and_improvements branch from c7bbc88 to 0ac510d Compare April 20, 2017 16:27

CyrilPeponnet added 8 commits April 20, 2017 09:30

Added IgnoreFilter for ACME domain cert generation Fix traefik#1197

84898b1

Fix regexp and typo in documentation

bb3859d

No need to store wildcard cert.

050a4dc

Added ignoreFilter to onDemand

8b2f6b8

Return onDemand acme cert only if it succeed

842c534

otherwise fallback to wildcard if any.

Fix typo on error msg

e03b4f9

Put filter checking into a function

511ecf4

Adding tests for ingnoreFilters

3a05e5d

CyrilPeponnet force-pushed the acme_fix_and_improvements branch from 0ac510d to 3a05e5d Compare April 20, 2017 16:31

ldez added area/acme and removed contributor/needs-resolve-conflicts labels Apr 21, 2017

ldez modified the milestone: 1.3 Apr 25, 2017

ldez added the priority/P1 need to be fixed in next release label Apr 25, 2017

ldez modified the milestone: 1.4 May 10, 2017

ldez added kind/enhancement a new or improved feature. and removed priority/P1 need to be fixed in next release status/1-needs-design-review labels May 10, 2017

ldez changed the title ~~Added IgnoreFilter for ACME domain cert generation Fix #1197~~ Added IgnoreFilter for ACME domain cert generation May 13, 2017

juliens requested changes Jun 7, 2017

View reviewed changes

CyrilPeponnet added 2 commits June 7, 2017 08:32

Fixing some typo

54b74f5

Merge branch 'master' into acme_fix_and_improvements

69f6ea3

ldez closed this Jun 7, 2017

ldez reopened this Jun 7, 2017

nmengin reviewed Jun 7, 2017

View reviewed changes

CyrilPeponnet added 2 commits June 7, 2017 13:45

Fix typo in debug sentence

1086460

Merge branch 'acme_fix_and_improvements' of https://github.com/CyrilP…

91e1b2d

…eponnet/traefik into acme_fix_and_improvements

juliens approved these changes Jun 7, 2017

View reviewed changes

nmengin reviewed Jun 9, 2017

View reviewed changes

CyrilPeponnet added 2 commits June 9, 2017 13:16

Don't log error if err is nil

ccb10f1

Set Errorf to Warnf if there is not error

06dc3a2

nmengin mentioned this pull request Jun 19, 2017

Add provided certificate checking before LE certificate generation with OnHostRule option #1772

Merged

ldez added the contributor/waiting-for-corrections label Jun 21, 2017

nmengin closed this Jul 12, 2017

ldez removed the status/2-needs-review label Nov 18, 2021

Added IgnoreFilter for ACME domain cert generation #1260

Added IgnoreFilter for ACME domain cert generation #1260

Conversation

CyrilPeponnet commented Mar 8, 2017 • edited by ldez

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w3blogfr commented Mar 23, 2017

emilevauge left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge Apr 6, 2017 • edited

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge commented Apr 20, 2017

emilevauge commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Jun 7, 2017

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin commented Jul 12, 2017

nmengin commented Jul 12, 2017

CyrilPeponnet commented Mar 8, 2017 •

edited by ldez

emilevauge left a comment •

edited

emilevauge Apr 6, 2017 •

edited