Return Proper Non-ACME certificate - Fixes Issue 672 #1018
Conversation
dtomcej
changed the title
fix spacing
|
LGTM 👍 |
| @@ -331,6 +334,14 @@ func (a *ACME) CreateLocalConfig(tlsConfig *tls.Config, checkOnDemandDomain func | |||
| func (a *ACME) getCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) { | |||
| domain := types.CanonicalDomain(clientHello.ServerName) | |||
| account := a.store.Get().(*Account) | |||
| //use regex to test for wildcard certs that might have been added into TLSConfig | |||
| for k := range a.TLSConfig.NameToCertificate { | |||
| selector := "^" + strings.Replace(k, "*.", ".*\\.?", -1) + "$" | |||
There was a problem hiding this comment.
@dtomcej Will this properly not match subdomains of a wildcard?
E,g, If I have a certificate for *.mysite.com, then foo.mysite.com should match but bar.foo.mysite.com should not match.
Should it be strings.Replace(k, "*.", "[^.]*\\.?", -1) instead?
There was a problem hiding this comment.
Since you cannot have a double wildcard certificate, sometimes people issue certs with wildcard subdomains as SANS. Example: *.mysite.com could have a SAN of *.foo.mysite.com, in which case it is up to the certificate to cover the SANs. I also search the SANs of the certificate here as well, so if your cert has both *.mysite.com with a SAN of *.foo.mysite.com, it will be matched either way.
There was a problem hiding this comment.
My concern is for the possibility of not having SANs. That is, I have a wildcard for *.mydomain.com. It will not cover *.test.mydomain.com, and I don't want to add SANs for that. Instead I want to use the wildcard for *.mydomain.com, but have ACME generate certificates for say bar.foo.mydomain.com.
Will the code as-is allow this, or will it attempt to use the wildcard even though it doesn't apply due to the lack of a SAN?
There was a problem hiding this comment.
Ah. I see what you are saying. As is, it will serve the static wildcard.
Feel free to open a new PR (Or a new issue with your proposed regex) to update the regex, and we can test it out. Im not against it. I think that if we are going to use ACME as a "fallback" when static certs are added, having them matched by a more accurate regex would be a good thing.
|
Is it possible that this change caused the Default Cert to be always sent? |
|
I have the same issue I think, I have a default wildcard cert (self signed), when I request a ACME cert, it's not used, only the wildcard cert is presented. |
Tighten regex match for wildcard certs [Addendum to #1018]
Fixes Issue #672