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

Vulnerable data exposed with metrics endpoint #1428

Closed
marqc opened this issue Mar 18, 2019 · 6 comments · Fixed by #1442
Closed

Vulnerable data exposed with metrics endpoint #1428

marqc opened this issue Mar 18, 2019 · 6 comments · Fixed by #1442

Comments

@marqc
Copy link
Contributor

marqc commented Mar 18, 2019

Requirement - what kind of business use case are you trying to solve?

Keep credentials secure and expose app metrics at same time.

Problem - what in Jaeger blocks you from solving the requirement?

Seen in jaeger-ingester. Output from "/metrics" endpoint shows whole cmdline, which contains datastore credentials "--es.username" and "--es.password".

Sample:

{
"cmdline": [
"/usr/bin/jaeger-ingester",
"--ingester.deadlockInterval=0",
"--kafka.brokers=node-1.mycompany.com:9092,node-2.mycompany.com:9092,node-3.mycompany.com:9092,",
"--kafka.topic=request-tracing",
"--kafka.group-id=jaeger-ingester-v001",
"--kafka.encoding=json",
"--es.server-urls",
"https://es5-int.mycompany.com:443",
"--es.username",
"changeit",
"--es.password",
"changeit",
"--es.sniffer=false",
"--ingester.http-port",
"8080",
"--health-check-http-port",
"8081"
],
"jaeger.bulk_index.attempts": 2.8229e+06,
"jaeger.bulk_index.errors": 0,
...

Proposal - what do you suggest to solve the problem or improve the existing situation?

Either remove cmdline parameter or parse properties and use some whitelist or blacklist to hide auth credentials.

Any open questions to address

@jpkrohling
Copy link
Contributor

cmdline should not be there.

@yurishkuro
Copy link
Member

I think this is only if you use expvar (instead of the default prometheus) for metrics.

@jpkrohling
Copy link
Contributor

I still believe they should not be part of /metrics, independent of the metrics backend in use.

@yurishkuro
Copy link
Member

Yes but expvar is the standard Go mechanism for exposing process info, and using it as "metrics" is really a debugging step, not a recommended production setup.

Also, isn't passing passwords via command line flags a big No in any case? Those can be viewed via ps.

@jpkrohling
Copy link
Contributor

I guess we have a bunch of things that, combined, turn this into a potential security problem.

  • We are exposing the process info via the public port, instead of admin port
  • We are not even attempting at cleaning sensitive options
  • Users should not pass passwords via command line :-)

So:

  • Users should try not to use passwords via command line, even though it's more common now in the containers world than before in the bare-metal world
  • We should not add options like cmdline to the output of /metrics
  • We should really, really move /metrics to the admin port...

@marqc
Copy link
Contributor Author

marqc commented Mar 18, 2019

I am currently forced to use expvar as metrics backend because of bug #1200

As You suggested, I will change a way of providing config to use envs instead of cmdline args.

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 a pull request may close this issue.

3 participants