From 8b1f497bc4699a76c8323a1466a93de4fc0f8b4d Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 10 Feb 2023 01:38:15 +0000 Subject: [PATCH] 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 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 Co-committed-by: Gusted --- server/certificates/certificates.go | 9 +++- server/upstream/domains.go | 71 +++++++++++++++-------------- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/server/certificates/certificates.go b/server/certificates/certificates.go index 1aa90a0..0bf5672 100644 --- a/server/certificates/certificates.go +++ b/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 diff --git a/server/upstream/domains.go b/server/upstream/domains.go index ba1c494..5b274b6 100644 --- a/server/upstream/domains.go +++ b/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) { - var ( - domains []string - valid bool - ) +func (o *Options) CheckCanonicalDomain(giteaClient *gitea.Client, actualDomain, mainDomainSuffix string, canonicalDomainCache cache.SetGetKey) (domain string, valid bool) { + // Check if this request is cached. 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 { - body, err := giteaClient.GiteaRawContent(o.TargetOwner, o.TargetRepo, o.TargetBranch, canonicalDomainConfig) - if err == nil { - for _, domain := range strings.Split(string(body), "\n") { - domain = strings.ToLower(domain) - domain = strings.TrimSpace(domain) - domain = strings.TrimPrefix(domain, "http://") - domain = strings.TrimPrefix(domain, "https://") - if len(domain) > 0 && !strings.HasPrefix(domain, "#") && !strings.ContainsAny(domain, "\t /") && strings.ContainsRune(domain, '.') { - domains = append(domains, domain) - } - if domain == actualDomain { - valid = true - } - } - } else { - 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) - } + return domains[0], valid + } + + body, err := giteaClient.GiteaRawContent(o.TargetOwner, o.TargetRepo, o.TargetBranch, canonicalDomainConfig) + if err == nil || err == gitea.ErrorNotFound { + log.Info().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo) + } + + var domains []string + for _, domain := range strings.Split(string(body), "\n") { + domain = strings.ToLower(domain) + domain = strings.TrimSpace(domain) + domain = strings.TrimPrefix(domain, "http://") + domain = strings.TrimPrefix(domain, "https://") + if len(domain) > 0 && !strings.HasPrefix(domain, "#") && !strings.ContainsAny(domain, "\t /") && strings.ContainsRune(domain, '.') { + domains = append(domains, domain) } - domains = append(domains, o.TargetOwner+mainDomainSuffix) - if domains[len(domains)-1] == actualDomain { + if domain == 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 }