diff --git a/connect/service.go b/connect/service.go index 6bbda08079..51bad44f65 100644 --- a/connect/service.go +++ b/connect/service.go @@ -74,8 +74,8 @@ func NewServiceWithLogger(serviceID string, client *api.Client, client: client, logger: logger, } - s.serverTLSCfg = newReloadableTLSConfig(defaultTLSConfig(serverVerifyCerts)) - s.clientTLSCfg = newReloadableTLSConfig(defaultTLSConfig(clientVerifyCerts)) + s.serverTLSCfg = newReloadableTLSConfig(defaultTLSConfig(newServerSideVerifier(client, serviceID))) + s.clientTLSCfg = newReloadableTLSConfig(defaultTLSConfig(clientSideVerifier)) // TODO(banks) run the background certificate sync return s, nil @@ -97,9 +97,9 @@ func NewDevServiceFromCertFiles(serviceID string, client *api.Client, // Note that newReloadableTLSConfig makes a copy so we can re-use the same // base for both client and server with swapped verifiers. - tlsCfg.VerifyPeerCertificate = serverVerifyCerts + setVerifier(tlsCfg, newServerSideVerifier(client, serviceID)) s.serverTLSCfg = newReloadableTLSConfig(tlsCfg) - tlsCfg.VerifyPeerCertificate = clientVerifyCerts + setVerifier(tlsCfg, clientSideVerifier) s.clientTLSCfg = newReloadableTLSConfig(tlsCfg) return s, nil } diff --git a/connect/testing.go b/connect/testing.go index f9a6a48506..c23b837010 100644 --- a/connect/testing.go +++ b/connect/testing.go @@ -26,10 +26,11 @@ func TestService(t testing.T, service string, ca *structs.CARoot) *Service { t.Fatal(err) } + // verify server without AuthZ call svc.serverTLSCfg = newReloadableTLSConfig( - TestTLSConfigWithVerifier(t, service, ca, serverVerifyCerts)) + TestTLSConfigWithVerifier(t, service, ca, newServerSideVerifier(nil, service))) svc.clientTLSCfg = newReloadableTLSConfig( - TestTLSConfigWithVerifier(t, service, ca, clientVerifyCerts)) + TestTLSConfigWithVerifier(t, service, ca, clientSideVerifier)) return svc } @@ -43,9 +44,9 @@ func TestTLSConfig(t testing.T, service string, ca *structs.CARoot) *tls.Config } // TestTLSConfigWithVerifier returns a *tls.Config suitable for use during -// tests, it will use the given verifyFunc to verify tls certificates. +// tests, it will use the given verifierFunc to verify tls certificates. func TestTLSConfigWithVerifier(t testing.T, service string, ca *structs.CARoot, - verifier verifyFunc) *tls.Config { + verifier verifierFunc) *tls.Config { t.Helper() cfg := defaultTLSConfig(verifier) diff --git a/connect/tls.go b/connect/tls.go index 89d5ccb542..d23b493967 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -5,17 +5,23 @@ import ( "crypto/x509" "errors" "io/ioutil" + "log" "sync" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/api" ) -// verifyFunc is the type of tls.Config.VerifyPeerCertificate for convenience. -type verifyFunc func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error +// verifierFunc is a function that can accept rawCertificate bytes from a peer +// and verify them against a given tls.Config. It's called from the +// tls.Config.VerifyPeerCertificate hook. We don't pass verifiedChains since +// that is always nil in our usage. Implementations can use the roots provided +// in the cfg to verify the certs. +type verifierFunc func(cfg *tls.Config, rawCerts [][]byte) error // defaultTLSConfig returns the standard config. -func defaultTLSConfig(verify verifyFunc) *tls.Config { - return &tls.Config{ +func defaultTLSConfig(v verifierFunc) *tls.Config { + cfg := &tls.Config{ MinVersion: tls.VersionTLS12, ClientAuth: tls.RequireAndVerifyClientCert, // We don't have access to go internals that decide if AES hardware @@ -34,12 +40,23 @@ func defaultTLSConfig(verify verifyFunc) *tls.Config { // We have to set this since otherwise Go will attempt to verify DNS names // match DNS SAN/CN which we don't want. We hook up VerifyPeerCertificate to // do our own path validation as well as Connect AuthZ. - InsecureSkipVerify: true, - VerifyPeerCertificate: verify, + InsecureSkipVerify: true, // Include h2 to allow connect http servers to automatically support http2. // See: https://github.com/golang/go/blob/917c33fe8672116b04848cf11545296789cafd3b/src/net/http/server.go#L2724-L2731 NextProtos: []string{"h2"}, } + setVerifier(cfg, v) + return cfg +} + +// setVerifier takes a *tls.Config and set's it's VerifyPeerCertificates hook to +// use the passed verifierFunc. +func setVerifier(cfg *tls.Config, v verifierFunc) { + if v != nil { + cfg.VerifyPeerCertificate = func(rawCerts [][]byte, chains [][]*x509.Certificate) error { + return v(cfg, rawCerts) + } + } } // reloadableTLSConfig exposes a tls.Config that can have it's certificates @@ -147,14 +164,104 @@ func verifyServerCertMatchesURI(certs []*x509.Certificate, return errors.New("peer certificate mismatch") } -// serverVerifyCerts is the verifyFunc for use on Connect servers. -func serverVerifyCerts(rawCerts [][]byte, chains [][]*x509.Certificate) error { - // TODO(banks): implement me - return nil +// newServerSideVerifier returns a verifierFunc that wraps the provided +// api.Client to verify the TLS chain and perform AuthZ for the server end of +// the connection. The service name provided is used as the target serviceID +// for the Authorization. +func newServerSideVerifier(client *api.Client, serviceID string) verifierFunc { + return func(tlsCfg *tls.Config, rawCerts [][]byte) error { + leaf, err := verifyChain(tlsCfg, rawCerts, false) + if err != nil { + return err + } + + // Check leaf is a cert we understand + if len(leaf.URIs) < 1 { + return errors.New("connect: invalid leaf certificate") + } + + certURI, err := connect.ParseCertURI(leaf.URIs[0]) + if err != nil { + return errors.New("connect: invalid leaf certificate URI") + } + + // No AuthZ if there is no client. + if client == nil { + return nil + } + + // Perform AuthZ + req := &api.AgentAuthorizeParams{ + // TODO(banks): this is jank, we have a serviceID from the Service setup + // but this needs to be a service name as the target. For now we are + // relying on them usually being the same but this will break when they + // are not. We either need to make Authorize endpoint optionally accept + // IDs somehow or rethink this as it will require fetching the service + // name sometime ahead of accepting requests (maybe along with TLS certs?) + // which feels gross and will take extra plumbing to expose it to here. + Target: serviceID, + ClientCertURI: certURI.URI().String(), + ClientCertSerial: connect.HexString(leaf.SerialNumber.Bytes()), + } + resp, err := client.Agent().ConnectAuthorize(req) + if err != nil { + return errors.New("connect: authz call failed: " + err.Error()) + } + if !resp.Authorized { + return errors.New("connect: authz denied: " + resp.Reason) + } + log.Println("[DEBUG] authz result", resp) + return nil + } } -// clientVerifyCerts is the verifyFunc for use on Connect clients. -func clientVerifyCerts(rawCerts [][]byte, chains [][]*x509.Certificate) error { - // TODO(banks): implement me - return nil +// clientSideVerifier is a verifierFunc that performs verification of certificates +// on the client end of the connection. For now it is just basic TLS +// verification since the identity check needs additional state and becomes +// clunky to customise the callback for every outgoing request. That is done +// within Service.Dial for now. +func clientSideVerifier(tlsCfg *tls.Config, rawCerts [][]byte) error { + _, err := verifyChain(tlsCfg, rawCerts, true) + return err +} + +// verifyChain performs standard TLS verification without enforcing remote +// hostname matching. +func verifyChain(tlsCfg *tls.Config, rawCerts [][]byte, client bool) (*x509.Certificate, error) { + + // Fetch leaf and intermediates. This is based on code form tls handshake. + if len(rawCerts) < 1 { + return nil, errors.New("tls: no certificates from peer") + } + certs := make([]*x509.Certificate, len(rawCerts)) + for i, asn1Data := range rawCerts { + cert, err := x509.ParseCertificate(asn1Data) + if err != nil { + return nil, errors.New("tls: failed to parse certificate from peer: " + err.Error()) + } + certs[i] = cert + } + + cas := tlsCfg.RootCAs + if client { + cas = tlsCfg.ClientCAs + } + + opts := x509.VerifyOptions{ + Roots: cas, + Intermediates: x509.NewCertPool(), + } + if !client { + // Server side only sets KeyUsages in tls. This defaults to ServerAuth in + // x509 lib. See + // https://github.com/golang/go/blob/ee7dd810f9ca4e63ecfc1d3044869591783b8b74/src/crypto/x509/verify.go#L866-L868 + opts.KeyUsages = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth} + } + + // All but the first cert are intermediates + for _, cert := range certs[1:] { + opts.Intermediates.AddCert(cert) + } + _, err := certs[0].Verify(opts) + return certs[0], err } diff --git a/connect/tls_test.go b/connect/tls_test.go index d13b786615..82b89440f0 100644 --- a/connect/tls_test.go +++ b/connect/tls_test.go @@ -1,10 +1,14 @@ package connect import ( + "crypto/tls" "crypto/x509" + "encoding/pem" "testing" + "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" ) @@ -100,3 +104,187 @@ func Test_verifyServerCertMatchesURI(t *testing.T) { }) } } + +func testCertPEMBlock(t *testing.T, pemValue string) []byte { + t.Helper() + // The _ result below is not an error but the remaining PEM bytes. + block, _ := pem.Decode([]byte(pemValue)) + require.NotNil(t, block) + require.Equal(t, "CERTIFICATE", block.Type) + return block.Bytes +} + +func TestClientSideVerifier(t *testing.T) { + ca1 := connect.TestCA(t, nil) + ca2 := connect.TestCA(t, ca1) + + webCA1PEM, _ := connect.TestLeaf(t, "web", ca1) + webCA2PEM, _ := connect.TestLeaf(t, "web", ca2) + + webCA1 := testCertPEMBlock(t, webCA1PEM) + xcCA2 := testCertPEMBlock(t, ca2.SigningCert) + webCA2 := testCertPEMBlock(t, webCA2PEM) + + tests := []struct { + name string + tlsCfg *tls.Config + rawCerts [][]byte + wantErr string + }{ + { + name: "ok service ca1", + tlsCfg: TestTLSConfig(t, "web", ca1), + rawCerts: [][]byte{webCA1}, + wantErr: "", + }, + { + name: "untrusted CA", + tlsCfg: TestTLSConfig(t, "web", ca2), // only trust ca2 + rawCerts: [][]byte{webCA1}, // present ca1 + wantErr: "unknown authority", + }, + { + name: "cross signed intermediate", + tlsCfg: TestTLSConfig(t, "web", ca1), // only trust ca1 + rawCerts: [][]byte{webCA2, xcCA2}, // present ca2 signed cert, and xc + wantErr: "", + }, + { + name: "cross signed without intermediate", + tlsCfg: TestTLSConfig(t, "web", ca1), // only trust ca1 + rawCerts: [][]byte{webCA2}, // present ca2 signed cert only + wantErr: "unknown authority", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + err := clientSideVerifier(tt.tlsCfg, tt.rawCerts) + if tt.wantErr == "" { + require.Nil(err) + } else { + require.NotNil(err) + require.Contains(err.Error(), tt.wantErr) + } + }) + } +} + +func TestServerSideVerifier(t *testing.T) { + ca1 := connect.TestCA(t, nil) + ca2 := connect.TestCA(t, ca1) + + webCA1PEM, _ := connect.TestLeaf(t, "web", ca1) + webCA2PEM, _ := connect.TestLeaf(t, "web", ca2) + + apiCA1PEM, _ := connect.TestLeaf(t, "api", ca1) + apiCA2PEM, _ := connect.TestLeaf(t, "api", ca2) + + webCA1 := testCertPEMBlock(t, webCA1PEM) + xcCA2 := testCertPEMBlock(t, ca2.SigningCert) + webCA2 := testCertPEMBlock(t, webCA2PEM) + + apiCA1 := testCertPEMBlock(t, apiCA1PEM) + apiCA2 := testCertPEMBlock(t, apiCA2PEM) + + // Setup a local test agent to query + agent := agent.NewTestAgent("test-consul", "") + defer agent.Shutdown() + + cfg := api.DefaultConfig() + cfg.Address = agent.HTTPAddr() + client, err := api.NewClient(cfg) + require.Nil(t, err) + + // Setup intentions to validate against. We actually default to allow so first + // setup a blanket deny rule for db, then only allow web. + connect := client.Connect() + ixn := &api.Intention{ + SourceNS: "default", + SourceName: "*", + DestinationNS: "default", + DestinationName: "db", + Action: api.IntentionActionDeny, + SourceType: api.IntentionSourceConsul, + Meta: map[string]string{}, + } + id, _, err := connect.IntentionCreate(ixn, nil) + require.Nil(t, err) + require.NotEmpty(t, id) + + ixn = &api.Intention{ + SourceNS: "default", + SourceName: "web", + DestinationNS: "default", + DestinationName: "db", + Action: api.IntentionActionAllow, + SourceType: api.IntentionSourceConsul, + Meta: map[string]string{}, + } + id, _, err = connect.IntentionCreate(ixn, nil) + require.Nil(t, err) + require.NotEmpty(t, id) + + tests := []struct { + name string + service string + tlsCfg *tls.Config + rawCerts [][]byte + wantErr string + }{ + { + name: "ok service ca1, allow", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca1), + rawCerts: [][]byte{webCA1}, + wantErr: "", + }, + { + name: "untrusted CA", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca2), // only trust ca2 + rawCerts: [][]byte{webCA1}, // present ca1 + wantErr: "unknown authority", + }, + { + name: "cross signed intermediate, allow", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca1), // only trust ca1 + rawCerts: [][]byte{webCA2, xcCA2}, // present ca2 signed cert, and xc + wantErr: "", + }, + { + name: "cross signed without intermediate", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca1), // only trust ca1 + rawCerts: [][]byte{webCA2}, // present ca2 signed cert only + wantErr: "unknown authority", + }, + { + name: "ok service ca1, deny", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca1), + rawCerts: [][]byte{apiCA1}, + wantErr: "denied", + }, + { + name: "cross signed intermediate, deny", + service: "db", + tlsCfg: TestTLSConfig(t, "db", ca1), // only trust ca1 + rawCerts: [][]byte{apiCA2, xcCA2}, // present ca2 signed cert, and xc + wantErr: "denied", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := newServerSideVerifier(client, tt.service) + err := v(tt.tlsCfg, tt.rawCerts) + if tt.wantErr == "" { + require.Nil(t, err) + } else { + require.NotNil(t, err) + require.Contains(t, err.Error(), tt.wantErr) + } + }) + } +}