From 6e768e8481ea13482428c70f90c21d842721d671 Mon Sep 17 00:00:00 2001 From: Colin Sullivan Date: Thu, 22 Feb 2024 15:41:01 -0700 Subject: [PATCH] updates based on feedback Signed-off-by: Colin Sullivan --- server/certstore/certstore_other.go | 2 +- server/certstore/certstore_windows.go | 41 ++++++++++++------- server/certstore_windows_test.go | 4 +- server/opts.go | 2 +- .../certs/tlsauth/certstore/update_keys.bat | 4 -- 5 files changed, 31 insertions(+), 22 deletions(-) delete mode 100644 test/configs/certs/tlsauth/certstore/update_keys.bat diff --git a/server/certstore/certstore_other.go b/server/certstore/certstore_other.go index e6e8aee73d..32cd7cc029 100644 --- a/server/certstore/certstore_other.go +++ b/server/certstore/certstore_other.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 The NATS Authors +// Copyright 2022-2024 The NATS Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at diff --git a/server/certstore/certstore_windows.go b/server/certstore/certstore_windows.go index 13d9548f88..6e6e24dc17 100644 --- a/server/certstore/certstore_windows.go +++ b/server/certstore/certstore_windows.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 The NATS Authors +// Copyright 2022-2024 The NATS Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -111,7 +111,13 @@ var ( crypto.SHA512: winWide("SHA512"), // BCRYPT_SHA512_ALGORITHM } - // MY is well-known system store on Windows that holds personal certificates + // MY is well-known system store on Windows that holds personal certificates. Read + // More about the CA locations here: + // https://learn.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/wcf/certificate-of-clientcertificate-element?redirectedfrom=MSDN + // https://superuser.com/questions/217719/what-are-the-windows-system-certificate-stores + // https://docs.microsoft.com/en-us/windows/win32/seccrypto/certificate-stores + // https://learn.microsoft.com/en-us/windows/win32/seccrypto/system-store-locations + // https://stackoverflow.com/questions/63286085/which-x509-storename-refers-to-the-certificates-stored-beneath-trusted-root-cert#:~:text=4-,StoreName.,is%20%22Intermediate%20Certification%20Authorities%22. winMyStore = winWide("MY") winIntermediateCAStore = winWide("CA") winRootStore = winWide("Root") @@ -146,31 +152,33 @@ type winPSSPaddingInfo struct { // request. If no certificates are found an error is returned. func createCACertsPool(cs *winCertStore, storeType uint32, caCertsMatch []string) (*x509.CertPool, error) { var errs []error - var count int caPool := x509.NewCertPool() for _, s := range caCertsMatch { lfs, err := cs.caCertsBySubjectMatch(s, storeType) if err != nil { - if errs == nil { - errs = make([]error, 0) - } errs = append(errs, err) } else { for _, lf := range lfs { caPool.AddCert(lf) - count++ } } } - // If no certs were added, return the errors. - if count == 0 { + // If every lookup failed return the errors. + if len(errs) == len(caCertsMatch) { return nil, fmt.Errorf("unable to match any CA certificate: %v", errs) } return caPool, nil } -// TLSConfig fulfills the same function as reading cert and key pair from pem files but -// sources the Windows certificate store instead +// TLSConfig fulfills the same function as reading cert and key pair from +// pem files but sources the Windows certificate store instead. The +// certMatchBy and certMatch fields search the "MY" certificate location +// for the first certificate that matches the certMatch field. The +// caCertsMatch field is used to search the Trusted Root, Third Party Root, +// and Intermediate Certificate Authority locations for certificates with +// Subjects matching the provided strings. If a match is found, the +// certificate is added to the pool that is used to verify the certificate +// chain. func TLSConfig(certStore StoreType, certMatchBy MatchByType, certMatch string, caCertsMatch []string, config *tls.Config) error { var ( leaf *x509.Certificate @@ -219,7 +227,7 @@ func TLSConfig(certStore StoreType, certMatchBy MatchByType, certMatch string, c return ErrNoPrivateKeyStoreRef } // Look for CA Certificates - if caCertsMatch != nil { + if len(caCertsMatch) != 0 { caPool, err := createCACertsPool(cs, scope, caCertsMatch) if err != nil { return err @@ -372,17 +380,22 @@ func (w *winCertStore) caCertsBySubjectMatch(subject string, storeType uint32) ( var ( leaf *x509.Certificate searchLocations = [3]*uint16{winRootStore, winAuthRootStore, winIntermediateCAStore} + rv []*x509.Certificate ) // surprisingly, an empty string returns a result. We'll treat this as an error. if subject == "" { - return nil, ErrFailedCertSearch + return nil, ErrBadCaCertMatchField } - rv := make([]*x509.Certificate, 0) for _, sr := range searchLocations { var err error if leaf, _, err = w.certSearch(winFindSubjectStr, subject, sr, storeType); err == nil { rv = append(rv, leaf) } else { + // Ignore the failed search from a single location. Errors we catch include + // ErrFailedX509Extract (resulting from a malformed certificate) and errors + // around invalid attributes, unsupported algorithms, etc. These are corner + // cases as certificates with these errors shouldn't have been allowed + // to be added to the store in the first place. if err != ErrFailedCertSearch { return nil, err } diff --git a/server/certstore_windows_test.go b/server/certstore_windows_test.go index 2cb8bc69ef..93ea441f57 100644 --- a/server/certstore_windows_test.go +++ b/server/certstore_windows_test.go @@ -1,4 +1,4 @@ -// Copyright 2022-2023 The NATS Authors +// Copyright 2022-2024 The NATS Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -61,7 +61,7 @@ func runConfiguredLeaf(t *testing.T, hubPort int, certStore string, matchBy stri cert_match: "%s" ca_certs_match: %s - # Above should be equivalent to: + # Test settings that succeed should be equivalent to: # cert_file: "../test/configs/certs/tlsauth/client.pem" # key_file: "../test/configs/certs/tlsauth/client-key.pem" # ca_file: "../test/configs/certs/tlsauth/ca.pem" diff --git a/server/opts.go b/server/opts.go index 0df33eb031..3368c73c9d 100644 --- a/server/opts.go +++ b/server/opts.go @@ -1,4 +1,4 @@ -// Copyright 2012-2023 The NATS Authors +// Copyright 2012-2024 The NATS Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at diff --git a/test/configs/certs/tlsauth/certstore/update_keys.bat b/test/configs/certs/tlsauth/certstore/update_keys.bat deleted file mode 100644 index 8b3b3df680..0000000000 --- a/test/configs/certs/tlsauth/certstore/update_keys.bat +++ /dev/null @@ -1,4 +0,0 @@ -REM C:\Progra~1\Git\usr\bin\openssl pkcs12 -export -nokeys -in ..\ca.pem -out ca.p12 -C:\Progra~1\Git\usr\bin\openssl pkcs12 -export -inkey ..\client-key.pem -in ..\client.pem -out client.p12 -C:\Progra~1\Git\usr\bin\openssl pkcs12 -export -inkey ..\server-key.pem -in ..\server.pem -out server.p12 -C:\Progra~1\Git\usr\bin\openssl pkcs12 -export -nokeys -in ..\ca.pem -out ca.p12 \ No newline at end of file