From fbd1c5f51a900158013e8d8e4593d9ca898f8b7e Mon Sep 17 00:00:00 2001 From: Andreas Auernhammer Date: Fri, 4 Sep 2020 08:33:37 +0200 Subject: [PATCH] certs: refactor cert manager to support multiple certificates (#10207) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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://:`). 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 --- cmd/admin-handlers.go | 2 +- cmd/common-main.go | 58 ++- cmd/config-current.go | 7 +- cmd/config/notify/parse.go | 35 +- cmd/gateway-main.go | 3 - cmd/globals.go | 2 +- cmd/server-main.go | 3 - cmd/signals.go | 3 - pkg/certs/certs.go | 402 ++++++++++++------ pkg/certs/certs_test.go | 55 +-- pkg/certs/{server2.key => new-private.key} | 0 pkg/certs/{server2.crt => new-public.crt} | 0 .../{server1.key => original-private.key} | 0 .../{server1.crt => original-public.crt} | 0 pkg/certs/private.key | 28 ++ pkg/certs/public.crt | 22 + pkg/event/target/webhook.go | 14 +- 17 files changed, 421 insertions(+), 213 deletions(-) rename pkg/certs/{server2.key => new-private.key} (100%) rename pkg/certs/{server2.crt => new-public.crt} (100%) rename pkg/certs/{server1.key => original-private.key} (100%) rename pkg/certs/{server1.crt => original-public.crt} (100%) create mode 100644 pkg/certs/private.key create mode 100644 pkg/certs/public.crt 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