Allow to use certificate even if domain validation fails (#160)

- Currently if the canonical domain validations fails(either for
legitimate reasons or for bug reasons like the request to Gitea/Forgejo
failing) it will use main domain certificate, which in the case for
custom domains will warrant a security error as the certificate isn't
issued to the custom domain.
- This patch handles this situation more gracefully and instead only
disallow obtaining a certificate if the domain validation fails, so in
the case that a certificate still exists it can still be used even if
the canonical domain validation fails. There's a small side effect,
legitimate users that remove domains from `.domain` will still be able
to use the removed domain(as long as the DNS records exists) as long as
the certificate currently hold by pages-server isn't expired.
- Given the increased usage in custom domains that are resulting in
errors, I think it ways more than the side effect.
- In order to future-proof against future slowdowns of instances, add a retry mechanism to the domain validation function, such that it's more likely to succeed even if the instance is not responding.
- Refactor the code a bit and add some comments.

Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-authored-by: 6543 <6543@obermui.de>
Reviewed-on: https://codeberg.org/Codeberg/pages-server/pulls/160
Reviewed-by: 6543 <6543@obermui.de>
Co-authored-by: Gusted <gusted@noreply.codeberg.org>
Co-committed-by: Gusted <gusted@noreply.codeberg.org>

server/certificates/certificates.go

@@ -70,6 +70,7 @@ func TLSConfig(mainDomainSuffix string,
 			}
 
 			targetOwner := ""
+			mayObtainCert := true
 			if strings.HasSuffix(sni, mainDomainSuffix) || strings.EqualFold(sni, mainDomainSuffix[1:]) {
 				// deliver default certificate for the main domain (*.codeberg.page)
 				sni = mainDomainSuffix
@@ -87,7 +88,9 @@ func TLSConfig(mainDomainSuffix string,
 					}
 					_, valid := targetOpt.CheckCanonicalDomain(giteaClient, sni, mainDomainSuffix, canonicalDomainCache)
 					if !valid {
-						sni = mainDomainSuffix
+						// We shouldn't obtain a certificate when we cannot check if the
+						// repository has specified this domain in the `.domains` file.
+						mayObtainCert = false
 					}
 				}
 			}
@@ -106,6 +109,10 @@ func TLSConfig(mainDomainSuffix string,
 					return nil, errors.New("won't request certificate for main domain, something really bad has happened")
 				}
 
+				if !mayObtainCert {
+					return nil, fmt.Errorf("won't request certificate for %q", sni)
+				}
+
 				tlsCertificate, err = obtainCert(acmeClient, []string{sni}, nil, targetOwner, dnsProvider, mainDomainSuffix, acmeUseRateLimits, certDB)
 				if err != nil {
 					return nil, err

server/upstream/domains.go

@@ -16,49 +16,54 @@ var canonicalDomainCacheTimeout = 15 * time.Minute
 const canonicalDomainConfig = ".domains"
 
 // CheckCanonicalDomain returns the canonical domain specified in the repo (using the `.domains` file).
-func (o *Options) CheckCanonicalDomain(giteaClient *gitea.Client, actualDomain, mainDomainSuffix string, canonicalDomainCache cache.SetGetKey) (string, bool) {
+func (o *Options) CheckCanonicalDomain(giteaClient *gitea.Client, actualDomain, mainDomainSuffix string, canonicalDomainCache cache.SetGetKey) (domain string, valid bool) {
-	var (
+	// Check if this request is cached.
-		domains []string
-		valid   bool
-	)
 	if cachedValue, ok := canonicalDomainCache.Get(o.TargetOwner + "/" + o.TargetRepo + "/" + o.TargetBranch); ok {
-		domains = cachedValue.([]string)
+		domains := cachedValue.([]string)
 		for _, domain := range domains {
 			if domain == actualDomain {
 				valid = true
 				break
 			}
 		}
-	} else {
+		return domains[0], valid
-		body, err := giteaClient.GiteaRawContent(o.TargetOwner, o.TargetRepo, o.TargetBranch, canonicalDomainConfig)
+	}
-		if err == nil {
+
-			for _, domain := range strings.Split(string(body), "\n") {
+	body, err := giteaClient.GiteaRawContent(o.TargetOwner, o.TargetRepo, o.TargetBranch, canonicalDomainConfig)
-				domain = strings.ToLower(domain)
+	if err == nil || err == gitea.ErrorNotFound {
-				domain = strings.TrimSpace(domain)
+		log.Info().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo)
-				domain = strings.TrimPrefix(domain, "http://")
+	}
-				domain = strings.TrimPrefix(domain, "https://")
+
-				if len(domain) > 0 && !strings.HasPrefix(domain, "#") && !strings.ContainsAny(domain, "\t /") && strings.ContainsRune(domain, '.') {
+	var domains []string
-					domains = append(domains, domain)
+	for _, domain := range strings.Split(string(body), "\n") {
-				}
+		domain = strings.ToLower(domain)
-				if domain == actualDomain {
+		domain = strings.TrimSpace(domain)
-					valid = true
+		domain = strings.TrimPrefix(domain, "http://")
-				}
+		domain = strings.TrimPrefix(domain, "https://")
-			}
+		if len(domain) > 0 && !strings.HasPrefix(domain, "#") && !strings.ContainsAny(domain, "\t /") && strings.ContainsRune(domain, '.') {
-		} else {
+			domains = append(domains, domain)
-			if err != gitea.ErrorNotFound {
-				log.Error().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo)
-			} else {
-				log.Info().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo)
-			}
 		}
-		domains = append(domains, o.TargetOwner+mainDomainSuffix)
+		if domain == actualDomain {
-		if domains[len(domains)-1] == actualDomain {
 			valid = true
 		}
-		if o.TargetRepo != "" && o.TargetRepo != "pages" {
-			domains[len(domains)-1] += "/" + o.TargetRepo
-		}
-		_ = canonicalDomainCache.Set(o.TargetOwner+"/"+o.TargetRepo+"/"+o.TargetBranch, domains, canonicalDomainCacheTimeout)
 	}
+
+	// Add [owner].[pages-domain] as valid domnain.
+	domains = append(domains, o.TargetOwner+mainDomainSuffix)
+	if domains[len(domains)-1] == actualDomain {
+		valid = true
+	}
+
+	// If the target repository isn't called pages, add `/[repository]` to the
+	// previous valid domain.
+	if o.TargetRepo != "" && o.TargetRepo != "pages" {
+		domains[len(domains)-1] += "/" + o.TargetRepo
+	}
+
+	// Add result to cache.
+	_ = canonicalDomainCache.Set(o.TargetOwner+"/"+o.TargetRepo+"/"+o.TargetBranch, domains, canonicalDomainCacheTimeout)
+
+	// Return the first domain from the list and return if any of the domains
+	// matched the requested domain.
 	return domains[0], valid
 }