setup grpc server with auto_encrypt certs #7086

hanshasselberg · 2020-01-17T22:10:08Z

HTTPS serves auto_encrypt certificates and so should GRPC. Also introduces a flag for -https-port, which I kept wondering why it is missing...

Fixes #7076.
Fixes #6590 since grpc TLS is now configurable via existing Consul TLS configuration.

$ openssl s_client -connect localhost:8502
CONNECTED(00000005)
depth=0 CN = c1.agnt.dummy.tr.consul
verify error:num=20:unable to get local issuer certificate
verify return:1
depth=0 CN = c1.agnt.dummy.tr.consul
verify error:num=21:unable to verify the first certificate
verify return:1
---
Certificate chain
 0 s:/CN=c1.agnt.dummy.tr.consul
   i:/CN=pri-d1baiug.consul.ca.95b8e492.consul
---
Server certificate
-----BEGIN CERTIFICATE-----
MIICGzCCAcGgAwIBAgIBCTAKBggqhkjOPQQDAjAwMS4wLAYDVQQDEyVwcmktZDFi
YWl1Zy5jb25zdWwuY2EuOTViOGU0OTIuY29uc3VsMB4XDTIwMDExNzIyMTc1OVoX
DTIwMDEyMDIyMTc1OVowIjEgMB4GA1UEAxMXYzEuYWdudC5kdW1teS50ci5jb25z
dWwwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATIyBEoTU0LGaJ1BC67xNyyyYR3
Na8MRfknExzh5A4HZVIPmG3mhjSh1IK65dtkZFDDJsgaqfc3Qx4yzNXizBY+o4HZ
MIHWMA4GA1UdDwEB/wQEAwIDuDAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUH
AwEwDAYDVR0TAQH/BAIwADApBgNVHQ4EIgQg5gFBtgnK74NuVwKdAhHNUdcS1vTJ
jIk+BkywaQfJrxEwKwYDVR0jBCQwIoAg5gFBtgnK74NuVwKdAhHNUdcS1vTJjIk+
BkywaQfJrxEwPwYDVR0RBDgwNoY0c3BpZmZlOi8vZHVtbXkudHJ1c3Rkb21haW4v
YWdlbnQvY2xpZW50L2RjL2RjMS9pZC9jMTAKBggqhkjOPQQDAgNIADBFAiBVdH+D
8XYvBNpolk+1rE5wxcfwLQ2OtV4nLxsPddEqIwIhAPerRVvuWZc9TX3CgQnj3Dyw
azs8ymmYhTigJ0oghhPY

freddygv

Nice work! Added some suggestions inline, let me know what you think.

freddygv · 2020-01-17T23:02:17Z

tlsutil/config.go

@@ -561,6 +569,12 @@ func (c *Configurator) VerifyServerHostname() bool {
 	return c.base.VerifyServerHostname || c.autoEncrypt.verifyServerHostname
 }

+// TransportCredentials returns credentials to be used with eg a grpc server.
+func (c *Configurator) TransportCredentials() *credentials.TransportCredentials {
+	creds := credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{*c.Cert()}})


*c.Cert() will panic if c.Cert() returns nil.

Currently the only call to TransportCredentials does a nil check before the function call, but I'm concerned about someone else calling this later without knowing about that behavior.

Maybe it would be useful to put in a nil check in this function just in case.

Alternatively, constructing the TransportCredentials could be done in the GRPCServer function, rather than being split out into a separate method.

Nice catch! There is a nil check now.

freddygv · 2020-01-17T23:13:55Z

agent/xds/server.go

-			return nil, err
+	if tlsConfigurator != nil {
+		if cert := tlsConfigurator.Cert(); cert != nil {
+			opts = append(opts, grpc.Creds(*tlsConfigurator.TransportCredentials()))


It seems a bit odd to have TransportCredentials return a pointer, but then dereference it for grpc.Creds().

I think TransportCredentials shouldn't return a pointer, since we only need a copy.

Suggested change

opts = append(opts, grpc.Creds(*tlsConfigurator.TransportCredentials()))

opts = append(opts, grpc.Creds(tlsConfigurator.TransportCredentials()))

freddygv · 2020-01-17T23:14:13Z

tlsutil/config.go

+// TransportCredentials returns credentials to be used with eg a grpc server.
+func (c *Configurator) TransportCredentials() *credentials.TransportCredentials {
+	creds := credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{*c.Cert()}})
+	return &creds


Suggested change

return &creds

return creds

See comment about TransportCredentials() pointer dereference.

hanshasselberg · 2020-01-20T12:41:50Z

@freddygv thank you for the review! I figured it would be great if grpc connections would use the same TLS config, the other consul stuff is using. Which not only fixes the cert problem, but also takes advantage of all the existing TLS configuration bits.

The pointer things you asked about, are now gone.

tlsutil/config.go

lornasong

makes sense to me! added a very minor suggestion for a comment inline.

tlsutil/config.go

freddygv

LGTM, dropped one question inline

freddygv · 2020-01-21T19:59:36Z

tlsutil/config.go

@@ -561,6 +567,16 @@ func (c *Configurator) VerifyServerHostname() bool {
 	return c.base.VerifyServerHostname || c.autoEncrypt.verifyServerHostname
 }

+// IncomingGRPCConfig generates a *tls.Config for incoming GRPC connections.
+func (c *Configurator) IncomingGRPCConfig() *tls.Config {
+	c.log("IncomingGRPCConfig")


What are these logs used for?

I am using them for debugging. They are showing the version of the config that is being used to generate the *tls.Config. Eg when you reload consul so that a new cert is being used, this log line should show a version increased by one. If it doesn't increase, something is wrong.

This is a backported commit based on: * hashicorp@11a571d * hashicorp#7086 To allow the grpc server used by consul connect to reload its tls info.

hanshasselberg force-pushed the auto_encrypt_grpc branch from ab4743d to ffa90a3 Compare January 17, 2020 22:22

hanshasselberg requested review from a team and lornasong January 17, 2020 22:29

hanshasselberg self-assigned this Jan 17, 2020

freddygv reviewed Jan 17, 2020

View reviewed changes

hanshasselberg added 2 commits January 20, 2020 10:13

setup grpc server with TLS config used across consul.

56d202c

add -https-port flag

22b019c

hanshasselberg force-pushed the auto_encrypt_grpc branch from ffa90a3 to 22b019c Compare January 20, 2020 12:23

do not assign cert

69d8910

hanshasselberg commented Jan 20, 2020

View reviewed changes

tlsutil/config.go Show resolved Hide resolved

hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 20, 2020

stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 20, 2020

hanshasselberg requested a review from rboyer January 20, 2020 12:54

hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 20, 2020

lornasong approved these changes Jan 21, 2020

View reviewed changes

tlsutil/config.go Show resolved Hide resolved

stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 21, 2020

freddygv approved these changes Jan 21, 2020

View reviewed changes

add comment

42dbe01

hanshasselberg merged commit 11a571d into master Jan 22, 2020

hanshasselberg deleted the auto_encrypt_grpc branch January 22, 2020 10:32

hanshasselberg mentioned this pull request Mar 9, 2020

grpc tls cert is not reloaded #7400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setup grpc server with auto_encrypt certs #7086

setup grpc server with auto_encrypt certs #7086

hanshasselberg commented Jan 17, 2020 •

edited

Loading

freddygv left a comment

freddygv Jan 17, 2020

hanshasselberg Jan 20, 2020

freddygv Jan 17, 2020 •

edited

Loading

freddygv Jan 17, 2020

hanshasselberg commented Jan 20, 2020 •

edited

Loading

lornasong left a comment

freddygv left a comment

freddygv Jan 21, 2020

hanshasselberg Jan 21, 2020

	opts = append(opts, grpc.Creds(*tlsConfigurator.TransportCredentials()))
	opts = append(opts, grpc.Creds(tlsConfigurator.TransportCredentials()))

setup grpc server with auto_encrypt certs #7086

setup grpc server with auto_encrypt certs #7086

Conversation

hanshasselberg commented Jan 17, 2020 • edited Loading

freddygv left a comment

Choose a reason for hiding this comment

freddygv Jan 17, 2020

Choose a reason for hiding this comment

hanshasselberg Jan 20, 2020

Choose a reason for hiding this comment

freddygv Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

freddygv Jan 17, 2020

Choose a reason for hiding this comment

hanshasselberg commented Jan 20, 2020 • edited Loading

lornasong left a comment

Choose a reason for hiding this comment

freddygv left a comment

Choose a reason for hiding this comment

freddygv Jan 21, 2020

Choose a reason for hiding this comment

hanshasselberg Jan 21, 2020

Choose a reason for hiding this comment

hanshasselberg commented Jan 17, 2020 •

edited

Loading

freddygv Jan 17, 2020 •

edited

Loading

hanshasselberg commented Jan 20, 2020 •

edited

Loading