Allow specifying simple gRPC transport credentials #508

tcolgate · 2018-09-07T09:19:48Z

No description provided.

tcolgate · 2018-09-07T12:07:08Z

This is a preliminary PR to see if you are interested in this, if so I'll fix up the tests.

bwplotka

I feel like we should differentiate between server certs and client certs. What if that is different? Also you state that in description that those flags are for incoming request and then you use it for store client connections.

Also, what if each storeAPI will required different key?

cmd/thanos/flags.go

bwplotka · 2018-09-07T13:30:02Z

cmd/thanos/flags.go

 	grpcBindAddr := cmd.Flag("grpc-address", "Listen ip:port address for gRPC endpoints (StoreAPI). Make sure this address is routable from other components if you use gossip, 'grpc-advertise-address' is empty and you require cross-node connection.").
 		Default("0.0.0.0:10901").String()

 	grpcAdvertiseAddr := cmd.Flag("grpc-advertise-address", "Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used.").
 		String()

+	grpcTLSInsecure := cmd.Flag("grpc-tls-insecure", "Do not use gRPC transport credentials to verify connections").Default("true").Bool()


Let's make sure in the name to be clear that this touches incoming requests.

Just note that only thanos query calls those grpc Requests. Rest is only serving those.

cmd/thanos/main.go

tcolgate · 2018-09-07T13:51:50Z

For the first stab I went for the simplest setup, but TBH this isn't what I'd do for production services (this is how the gRPC examples set things up).
If may not be immediately obvious, but the client cert there is not a client cert, it's just a client CA to trust.

Ideally we'd have:

explicit CA for verifying clients
explicit CA for verifying servers
explicit client cert/key
explicit server cert/key
options for client verification

Unfortunately this ends up amounting to an awful lot of cli flags.
Perhaps we should stick to only offering server verification first, but make that clearer from the name?

grpc-server-cert
grpc-server-key
grpc-client-ca

Thoughts?

tcolgate · 2018-09-07T15:33:48Z

@Bplotka updated the arg names, they still aren't very pretty. If you have specific ideas, please do say. (personally I'd have something like --query.tls.caCert , --grpc.tls.cert --grpc.tls.key, some of the existing args seem to follow that kind of format, but I've not been able to discern a pattern to them.

opted for creds rather than tlsCreds, it's a little more accurate, and matches the functions they are then used with.

tcolgate · 2018-09-08T08:17:10Z

Mostly note to self. Will add client key/cert and server client verification CA. Need this (or actual per roc auth cress) for minimal secure use case

tcolgate · 2018-09-10T09:09:23Z

@Bplotka updated to provide full client cert and server side verification.

bwplotka

It looks really really nice! Some minor issues only. Thanks!

cmd/thanos/main.go

bwplotka · 2018-09-12T20:48:18Z

cmd/thanos/query.go

+			*insecure,
+			*srvcert,
+			*srvkey,
+			*srvclientca,


let's make it camelCase everywhere: srvKey srvClientCA etc

cmd/thanos/query.go

cmd/thanos/flags.go

cmd/thanos/query.go

bwplotka · 2018-09-13T11:22:02Z

Can we have some rebase?

tcolgate · 2018-09-13T11:40:43Z

@Bplotka opened a whole can of rebase!

tcolgate · 2018-09-13T11:56:21Z

@Bplotka fixed my dep woes, docs and deps updated.

cmd/thanos/main.go

bwplotka

Thanks for addressing the comments. Two more suggestions before merging.

cmd/thanos/main.go

cmd/thanos/flags.go

tcolgate · 2018-09-18T19:35:53Z

@bwplotka we're running this locally with seeming success. Have yet to confirm query -> multiple store gateways over a WAN, but single query to single gateway in cluster is fine.

bwplotka · 2018-09-19T12:26:54Z

Let's change the WIP title and will do review today (:

tcolgate · 2018-09-19T14:13:00Z

@bwplotka rebased on master, not quite sure what is going on with the Gopkg.lock, possible my version of dep is too old/new?

bwplotka · 2018-09-19T14:43:37Z

what do you use for deps? Can you use reset your changes on .lock and do make deps? this is installing and using pinned version of dep (:

tcolgate · 2018-09-19T14:57:10Z

@bwplotka reset it and make dep, looks better now

bwplotka · 2018-09-19T15:07:36Z

finally some tooling work paid off 😅

bwplotka

Nice work, I added some minor suggestions, but looks really good! Thanks!

cmd/thanos/main.go

bwplotka · 2018-09-19T19:57:31Z

cmd/thanos/main.go

@@ -227,6 +231,43 @@ func defaultGRPCServerOpts(logger log.Logger, reg *prometheus.Registry, tracer o
 			grpc_recovery.StreamServerInterceptor(grpc_recovery.WithRecoveryHandler(grpcPanicRecoveryHandler)),
 		),
 	}
+
+	if key == "" || cert == "" {


I think we need error if any of this is filled but not all. Also if clientCA is filled but those keys not.

not sure if you want a log and an error, or just an error, and is returning an errors.New OK?

cmd/thanos/flags.go

cmd/thanos/query.go

bwplotka · 2018-09-19T20:05:31Z

cmd/thanos/query.go

+	creds := credentials.NewTLS(tlsCfg)
+	dialOpts = append(dialOpts, grpc.WithTransportCredentials(creds))
+
+	return dialOpts, nil


ditto, we can just inline this.

bwplotka

Almost there, sorry for being so strict (: Hope you are not mad... but I think this is the very last suggestion and we are rdy to merge! Approving in advance.

bwplotka · 2018-09-21T18:08:38Z

cmd/thanos/main.go

+		level.Info(logger).Log("msg", "gRPC server TLS client verification enabled")
+	}
+
+	creds := credentials.NewTLS(tlsCfg)


i don't think you need creds variable

So our thinking in Thanos is - if you do a variable it means that you need this in more places than just one. Otherwise just inline (: Do you agree with this?

bwplotka · 2018-09-21T18:09:19Z

cmd/thanos/query.go

@@ -28,18 +31,24 @@ import (
 	"github.com/prometheus/prometheus/promql"
 	"github.com/prometheus/tsdb/labels"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials"
 	"gopkg.in/alecthomas/kingpin.v2"
 )

 // registerQuery registers a query command.


BTW what happened to GitHub - coloring for golang sucks ... now there is some red mark on go comments??

or is it only my browser?

It's doing it for me too, I've no idea what is going on there.

cmd/thanos/main.go

tcolgate · 2018-09-21T19:29:09Z

No, review is completely cool, very important to match the project style (it's also a better patch for it)

bwplotka

LGTM 🏷️

bwplotka · 2018-09-21T19:49:45Z

Thanks! That is epic addition.

tcolgate mentioned this pull request Sep 7, 2018

Securing gRPC? #468

Closed

tcolgate changed the title ~~Allow specifying simple gRPC transport credentials~~ [WIP] Allow specifying simple gRPC transport credentials Sep 7, 2018

bwplotka requested changes Sep 7, 2018

View reviewed changes

tcolgate force-pushed the grpccreds branch 2 times, most recently from fb740ea to 25678f5 Compare September 7, 2018 15:30

tcolgate force-pushed the grpccreds branch from 25678f5 to c1ff3db Compare September 7, 2018 15:36

tcolgate force-pushed the grpccreds branch from c1ff3db to e2ccb50 Compare September 10, 2018 09:08

bwplotka requested changes Sep 12, 2018

View reviewed changes

bwplotka mentioned this pull request Sep 12, 2018

Released v0.1.0 #516

Merged

4 tasks

tcolgate force-pushed the grpccreds branch 3 times, most recently from b448646 to 0c41c21 Compare September 13, 2018 09:55

tcolgate force-pushed the grpccreds branch from 0c41c21 to eb0d572 Compare September 13, 2018 11:40

tcolgate force-pushed the grpccreds branch from eb0d572 to c0027a5 Compare September 13, 2018 11:55

bwplotka reviewed Sep 13, 2018

View reviewed changes

cmd/thanos/main.go Outdated Show resolved Hide resolved

bwplotka requested changes Sep 13, 2018

View reviewed changes

cmd/thanos/main.go Outdated Show resolved Hide resolved

cmd/thanos/flags.go Outdated Show resolved Hide resolved

tcolgate force-pushed the grpccreds branch 4 times, most recently from 00622b2 to 113a71e Compare September 14, 2018 15:08

tcolgate changed the title ~~[WIP] Allow specifying simple gRPC transport credentials~~ Allow specifying simple gRPC transport credentials Sep 19, 2018

tcolgate force-pushed the grpccreds branch from 113a71e to ec28508 Compare September 19, 2018 14:11

tcolgate force-pushed the grpccreds branch from ec28508 to 1d8fa4e Compare September 19, 2018 14:55

bwplotka reviewed Sep 19, 2018

View reviewed changes

tcolgate force-pushed the grpccreds branch from 065a7d8 to f43bd6e Compare September 20, 2018 08:31

bwplotka approved these changes Sep 21, 2018

View reviewed changes

Tristan Colgate added 2 commits September 21, 2018 20:41

Allow specifying simple gRPC transport credentials

33b4c31

fix review comments

35284f6

tcolgate force-pushed the grpccreds branch from f43bd6e to 35284f6 Compare September 21, 2018 19:46

bwplotka approved these changes Sep 21, 2018

View reviewed changes

bwplotka merged commit 1e1f9e5 into thanos-io:master Sep 21, 2018

benjaminhuo mentioned this pull request May 8, 2019

[Feature] Per-Store TLS configuration in Thanos Query #977

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying simple gRPC transport credentials #508

Allow specifying simple gRPC transport credentials #508

tcolgate commented Sep 7, 2018

tcolgate commented Sep 7, 2018

bwplotka left a comment

bwplotka Sep 7, 2018

tcolgate Sep 7, 2018

tcolgate commented Sep 7, 2018

tcolgate commented Sep 7, 2018 •

edited

Loading

tcolgate commented Sep 8, 2018

tcolgate commented Sep 10, 2018

bwplotka left a comment

bwplotka Sep 12, 2018

bwplotka commented Sep 13, 2018

tcolgate commented Sep 13, 2018

tcolgate commented Sep 13, 2018

bwplotka left a comment

tcolgate commented Sep 18, 2018

bwplotka commented Sep 19, 2018

tcolgate commented Sep 19, 2018

bwplotka commented Sep 19, 2018

tcolgate commented Sep 19, 2018

bwplotka commented Sep 19, 2018

bwplotka left a comment

bwplotka Sep 19, 2018

tcolgate Sep 20, 2018

bwplotka Sep 19, 2018

bwplotka left a comment

bwplotka Sep 21, 2018

bwplotka Sep 21, 2018

bwplotka Sep 21, 2018

tcolgate Sep 21, 2018

tcolgate commented Sep 21, 2018

bwplotka left a comment

bwplotka commented Sep 21, 2018

Allow specifying simple gRPC transport credentials #508

Allow specifying simple gRPC transport credentials #508

Conversation

tcolgate commented Sep 7, 2018

tcolgate commented Sep 7, 2018

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcolgate commented Sep 7, 2018

tcolgate commented Sep 7, 2018 • edited Loading

tcolgate commented Sep 8, 2018

tcolgate commented Sep 10, 2018

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Sep 13, 2018

tcolgate commented Sep 13, 2018

tcolgate commented Sep 13, 2018

bwplotka left a comment

Choose a reason for hiding this comment

tcolgate commented Sep 18, 2018

bwplotka commented Sep 19, 2018

tcolgate commented Sep 19, 2018

bwplotka commented Sep 19, 2018

tcolgate commented Sep 19, 2018

bwplotka commented Sep 19, 2018

bwplotka left a 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

bwplotka left a 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

tcolgate commented Sep 21, 2018

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Sep 21, 2018

tcolgate commented Sep 7, 2018 •

edited

Loading