httpcaddyfile: Improve AP logic with OnDemand

We have users that have site blocks like *.*.tld with on-demand TLS
enabled. While *.*.tld does not qualify for a publicly-trusted cert due
to its wildcards, On-Demand TLS does not actually obtain a cert with
those wildcards, since it uses the actual hostname on the handshake.

This improves on that logic, but I am still not 100% satisfied with the
result since I think we need to also check if another site block is more
specific, like foo.example.tld, which might not have on-demand TLS
enabled, and make sure an automation policy gets created before the
more general policy with on-demand...
This commit is contained in:
Matthew Holt 2020-10-22 12:40:23 -06:00
parent 97caf368ee
commit b6686a54d8
No known key found for this signature in database
GPG key ID: 2A349DD577D586A5
2 changed files with 31 additions and 5 deletions

View file

@ -213,7 +213,17 @@ func (st ServerType) buildTLSApp(
if ap.Issuer == nil {
var internal, external []string
for _, s := range ap.Subjects {
if certmagic.SubjectQualifiesForPublicCert(s) {
if !certmagic.SubjectQualifiesForCert(s) {
return nil, warnings, fmt.Errorf("subject does not qualify for certificate: '%s'", s)
}
// we don't use certmagic.SubjectQualifiesForPublicCert() because of one nuance:
// names like *.*.tld that may not qualify for a public certificate are actually
// fine when used with OnDemand, since OnDemand (currently) does not obtain
// wildcards (if it ever does, there will be a separate config option to enable
// it that we would need to check here) since the hostname is known at handshake;
// and it is unexpected to switch to internal issuer when the user wants to get
// regular certificates on-demand for a class of certs like *.*.tld.
if !certmagic.SubjectIsIP(s) && !certmagic.SubjectIsInternal(s) && (strings.Count(s, "*.") < 2 || ap.OnDemand) {
external = append(external, s)
} else {
internal = append(internal, s)

View file

@ -334,10 +334,26 @@ func (t *TLS) AddAutomationPolicy(ap *AutomationPolicy) error {
if err != nil {
return err
}
for i, other := range t.Automation.Policies {
// if a catch-all policy (or really, any policy with
// fewer names) exists, prioritize this new policy
if len(other.Subjects) < len(ap.Subjects) {
// sort new automation policies just before any other which is a superset
// of this one; if we find an existing policy that covers every subject in
// ap but less specifically (e.g. a catch-all policy, or one with wildcards
// or with fewer subjects), insert ap just before it, otherwise ap would
// never be used because the first matching policy is more general
for i, existing := range t.Automation.Policies {
// first see if existing is superset of ap for all names
var otherIsSuperset bool
outer:
for _, thisSubj := range ap.Subjects {
for _, otherSubj := range existing.Subjects {
if certmagic.MatchWildcard(thisSubj, otherSubj) {
otherIsSuperset = true
break outer
}
}
}
// if existing AP is a superset or if it contains fewer names (i.e. is
// more general), then new AP is more specific, so insert before it
if otherIsSuperset || len(existing.Subjects) < len(ap.Subjects) {
t.Automation.Policies = append(t.Automation.Policies[:i],
append([]*AutomationPolicy{ap}, t.Automation.Policies[i:]...)...)
return nil