Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/thanos/receive: remote-write client+server TLS #1668

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

squat
Copy link
Member

@squat squat commented Oct 18, 2019

This commit gives the Thanos receive component the capability to use TLS
in both the remote-write client and server. This means that Thanos
receive can now authenticate all requests.

In order to accomplish this change, this commit abstracts the majority
of the logic of defaultGRPCServerOpts into a reusable func for gRPC
and HTTP servers and creates a similar func for TLS client
configurations.

Signed-off-by: Lucas Servén Marín [email protected]

cc @bwplotka @brancz, this is following our conversation yesterday about missing authn and encryption options for Thanos receive.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM generally, some small nits only 👍

@@ -248,16 +248,28 @@ func registerMetrics(mux *http.ServeMux, g prometheus.Gatherer) {

func defaultGRPCServerOpts(logger log.Logger, cert, key, clientCA string) ([]grpc.ServerOption, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is literally defaultGRPCTLSServerOpts right? no other opts (:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me (:

}

func defaultTLSClientOpts(logger log.Logger, secure bool, cert, key, caCert, serverName string) (*tls.Config, error) {
if !secure {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this does not make sense from a high level, but I think... let's remove this. With this change if someone use secure false and key and cert set we silently ignore key and cert. I think failing would be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case should we remove the -secure flag as well? I also think it is confusing to explicitly have to opt into client TLS in addition to specifying the certs. but it's a breaking change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes - secure only makes sense if you have certs (?)

Copy link
Member Author

@squat squat Oct 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'm looking at this again and we actually need -secure in order to enable TLS when I don't provide a client cert/key, e.g. I want to use TLS but no client authentication. The issue here is that every gRPC ClientConn created by the querier re-uses the same set of []DialOpt, which must be either secure OR insecure, but not both. For this reason, we need the -secure flag to be able to tell my gRPC client that I want to enable TLS even if not providing any other flags, like a cert and key.

Nevertheless, this is only a requirement for the gRPC client. The HTTP client can be given a TLS config even when connected to non-HTTPS servers and thus doesn't need a -secure flag.

@squat squat force-pushed the clienttls branch 2 times, most recently from cd01b57 to 265afbb Compare October 18, 2019 22:27
This commit gives the Thanos receive component the capability to use TLS
in both the remote-write client and server. This means that Thanos
receive can now authenticate all requests.

In order to accomplish this change, this commit abstracts the majority
of the logic of `defaultGRPCServerOpts` into a reusable func for gRPC
and HTTP servers and creates a similar func for TLS client
configurations.

Signed-off-by: Lucas Servén Marín <[email protected]>
@squat
Copy link
Member Author

squat commented Oct 22, 2019

ping @bwplotka (:

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thx 👍

@bwplotka bwplotka merged commit 7005b65 into thanos-io:master Oct 23, 2019
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
This commit gives the Thanos receive component the capability to use TLS
in both the remote-write client and server. This means that Thanos
receive can now authenticate all requests.

In order to accomplish this change, this commit abstracts the majority
of the logic of `defaultGRPCServerOpts` into a reusable func for gRPC
and HTTP servers and creates a similar func for TLS client
configurations.

Signed-off-by: Lucas Servén Marín <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants