fix: looking DNS setting during tls cert provision (#1067)

getfider · May 8, 2022 · 603508c · 603508c
1 parent 55efacf
commit 603508c
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 61 deletions.
diff --git a/app/pkg/web/autocert_test.go b/app/pkg/web/autocert_test.go
diff --git a/app/pkg/web/ssl.go b/app/pkg/web/ssl.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"net"
 	"net/http"
 	"strings"
 
@@ -43,6 +44,8 @@ func getDefaultTLSConfig(autoSSL bool) *tls.Config {
 var errInvalidHostName = errors.New("autotls: invalid hostname")
 
 func isValidHostName(ctx context.Context, host string) error {
+	// In this context, host can only be custom domains, not a subdomain of fider.io
+
 	if host == "" {
 		return errors.Wrap(errInvalidHostName, "host cannot be empty.")
 	}
@@ -59,16 +62,30 @@ func isValidHostName(ctx context.Context, host string) error {
 		return errors.Wrap(err, "failed start new transaction")
 	}
 	defer trx.MustCommit()
+	dbCtx := context.WithValue(ctx, app.TransactionCtxKey, trx)
+
+	getTenant := &query.GetTenantByDomain{Domain: host}
+	err = bus.Dispatch(dbCtx, getTenant)
+	if err != nil {
+		if errors.Cause(err) == app.ErrNotFound {
+			return errors.Wrap(errInvalidHostName, "no tenant found with cname %s", host)
+		}
+		return errors.Wrap(err, "failed to get tenant by cname")
+	}
+
+	cname, err := net.DefaultResolver.LookupCNAME(ctx, host)
+	if err != nil {
+		return errors.Wrap(err, "failed to lookup CNAME")
+	}
 
-	isAvailable := &query.IsCNAMEAvailable{CNAME: host}
-	newCtx := context.WithValue(ctx, app.TransactionCtxKey, trx)
-	if err := bus.Dispatch(newCtx, isAvailable); err != nil {
-		return errors.Wrap(err, "failed to find tenant by cname")
+	if cname == "" {
+		return errors.Wrap(errInvalidHostName, "no CNAME DNS record found for %s", host)
 	}
 
-	if isAvailable.Result {
-		return errors.Wrap(errInvalidHostName, "no tenants found with cname %s", host)
+	if strings.TrimSuffix(cname, ".") != getTenant.Result.Subdomain+env.MultiTenantDomain() {
+		return errors.Wrap(errInvalidHostName, "cname %s (from %s) doesn't match configured host %s", cname, host, getTenant.Result.Subdomain+env.MultiTenantDomain())
 	}
+
 	return nil
 }
 
@@ -128,6 +145,11 @@ func (m *CertificateManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Ce
 			return &m.cert, nil
 		}
 
+		// If it's an IP address, just return the cert we have
+		if net.ParseIP(serverName) != nil {
+			return &m.cert, nil
+		}
+
 		// throw an error if it doesn't match the leaf certificate but still ends with current hostname, example:
 		// hostdomain is myserver.com and the certificate is *.myserver.com
 		// serverName is something.else.myserver.com, it should throw an error
@@ -136,9 +158,14 @@ func (m *CertificateManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Ce
 		}
 	}
 
+	//TODO: consider recovering from a possible panic here
 	cert, err := m.autotls.GetCertificate(hello)
-	if err != nil && errors.Cause(err) != errInvalidHostName {
-		log.Error(m.ctx, err)
+	if err != nil {
+		if errors.Cause(err) == errInvalidHostName {
+			log.Warn(m.ctx, err.Error())
+		} else {
+			log.Error(m.ctx, err)
+		}
 	}
 
 	return cert, err

diff --git a/app/pkg/web/ssl_test.go b/app/pkg/web/ssl_test.go
@@ -4,14 +4,59 @@ import (
 	"context"
 	"crypto/tls"
 	"testing"
+	"time"
 
+	"github.com/getfider/fider/app"
+	"github.com/getfider/fider/app/models/entity"
+	"github.com/getfider/fider/app/models/query"
 	"github.com/getfider/fider/app/pkg/bus"
 	"github.com/getfider/fider/app/pkg/env"
 	"github.com/getfider/fider/app/services/blob/fs"
 
 	. "github.com/getfider/fider/app/pkg/assert"
 )
 
+// this handler relies on real DNS records on feedbacktest.goenning.net
+// the correct subdomains are defined below to simulate a configuration match for tests
+func mockGetTenantWithCorrectSubdomains(ctx context.Context, q *query.GetTenantByDomain) error {
+	if q.Domain == "feedbacktest.goenning.net" {
+		q.Result = &entity.Tenant{Name: "Feedback for goenning.net", Subdomain: "goenning"}
+		return nil
+	} else {
+		return app.ErrNotFound
+	}
+}
+
+// this handler relies on real DNS records on feedbacktest.goenning.net
+// wrong subdomains are defined below to simulate a mismatch in configuration for tests
+func mockGetTenantWithIncorrectSubdomains(ctx context.Context, q *query.GetTenantByDomain) error {
+	if q.Domain == "feedbacktest.goenning.net" {
+		q.Result = &entity.Tenant{Name: "Feedback for goenning.net", Subdomain: "demo"}
+		return nil
+	} else {
+		return app.ErrNotFound
+	}
+}
+
+func TestUseAutoCert_WhenCNAMEAreRegistered(t *testing.T) {
+	RegisterT(t)
+	bus.Init(fs.Service{})
+	bus.AddHandler(mockGetTenantWithCorrectSubdomains)
+
+	manager, err := NewCertificateManager(context.Background(), "", "")
+	Expect(err).IsNil()
+
+	cert, err := manager.GetCertificate(&tls.ClientHelloInfo{
+		ServerName: "feedbacktest.goenning.net",
+	})
+	Expect(err.Error()).ContainsSubstring(`acme/autocert: unable to satisfy`)
+	Expect(err.Error()).ContainsSubstring(`for domain "feedbacktest.goenning.net": no viable challenge type found`)
+	Expect(cert).IsNil()
+
+	// GetCertificate starts a fire and forget go routine to delete items from cache, give it 2sec to complete it
+	time.Sleep(2 * time.Second)
+}
+
 func TestGetCertificate(t *testing.T) {
 	RegisterT(t)
 
@@ -24,8 +69,10 @@ func TestGetCertificate(t *testing.T) {
 		{"multi", "all-test-fider-io", "fider"},
 		{"multi", "all-test-fider-io", "feedback.test.fider.io"},
 		{"multi", "all-test-fider-io", "FEEDBACK.test.fider.io"},
+		{"multi", "all-test-fider-io", "44.194.119.243"},
 		{"single", "test-fider-io", "test.fider.io"},
 		{"single", "test-fider-io", "fider.io"},
+		{"single", "test-fider-io", "44.194.119.243"},
 	}
 
 	for _, testCase := range testCases {
@@ -45,29 +92,43 @@ func TestGetCertificate(t *testing.T) {
 	}
 }
 
-func TestGetCertificate_WhenCNAMEAreInvalid(t *testing.T) {
+func TestGetCertificate_WhenCNAMEAreNotConfigured(t *testing.T) {
 	RegisterT(t)
 	bus.Init(fs.Service{})
-	bus.AddHandler(mockIsCNAMEAvailable)
+	bus.AddHandler(mockGetTenantWithCorrectSubdomains)
 
 	manager, err := NewCertificateManager(context.Background(), "", "")
 	Expect(err).IsNil()
 
-	invalidServerNames := []string{"feedback.heyworld.com", "2.2.2.2"}
+	invalidServerNames := []string{"feedback.heyworld.com", "ideas.app.com"}
 
 	for _, serverName := range invalidServerNames {
 		cert, err := manager.GetCertificate(&tls.ClientHelloInfo{
 			ServerName: serverName,
 		})
-		Expect(err.Error()).ContainsSubstring(`no tenants found with cname ` + serverName)
+		Expect(err.Error()).ContainsSubstring(`no tenant found with cname ` + serverName)
 		Expect(cert).IsNil()
 	}
 }
 
+func TestGetCertificate_WhenCNAMEDoesntMatch(t *testing.T) {
+	RegisterT(t)
+	bus.Init(fs.Service{})
+
+	bus.AddHandler(mockGetTenantWithIncorrectSubdomains)
+
+	manager, err := NewCertificateManager(context.Background(), "", "")
+	Expect(err).IsNil()
+
+	cert, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "feedbacktest.goenning.net"})
+	Expect(err.Error()).ContainsSubstring("cname goenning.test.fider.io. (from feedbacktest.goenning.net) doesn't match configured host demo.test.fider.io")
+	Expect(cert).IsNil()
+}
+
 func TestGetCertificate_ServerNameMatchesCertificate_ShouldReturnIt(t *testing.T) {
 	RegisterT(t)
 	bus.Init(fs.Service{})
-	bus.AddHandler(mockIsCNAMEAvailable)
+	bus.AddHandler(mockGetTenantWithCorrectSubdomains)
 
 	certFile := env.Etc("dev-fider-io.crt")
 	certKey := env.Etc("dev-fider-io.key")
@@ -88,7 +149,7 @@ func TestGetCertificate_ServerNameMatchesCertificate_ShouldReturnIt(t *testing.T
 func TestGetCertificate_ServerNameDoesntMatchCertificate_ButEndsWithHostName_ShouldThrow(t *testing.T) {
 	RegisterT(t)
 	bus.Init(fs.Service{})
-	bus.AddHandler(mockIsCNAMEAvailable)
+	bus.AddHandler(mockGetTenantWithCorrectSubdomains)
 
 	env.Config.HostDomain = "dev.fider.io"
 	certFile := env.Etc("dev-fider-io.crt")