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

Allow to use TLS with ES basic auth #1388

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

pavolloffay
Copy link
Member

Resolves #1327

Signed-off-by: Pavol Loffay ploffay@redhat.com

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@yurishkuro
Copy link
Member

I don't follow the use case. If you are using username/password, you only need to talk over https. If you use a cert, your identity is already encoded in it, why pass pwd too?

@pavolloffay
Copy link
Member Author

I think @jpkrohling can explain this well.

The basic header is used for authentication, whereas provided CA cert will enable TLS for secure communication.

@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #1388 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1388   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         164     164           
  Lines        7453    7460    +7     
======================================
+ Hits         7453    7460    +7
Impacted Files Coverage Δ
plugin/storage/es/options.go 100% <100%> (ø) ⬆️
plugin/storage/es/dependencystore/storage.go 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8ab694...4bc92be. Read the comment docs.

@jpkrohling
Copy link
Contributor

If you use a cert, your identity is already encoded in it, why pass pwd too?

From what I understand from this PR, this is not about enabling client cert auth, but rather, configuring the client to trust a CA, so that the server's cert is trusted.

@pavolloffay
Copy link
Member Author

configuring the client to trust a CA, so that the server's cert is trusted.

That makes sense. Is my hypothesis correct that it will use TLS and basic auth is used to verify the identity?

@jpkrohling so does this PR makes sense. It should resolve #1327

@jpkrohling
Copy link
Contributor

It certainly makes sense, especially for platforms like Kubernetes where a CA generates certs for services. The cert for this (root) CA should then be trusted by all services running in the platform, as opposed to trusting each service's cert individually.

@yurishkuro
Copy link
Member

ok. My only concern is that the CLI options are now misleading, since it's natural to expect that -tls- options would only be considered if -tls-enabled flag is set, which is not the case here anymore.

@pavolloffay
Copy link
Member Author

We allow CA also with token, I will change the flag message.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay
Copy link
Member Author

My only concern is that the CLI options are now misleading, since it's natural to expect that -tls- options would only be considered if -tls-enabled flag is set, which is not the case here anymore

I have changed flag messages saying that es.tls enables tls with client certs.

@yurishkuro
Copy link
Member

Lgtm

@pavolloffay pavolloffay merged commit cbf9e02 into jaegertracing:master Feb 28, 2019
@ghost ghost removed the review label Feb 28, 2019
annanay25 pushed a commit to annanay25/jaeger that referenced this pull request Mar 1, 2019
* Allow to use TLS with ES basic auth

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Change flag messages

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants