certs: refactor cert manager to support multiple certificates (#10207)

This commit refactors the certificate management implementation
in the `certs` package such that multiple certificates can be
specified at the same time. Therefore, the following layout of
the `certs/` directory is expected:
```
certs/
 │
 ├─ public.crt
 ├─ private.key
 ├─ CAs/          // CAs directory is ignored
 │   │
 │    ...
 │
 ├─ example.com/
 │   │
 │   ├─ public.crt
 │   └─ private.key
 └─ foobar.org/
     │
     ├─ public.crt
     └─ private.key
   ...
```

However, directory names like `example.com` are just for human
readability/organization and don't have any meaning w.r.t whether
a particular certificate is served or not. This decision is made based
on the SNI sent by the client and the SAN of the certificate.

***

The `Manager` will pick a certificate based on the client trying
to establish a TLS connection. In particular, it looks at the client
hello (i.e. SNI) to determine which host the client tries to access.
If the manager can find a certificate that matches the SNI it
returns this certificate to the client.

However, the client may choose to not send an SNI or tries to access
a server directly via IP (`https://<ip>:<port>`). In this case, we
cannot use the SNI to determine which certificate to serve. However,
we also should not pick "the first" certificate that would be accepted
by the client (based on crypto. parameters - like a signature algorithm)
because it may be an internal certificate that contains internal hostnames. 
We would disclose internal infrastructure details doing so.

Therefore, the `Manager` returns the "default" certificate when the
client does not specify an SNI. The default certificate the top-level
`public.crt` - i.e. `certs/public.crt`.

This approach has some consequences:
 - It's the operator's responsibility to ensure that the top-level
   `public.crt` does not disclose any information (i.e. hostnames)
   that are not publicly visible. However, this was the case in the
   past already.
 - Any other `public.crt` - except for the top-level one - must not
   contain any IP SAN. The reason for this restriction is that the
   Manager cannot match a SNI to an IP b/c the SNI is the server host
   name. The entire purpose of SNI is to indicate which host the client
   tries to connect to when multiple hosts run on the same IP. So, a
   client will not set the SNI to an IP.
   If we would allow IP SANs in a lower-level `public.crt` a user would
   expect that it is possible to connect to MinIO directly via IP address
   and that the MinIO server would pick "the right" certificate. However,
   the MinIO server cannot determine which certificate to serve, and
   therefore always picks the "default" one. This may lead to all sorts
   of confusing errors like:
   "It works if I use `https:instance.minio.local` but not when I use
   `https://10.0.2.1`.

These consequences/limitations should be pointed out / explained in our
docs in an appropriate way. However, the support for multiple
certificates should not have any impact on how deployment with a single
certificate function today.

Co-authored-by: Harshavardhana <harsha@minio.io>

cmd/admin-handlers.go

@@ -1557,7 +1557,7 @@ func fetchLambdaInfo(cfg config.Config) []map[string][]madmin.TargetIDStatus {
 	// Fetch the configured targets
 	tr := NewGatewayHTTPTransport()
 	defer tr.CloseIdleConnections()
-	targetList, err := notify.FetchRegisteredTargets(cfg, GlobalContext.Done(), tr, true, false)
+	targetList, err := notify.FetchRegisteredTargets(GlobalContext, cfg, tr, true, false)
 	if err != nil && err != notify.ErrTargetsOffline {
 		logger.LogIf(GlobalContext, err)
 		return nil

cmd/common-main.go

@@ -20,6 +20,7 @@ import (
 	"crypto/x509"
 	"encoding/gob"
 	"errors"
+	"fmt"
 	"net"
 	"net/url"
 	"os"
@@ -293,7 +294,7 @@ func logStartupMessage(msg string) {
 	logger.StartupMessage(msg)
 }
 
-func getTLSConfig() (x509Certs []*x509.Certificate, c *certs.Certs, secureConn bool, err error) {
+func getTLSConfig() (x509Certs []*x509.Certificate, manager *certs.Manager, secureConn bool, err error) {
 	if !(isFile(getPublicCertFile()) && isFile(getPrivateKeyFile())) {
 		return nil, nil, false, nil
 	}
@@ -302,11 +303,62 @@ func getTLSConfig() (x509Certs []*x509.Certificate, c *certs.Certs, secureConn b
 		return nil, nil, false, err
 	}
 
-	c, err = certs.New(getPublicCertFile(), getPrivateKeyFile(), config.LoadX509KeyPair)
+	manager, err = certs.NewManager(GlobalContext, getPublicCertFile(), getPrivateKeyFile(), config.LoadX509KeyPair)
 	if err != nil {
 		return nil, nil, false, err
 	}
 
+	// MinIO has support for multiple certificates. It expects the following structure:
+	//  certs/
+	//   │
+	//   ├─ public.crt
+	//   ├─ private.key
+	//   │
+	//   ├─ example.com/
+	//   │   │
+	//   │   ├─ public.crt
+	//   │   └─ private.key
+	//   └─ foobar.org/
+	//      │
+	//      ├─ public.crt
+	//      └─ private.key
+	//   ...
+	//
+	// Therefore, we read all filenames in the cert directory and check
+	// for each directory whether it contains a public.crt and private.key.
+	// If so, we try to add it to certificate manager.
+	root, err := os.Open(globalCertsDir.Get())
+	if err != nil {
+		return nil, nil, false, err
+	}
+	defer root.Close()
+
+	files, err := root.Readdir(-1)
+	if err != nil {
+		return nil, nil, false, err
+	}
+	for _, file := range files {
+		// We exclude any regular file and the "CAs/" directory.
+		// The "CAs/" directory contains (root) CA certificates
+		// that MinIO adds to its list of trusted roots (tls.Config.RootCAs).
+		// Therefore, "CAs/" does not contain X.509 certificates that
+		// are meant to be served by MinIO.
+		if !file.IsDir() || file.Name() == "CAs" {
+			continue
+		}
+
+		var (
+			certFile = filepath.Join(root.Name(), file.Name(), publicCertFile)
+			keyFile  = filepath.Join(root.Name(), file.Name(), privateKeyFile)
+		)
+		if !isFile(certFile) || !isFile(keyFile) {
+			continue
+		}
+		if err := manager.AddCertificate(certFile, keyFile); err != nil {
+			err = fmt.Errorf("Failed to load TLS certificate '%s': %v", certFile, err)
+			logger.LogIf(GlobalContext, err, logger.Minio)
+		}
+	}
 	secureConn = true
-	return x509Certs, c, secureConn, nil
+	return x509Certs, manager, secureConn, nil
 }

cmd/config-current.go

@@ -306,8 +306,7 @@ func validateConfig(s config.Config, setDriveCount int) error {
 		return err
 	}
 
-	return notify.TestNotificationTargets(s, GlobalContext.Done(), NewGatewayHTTPTransport(),
+	return notify.TestNotificationTargets(GlobalContext, s, NewGatewayHTTPTransport(), globalNotificationSys.ConfiguredTargetIDs())
-		globalNotificationSys.ConfiguredTargetIDs())
 }
 
 func lookupConfigs(s config.Config, setDriveCount int) {
@@ -487,12 +486,12 @@ func lookupConfigs(s config.Config, setDriveCount int) {
 		}
 	}
 
-	globalConfigTargetList, err = notify.GetNotificationTargets(s, GlobalContext.Done(), NewGatewayHTTPTransport(), false)
+	globalConfigTargetList, err = notify.GetNotificationTargets(GlobalContext, s, NewGatewayHTTPTransport(), false)
 	if err != nil {
 		logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err))
 	}
 
-	globalEnvTargetList, err = notify.GetNotificationTargets(newServerConfig(), GlobalContext.Done(), NewGatewayHTTPTransport(), true)
+	globalEnvTargetList, err = notify.GetNotificationTargets(GlobalContext, newServerConfig(), NewGatewayHTTPTransport(), true)
 	if err != nil {
 		logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err))
 	}

cmd/config/notify/parse.go

@@ -43,11 +43,10 @@ var ErrTargetsOffline = errors.New("one or more targets are offline. Please use
 
 // TestNotificationTargets is similar to GetNotificationTargets()
 // avoids explicit registration.
-func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport,
+func TestNotificationTargets(ctx context.Context, cfg config.Config, transport *http.Transport, targetIDs []event.TargetID) error {
-	targetIDs []event.TargetID) error {
 	test := true
 	returnOnTargetError := true
-	targets, err := RegisterNotificationTargets(cfg, doneCh, transport, targetIDs, test, returnOnTargetError)
+	targets, err := RegisterNotificationTargets(ctx, cfg, transport, targetIDs, test, returnOnTargetError)
 	if err == nil {
 		// Close all targets since we are only testing connections.
 		for _, t := range targets.TargetMap() {
@@ -60,9 +59,9 @@ func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transpor
 
 // GetNotificationTargets registers and initializes all notification
 // targets, returns error if any.
-func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, test bool) (*event.TargetList, error) {
+func GetNotificationTargets(ctx context.Context, cfg config.Config, transport *http.Transport, test bool) (*event.TargetList, error) {
 	returnOnTargetError := false
-	return RegisterNotificationTargets(cfg, doneCh, transport, nil, test, returnOnTargetError)
+	return RegisterNotificationTargets(ctx, cfg, transport, nil, test, returnOnTargetError)
 }
 
 // RegisterNotificationTargets - returns TargetList which contains enabled targets in serverConfig.
@@ -70,8 +69,8 @@ func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport
 // * Add a new target in pkg/event/target package.
 // * Add newly added target configuration to serverConfig.Notify.<TARGET_NAME>.
 // * Handle the configuration in this function to create/add into TargetList.
-func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, targetIDs []event.TargetID, test bool, returnOnTargetError bool) (*event.TargetList, error) {
+func RegisterNotificationTargets(ctx context.Context, cfg config.Config, transport *http.Transport, targetIDs []event.TargetID, test bool, returnOnTargetError bool) (*event.TargetList, error) {
-	targetList, err := FetchRegisteredTargets(cfg, doneCh, transport, test, returnOnTargetError)
+	targetList, err := FetchRegisteredTargets(ctx, cfg, transport, test, returnOnTargetError)
 	if err != nil {
 		return targetList, err
 	}
@@ -94,7 +93,7 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran
 // FetchRegisteredTargets - Returns a set of configured TargetList
 // If `returnOnTargetError` is set to true, The function returns when a target initialization fails
 // Else, the function will return a complete TargetList irrespective of errors
-func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, test bool, returnOnTargetError bool) (_ *event.TargetList, err error) {
+func FetchRegisteredTargets(ctx context.Context, cfg config.Config, transport *http.Transport, test bool, returnOnTargetError bool) (_ *event.TargetList, err error) {
 	targetList := event.NewTargetList()
 	var targetsOffline bool
 
@@ -167,7 +166,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewAMQPTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewAMQPTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -188,7 +187,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewElasticsearchTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewElasticsearchTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -209,7 +208,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 			continue
 		}
 		args.TLS.RootCAs = transport.TLSClientConfig.RootCAs
-		newTarget, err := target.NewKafkaTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewKafkaTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -230,7 +229,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 			continue
 		}
 		args.RootCAs = transport.TLSClientConfig.RootCAs
-		newTarget, err := target.NewMQTTTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewMQTTTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -250,7 +249,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewMySQLTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewMySQLTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -270,7 +269,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewNATSTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewNATSTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -290,7 +289,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewNSQTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewNSQTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -310,7 +309,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewPostgreSQLTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewPostgreSQLTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -330,7 +329,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewRedisTarget(id, args, doneCh, logger.LogOnceIf, test)
+		newTarget, err := target.NewRedisTarget(id, args, ctx.Done(), logger.LogOnceIf, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {
@@ -350,7 +349,7 @@ func FetchRegisteredTargets(cfg config.Config, doneCh <-chan struct{}, transport
 		if !args.Enable {
 			continue
 		}
-		newTarget, err := target.NewWebhookTarget(id, args, doneCh, logger.LogOnceIf, transport, test)
+		newTarget, err := target.NewWebhookTarget(ctx, id, args, logger.LogOnceIf, transport, test)
 		if err != nil {
 			targetsOffline = true
 			if returnOnTargetError {

cmd/gateway-main.go

@@ -293,9 +293,6 @@ func StartGateway(ctx *cli.Context, gw Gateway) {
 
 	newObject, err := gw.NewGatewayLayer(globalActiveCred)
 	if err != nil {
-		// Stop watching for any certificate changes.
-		globalTLSCerts.Stop()
-
 		globalHTTPServer.Shutdown()
 		logger.FatalIf(err, "Unable to initialize gateway backend")
 	}

cmd/globals.go

@@ -167,7 +167,7 @@ var (
 	// IsSSL indicates if the server is configured with SSL.
 	globalIsSSL bool
 
-	globalTLSCerts *certs.Certs
+	globalTLSCerts *certs.Manager
 
 	globalHTTPServer        *xhttp.Server
 	globalHTTPServerErrorCh = make(chan error)

cmd/server-main.go

@@ -506,9 +506,6 @@ func serverMain(ctx *cli.Context) {
 	newObject, err := newObjectLayer(GlobalContext, globalEndpoints)
 	logger.SetDeploymentID(globalDeploymentID)
 	if err != nil {
-		// Stop watching for any certificate changes.
-		globalTLSCerts.Stop()
-
 		globalHTTPServer.Shutdown()
 		logger.Fatal(err, "Unable to initialize backend")
 	}

cmd/signals.go

@@ -50,9 +50,6 @@ func handleSignals() {
 			globalNotificationSys.RemoveAllRemoteTargets()
 		}
 
-		// Stop watching for any certificate changes.
-		globalTLSCerts.Stop()
-
 		if httpServer := newHTTPServerFn(); httpServer != nil {
 			err = httpServer.Shutdown()
 			logger.LogIf(context.Background(), err)

pkg/certs/certs.go

@@ -17,7 +17,11 @@
 package certs
 
 import (
+	"context"
 	"crypto/tls"
+	"crypto/x509"
+	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
@@ -26,177 +30,317 @@ import (
 	"github.com/rjeczalik/notify"
 )
 
-// A Certs represents a certificate manager able to watch certificate
+// LoadX509KeyPairFunc is a function that parses a private key and
-// and key pairs for changes.
+// certificate file and returns a TLS certificate on success.
-type Certs struct {
+type LoadX509KeyPairFunc func(certFile, keyFile string) (tls.Certificate, error)
-	sync.RWMutex
+
-	// user input params.
+// GetCertificateFunc is a callback that allows a TLS stack deliver different
-	certFile string
+// certificates based on the client trying to establish a TLS connection.
-	keyFile  string
+//
-	loadCert LoadX509KeyPairFunc
+// For example, a GetCertificateFunc can return different TLS certificates depending
+// upon the TLS SNI sent by the client.
+type GetCertificateFunc func(hello *tls.ClientHelloInfo) (*tls.Certificate, error)
 
-	// points to the latest certificate.
+// Manager is a TLS certificate manager that can handle multiple certificates.
-	cert *tls.Certificate
+// When a client tries to establish a TLS connection, Manager will try to
+// pick a certificate that can be validated by the client.
+//
+// For instance, if the client specifies a TLS SNI then Manager will try to
+// find the corresponding certificate. If there is no such certificate it
+// will fallback to the certificate named public.crt.
+//
+// Manager will automatically reload certificates if the corresponding file changes.
+type Manager struct {
+	lock         sync.RWMutex
+	certificates map[pair]*tls.Certificate // Mapping: certificate file name => TLS certificates
+	defaultCert  pair
 
-	// internal param to track for events, also
+	loadX509KeyPair LoadX509KeyPairFunc
-	// used to close the watcher.
+	events          chan notify.EventInfo
-	e chan notify.EventInfo
+	ctx             context.Context
 }
 
-// LoadX509KeyPairFunc - provides a type for custom cert loader function.
+// pair represents a certificate and private key file tuple.
-type LoadX509KeyPairFunc func(certFile, keyFile string) (tls.Certificate, error)
+type pair struct {
+	KeyFile  string
+	CertFile string
+}
 
-// New initializes a new certs monitor.
+// NewManager returns a new Manager that handles one certificate specified via
-func New(certFile, keyFile string, loadCert LoadX509KeyPairFunc) (*Certs, error) {
+// the certFile and keyFile. It will use the loadX509KeyPair function to (re)load
-	certFileIsLink, err := checkSymlink(certFile)
+// certificates.
+//
+// The certificate loaded from certFile is considered the default certificate.
+// If a client does not send the TLS SNI extension then Manager will return
+// this certificate.
+func NewManager(ctx context.Context, certFile, keyFile string, loadX509KeyPair LoadX509KeyPairFunc) (manager *Manager, err error) {
+	certFile, err = filepath.Abs(certFile)
 	if err != nil {
 		return nil, err
 	}
-	keyFileIsLink, err := checkSymlink(keyFile)
+	keyFile, err = filepath.Abs(keyFile)
 	if err != nil {
 		return nil, err
 	}
-	c := &Certs{
-		certFile: certFile,
-		keyFile:  keyFile,
-		loadCert: loadCert,
-		// Make the channel buffered to ensure no event is dropped. Notify will drop
-		// an event if the receiver is not able to keep up the sending pace.
-		e: make(chan notify.EventInfo, 1),
-	}
 
-	if certFileIsLink && keyFileIsLink {
+	manager = &Manager{
-		if err := c.watchSymlinks(); err != nil {
+		certificates: map[pair]*tls.Certificate{},
-			return nil, err
+		defaultCert: pair{
+			KeyFile:  keyFile,
+			CertFile: certFile,
+		},
+		loadX509KeyPair: loadX509KeyPair,
+		events:          make(chan notify.EventInfo, 1),
+		ctx:             ctx,
 	}
-	} else {
+	if err := manager.AddCertificate(certFile, keyFile); err != nil {
-		if err := c.watch(); err != nil {
 		return nil, err
 	}
+	go manager.watchFileEvents()
+	return manager, nil
 }
 
-	return c, nil
+// AddCertificate adds the TLS certificate in certFile resp. keyFile
+// to the Manager.
+//
+// If there is already a certificate with the same base name it will be
+// replaced by the newly added one.
+func (m *Manager) AddCertificate(certFile, keyFile string) (err error) {
+	certFile, err = filepath.Abs(certFile)
+	if err != nil {
+		return err
 	}
-
+	keyFile, err = filepath.Abs(keyFile)
-func checkSymlink(file string) (bool, error) {
-	st, err := os.Lstat(file)
 	if err != nil {
-		return false, err
+		return err
 	}
-	return st.Mode()&os.ModeSymlink == os.ModeSymlink, nil
+	certFileIsLink, err := isSymlink(certFile)
+	if err != nil {
+		return err
 	}
-
+	keyFileIsLink, err := isSymlink(keyFile)
-// watchSymlinks reloads symlinked files since fsnotify cannot watch
-// on symbolic links.
-func (c *Certs) watchSymlinks() (err error) {
-	cert, err := c.loadCert(c.certFile, c.keyFile)
 	if err != nil {
 		return err
 	}
-	c.Lock()
+	if certFileIsLink && !keyFileIsLink {
-	c.cert = &cert
+		return fmt.Errorf("certs: '%s' is a symlink but '%s' is a regular file", certFile, keyFile)
-	c.Unlock()
-	go func() {
-		for {
-			select {
-			case <-c.e:
-				// Once stopped exits this routine.
-				return
-			case <-time.After(24 * time.Hour):
-				cert, cerr := c.loadCert(c.certFile, c.keyFile)
-				if cerr != nil {
-					continue
 	}
-				c.Lock()
+	if keyFileIsLink && !certFileIsLink {
-				c.cert = &cert
+		return fmt.Errorf("certs: '%s' is a symlink but '%s' is a regular file", keyFile, certFile)
-				c.Unlock()
+	}
+
+	certificate, err := m.loadX509KeyPair(certFile, keyFile)
+	if err != nil {
+		return err
 	}
+	// We set the certificate leaf to the actual certificate such that
+	// we don't have to do the parsing (multiple times) when matching the
+	// certificate to the client hello. This a performance optimisation.
+	if certificate.Leaf == nil {
+		certificate.Leaf, err = x509.ParseCertificate(certificate.Certificate[0])
+		if err != nil {
+			return err
 		}
-	}()
-	return nil
 	}
 
-// watch starts watching for changes to the certificate
+	p := pair{
-// and key files. On any change the certificate and key
+		CertFile: certFile,
-// are reloaded. If there is an issue the loading will fail
+		KeyFile:  keyFile,
-// and the old (if any) certificates and keys will continue
+	}
-// to be used.
+	m.lock.Lock()
-func (c *Certs) watch() (err error) {
+	defer m.lock.Unlock()
-	defer func() {
+
-		if err != nil {
+	// We don't allow IP SANs in certificates - except for the "default" certificate
-			// Stop any watches previously setup after an error.
+	// which is, by convention, the first certificate added to the manager. The problem
-			notify.Stop(c.e)
+	// with allowing IP SANs in more than one certificate is that the manager usually can't
+	// match the client SNI to a SAN since the SNI is meant to communicate the destination
+	// host name and clients will not set the SNI to an IP address.
+	// Allowing multiple certificates with IP SANs lead to errors that confuses users - like:
+	// "It works for `https://instance.minio.local` but not for `https://10.0.2.1`"
+	if len(m.certificates) > 0 && len(certificate.Leaf.IPAddresses) > 0 {
+		return errors.New("cert: certificate must not contain any IP SANs: only the default certificate may contain IP SANs")
 	}
-	}()
+	m.certificates[p] = &certificate
 
+	if certFileIsLink && keyFileIsLink {
+		go m.watchSymlinks(certFile, keyFile)
+	} else {
 		// Windows doesn't allow for watching file changes but instead allows
 		// for directory changes only, while we can still watch for changes
 		// on files on other platforms. Watch parent directory on all platforms
 		// for simplicity.
-	if err = notify.Watch(filepath.Dir(c.certFile), c.e, eventWrite...); err != nil {
+		if err = notify.Watch(filepath.Dir(certFile), m.events, eventWrite...); err != nil {
 			return err
 		}
-	if err = notify.Watch(filepath.Dir(c.keyFile), c.e, eventWrite...); err != nil {
+		if err = notify.Watch(filepath.Dir(keyFile), m.events, eventWrite...); err != nil {
 			return err
 		}
-	cert, err := c.loadCert(c.certFile, c.keyFile)
+	}
+	return nil
+}
+
+// watchSymlinks starts an endless loop reloading the
+// certFile and keyFile periodically.
+func (m *Manager) watchSymlinks(certFile, keyFile string) {
+	for {
+		select {
+		case <-m.ctx.Done():
+			return // Once stopped exits this routine.
+		case <-time.After(24 * time.Hour):
+			certificate, err := m.loadX509KeyPair(certFile, keyFile)
 			if err != nil {
-		return err
+				continue
 			}
-	c.Lock()
+			if certificate.Leaf == nil { // This is a performance optimisation
-	c.cert = &cert
+				certificate.Leaf, err = x509.ParseCertificate(certificate.Certificate[0])
-	c.Unlock()
 				if err != nil {
-		return err
+					continue
 				}
-	go c.run()
-	return nil
 			}
 
-func (c *Certs) run() {
+			p := pair{
-	for event := range c.e {
+				CertFile: certFile,
-		base := filepath.Base(event.Path())
+				KeyFile:  keyFile,
-		if isWriteEvent(event.Event()) {
+			}
-			certChanged := base == filepath.Base(c.certFile)
+			m.lock.Lock()
-			keyChanged := base == filepath.Base(c.keyFile)
+			m.certificates[p] = &certificate
-			if certChanged || keyChanged {
+			m.lock.Unlock()
-				cert, err := c.loadCert(c.certFile, c.keyFile)
+		}
+	}
+}
+
+// watchFileEvents starts an endless loop waiting for file systems events.
+// Once an event occurs it reloads the private key and certificate that
+// has changed, if any.
+func (m *Manager) watchFileEvents() {
+	for {
+		select {
+		case <-m.ctx.Done():
+			return
+		case event := <-m.events:
+			if !isWriteEvent(event.Event()) {
+				continue
+			}
+
+			for pair := range m.certificates {
+				if p := event.Path(); pair.KeyFile == p || pair.CertFile == p {
+					certificate, err := m.loadX509KeyPair(pair.CertFile, pair.KeyFile)
+					if err != nil {
+						continue
+					}
+					if certificate.Leaf == nil { // This is performance optimisation
+						certificate.Leaf, err = x509.ParseCertificate(certificate.Certificate[0])
 						if err != nil {
-					// ignore the error continue to use
-					// old certificates.
 							continue
 						}
-				c.Lock()
+					}
-				c.cert = &cert
+					m.lock.Lock()
-				c.Unlock()
+					m.certificates[pair] = &certificate
+					m.lock.Unlock()
+				}
 			}
 		}
 	}
 }
 
-// GetCertificateFunc provides a GetCertificate type for custom client implementations.
+// GetCertificate returns a TLS certificate based on the client hello.
-type GetCertificateFunc func(hello *tls.ClientHelloInfo) (*tls.Certificate, error)
+//
+// It tries to find a certificate that would be accepted by the client
+// according to the client hello. However, if no certificate can be
+// found GetCertificate returns the certificate loaded from the
+// Public file.
+func (m *Manager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
+	m.lock.RLock()
+	defer m.lock.RUnlock()
+
+	// If the client does not send a SNI we return the "default"
+	// certificate. A client may not send a SNI - e.g. when trying
+	// to connect to an IP directly (https://<ip>:<port>).
+	//
+	// In this case we don't know which the certificate the client
+	// asks for. It may be a public-facing certificate issued by a
+	// public CA or an internal certificate containing internal domain
+	// names.
+	// Now, we should not serve "the first" certificate that would be
+	// accepted by the client based on the Client Hello. Otherwise, we
+	// may expose an internal certificate to the client that contains
+	// internal domain names. That way we would disclose internal
+	// infrastructure details.
+	//
+	// Therefore, we serve the "default" certificate - which by convention
+	// is the first certificate added to the Manager. It's the calling code's
+	// responsibility to ensure that the "public-facing" certificate is used
+	// when creating a Manager instance.
+	if hello.ServerName == "" {
+		certificate := m.certificates[m.defaultCert]
+		return certificate, nil
+	}
+
+	// Optimization: If there is just one certificate, always serve that one.
+	if len(m.certificates) == 1 {
+		for _, certificate := range m.certificates {
+			return certificate, nil
+		}
+	}
+
+	// Iterate over all certificates and return the first one that would
+	// be accepted by the peer (TLS client) based on the client hello.
+	// In particular, the client usually specifies the requested host/domain
+	// via SNI.
+	//
+	// Note: The certificate.Leaf should be non-nil and contain the actual
+	// client certificate of MinIO that should be presented to the peer (TLS client).
+	// Otherwise, the leaf certificate has to be parsed again - which is kind of
+	// expensive and may cause a performance issue. For more information, check the
+	// docs of tls.ClientHelloInfo.SupportsCertificate.
+	for _, certificate := range m.certificates {
+		if err := hello.SupportsCertificate(certificate); err == nil {
+			return certificate, nil
+		}
+	}
+	return nil, errors.New("certs: no server certificate is supported by peer")
+}
+
+// GetClientCertificate returns a TLS certificate for mTLS based on the
+// certificate request.
+//
+// It tries to find a certificate that would be accepted by the server
+// according to the certificate request. However, if no certificate can be
+// found GetClientCertificate returns the certificate loaded from the
+// Public file.
+func (m *Manager) GetClientCertificate(reqInfo *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+	m.lock.RLock()
+	defer m.lock.RUnlock()
 
-// GetCertificate returns the loaded certificate for use by
+	// Optimization: If there is just one certificate, always serve that one.
-// the TLSConfig fields GetCertificate field in a http.Server.
+	if len(m.certificates) == 1 {
-func (c *Certs) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
+		for _, certificate := range m.certificates {
-	c.RLock()
+			return certificate, nil
-	defer c.RUnlock()
+		}
-	return c.cert, nil
 	}
 
-// GetClientCertificate returns the loaded certificate for use by
+	// Iterate over all certificates and return the first one that would
-// the TLSConfig fields GetClientCertificate field in a http.Server.
+	// be accepted by the peer (TLS server) based on reqInfo.
-func (c *Certs) GetClientCertificate(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+	//
-	c.RLock()
+	// Note: The certificate.Leaf should be non-nil and contain the actual
-	defer c.RUnlock()
+	// client certificate of MinIO that should be presented to the peer (TLS server).
-	return c.cert, nil
+	// Otherwise, the leaf certificate has to be parsed again - which is kind of
+	// expensive and may cause a performance issue. For more information, check the
+	// docs of tls.CertificateRequestInfo.SupportsCertificate.
+	for _, certificate := range m.certificates {
+		if err := reqInfo.SupportsCertificate(certificate); err == nil {
+			return certificate, nil
+		}
+	}
+	return nil, errors.New("certs: no client certificate is supported by peer")
 }
 
-// Stop tells loader to stop watching for changes to the
+// isSymlink returns true if the given file
-// certificate and key files.
+// is a symbolic link.
-func (c *Certs) Stop() {
+func isSymlink(file string) (bool, error) {
-	if c != nil {
+	st, err := os.Lstat(file)
-		notify.Stop(c.e)
+	if err != nil {
+		return false, err
 	}
+	return st.Mode()&os.ModeSymlink == os.ModeSymlink, nil
 }

pkg/certs/certs_test.go

@@ -17,6 +17,7 @@
 package certs_test
 
 import (
+	"context"
 	"crypto/tls"
 	"io"
 	"os"
@@ -31,55 +32,57 @@ func updateCerts(crt, key string) {
 	// ignore error handling
 	crtSource, _ := os.Open(crt)
 	defer crtSource.Close()
-	crtDest, _ := os.Create("server.crt")
+	crtDest, _ := os.Create("public.crt")
 	defer crtDest.Close()
 	io.Copy(crtDest, crtSource)
 
 	keySource, _ := os.Open(key)
 	defer keySource.Close()
-	keyDest, _ := os.Create("server.key")
+	keyDest, _ := os.Create("private.key")
 	defer keyDest.Close()
 	io.Copy(keyDest, keySource)
 }
 
-func TestCertNew(t *testing.T) {
+func TestNewManager(t *testing.T) {
-	c, err := certs.New("server.crt", "server.key", tls.LoadX509KeyPair)
+	ctx, cancelFn := context.WithCancel(context.Background())
+	defer cancelFn()
+	c, err := certs.NewManager(ctx, "public.crt", "private.key", tls.LoadX509KeyPair)
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer c.Stop()
 	hello := &tls.ClientHelloInfo{}
 	gcert, err := c.GetCertificate(hello)
 	if err != nil {
 		t.Fatal(err)
 	}
-	expectedCert, err := tls.LoadX509KeyPair("server.crt", "server.key")
+	expectedCert, err := tls.LoadX509KeyPair("public.crt", "private.key")
 	if err != nil {
 		t.Fatal(err)
 	}
 	if !reflect.DeepEqual(gcert.Certificate, expectedCert.Certificate) {
 		t.Error("certificate doesn't match expected certificate")
 	}
-	_, err = certs.New("server.crt", "server2.key", tls.LoadX509KeyPair)
+	_, err = certs.NewManager(ctx, "public.crt", "new-private.key", tls.LoadX509KeyPair)
 	if err == nil {
 		t.Fatal("Expected to fail but got success")
 	}
 }
 
 func TestValidPairAfterWrite(t *testing.T) {
-	expectedCert, err := tls.LoadX509KeyPair("server2.crt", "server2.key")
+	ctx, cancelFn := context.WithCancel(context.Background())
+	defer cancelFn()
+	expectedCert, err := tls.LoadX509KeyPair("new-public.crt", "new-private.key")
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	c, err := certs.New("server.crt", "server.key", tls.LoadX509KeyPair)
+	c, err := certs.NewManager(ctx, "public.crt", "private.key", tls.LoadX509KeyPair)
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer c.Stop()
 
-	updateCerts("server2.crt", "server2.key")
+	updateCerts("new-public.crt", "new-private.key")
-	defer updateCerts("server1.crt", "server1.key")
+	defer updateCerts("original-public.crt", "original-private.key")
 
 	// Wait for the write event..
 	time.Sleep(200 * time.Millisecond)
@@ -104,31 +107,3 @@ func TestValidPairAfterWrite(t *testing.T) {
 		t.Error("client certificate doesn't match expected certificate")
 	}
 }
-
-func TestStop(t *testing.T) {
-	expectedCert, err := tls.LoadX509KeyPair("server2.crt", "server2.key")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	c, err := certs.New("server.crt", "server.key", tls.LoadX509KeyPair)
-	if err != nil {
-		t.Fatal(err)
-	}
-	c.Stop()
-
-	// No one is listening on the event, will be ignored and
-	// certificate will not be reloaded.
-	updateCerts("server2.crt", "server2.key")
-	defer updateCerts("server1.crt", "server1.key")
-
-	hello := &tls.ClientHelloInfo{}
-	gcert, err := c.GetCertificate(hello)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if reflect.DeepEqual(gcert.Certificate, expectedCert.Certificate) {
-		t.Error("certificate shouldn't match, but matched")
-	}
-}

pkg/certs/private.key

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDPszxaYwn+mIz6
+IGuUlmvWwUs/yWTH4MC17qey2N5MqcxlfIWHUugcBsbGhi/e1druFW0s7YGMxp+G
++Q1IezxX+VmVaJCN8AgSowbYgpRdpRQ+mhGeQby0JcvO16fyPnUJBz3GGel2bcK8
+fcQyT0TVapCiD9oURVmdvDSsRXz+EoPlOve8AWciHHgm1ItO5qdPRP5YtcJfLiwK
+noYnpda2d9SzmYk+Q2JFArooF7/A1DYz9bXCMo3qp0gQlMpSMDR+MCbxHBzBBr+f
+QG8QdDrzWQ2slhniBhFDk0LuPCBLlSeIzkp+DoAGDXf3hWYhechlabZ7nfngg5er
+Ez776WCFAgMBAAECggEBAJcHRyCWmcLm3MRY5MF0K9BKV9R3NnBdTuQ8OPdE2Ui3
+w6gcRuBi+eK/TrU3CAIqUXsEW5Hq1mQuXfwAh5cn/XYfG/QXx91eKBCdOTIgqY/6
+pODsmVkRhg0c2rl6eWYd4m6BNHsjhm8WWx9C+HJ4z528UpV1n2dUElkvbMHD+aKp
+Ndwd0W+0PCn/BjMn/sdyy01f8sfaK2Zoy7HBw/fGeBDNLFFj3Iz7BqXYeS+OyfLN
+B4xD5I5fFqt1iJeyqVPzGkOAYSqisijbM1GtZJCeVp37/+IDylCKTO3l8Xd8x73U
+qTYcYT3heSHyUC2xCM6Va2YkSrOHeqbq91QgHh9LVrUCgYEA9t/wE2S8TE2l1IG9
+68SXdhyaXTnB2qSL7ggY0uazPzBNLQpNMOxicZ6/4QGEi3hSuCqGxxGo9UEoTsVd
+pk8oIeDULdPVi4NQxSmkxUyArs/dzOMygUPyosOiEc8z6jWFFKDcQ7mnZnay8dZ4
+e4j+/hZDONtDrJ+zH2xu98ZrJPcCgYEA12CbSRbCkTiRj/dq8Qvgp6+ceTVcAbnk
+MWpAhZQaXHrG3XP0L7QTIHG/7a09Mln92zjuAFXDp/Vc5NdxeXcnj9j6oUAxq+0I
+dq+vibzjROemmvnmQvXGY9tc0ns6u7GjM0+Sicmas+IH4vuum/aRasABfVe2XBwe
+4fVs0n7yU2MCgYA7KevFGg0uVCV7yiQTzqdlvPEZim/00B5gyzv3vyYR7KdyNdfN
+87ib9imR6OU0738Td82ZA5h0PktEpXQOGUZK6DCxUuUIbE39Ej/UsMLeIh7LrV87
+L2eErlG25utQI8di7DIdYO7HVYcJAhcZs/k4N2mgxJtxUUyCKWBmrPycfQKBgAo7
+0uUUKcaQs4ntra0qbVBKbdrsiCSk2ozmiY5PTTlbtBtNqSqjGc2O2hnHA4Ni90b1
+W4m0iYlvhSxyeDfXS4/wNWh4DmQm7SIGkwaubPYXM7llamWAHB8eiziNFmtYs3J6
+s3HMnIczlEBayR8sBhjWaruz8TxLMcR2zubplUYVAoGBAItxeC9IT8BGJoZB++qM
+f2LXCqJ383x0sDHhwPMFPtwUTzAwc5BJgQe9zFktW5CBxsER+MnUZjlrarT1HQfH
+1Y1mJQXtwuBKG4pPPZphH0yoVlYcWkBTMw/KmlVlwRclEzRQwV3TPD+i6ieKeZhz
+9eZwhS3H+Zb/693WbBDyH8L+
+-----END PRIVATE KEY-----

pkg/certs/public.crt

@@ -0,0 +1,22 @@
+-----BEGIN CERTIFICATE-----
+MIIDqjCCApKgAwIBAgIJAOcv4FsrflS4MA0GCSqGSIb3DQEBCwUAMGoxCzAJBgNV
+BAYTAlVTMQswCQYDVQQIDAJDQTEVMBMGA1UEBwwMUmVkd29vZCBDaXR5MQ4wDAYD
+VQQKDAVNaW5pbzEUMBIGA1UECwwLRW5naW5lZXJpbmcxETAPBgNVBAMMCG1pbmlv
+LmlvMB4XDTE4MDUyMDA4NDc0MFoXDTE5MDUyMDA4NDc0MFowajELMAkGA1UEBhMC
+VVMxCzAJBgNVBAgMAkNBMRUwEwYDVQQHDAxSZWR3b29kIENpdHkxDjAMBgNVBAoM
+BU1pbmlvMRQwEgYDVQQLDAtFbmdpbmVlcmluZzERMA8GA1UEAwwIbWluaW8uaW8w
+ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDPszxaYwn+mIz6IGuUlmvW
+wUs/yWTH4MC17qey2N5MqcxlfIWHUugcBsbGhi/e1druFW0s7YGMxp+G+Q1IezxX
++VmVaJCN8AgSowbYgpRdpRQ+mhGeQby0JcvO16fyPnUJBz3GGel2bcK8fcQyT0TV
+apCiD9oURVmdvDSsRXz+EoPlOve8AWciHHgm1ItO5qdPRP5YtcJfLiwKnoYnpda2
+d9SzmYk+Q2JFArooF7/A1DYz9bXCMo3qp0gQlMpSMDR+MCbxHBzBBr+fQG8QdDrz
+WQ2slhniBhFDk0LuPCBLlSeIzkp+DoAGDXf3hWYhechlabZ7nfngg5erEz776WCF
+AgMBAAGjUzBRMB0GA1UdDgQWBBRzC09a+3AlbFDg6BsvELolmO8jYjAfBgNVHSME
+GDAWgBRzC09a+3AlbFDg6BsvELolmO8jYjAPBgNVHRMBAf8EBTADAQH/MA0GCSqG
+SIb3DQEBCwUAA4IBAQBl0cx7qbidKjhoZ1Iv4pCD8xHZgtuWEDApPoGuMtVS66jJ
++oj0ncD5xCtv9XqXtshE65FIsEWnDOIwa+kyjMnxHbFwxveWBT4W0twtqwbVs7NE
+I0So6cEmSx4+rB0XorY6mIbD3O9YAStelNhB1jVfQfIMSByYkcGq2Fh+B1LHlOrz
+06LJdwYMiILzK0c5fvjZvsDq/9EK+Xo66hphKjs5cl1t9WK7wKOCoZDt2lOTZqEq
+UWYGPWlTAxSWQxO4WnvSKqFdsRi8fOO3KlDq1eNqeDSGGCI0DTGgJxidHIpfOPEF
+s/zojgc5npE32/1n8og6gLcv7LIKelBfMhUrFTp7
+-----END CERTIFICATE-----

pkg/event/target/webhook.go

@@ -215,10 +215,8 @@ func (target *WebhookTarget) Close() error {
 }
 
 // NewWebhookTarget - creates new Webhook target.
-func NewWebhookTarget(id string, args WebhookArgs, doneCh <-chan struct{}, loggerOnce func(ctx context.Context, err error, id interface{}, kind ...interface{}), transport *http.Transport, test bool) (*WebhookTarget, error) {
+func NewWebhookTarget(ctx context.Context, id string, args WebhookArgs, loggerOnce func(ctx context.Context, err error, id interface{}, kind ...interface{}), transport *http.Transport, test bool) (*WebhookTarget, error) {
-
 	var store Store
-
 	target := &WebhookTarget{
 		id:         event.TargetID{ID: id, Name: "webhook"},
 		args:       args,
@@ -226,11 +224,11 @@ func NewWebhookTarget(id string, args WebhookArgs, doneCh <-chan struct{}, logge
 	}
 
 	if target.args.ClientCert != "" && target.args.ClientKey != "" {
-		c, err := certs.New(target.args.ClientCert, target.args.ClientKey, tls.LoadX509KeyPair)
+		manager, err := certs.NewManager(ctx, target.args.ClientCert, target.args.ClientKey, tls.LoadX509KeyPair)
 		if err != nil {
 			return target, err
 		}
-		transport.TLSClientConfig.GetClientCertificate = c.GetClientCertificate
+		transport.TLSClientConfig.GetClientCertificate = manager.GetClientCertificate
 	}
 	target.httpClient = &http.Client{Transport: transport}
 
@@ -247,16 +245,16 @@ func NewWebhookTarget(id string, args WebhookArgs, doneCh <-chan struct{}, logge
 	_, err := target.IsActive()
 	if err != nil {
 		if target.store == nil || err != errNotConnected {
-			target.loggerOnce(context.Background(), err, target.ID())
+			target.loggerOnce(ctx, err, target.ID())
 			return target, err
 		}
 	}
 
 	if target.store != nil && !test {
 		// Replays the events from the store.
-		eventKeyCh := replayEvents(target.store, doneCh, target.loggerOnce, target.ID())
+		eventKeyCh := replayEvents(target.store, ctx.Done(), target.loggerOnce, target.ID())
 		// Start replaying events from the store.
-		go sendEvents(target, eventKeyCh, doneCh, target.loggerOnce)
+		go sendEvents(target, eventKeyCh, ctx.Done(), target.loggerOnce)
 	}
 
 	return target, nil