diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index 0940ef27f..41058a31c 100644 --- a/cmd/admin-handlers.go +++ b/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 diff --git a/cmd/common-main.go b/cmd/common-main.go index f1d2ec89f..d9bac566f 100644 --- a/cmd/common-main.go +++ b/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 } diff --git a/cmd/config-current.go b/cmd/config-current.go index 80dc177db..7f5e2c1bc 100644 --- a/cmd/config-current.go +++ b/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(), - globalNotificationSys.ConfiguredTargetIDs()) + return notify.TestNotificationTargets(GlobalContext, s, NewGatewayHTTPTransport(), 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)) } diff --git a/cmd/config/notify/parse.go b/cmd/config/notify/parse.go index 4af746fc6..25764af38 100644 --- a/cmd/config/notify/parse.go +++ b/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, - targetIDs []event.TargetID) error { +func TestNotificationTargets(ctx context.Context, cfg config.Config, transport *http.Transport, 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.. // * 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) { - targetList, err := FetchRegisteredTargets(cfg, doneCh, transport, test, returnOnTargetError) +func RegisterNotificationTargets(ctx context.Context, cfg config.Config, transport *http.Transport, targetIDs []event.TargetID, test bool, returnOnTargetError bool) (*event.TargetList, error) { + 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 { diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index 3c237ffcc..f5863972e 100644 --- a/cmd/gateway-main.go +++ b/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") } diff --git a/cmd/globals.go b/cmd/globals.go index 35ac67467..c478e2294 100644 --- a/cmd/globals.go +++ b/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) diff --git a/cmd/server-main.go b/cmd/server-main.go index b3266dfe4..595b905e4 100644 --- a/cmd/server-main.go +++ b/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") } diff --git a/cmd/signals.go b/cmd/signals.go index 82b9639b9..b1e08862f 100644 --- a/cmd/signals.go +++ b/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) diff --git a/pkg/certs/certs.go b/pkg/certs/certs.go index 1888db90f..2ba70be98 100644 --- a/pkg/certs/certs.go +++ b/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 -// and key pairs for changes. -type Certs struct { - sync.RWMutex - // user input params. - certFile string - keyFile string - loadCert LoadX509KeyPairFunc - - // points to the latest certificate. - cert *tls.Certificate - - // internal param to track for events, also - // used to close the watcher. - e chan notify.EventInfo +// LoadX509KeyPairFunc is a function that parses a private key and +// certificate file and returns a TLS certificate on success. +type LoadX509KeyPairFunc func(certFile, keyFile string) (tls.Certificate, error) + +// GetCertificateFunc is a callback that allows a TLS stack deliver different +// certificates based on the client trying to establish a TLS connection. +// +// 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) + +// Manager is a TLS certificate manager that can handle multiple certificates. +// 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 + + loadX509KeyPair LoadX509KeyPairFunc + events chan notify.EventInfo + ctx context.Context } -// LoadX509KeyPairFunc - provides a type for custom cert loader function. -type LoadX509KeyPairFunc func(certFile, keyFile string) (tls.Certificate, error) +// pair represents a certificate and private key file tuple. +type pair struct { + KeyFile string + CertFile string +} -// New initializes a new certs monitor. -func New(certFile, keyFile string, loadCert LoadX509KeyPairFunc) (*Certs, error) { - certFileIsLink, err := checkSymlink(certFile) +// NewManager returns a new Manager that handles one certificate specified via +// the certFile and keyFile. It will use the loadX509KeyPair function to (re)load +// 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 { - if err := c.watchSymlinks(); err != nil { - return nil, err - } - } else { - if err := c.watch(); err != nil { - return nil, err - } + manager = &Manager{ + certificates: map[pair]*tls.Certificate{}, + defaultCert: pair{ + KeyFile: keyFile, + CertFile: certFile, + }, + loadX509KeyPair: loadX509KeyPair, + events: make(chan notify.EventInfo, 1), + ctx: ctx, } - - return c, nil -} - -func checkSymlink(file string) (bool, error) { - st, err := os.Lstat(file) - if err != nil { - return false, err + if err := manager.AddCertificate(certFile, keyFile); err != nil { + return nil, err } - return st.Mode()&os.ModeSymlink == os.ModeSymlink, nil + go manager.watchFileEvents() + return manager, nil } -// 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) +// 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 } - c.Lock() - c.cert = &cert - 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() - c.cert = &cert - c.Unlock() - } - } - }() - return nil -} - -// watch starts watching for changes to the certificate -// and key files. On any change the certificate and key -// are reloaded. If there is an issue the loading will fail -// and the old (if any) certificates and keys will continue -// to be used. -func (c *Certs) watch() (err error) { - defer func() { - if err != nil { - // Stop any watches previously setup after an error. - notify.Stop(c.e) - } - }() - - // 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 { + keyFile, err = filepath.Abs(keyFile) + if err != nil { return err } - if err = notify.Watch(filepath.Dir(c.keyFile), c.e, eventWrite...); err != nil { + certFileIsLink, err := isSymlink(certFile) + if err != nil { return err } - cert, err := c.loadCert(c.certFile, c.keyFile) + keyFileIsLink, err := isSymlink(keyFile) if err != nil { return err } - c.Lock() - c.cert = &cert - c.Unlock() + if certFileIsLink && !keyFileIsLink { + return fmt.Errorf("certs: '%s' is a symlink but '%s' is a regular file", certFile, keyFile) + } + if keyFileIsLink && !certFileIsLink { + return fmt.Errorf("certs: '%s' is a symlink but '%s' is a regular file", keyFile, certFile) + } + + certificate, err := m.loadX509KeyPair(certFile, keyFile) if err != nil { return err } - go c.run() + // 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 + } + } + + p := pair{ + CertFile: certFile, + KeyFile: keyFile, + } + m.lock.Lock() + defer m.lock.Unlock() + + // We don't allow IP SANs in certificates - except for the "default" certificate + // which is, by convention, the first certificate added to the manager. The problem + // 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(certFile), m.events, eventWrite...); err != nil { + return err + } + if err = notify.Watch(filepath.Dir(keyFile), m.events, eventWrite...); err != nil { + return err + } + } return nil } -func (c *Certs) run() { - for event := range c.e { - base := filepath.Base(event.Path()) - if isWriteEvent(event.Event()) { - certChanged := base == filepath.Base(c.certFile) - keyChanged := base == filepath.Base(c.keyFile) - if certChanged || keyChanged { - cert, err := c.loadCert(c.certFile, c.keyFile) +// 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 { + continue + } + if certificate.Leaf == nil { // This is a 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 - c.Unlock() } + + p := pair{ + CertFile: certFile, + KeyFile: keyFile, + } + m.lock.Lock() + m.certificates[p] = &certificate + m.lock.Unlock() } } } -// GetCertificateFunc provides a GetCertificate type for custom client implementations. -type GetCertificateFunc func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) +// 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 { + continue + } + } + m.lock.Lock() + m.certificates[pair] = &certificate + m.lock.Unlock() + } + } + } + } +} + +// GetCertificate returns a TLS certificate based on the client hello. +// +// 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://:). + // + // 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 + } + } -// GetCertificate returns the loaded certificate for use by -// the TLSConfig fields GetCertificate field in a http.Server. -func (c *Certs) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - c.RLock() - defer c.RUnlock() - return c.cert, 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 the loaded certificate for use by -// the TLSConfig fields GetClientCertificate field in a http.Server. -func (c *Certs) GetClientCertificate(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) { - c.RLock() - defer c.RUnlock() - return c.cert, nil +// 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() + + // 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 server) based on reqInfo. + // + // Note: The certificate.Leaf should be non-nil and contain the actual + // client certificate of MinIO that should be presented to the peer (TLS server). + // 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 -// certificate and key files. -func (c *Certs) Stop() { - if c != nil { - notify.Stop(c.e) +// isSymlink returns true if the given file +// is a symbolic link. +func isSymlink(file string) (bool, error) { + st, err := os.Lstat(file) + if err != nil { + return false, err } + return st.Mode()&os.ModeSymlink == os.ModeSymlink, nil } diff --git a/pkg/certs/certs_test.go b/pkg/certs/certs_test.go index 2eeaac44e..0f05abbb5 100644 --- a/pkg/certs/certs_test.go +++ b/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) { - c, err := certs.New("server.crt", "server.key", tls.LoadX509KeyPair) +func TestNewManager(t *testing.T) { + 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") - defer updateCerts("server1.crt", "server1.key") + updateCerts("new-public.crt", "new-private.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") - } -} diff --git a/pkg/certs/server2.key b/pkg/certs/new-private.key similarity index 100% rename from pkg/certs/server2.key rename to pkg/certs/new-private.key diff --git a/pkg/certs/server2.crt b/pkg/certs/new-public.crt similarity index 100% rename from pkg/certs/server2.crt rename to pkg/certs/new-public.crt diff --git a/pkg/certs/server1.key b/pkg/certs/original-private.key similarity index 100% rename from pkg/certs/server1.key rename to pkg/certs/original-private.key diff --git a/pkg/certs/server1.crt b/pkg/certs/original-public.crt similarity index 100% rename from pkg/certs/server1.crt rename to pkg/certs/original-public.crt diff --git a/pkg/certs/private.key b/pkg/certs/private.key new file mode 100644 index 000000000..25c46cefd --- /dev/null +++ b/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----- diff --git a/pkg/certs/public.crt b/pkg/certs/public.crt new file mode 100644 index 000000000..f0d8d2d91 --- /dev/null +++ b/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----- diff --git a/pkg/event/target/webhook.go b/pkg/event/target/webhook.go index 7cfa62f53..0181aa244 100644 --- a/pkg/event/target/webhook.go +++ b/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