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

Clarified consumer-to-query security support. #522

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

tillig
Copy link
Contributor

@tillig tillig commented Aug 25, 2021

Signed-off-by: Travis Illig [email protected]

Which problem is this PR solving?

Resolves #475

Short description of the changes

Clarification on what is supported for encryption and authentication in collector-to-query communications. Previously the green checkmark indicated there was support but the text was inconsistent with that; this resolves the ambiguity.

@@ -38,7 +38,7 @@ Clients can be configured to communicate directly with the Collector via HTTP. U

## Consumers to Query Service

* {{< check_yes >}} HTTP - no TLS/authentication.
* {{< check_yes >}} gRPC - no TLS/authentication.
* {{< check_yes >}} HTTP - TLS supported, no authentication supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive my ignorance as I don't know much about TLS; I thought authentication is built into TLS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLS is about encrypting the communication channel, not proving you are who you say you are. mTLS (mutual TLS) has authentication built in because it involves an exchange of client certificates.

Think about it like this - you visit https://www.amazon.com from your browser. That's using TLS, but Amazon doesn't know who you are until you authenticate (e.g., username + password). Same principle here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for that explanation, I didn't know about mTLS, but did a bit of reading about it; I guess with TLS, the server is authenticated, but not the client, and this is the gap that mTLS fills.

How did you determine that HTTP doesn't support mTLS?

I was looking at the source code and I think here is where we enable the RequireAndVerifyClientCert flag (mTLS).

IIUC it looks like that RequireAndVerifyClientCert flag is enabled when the .tls.client-ca option is enabled, and the initialization of these flags appear to be done for both gRPC and HTTP in the query service.

It's quite likely I'm misunderstanding something though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going by the latest comment on the related issue since I mentioned I don't actually know the answer. If I need to fix it, please do let me know. We're outside my Go + Jaeger source expertise here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I think both gRPC and HTTP servers now support mTLS. I closed #2249.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that as of 1.22? I want to make sure I update the right version(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's right.

@albertteoh
Copy link
Contributor

lgtm, thanks @tillig!

## Client to Collector section also seems like it needs updating (do you agree @yurishkuro?), but perhaps we can address that in another PR.

@@ -38,7 +38,7 @@ Clients can be configured to communicate directly with the Collector via HTTP. U

## Consumers to Query Service

* {{< check_yes >}} HTTP - no TLS/authentication.
* {{< check_yes >}} gRPC - no TLS/authentication.
* {{< check_yes >}} HTTP - TLS with client cert authentication supported.
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could use the same text as for Cassandra above: "TLS with mTLS supported"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the same text from the Agent to Collector section for gRPC. Should I switch them all to "TLS with mTLS supported" ?

@tillig
Copy link
Contributor Author

tillig commented Aug 25, 2021

I don't mind fixing Client to Collector but I don't know what it should be or for which versions. If you tell me, I can fix it. Or, yeah, a different PR. I was really just hoping to disambiguate the green check vs "not supported" text. 😄

@yurishkuro yurishkuro merged commit cfa0f34 into jaegertracing:master Aug 25, 2021
@yurishkuro
Copy link
Member

no worries, good as is. Thanks!

@tillig tillig deleted the issue-475 branch August 25, 2021 19:52
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.

Change information about TLS support for the query service
3 participants