From 145d1e32cd39e4674e1c311f5b394db759bfee7d Mon Sep 17 00:00:00 2001 From: JeremyRand Date: Wed, 14 Feb 2018 05:03:04 +0000 Subject: [PATCH] certinject: Fix various issues found by static analysis. --- certinject/certinject.go | 11 +++++++---- certinject/certinject_linux.go | 8 +++++++- certinject/certinject_notlinux.go | 5 ++++- certinject/certinject_notwindows.go | 18 +++++++++--------- certinject/certinject_windows.go | 21 ++++++++++++++------- certinject/cryptoapi_windows.go | 3 ++- certinject/nss.go | 28 ++++++++++++++++++++-------- certinject/nss_notwindows.go | 2 +- certinject/nss_windows.go | 2 +- 9 files changed, 65 insertions(+), 33 deletions(-) diff --git a/certinject/certinject.go b/certinject/certinject.go index 513a9a0..c4da3be 100644 --- a/certinject/certinject.go +++ b/certinject/certinject.go @@ -1,11 +1,14 @@ package certinject import ( - "gopkg.in/hlandau/easyconfig.v1/cflag" + "gopkg.in/hlandau/easyconfig.v1/cflag" ) var ( - flagGroup = cflag.NewGroup(nil, "certstore") - nssFlag = cflag.Bool(flagGroup, "nss", false, nssExplain) - certExpirePeriod = cflag.Int(flagGroup, "expire", 60 * 30, "Duration (in seconds) after which TLS certs will be removed from the trust store. Making this smaller than the DNS TTL (default 600) may cause TLS errors.") + flagGroup = cflag.NewGroup(nil, "certstore") + nssFlag = cflag.Bool(flagGroup, "nss", false, nssExplain) + certExpirePeriod = cflag.Int(flagGroup, "expire", 60*30, "Duration "+ + "(in seconds) after which TLS certs will be removed from the "+ + "trust store. Making this smaller than the DNS TTL (default "+ + "600) may cause TLS errors.") ) diff --git a/certinject/certinject_linux.go b/certinject/certinject_linux.go index fb175f4..ede3d1c 100644 --- a/certinject/certinject_linux.go +++ b/certinject/certinject_linux.go @@ -1,5 +1,11 @@ package certinject var ( - nssExplain = "Synchronize TLS certs to an NSS sqlite3 trust store? This enables HTTPS to work with NSS web browsers such as Chromium/Chrome. Only use if you've set up NUMS HPKP in Chromium/Chrome as per documentation. If you haven't set up NUMS HPKP, or if you access the configured NSS sqlite3 trust store from browsers not based on Chromium, this is unsafe and should not be used." + nssExplain = "Synchronize TLS certs to an NSS sqlite3 trust store? " + + "This enables HTTPS to work with NSS web browsers such as " + + "Chromium/Chrome. Only use if you've set up NUMS HPKP in " + + "Chromium/Chrome as per documentation. If you haven't set " + + "up NUMS HPKP, or if you access the configured NSS sqlite3 " + + "trust store from browsers not based on Chromium, this is " + + "unsafe and should not be used." ) diff --git a/certinject/certinject_notlinux.go b/certinject/certinject_notlinux.go index 5368a65..28d7f19 100644 --- a/certinject/certinject_notlinux.go +++ b/certinject/certinject_notlinux.go @@ -3,5 +3,8 @@ package certinject var ( - nssExplain = "(Unsafe and experimental!) Synchronize TLS certs to an NSS sqlite3 trust store? This enables HTTPS to work with some NSS-based software. This is currently unsafe and should not be used." + nssExplain = "(Unsafe and experimental!) Synchronize TLS certs to " + + "an NSS sqlite3 trust store? This enables HTTPS to work " + + "with some NSS-based software. This is currently unsafe " + + "and should not be used." ) diff --git a/certinject/certinject_notwindows.go b/certinject/certinject_notwindows.go index 23f7f51..ee70a3e 100644 --- a/certinject/certinject_notwindows.go +++ b/certinject/certinject_notwindows.go @@ -4,25 +4,25 @@ package certinject import "github.com/hlandau/xlog" -// This package is used to add and remove certificates to the system trust +// This package is used to add and remove certificates to the system trust // store. // Currently only supports NSS sqlite3 stores. var log, Log = xlog.New("ncdns.certinject") -// Injects the given cert into all configured trust stores. +// InjectCert injects the given cert into all configured trust stores. func InjectCert(derBytes []byte) { - if nssFlag.Value() { - injectCertNss(derBytes) - } + if nssFlag.Value() { + injectCertNss(derBytes) + } } -// Cleans expired certs from all configured trust stores. +// CleanCerts cleans expired certs from all configured trust stores. func CleanCerts() { - if nssFlag.Value() { - cleanCertsNss() - } + if nssFlag.Value() { + cleanCertsNss() + } } diff --git a/certinject/certinject_windows.go b/certinject/certinject_windows.go index 288b8ad..2670ff6 100644 --- a/certinject/certinject_windows.go +++ b/certinject/certinject_windows.go @@ -12,28 +12,35 @@ import ( var log, Log = xlog.New("ncdns.certinject") var ( - cryptoApiFlag = cflag.Bool(flagGroup, "cryptoapi", false, "Synchronize TLS certs to the CryptoAPI trust store? This enables HTTPS to work with Chromium/Chrome. Only use if you've set up NUMS HPKP in Chromium/Chrome as per documentation. If you haven't set up NUMS HPKP, or if you access ncdns from browsers not based on Chromium or Firefox, this is unsafe and should not be used.") + cryptoApiFlag = cflag.Bool(flagGroup, "cryptoapi", false, + "Synchronize TLS certs to the CryptoAPI trust store? This "+ + "enables HTTPS to work with Chromium/Chrome. Only "+ + "use if you've set up NUMS HPKP in Chromium/Chrome "+ + "as per documentation. If you haven't set up NUMS "+ + "HPKP, or if you access ncdns from browsers not "+ + "based on Chromium or Firefox, this is unsafe and "+ + "should not be used.") ) -// Injects the given cert into all configured trust stores. +// InjectCert injects the given cert into all configured trust stores. func InjectCert(derBytes []byte) { if cryptoApiFlag.Value() { injectCertCryptoApi(derBytes) } if nssFlag.Value() { - injectCertNss(derBytes) - } + injectCertNss(derBytes) + } } -// Cleans expired certs from all configured trust stores. +// CleanCerts cleans expired certs from all configured trust stores. func CleanCerts() { if cryptoApiFlag.Value() { cleanCertsCryptoApi() } if nssFlag.Value() { - cleanCertsNss() - } + cleanCertsNss() + } } diff --git a/certinject/cryptoapi_windows.go b/certinject/cryptoapi_windows.go index 5451e50..5e52106 100644 --- a/certinject/cryptoapi_windows.go +++ b/certinject/cryptoapi_windows.go @@ -4,10 +4,11 @@ import ( "crypto/sha1" "encoding/hex" "fmt" - "golang.org/x/sys/windows/registry" "math" "strings" "time" + + "golang.org/x/sys/windows/registry" ) // In 64-bit Windows, this key is shared between 64-bit and 32-bit applications. diff --git a/certinject/nss.go b/certinject/nss.go index a6c54ed..63a4e8d 100644 --- a/certinject/nss.go +++ b/certinject/nss.go @@ -10,8 +10,11 @@ import "math" import "time" import "gopkg.in/hlandau/easyconfig.v1/cflag" -var certDir = cflag.String(flagGroup, "nsscertdir", "", "Directory to store certificate files. Only use a directory that only ncdns can write to. (Required if nss is set.)") -var nssDir = cflag.String(flagGroup, "nssdbdir", "", "Directory that contains NSS's cert9.db. (Required if nss is set.)") +var certDir = cflag.String(flagGroup, "nsscertdir", "", "Directory to store "+ + "certificate files. Only use a directory that only ncdns can write "+ + "to. (Required if nss is set.)") +var nssDir = cflag.String(flagGroup, "nssdbdir", "", "Directory that "+ + "contains NSS's cert9.db. (Required if nss is set.)") func injectCertNss(derBytes []byte) { @@ -33,7 +36,8 @@ func injectCertNss(derBytes []byte) { nickname := nicknameFromFingerprintHexNss(fingerprintHex) // TODO: check whether we can replace CP with just P. - cmd := exec.Command(nssCertutilName, "-d", "sql:" + nssDir.Value(), "-A", "-t", "CP,,", "-n", nickname, "-a", "-i", path) + cmd := exec.Command(nssCertutilName, "-d", "sql:"+nssDir.Value(), "-A", + "-t", "CP,,", "-n", nickname, "-a", "-i", path) err := cmd.Run() if err != nil { @@ -67,12 +71,15 @@ func cleanCertsNss() { filename := f.Name() - fingerprintHex := strings.Replace(filename, ".pem", "", -1) + fingerprintHex := strings.Replace(filename, ".pem", "", + -1) - nickname := nicknameFromFingerprintHexNss(fingerprintHex) + nickname := nicknameFromFingerprintHexNss( + fingerprintHex) // Delete the cert from NSS - cmd := exec.Command(nssCertutilName, "-d", "sql:" + nssDir.Value(), "-D", "-n", nickname) + cmd := exec.Command(nssCertutilName, "-d", "sql:"+ + nssDir.Value(), "-D", "-n", nickname) err := cmd.Run() if err != nil { @@ -81,6 +88,9 @@ func cleanCertsNss() { // Also delete the cert from the filesystem err = os.Remove(certDir.Value() + "/" + filename) + if err != nil { + log.Fatal(err) + } } } @@ -91,8 +101,10 @@ func checkCertExpiredNss(certFile os.FileInfo) (bool, error) { // Get the last modified time certFileModTime := certFile.ModTime() - // If the cert's last modified timestamp differs too much from the current time in either direction, consider it expired - expired := math.Abs( time.Since(certFileModTime).Seconds() ) > float64(certExpirePeriod.Value()) + // If the cert's last modified timestamp differs too much from the + // current time in either direction, consider it expired + expired := math.Abs(time.Since(certFileModTime).Seconds()) > + float64(certExpirePeriod.Value()) return expired, nil diff --git a/certinject/nss_notwindows.go b/certinject/nss_notwindows.go index 930bf90..d242f5f 100644 --- a/certinject/nss_notwindows.go +++ b/certinject/nss_notwindows.go @@ -2,6 +2,6 @@ package certinject -var ( +const ( nssCertutilName = "certutil" ) diff --git a/certinject/nss_windows.go b/certinject/nss_windows.go index 3a5c223..b24bef3 100644 --- a/certinject/nss_windows.go +++ b/certinject/nss_windows.go @@ -1,5 +1,5 @@ package certinject -var ( +const ( nssCertutilName = "nss-certutil" )