Enable TLS authentication for etcd #489

shtripat · 2017-12-18T08:56:02Z

Signed-off-by: Shubhendu shtripat@redhat.com

centos-ci · 2017-12-18T08:56:03Z

Can one of the admins verify this patch?

shtripat · 2017-12-18T08:58:03Z

This is a work in progress PR. Need to verify the changes with certificates in place.
PLEASE DO NOT MERGE.

prashanthpai · 2017-12-18T11:52:13Z

add to whitelist

shtripat · 2017-12-21T12:41:02Z

Verified the changes with below steps

Followed https://coreos.com/os/docs/latest/generate-self-signed-certificates.html for generating self-signed certificates for etcd using cfssl
Added the certificate details in gd2 conf file as below

workdir: "/var/lib/gd2"
peeraddress: "{hostname}:24008"
clientaddress: "{hostname}:24007"
etcdcurls: "https://{hostname}:2379"
etcdpurls: "https://{hostname}:2380"
usetls: true
servercertfile: "/var/lib/gd2/certificates/server.pem"
serverkeyfile: "/var/lib/gd2/certificates/server-key.pem"
cafile: "/var/lib/gd2/certificates/ca.pem"
trustedcafile: "/var/lib/gd2/certificates/ca.pem"
clientcertfile: "/var/lib/gd2/certificates/client.pem"
clientkeyfile: "/var/lib/gd2/certificates/client-key.pem"
peercertfile: "/var/lib/gd2/certificates/member1.pem"
peerkeyfile: "/var/lib/gd2/certificates/member1-key.pem"

Started gd2 using command ./build/glusterd2 --config conf.yaml
Verified etcd connection using CURL command curl --cacert /var/lib/gd2/certificates/ca.pem --cert /var/lib/gd2/certificates/client.pem --key /var/lib/gd2/certificates/client-key.pem -XGET https://{hostname}:2379/v2/keys and it shows the below output

{"action":"get","node":{"dir":true}}

Tried running curl without passing certificate details like curl -XGET https://{hostname}:2379/v2/keys and the output is as below

curl: (60) SSL certificate problem: unable to get local issuer certificate
More details here: https://curl.haxx.se/docs/sslcerts.html

curl performs SSL certificate verification by default, using a "bundle"
 of Certificate Authority (CA) public keys (CA certs). If the default
 bundle file isn't adequate, you can specify an alternate file
 using the --cacert option.
If this HTTPS server uses a certificate signed by a CA represented in
 the bundle, the certificate verification probably failed due to a
 problem with the certificate (it might be expired, or the name might
 not match the domain name in the URL).
If you'd like to turn off curl's verification of the certificate, use
 the -k (or --insecure) option.
HTTPS-proxy has similar options --proxy-cacert and --proxy-insecure.

shtripat · 2017-12-21T12:41:41Z

@kshlm @aravindavk @atinmu @prashanthpai please review the changes.

shtripat · 2017-12-21T15:03:55Z

Also tested by disabling TLS as below

workdir: "/var/lib/gd2"
peeraddress: "{hostname}:24008"
clientaddress: "{hostname}:24007"
etcdcurls: "http://{hostname}:2379"
etcdpurls: "http://{hostname}:2380"
usetls: false

and works as expected. Tried now the command curl -XGET http://{hostname}:2379/v2/keys and shows the below output

{"action":"get","node":{"dir":true}}

kshlm

You should definitely move the defaults that have been set in elasticetcd out, and into GD2.

Also, I'd like to understand what all the different certs are for. And if possible, reduce the number.

kshlm · 2017-12-22T07:56:07Z

glusterd2/store/config.go

+
+	Dir           string
+	ConfFile      string
+	SrvrCertFile  string


What's the difference between Server, Client and Peer here?

Why do we need so many certs and CA files? Couldn't we just use the same cert and ca for all?
Ideally, GD2 should be using the same identity ie., the same certs for all servers it exposes (rest, grpc, sunrpc, elasticetcd), and client connections it establishes (grpc, elasticetcd, external etcd). The only place I see we may possibly be required to use a different cert, would be the external etcd case, where the etcd provider may provide the cert for us.

Server has its own certificate and key and server should be started that set.
Clients get a public key and certificate which they use for getting authenticated at server.
Similarly peer-to-peer communication is also authenticated using a pair of certificate and key.

Same server certificates and keys it possible as we need below json input for cfssl

{ "CN": "My own CA", "hosts": [ "{host1}", "{host2}", .......... ], "key": { "algo": "ecdsa", "size": 256 }, "names": [ { "C": "US", "ST": "CA", "L": "San Francisco" } ] }

I think similar thing is possible for clients as well with below json input

{ "CN": "My own CA", "hosts": ["{clnt1}", "clnt2", ......], "key": { "algo": "ecdsa", "size": 256 }, "names": [ { "C": "US", "ST": "CA", "L": "San Francisco" } ] }

Using same certificates for rest, grpc, sunrpc, elasticetcd is something we might need to check I feel.

kshlm · 2017-12-22T08:06:24Z

glusterd2/store/remote.go

+	var e error
+	if conf.UseTLS {
+		tlsConfig = &tls.Config{
+			MinVersion: tls.VersionTLS10,


These are the defaults for these. There is no need to set them unless you change to something different. I'd prefer if we just stick to tls1.2, but that's just my opinion.

@kshlm do you want min version to be set as 1.2?

kshlm · 2017-12-22T08:16:08Z

pkg/elasticetcd/config.go

+	DefaultName          = "elasticetcd"
+	DefaultIdealSize     = 3
+	DefaultDir           = "."
+	DefaultCertFile      = "/var/lib/gd2/certificates/server.crt"


The elasticetcd package should not contain defaults referencing GD2. It is expected to remain a standalone package, without any ties to the rest of GD2. Please move these defaults to GD2, preferrably to the store package for now.

Also, we shouldn't hardcode paths, as GD2 can use different working/config/state directories. Instead, I'd suggest setting a default directory name for a certs directory, and default filenames for the certs. Then you could build the paths based on the configured directories when you need to pass the paths. For eg.

const ( DefaultCertDir = "certificates" DefaultCertFile = "server.crt" ) serverCert := path.Join(config.GetString("localstatedir", DefaultCertDir, DefaultCertFile)

Agree with dirname and filename thing. Will change accordingly.

Now its removed from EE config and whatever value is set in config file would be considered as full path if the certificates. User can choose to keep the certs in any path.

kshlm · 2017-12-22T08:17:52Z

pkg/elasticetcd/config.go

+		CURLs:         defaultCURLs,
+		PURLs:         defaultPURLs,
+		IdealSize:     DefaultIdealSize,
+		CertFile:      DefaultCertFile,


It's not necessary for the cert files to be set in the default config, as they are not necessary for the operation of elasticetcd.

Ack. Will remove these.

shtripat · 2017-12-22T08:38:25Z

You should definitely move the defaults that have been set in elasticetcd out, and into GD2.

Ack.

Also, I'd like to understand what all the different certs are for. And if possible, reduce the number.

The no. of certificates is as per the guidelines from etcd documents. For sure server and client certificates are to be different. Its like, using CA we create server certificates which is private to server. The client certificates are to be given to clients which connect to the server. Yes the client certificates could be same for all. Still I strongly feel the peer certificates should be different from client-server combo. We should not mix client-to-server communication with peer-to-peer communication.

So in total we need at least below files I feel

ca.pem
server.pem, server-key.pem
client.pem, client-key.pem
peer.pem, peer-client.pem

comments??

kshlm · 2017-12-22T09:34:30Z

The recommendations in the etcd document are for general etcd deployments, where you have an etcd cluster, and external consumers. In that case it makes sense to split TLS for internal etcd cluster interface (peer cert), and external client API (server cert and client cert).

In GD2 with elasticetcd, we are a single entity. We are the etcd cluster doing the internal operations, and serving etcd kv store with ourselves as clients. So, it is okay (IMO) for us to be using a single certificate which identifies us, for all cases.

If we really need to, we could split up TLS configuration for internal and external traffic. A cert and CA for internal traffic (etcd peer traffic and GD2 gRPC traffic). And a seperate cert and CA for external facing interfaces (GD2 ReST, SunRPC, etcd client API). External clients (GD2 or etcd) get their own certs, signed using the extrenal interface CA.

shtripat · 2017-12-22T09:53:07Z

I agree to the suggestion of two sets one for internal traffic and one for external traffic.
Should we include gRPC, SunRPC and GD2 REST as well as part of this PR only?

I feel its better we rename the certs and key configuration names accordingly in this PR. And we have separate PRs for enabling TLS for gRPC, SunRPC and GD2 REST using the same cert configurations.

In that case I suggest below configurations to be maintained

CAFile: The pem file for certifying authority
GD2IntCertFile: Certificate for internal communication (etcd peer-to-peer and gRPC)
GD2IntCertFile: Key for internal communication (etcd peer-to-peer and gRPC)
GD2ExtCertFile: Certificate for external communication (GD2 REST, SunRPC and etcd client)
GD2ExtKeyFile: Key for external communication (GD2 REST, SunRPC and etcd client)

Suggestions? @prashanthpai @aravindavk @amarts @atinmu
@kshlm are we ok with the names of configurations? or may be we can have name suggestions.

kshlm · 2017-12-22T10:53:38Z

Let it be 'Client*' and 'Peer*' for external and internal respectively. We already call the addresses we listen on as ClientAddress and PeerAddress. And have these configs in the main GD2 config.

shtripat · 2017-12-22T16:43:55Z

Now we have client and server certs only. Instead of calling peer certs its just CertFile and KeyFile for server.

shtripat · 2017-12-27T08:15:04Z

Can others also please review the PR?

atinmu · 2017-12-29T10:01:47Z

@shtripat What's pending on this patch? Are you yet to address comments from @kshlm ? I see this patch needs a rebase.

shtripat · 2017-12-29T10:11:09Z

@atinmu comments from Kaushal taken care already. Will rebase and push again. @kshlm needs to have a look once more.

kshlm

@shtripat Sorry for the really late review.

The PR looks good. A few of the files haven't been gofmted, and is causing CI failure. You'll need to fix that.

As you do that, I also request that you move the config option keys out of store and into the main config. They're more suitable there.

kshlm · 2018-01-03T08:12:39Z

glusterd2/store/config.go

+	certFile         = "cert-file"
+	keyFile          = "key-file"
+	caFile           = "caFile"
+	clntCertFile     = "client-cert-file"


It would be preferrable to add this to glusterd2/config.go. I'd want to use the same options for tls certs for other servers.

Also, for uniformity change 'caFile' to 'ca-file'

done. will update the PR accordingly.

Moving the constants to glusterd2/config.go causes cyclic dependencies as below

package github.com/gluster/glusterd2/glusterd2 imports github.com/gluster/glusterd2/glusterd2/daemon imports github.com/gluster/glusterd2/glusterd2/events imports github.com/gluster/glusterd2/glusterd2/store imports github.com/gluster/glusterd2/glusterd2

How to go about this?

I feel rather we can have a commons package with all the common stuff defined which could be used across modules like glusterd2, glustercli etc.

@kshlm thoughts??

kshlm · 2018-01-03T11:33:36Z

@shtripat Still failing lint checks on centos-ci.

10:06:21 ./pkg/elasticetcd/client.go does not conform to gofmt rules. Run: gofmt -s -w ./pkg/elasticetcd/client.go
10:06:21 ./pkg/elasticetcd/config.go does not conform to gofmt rules. Run: gofmt -s -w ./pkg/elasticetcd/config.go
10:06:22 ./commons/constants.go:4:2: exported const UseTLS should have comment (or a comment on this block) or be unexported

Also, when fixing this. We keep common packages under pkg/.

shtripat · 2018-01-03T11:40:05Z

Ah I ran only ran gofmt -d {dir}. Will do the needed and update.
Also is it fine if I move commons/constants.go to pkg/commons/constants.go ?

prashanthpai · 2018-01-03T17:05:48Z

pkg/commons/constants.go

@@ -0,0 +1,11 @@
+package commons


We have tried to avoid doing something like this if the reason was just to circumvent cyclic dependency. I'm okay duplicating this if multiple packages require just these constants.

@kshlm @aravindavk what is your thought on this?

I'm actually okay with doing this, as I don't want to duplicate code.

@prashanthpai also its not just about avoiding the cyclic dependency. Rather its more to do with reuse of common constants. I personally agree with @kshlm here. @aravindavk ?

commons package looks good to me, otherwise we will end up editing in multiple places if we have to change.

Okay. Fair enough.

I'm afraid that we might end up putting lot more stuff (like struct definitions) under common because of its name. I want to discourage that practice. IIUC, the pkg directory is meant to be home for reusable code like libraries which can be used by other projects too.

What are you thoughts about naming this package as constants because that's what it provides ?

You mean a separate package constants parallel to pkg? I am fine with that.

Signed-off-by: Shubhendu <shtripat@redhat.com>

shtripat force-pushed the etcd-auth branch from d277dfc to 130e334 Compare December 18, 2017 08:57

shtripat force-pushed the etcd-auth branch 2 times, most recently from 561fd25 to 01bac99 Compare December 18, 2017 09:59

shtripat force-pushed the etcd-auth branch from 01bac99 to c5cdc5d Compare December 19, 2017 05:00

prashanthpai added this to Doing in GlusterD-2.0 Rogue One Dec 19, 2017

shtripat force-pushed the etcd-auth branch 3 times, most recently from 775a5e9 to 24969e9 Compare December 21, 2017 12:31

shtripat mentioned this pull request Dec 22, 2017

Etcd access authentication and filtering #262

Open

3 tasks

kshlm suggested changes Dec 22, 2017

View reviewed changes

shtripat force-pushed the etcd-auth branch from 24969e9 to 5c094f0 Compare December 22, 2017 09:05

shtripat force-pushed the etcd-auth branch from 5c094f0 to 0ee3d28 Compare December 22, 2017 16:42

shtripat force-pushed the etcd-auth branch from 0ee3d28 to 26258e6 Compare December 29, 2017 11:28

kshlm suggested changes Jan 3, 2018

View reviewed changes

shtripat force-pushed the etcd-auth branch from 51cf64c to 465e62f Compare January 3, 2018 10:02

shtripat force-pushed the etcd-auth branch from 465e62f to e456dec Compare January 3, 2018 11:55

prashanthpai reviewed Jan 3, 2018

View reviewed changes

Shubhendu added 2 commits January 4, 2018 19:53

Enable TLS authentication for etcd

5516ea7

Signed-off-by: Shubhendu <shtripat@redhat.com>

Incorporated review comments

ec2d70f

Signed-off-by: Shubhendu <shtripat@redhat.com>

shtripat force-pushed the etcd-auth branch from e456dec to be3f2af Compare January 4, 2018 14:24

Incorporated new set of review comments

1d4e6f9

Signed-off-by: Shubhendu <shtripat@redhat.com>

shtripat force-pushed the etcd-auth branch from be3f2af to 1d4e6f9 Compare January 5, 2018 04:27

Merge branch 'master' into etcd-auth

e0f90c8

kshlm approved these changes Jan 8, 2018

View reviewed changes

kshlm merged commit 9160b56 into gluster:master Jan 8, 2018

prashanthpai moved this from Doing to Done in GlusterD-2.0 Rogue One Jan 25, 2018

JohnStrunk mentioned this pull request Jun 26, 2018

TLS support gluster/anthill#9

Open

Enable TLS authentication for etcd #489

Enable TLS authentication for etcd #489

Conversation

shtripat commented Dec 18, 2017

centos-ci commented Dec 18, 2017

shtripat commented Dec 18, 2017

prashanthpai commented Dec 18, 2017

shtripat commented Dec 21, 2017

shtripat commented Dec 21, 2017

shtripat commented Dec 21, 2017 • edited

kshlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtripat Dec 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtripat commented Dec 22, 2017 • edited

kshlm commented Dec 22, 2017

shtripat commented Dec 22, 2017

kshlm commented Dec 22, 2017 • edited

shtripat commented Dec 22, 2017

shtripat commented Dec 27, 2017

atinmu commented Dec 29, 2017

shtripat commented Dec 29, 2017

kshlm left a comment

Choose a reason for hiding this comment

kshlm Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshlm commented Jan 3, 2018

shtripat commented Jan 3, 2018

prashanthpai Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtripat Jan 4, 2018 • edited

Choose a reason for hiding this comment

shtripat commented Dec 21, 2017 •

edited

shtripat Dec 22, 2017 •

edited

shtripat commented Dec 22, 2017 •

edited

kshlm commented Dec 22, 2017 •

edited

kshlm Jan 3, 2018 •

edited

prashanthpai Jan 3, 2018 •

edited

shtripat Jan 4, 2018 •

edited