cli: improve the file safety of 'consul tls' subcommands

agent/agent_endpoint.go

@@ -1256,7 +1256,7 @@ func (s *HTTPServer) AgentToken(resp http.ResponseWriter, req *http.Request) (in
 			return nil, fmt.Errorf("Failed to marshal tokens for persistence: %v", err)
 		}
 
-		if err := file.WriteAtomicWithPerms(filepath.Join(s.agent.config.DataDir, tokensPath), data, 0600); err != nil {
+		if err := file.WriteAtomicWithPerms(filepath.Join(s.agent.config.DataDir, tokensPath), data, 0700, 0600); err != nil {
 			s.agent.logger.Warn("failed to persist tokens", "error", err)
 			return nil, fmt.Errorf("Failed to persist tokens - %v", err)
 		}

command/login/login.go

@@ -126,7 +126,7 @@ func (c *cmd) Run(args []string) int {
 
 func (c *cmd) writeToSink(tok *api.ACLToken) error {
 	payload := []byte(tok.SecretID)
-	return file.WriteAtomicWithPerms(c.tokenSinkFile, payload, 0600)
+	return file.WriteAtomicWithPerms(c.tokenSinkFile, payload, 0755, 0600)
 }
 
 func (c *cmd) Synopsis() string {

command/tls/ca/create/tls_ca_create.go

@@ -3,10 +3,10 @@ package create
 import (
 	"flag"
 	"fmt"
-	"os"
 
 	"github.com/hashicorp/consul/command/flags"
 	"github.com/hashicorp/consul/command/tls"
+	"github.com/hashicorp/consul/lib/file"
 	"github.com/hashicorp/consul/tlsutil"
 	"github.com/mitchellh/cli"
 )
@@ -70,26 +70,30 @@ func (c *cmd) Run(args []string) int {
 	s, pk, err := tlsutil.GeneratePrivateKey()
 	if err != nil {
 		c.UI.Error(err.Error())
+		return 1
 	}
+
 	constraints := []string{}
 	if c.constraint {
 		constraints = append(c.additionalConstraints, []string{c.domain, "localhost"}...)
 	}
+
 	ca, err := tlsutil.GenerateCA(s, sn, c.days, constraints)
 	if err != nil {
 		c.UI.Error(err.Error())
+		return 1
 	}
-	caFile, err := os.Create(certFileName)
-	if err != nil {
+
+	if err := file.WriteAtomicWithPerms(certFileName, []byte(ca), 0755, 0666); err != nil {
 		c.UI.Error(err.Error())
+		return 1
 	}
-	caFile.WriteString(ca)
 	c.UI.Output("==> Saved " + certFileName)
-	pkFile, err := os.Create(pkFileName)
-	if err != nil {
+
+	if err := file.WriteAtomicWithPerms(pkFileName, []byte(pk), 0755, 0666); err != nil {
 		c.UI.Error(err.Error())
+		return 1
 	}
-	pkFile.WriteString(pk)
 	c.UI.Output("==> Saved " + pkFileName)
 
 	return 0

command/tls/ca/create/tls_ca_create_test.go

@@ -1,9 +1,10 @@
 package create
 
 import (
+	"crypto"
+	"crypto/x509"
 	"io/ioutil"
 	"os"
-	"path"
 	"strings"
 	"testing"
 	"time"
@@ -22,81 +23,104 @@ func TestValidateCommand_noTabs(t *testing.T) {
 }
 
 func TestCACreateCommand(t *testing.T) {
-	require := require.New(t)
-
-	previousDirectory, err := os.Getwd()
-	require.NoError(err)
-
 	testDir := testutil.TempDir(t, "ca-create")
-
 	defer os.RemoveAll(testDir)
-	defer os.Chdir(previousDirectory)
-
-	os.Chdir(testDir)
-
-	ui := cli.NewMockUi()
-	cmd := New(ui)
 
-	require.Equal(0, cmd.Run(nil), "ca create should exit 0")
+	defer switchToTempDir(t, testDir)()
 
-	errOutput := ui.ErrorWriter.String()
-	require.Equal("", errOutput)
-
-	caPem := path.Join(testDir, "consul-agent-ca.pem")
-	require.FileExists(caPem)
-
-	certData, err := ioutil.ReadFile(caPem)
-	require.NoError(err)
-
-	cert, err := connect.ParseCert(string(certData))
-	require.NoError(err)
-	require.NotNil(cert)
+	type testcase struct {
+		name       string
+		args       []string
+		caPath     string
+		keyPath    string
+		extraCheck func(t *testing.T, cert *x509.Certificate)
+	}
+	// The following subtests must run serially.
+	cases := []testcase{
+		{"ca defaults",
+			nil,
+			"consul-agent-ca.pem",
+			"consul-agent-ca-key.pem",
+			func(t *testing.T, cert *x509.Certificate) {
+				require.Equal(t, 1825*24*time.Hour, time.Until(cert.NotAfter).Round(24*time.Hour))
+				require.False(t, cert.PermittedDNSDomainsCritical)
+				require.Len(t, cert.PermittedDNSDomains, 0)
+			},
+		},
+		{"ca options",
+			[]string{
+				"-days=365",
+				"-name-constraint=true",
+				"-domain=foo",
+				"-additional-name-constraint=bar",
+			},
+			"foo-agent-ca.pem",
+			"foo-agent-ca-key.pem",
+			func(t *testing.T, cert *x509.Certificate) {
+				require.Equal(t, 365*24*time.Hour, time.Until(cert.NotAfter).Round(24*time.Hour))
+				require.True(t, cert.PermittedDNSDomainsCritical)
+				require.Len(t, cert.PermittedDNSDomains, 3)
+				require.ElementsMatch(t, cert.PermittedDNSDomains, []string{"foo", "localhost", "bar"})
+			},
+		},
+	}
+	for _, tc := range cases {
+		tc := tc
+		require.True(t, t.Run(tc.name, func(t *testing.T) {
+			ui := cli.NewMockUi()
+			cmd := New(ui)
+			require.Equal(t, 0, cmd.Run(tc.args))
+			require.Equal(t, "", ui.ErrorWriter.String())
+
+			cert, _ := expectFiles(t, tc.caPath, tc.keyPath)
+			require.Contains(t, cert.Subject.CommonName, "Consul Agent CA")
+			require.True(t, cert.BasicConstraintsValid)
+			require.Equal(t, x509.KeyUsageCertSign|x509.KeyUsageCRLSign|x509.KeyUsageDigitalSignature, cert.KeyUsage)
+			require.True(t, cert.IsCA)
+			require.Equal(t, cert.AuthorityKeyId, cert.SubjectKeyId)
+			tc.extraCheck(t, cert)
+		}))
+	}
 
-	require.Equal(1825*24*time.Hour, time.Until(cert.NotAfter).Round(24*time.Hour))
-	require.False(cert.PermittedDNSDomainsCritical)
-	require.Len(cert.PermittedDNSDomains, 0)
 }
 
-func TestCACreateCommandWithOptions(t *testing.T) {
-	require := require.New(t)
+func expectFiles(t *testing.T, caPath, keyPath string) (*x509.Certificate, crypto.Signer) {
+	t.Helper()
 
-	previousDirectory, err := os.Getwd()
-	require.NoError(err)
+	require.FileExists(t, caPath)
+	require.FileExists(t, keyPath)
 
-	testDir := testutil.TempDir(t, "ca-create")
+	caData, err := ioutil.ReadFile(caPath)
+	require.NoError(t, err)
+	keyData, err := ioutil.ReadFile(keyPath)
+	require.NoError(t, err)
 
-	defer os.RemoveAll(testDir)
-	defer os.Chdir(previousDirectory)
+	ca, err := connect.ParseCert(string(caData))
+	require.NoError(t, err)
+	require.NotNil(t, ca)
 
-	os.Chdir(testDir)
+	signer, err := connect.ParseSigner(string(keyData))
+	require.NoError(t, err)
+	require.NotNil(t, signer)
 
-	ui := cli.NewMockUi()
-	cmd := New(ui)
+	return ca, signer
+}
 
-	args := []string{
-		"-days=365",
-		"-name-constraint=true",
-		"-domain=foo",
-		"-additional-name-constraint=bar",
+// switchToTempDir is meant to be used in a defer statement like:
+//
+//   defer switchToTempDir(t, testDir)()
+//
+// This exploits the fact that the body of a defer is evaluated
+// EXCEPT for the final function call invocation inline with the code
+// where it is found. Only the final evaluation happens in the defer
+// at a later time. In this case it means we switch to the temp
+// directory immediately and defer switching back in one line of test
+// code.
+func switchToTempDir(t *testing.T, testDir string) func() {
+	previousDirectory, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Chdir(testDir))
+	return func() {
+		os.Chdir(previousDirectory)
 	}
-
-	require.Equal(0, cmd.Run(args), "ca create should exit 0")
-
-	errOutput := ui.ErrorWriter.String()
-	require.Equal("", errOutput)
-
-	caPem := path.Join(testDir, "foo-agent-ca.pem")
-	require.FileExists(caPem)
-
-	certData, err := ioutil.ReadFile(caPem)
-	require.NoError(err)
-
-	cert, err := connect.ParseCert(string(certData))
-	require.NoError(err)
-	require.NotNil(cert)
-
-	require.Equal(365*24*time.Hour, time.Until(cert.NotAfter).Round(24*time.Hour))
-	require.True(cert.PermittedDNSDomainsCritical)
-	require.Len(cert.PermittedDNSDomains, 3)
-	require.ElementsMatch(cert.PermittedDNSDomains, []string{"foo", "localhost", "bar"})
 }

command/tls/cert/create/tls_cert_create.go

@@ -6,11 +6,11 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net"
-	"os"
 	"strings"
 
 	"github.com/hashicorp/consul/command/flags"
 	"github.com/hashicorp/consul/command/tls"
+	"github.com/hashicorp/consul/lib/file"
 	"github.com/hashicorp/consul/tlsutil"
 	"github.com/mitchellh/cli"
 )
@@ -98,10 +98,14 @@ func (c *cmd) Run(args []string) int {
 
 	if c.server {
 		name = fmt.Sprintf("server.%s.%s", c.dc, c.domain)
-		DNSNames = append(DNSNames, []string{name, "localhost"}...)
+
+		DNSNames = append(DNSNames, name)
+		DNSNames = append(DNSNames, "localhost")
+
 		IPAddresses = append(IPAddresses, net.ParseIP("127.0.0.1"))
 		extKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}
 		prefix = fmt.Sprintf("%s-server-%s", c.dc, c.domain)
+
 	} else if c.client {
 		name = fmt.Sprintf("client.%s.%s", c.dc, c.domain)
 		DNSNames = append(DNSNames, []string{name, "localhost"}...)
@@ -174,24 +178,20 @@ func (c *cmd) Run(args []string) int {
 	}
 
 	if err = tlsutil.Verify(string(cert), pub, name); err != nil {
-		c.UI.Error("==> " + err.Error())
+		c.UI.Error(err.Error())
 		return 1
 	}
 
-	certFile, err := os.Create(certFileName)
-	if err != nil {
+	if err := file.WriteAtomicWithPerms(certFileName, []byte(pub), 0755, 0666); err != nil {
 		c.UI.Error(err.Error())
 		return 1
 	}
-	certFile.WriteString(pub)
 	c.UI.Output("==> Saved " + certFileName)
 
-	pkFile, err := os.Create(pkFileName)
-	if err != nil {
+	if err := file.WriteAtomicWithPerms(pkFileName, []byte(priv), 0755, 0666); err != nil {
 		c.UI.Error(err.Error())
 		return 1
 	}
-	pkFile.WriteString(priv)
 	c.UI.Output("==> Saved " + pkFileName)
 
 	return 0