From 39b567a55aa0692e0fbd81b3d690c3461b953c06 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Mon, 29 Jun 2020 15:10:56 -0400 Subject: [PATCH] Fix auto_encrypt IP/DNS SANs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial auto encrypt CSR wasn’t containing the user supplied IP and DNS SANs. This fixes that. Also We were configuring a default :: IP SAN. This should be ::1 instead and was fixed. --- agent/agent.go | 2 +- agent/cache-types/connect_ca_leaf.go | 2 +- agent/consul/auto_encrypt.go | 87 ++++++++++++++------------ agent/consul/auto_encrypt_test.go | 92 +++++++++++++++++++++++++++- 4 files changed, 141 insertions(+), 42 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index db09313601..a797ca5294 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -840,7 +840,7 @@ func (a *Agent) setupClientAutoEncrypt(ctx context.Context) (*structs.SignedResp } addrs = append(addrs, retryJoinAddrs(disco, retryJoinSerfVariant, "LAN", a.config.RetryJoinLAN, a.logger)...) - reply, priv, err := client.RequestAutoEncryptCerts(ctx, addrs, a.config.ServerPort, a.tokens.AgentToken()) + reply, priv, err := client.RequestAutoEncryptCerts(ctx, addrs, a.config.ServerPort, a.tokens.AgentToken(), a.config.AutoEncryptDNSSAN, a.config.AutoEncryptIPSAN) if err != nil { return nil, err } diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index afda5a7822..51ce12a0c6 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -532,7 +532,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, } commonName = connect.AgentCN(req.Agent, roots.TrustDomain) dnsNames = append([]string{"localhost"}, req.DNSSAN...) - ipAddresses = append([]net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::")}, req.IPSAN...) + ipAddresses = append([]net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, req.IPSAN...) } else { return result, errors.New("URI must be either service or agent") } diff --git a/agent/consul/auto_encrypt.go b/agent/consul/auto_encrypt.go index 4f176933ff..e4bca49005 100644 --- a/agent/consul/auto_encrypt.go +++ b/agent/consul/auto_encrypt.go @@ -19,7 +19,53 @@ const ( retryJitterWindow = 30 * time.Second ) -func (c *Client) RequestAutoEncryptCerts(ctx context.Context, servers []string, port int, token string) (*structs.SignedResponse, string, error) { +func (c *Client) autoEncryptCSR(extraDNSSANs []string, extraIPSANs []net.IP) (string, string, error) { + // We don't provide the correct host here, because we don't know any + // better at this point. Apart from the domain, we would need the + // ClusterID, which we don't have. This is why we go with + // dummyTrustDomain the first time. Subsequent CSRs will have the + // correct TrustDomain. + id := &connect.SpiffeIDAgent{ + Host: dummyTrustDomain, + Datacenter: c.config.Datacenter, + Agent: c.config.NodeName, + } + + conf, err := c.config.CAConfig.GetCommonConfig() + if err != nil { + return "", "", err + } + + if conf.PrivateKeyType == "" { + conf.PrivateKeyType = connect.DefaultPrivateKeyType + } + if conf.PrivateKeyBits == 0 { + conf.PrivateKeyBits = connect.DefaultPrivateKeyBits + } + + // Create a new private key + pk, pkPEM, err := connect.GeneratePrivateKeyWithConfig(conf.PrivateKeyType, conf.PrivateKeyBits) + if err != nil { + return "", "", err + } + + dnsNames := append([]string{"localhost"}, extraDNSSANs...) + ipAddresses := append([]net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, extraIPSANs...) + + // Create a CSR. + // + // The Common Name includes the dummy trust domain for now but Server will + // override this when it is signed anyway so it's OK. + cn := connect.AgentCN(c.config.NodeName, dummyTrustDomain) + csr, err := connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses) + if err != nil { + return "", "", err + } + + return pkPEM, csr, nil +} + +func (c *Client) RequestAutoEncryptCerts(ctx context.Context, servers []string, port int, token string, extraDNSSANs []string, extraIPSANs []net.IP) (*structs.SignedResponse, string, error) { errFn := func(err error) (*structs.SignedResponse, string, error) { return nil, "", err } @@ -36,44 +82,7 @@ func (c *Client) RequestAutoEncryptCerts(ctx context.Context, servers []string, return errFn(fmt.Errorf("No servers to request AutoEncrypt.Sign")) } - // We don't provide the correct host here, because we don't know any - // better at this point. Apart from the domain, we would need the - // ClusterID, which we don't have. This is why we go with - // dummyTrustDomain the first time. Subsequent CSRs will have the - // correct TrustDomain. - id := &connect.SpiffeIDAgent{ - Host: dummyTrustDomain, - Datacenter: c.config.Datacenter, - Agent: c.config.NodeName, - } - - conf, err := c.config.CAConfig.GetCommonConfig() - if err != nil { - return errFn(err) - } - - if conf.PrivateKeyType == "" { - conf.PrivateKeyType = connect.DefaultPrivateKeyType - } - if conf.PrivateKeyBits == 0 { - conf.PrivateKeyBits = connect.DefaultPrivateKeyBits - } - - // Create a new private key - pk, pkPEM, err := connect.GeneratePrivateKeyWithConfig(conf.PrivateKeyType, conf.PrivateKeyBits) - if err != nil { - return errFn(err) - } - - dnsNames := []string{"localhost"} - ipAddresses := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::")} - - // Create a CSR. - // - // The Common Name includes the dummy trust domain for now but Server will - // override this when it is signed anyway so it's OK. - cn := connect.AgentCN(c.config.NodeName, dummyTrustDomain) - csr, err := connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses) + pkPEM, csr, err := c.autoEncryptCSR(extraDNSSANs, extraIPSANs) if err != nil { return errFn(err) } diff --git a/agent/consul/auto_encrypt_test.go b/agent/consul/auto_encrypt_test.go index 85ed253f4e..fd8725725f 100644 --- a/agent/consul/auto_encrypt_test.go +++ b/agent/consul/auto_encrypt_test.go @@ -2,11 +2,17 @@ package consul import ( "context" + "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" "net" + "net/url" "os" "testing" "time" + "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/require" @@ -98,7 +104,7 @@ func TestAutoEncrypt_RequestAutoEncryptCerts(t *testing.T) { doneCh := make(chan struct{}) var err error go func() { - _, _, err = c1.RequestAutoEncryptCerts(ctx, servers, port, token) + _, _, err = c1.RequestAutoEncryptCerts(ctx, servers, port, token, nil, nil) close(doneCh) }() select { @@ -113,3 +119,87 @@ func TestAutoEncrypt_RequestAutoEncryptCerts(t *testing.T) { // try to request certs. } } + +func TestAutoEncrypt_autoEncryptCSR(t *testing.T) { + type testCase struct { + conf *Config + extraDNSSANs []string + extraIPSANs []net.IP + err string + + // to validate the csr + expectedSubject pkix.Name + expectedSigAlg x509.SignatureAlgorithm + expectedPubAlg x509.PublicKeyAlgorithm + expectedDNSNames []string + expectedIPs []net.IP + expectedURIs []*url.URL + } + + cases := map[string]testCase{ + "sans": { + conf: &Config{ + Datacenter: "dc1", + NodeName: "test-node", + CAConfig: &structs.CAConfiguration{}, + }, + extraDNSSANs: []string{"foo.local", "bar.local"}, + extraIPSANs: []net.IP{net.IPv4(198, 18, 0, 1), net.IPv4(198, 18, 0, 2)}, + expectedSubject: pkix.Name{ + CommonName: connect.AgentCN("test-node", dummyTrustDomain), + Names: []pkix.AttributeTypeAndValue{ + { + // 2,5,4,3 is the CommonName type ASN1 identifier + Type: asn1.ObjectIdentifier{2, 5, 4, 3}, + Value: "testnode.agnt.dummy.tr.consul", + }, + }, + }, + expectedSigAlg: x509.ECDSAWithSHA256, + expectedPubAlg: x509.ECDSA, + expectedDNSNames: []string{ + "localhost", + "foo.local", + "bar.local", + }, + expectedIPs: []net.IP{ + {127, 0, 0, 1}, + net.ParseIP("::1"), + {198, 18, 0, 1}, + {198, 18, 0, 2}, + }, + expectedURIs: []*url.URL{ + { + Scheme: "spiffe", + Host: dummyTrustDomain, + Path: "/agent/client/dc/dc1/id/test-node", + }, + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + client := Client{config: tcase.conf} + + _, csr, err := client.autoEncryptCSR(tcase.extraDNSSANs, tcase.extraIPSANs) + if tcase.err == "" { + require.NoError(t, err) + + request, err := connect.ParseCSR(csr) + require.NoError(t, err) + require.NotNil(t, request) + + require.Equal(t, tcase.expectedSubject, request.Subject) + require.Equal(t, tcase.expectedSigAlg, request.SignatureAlgorithm) + require.Equal(t, tcase.expectedPubAlg, request.PublicKeyAlgorithm) + require.Equal(t, tcase.expectedDNSNames, request.DNSNames) + require.Equal(t, tcase.expectedIPs, request.IPAddresses) + require.Equal(t, tcase.expectedURIs, request.URIs) + } else { + require.Error(t, err) + require.Empty(t, csr) + } + }) + } +}