Skip to content

Commit

Permalink
Fix on-demand TLS when SNI is empty
Browse files Browse the repository at this point in the history
A recent change passed in ClientHello into loadCertFromStorage.
Then it used hello.ServerName directly, but this is empty
if the client connects via IP address.
Previously, we passed in the name from the ClientHello
which would be the SNI if set, or the conn IP.
We now use getNameFromClientHello() as we should.

Fixes caddyserver/caddy#5758
  • Loading branch information
mholt committed Aug 17, 2023
1 parent 37f5766 commit 5bca6d1
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,6 @@ func DefaultCertificateSelector(hello *tls.ClientHelloInfo, choices []Certificat
// This function is safe for concurrent use.
func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.ClientHelloInfo, loadOrObtainIfNecessary bool) (Certificate, error) {
logger := logWithRemote(cfg.Logger.Named("handshake"), hello)
name := cfg.getNameFromClientHello(hello)

// First check our in-memory cache to see if we've already loaded it
cert, matched, defaulted := cfg.getCertificateFromCache(hello)
Expand All @@ -274,6 +273,8 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client
return cert, nil
}

name := cfg.getNameFromClientHello(hello)

// By this point, we need to load or obtain a certificate. If a swarm of requests comes in for the same
// domain, avoid pounding manager or storage thousands of times simultaneously. We do a similar sync
// strategy for obtaining certificate during handshake.
Expand Down Expand Up @@ -385,8 +386,10 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client
return Certificate{}, fmt.Errorf("no certificate available for '%s'", name)
}

// loadCertFromStorage loads the certificate for name from storage and maintains it
// (as this is only called with on-demand TLS enabled).
func (cfg *Config) loadCertFromStorage(ctx context.Context, logger *zap.Logger, hello *tls.ClientHelloInfo) (Certificate, error) {
name := normalizedName(hello.ServerName)
name := cfg.getNameFromClientHello(hello)
loadedCert, err := cfg.CacheManagedCertificate(ctx, name)
if errors.Is(err, fs.ErrNotExist) {
// If no exact match, try a wildcard variant, which is something we can still use
Expand Down Expand Up @@ -550,6 +553,7 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe
// quite common considering not all certs have issuer URLs that support it.
log.Warn("stapling OCSP",
zap.String("server_name", hello.ServerName),
zap.Strings("sans", cert.Names),
zap.Error(err))
} else {
log.Debug("successfully stapled new OCSP response",
Expand Down

0 comments on commit 5bca6d1

Please sign in to comment.