Merge pull request #4421 from gkrizek/tls-disable-autofill

tls: add --tlsdisableautofill flag to prevent sensitive data leaks

cert/go.mod

@@ -1,3 +1,5 @@
 module github.com/lightningnetwork/lnd/cert
 
 go 1.13
+
+require github.com/stretchr/testify v1.5.1

cert/go.sum

@@ -0,0 +1,11 @@
+github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
+github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
+github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
+gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=

cert/selfsigned.go

@@ -32,8 +32,9 @@ var (
 )
 
 // ipAddresses returns the parserd IP addresses to use when creating the TLS
-// certificate.
-func ipAddresses(tlsExtraIPs []string) ([]net.IP, error) {
+// certificate. If tlsDisableAutofill is true, we don't include interface
+// addresses to protect users privacy.
+func ipAddresses(tlsExtraIPs []string, tlsDisableAutofill bool) ([]net.IP, error) {
 	// Collect the host's IP addresses, including loopback, in a slice.
 	ipAddresses := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}
 
@@ -47,6 +48,10 @@ func ipAddresses(tlsExtraIPs []string) ([]net.IP, error) {
 		ipAddresses = append(ipAddresses, ipAddr)
 	}
 
+	// To protect their privacy, some users might not want to have all
+	// their network addresses include in the certificate as this could
+	// leak sensitive information.
+	if !tlsDisableAutofill {
 		// Add all the interface IPs that aren't already in the slice.
 		addrs, err := net.InterfaceAddrs()
 		if err != nil {
@@ -58,6 +63,7 @@ func ipAddresses(tlsExtraIPs []string) ([]net.IP, error) {
 				addIP(ipAddr)
 			}
 		}
+	}
 
 	// Add extra IPs to the slice.
 	for _, ip := range tlsExtraIPs {
@@ -72,10 +78,14 @@ func ipAddresses(tlsExtraIPs []string) ([]net.IP, error) {
 
 // dnsNames returns the host and DNS names to use when creating the TLS
 // ceftificate.
-func dnsNames(tlsExtraDomains []string) (string, []string) {
+func dnsNames(tlsExtraDomains []string, tlsDisableAutofill bool) (string, []string) {
 	// Collect the host's names into a slice.
 	host, err := os.Hostname()
-	if err != nil {
+
+	// To further protect their privacy, some users might not want
+	// to have their hostname include in the certificate as this could
+	// leak sensitive information.
+	if err != nil || tlsDisableAutofill {
 		// Nothing much we can do here, other than falling back to
 		// localhost as fallback. A hostname can still be provided with
 		// the tlsExtraDomain parameter if the problem persists on a
@@ -89,6 +99,13 @@ func dnsNames(tlsExtraDomains []string) (string, []string) {
 	}
 	dnsNames = append(dnsNames, tlsExtraDomains...)
 
+	// Because we aren't including the hostname in the certificate when
+	// tlsDisableAutofill is set, we will use the first extra domain
+	// specified by the user, if it's set, as the Common Name.
+	if tlsDisableAutofill && len(tlsExtraDomains) > 0 {
+		host = tlsExtraDomains[0]
+	}
+
 	// Also add fake hostnames for unix sockets, otherwise hostname
 	// verification will fail in the client.
 	dnsNames = append(dnsNames, "unix", "unixpacket")
@@ -104,10 +121,10 @@ func dnsNames(tlsExtraDomains []string) (string, []string) {
 // and domains given. The certificate is considered up to date if it was
 // created with _exactly_ the IPs and domains given.
 func IsOutdated(cert *x509.Certificate, tlsExtraIPs,
-	tlsExtraDomains []string) (bool, error) {
+	tlsExtraDomains []string, tlsDisableAutofill bool) (bool, error) {
 
 	// Parse the slice of IP strings.
-	ips, err := ipAddresses(tlsExtraIPs)
+	ips, err := ipAddresses(tlsExtraIPs, tlsDisableAutofill)
 	if err != nil {
 		return false, err
 	}
@@ -147,7 +164,7 @@ func IsOutdated(cert *x509.Certificate, tlsExtraIPs,
 	}
 
 	// Get the full list of DNS names to use.
-	_, dnsNames := dnsNames(tlsExtraDomains)
+	_, dnsNames := dnsNames(tlsExtraDomains, tlsDisableAutofill)
 
 	// We do the same kind of deduplication for the DNS names.
 	dns1 := make(map[string]struct{})
@@ -186,7 +203,8 @@ func IsOutdated(cert *x509.Certificate, tlsExtraIPs,
 // This function is adapted from https://github.com/btcsuite/btcd and
 // https://github.com/btcsuite/btcutil
 func GenCertPair(org, certFile, keyFile string, tlsExtraIPs,
-	tlsExtraDomains []string, certValidity time.Duration) error {
+	tlsExtraDomains []string, tlsDisableAutofill bool,
+	certValidity time.Duration) error {
 
 	now := time.Now()
 	validUntil := now.Add(certValidity)
@@ -204,8 +222,8 @@ func GenCertPair(org, certFile, keyFile string, tlsExtraIPs,
 
 	// Get all DNS names and IP addresses to use when creating the
 	// certificate.
-	host, dnsNames := dnsNames(tlsExtraDomains)
-	ipAddresses, err := ipAddresses(tlsExtraIPs)
+	host, dnsNames := dnsNames(tlsExtraDomains, tlsDisableAutofill)
+	ipAddresses, err := ipAddresses(tlsExtraIPs, tlsDisableAutofill)
 	if err != nil {
 		return err
 	}

cert/selfsigned_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/lightningnetwork/lnd/cert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -26,7 +27,7 @@ func TestIsOutdatedCert(t *testing.T) {
 	// Generate TLS files with two extra IPs and domains.
 	err = cert.GenCertPair(
 		"lnd autogenerated cert", certPath, keyPath, extraIPs[:2],
-		extraDomains[:2], cert.DefaultAutogenValidity,
+		extraDomains[:2], false, cert.DefaultAutogenValidity,
 	)
 	if err != nil {
 		t.Fatal(err)
@@ -48,7 +49,7 @@ func TestIsOutdatedCert(t *testing.T) {
 			// above.
 			outdated, err := cert.IsOutdated(
 				parsedCert, extraIPs[:numIPs],
-				extraDomains[:numDomains],
+				extraDomains[:numDomains], false,
 			)
 			if err != nil {
 				t.Fatal(err)
@@ -81,7 +82,7 @@ func TestIsOutdatedPermutation(t *testing.T) {
 	// Generate TLS files from the IPs and domains.
 	err = cert.GenCertPair(
 		"lnd autogenerated cert", certPath, keyPath, extraIPs[:],
-		extraDomains[:], cert.DefaultAutogenValidity,
+		extraDomains[:], false, cert.DefaultAutogenValidity,
 	)
 	if err != nil {
 		t.Fatal(err)
@@ -102,7 +103,7 @@ func TestIsOutdatedPermutation(t *testing.T) {
 		dupDNS[i] = extraDomains[i/2]
 	}
 
-	outdated, err := cert.IsOutdated(parsedCert, dupIPs, dupDNS)
+	outdated, err := cert.IsOutdated(parsedCert, dupIPs, dupDNS, false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -123,7 +124,7 @@ func TestIsOutdatedPermutation(t *testing.T) {
 		revDNS[i] = extraDomains[len(extraDomains)-1-i]
 	}
 
-	outdated, err = cert.IsOutdated(parsedCert, revIPs, revDNS)
+	outdated, err = cert.IsOutdated(parsedCert, revIPs, revDNS, false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -133,3 +134,59 @@ func TestIsOutdatedPermutation(t *testing.T) {
 			"considered outdated")
 	}
 }
+
+// TestTLSDisableAutofill checks that setting the --tlsdisableautofill flag
+// does not add interface ip addresses or hostnames to the cert.
+func TestTLSDisableAutofill(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "certtest")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	certPath := tempDir + "/tls.cert"
+	keyPath := tempDir + "/tls.key"
+
+	// Generate TLS files with two extra IPs and domains and no interface IPs.
+	err = cert.GenCertPair(
+		"lnd autogenerated cert", certPath, keyPath, extraIPs[:2],
+		extraDomains[:2], true, cert.DefaultAutogenValidity,
+	)
+	require.NoError(
+		t, err,
+		"unable to generate tls certificate pair",
+	)
+
+	_, parsedCert, err := cert.LoadCert(
+		certPath, keyPath,
+	)
+	require.NoError(
+		t, err,
+		"unable to load tls certificate pair",
+	)
+
+	// Check if the TLS cert is outdated while still preventing
+	// interface IPs from being used. Should not be outdated
+	shouldNotBeOutdated, err := cert.IsOutdated(
+		parsedCert, extraIPs[:2],
+		extraDomains[:2], true,
+	)
+	require.NoError(t, err)
+
+	require.Equal(
+		t, false, shouldNotBeOutdated,
+		"TLS Certificate was marked as outdated when it should not be",
+	)
+
+	// Check if the TLS cert is outdated while allowing for
+	// interface IPs to be used. Should report as outdated.
+	shouldBeOutdated, err := cert.IsOutdated(
+		parsedCert, extraIPs[:2],
+		extraDomains[:2], false,
+	)
+	require.NoError(t, err)
+
+	require.Equal(
+		t, true, shouldBeOutdated,
+		"TLS Certificate was not marked as outdated when it should be",
+	)
+}

config.go

@@ -147,6 +147,7 @@ type Config struct {
 	TLSExtraIPs        []string `long:"tlsextraip" description:"Adds an extra ip to the generated certificate"`
 	TLSExtraDomains    []string `long:"tlsextradomain" description:"Adds an extra domain to the generated certificate"`
 	TLSAutoRefresh     bool     `long:"tlsautorefresh" description:"Re-generate TLS certificate and key if the IPs or domains are changed"`
+	TLSDisableAutofill bool     `long:"tlsdisableautofill" description:"Do not include the interface IPs or the system hostname in TLS certificate, use first --tlsextradomain as Common Name instead, if set"`
 
 	NoMacaroons     bool          `long:"no-macaroons" description:"Disable macaroon authentication"`
 	AdminMacPath    string        `long:"adminmacaroonpath" description:"Path to write the admin macaroon for lnd's RPC and REST services if it doesn't exist"`

lnd.go

@@ -754,7 +754,7 @@ func getTLSConfig(cfg *Config) (*tls.Config, *credentials.TransportCredentials,
 		err := cert.GenCertPair(
 			"lnd autogenerated cert", cfg.TLSCertPath,
 			cfg.TLSKeyPath, cfg.TLSExtraIPs, cfg.TLSExtraDomains,
-			cert.DefaultAutogenValidity,
+			cfg.TLSDisableAutofill, cert.DefaultAutogenValidity,
 		)
 		if err != nil {
 			return nil, nil, "", err
@@ -776,7 +776,8 @@ func getTLSConfig(cfg *Config) (*tls.Config, *credentials.TransportCredentials,
 	refresh := false
 	if cfg.TLSAutoRefresh {
 		refresh, err = cert.IsOutdated(
-			parsedCert, cfg.TLSExtraIPs, cfg.TLSExtraDomains,
+			parsedCert, cfg.TLSExtraIPs,
+			cfg.TLSExtraDomains, cfg.TLSDisableAutofill,
 		)
 		if err != nil {
 			return nil, nil, "", err
@@ -803,7 +804,7 @@ func getTLSConfig(cfg *Config) (*tls.Config, *credentials.TransportCredentials,
 		err = cert.GenCertPair(
 			"lnd autogenerated cert", cfg.TLSCertPath,
 			cfg.TLSKeyPath, cfg.TLSExtraIPs, cfg.TLSExtraDomains,
-			cert.DefaultAutogenValidity,
+			cfg.TLSDisableAutofill, cert.DefaultAutogenValidity,
 		)
 		if err != nil {
 			return nil, nil, "", err