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

fix: use wait-http-header to avoid printing cleartext credentials #3241

Conversation

claudio-benfatto
Copy link
Contributor

@claudio-benfatto claudio-benfatto commented Sep 15, 2021

When passing the basic auth credentials embedded within the Elasticsearch url, those are printed out to the console in case of authentication failure.

For example:

2021/09/15 14:16:55 Waiting for: https://datacatalog_user:password@vpc-datacatalog-storage-dev-masdasdizfasdadwu6ggmej2x4.eu-west-1.es.amazonaws.com:443

Passing those credentials as headers via the -wait-http-header parameter prevents this leakage and works as well.

I tested the container on AWS both with basic authentication enabled and disabled.

Related issues: #2933

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

pass basic auth credentials to -wait-http-header inside a header

basic auth credentials were printed on failure to the console
together with the elasticsearch url.
Passing them as headers explicitly avoids this unwanted behaviour
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this :)

@shirshanka shirshanka merged commit 2c63efa into datahub-project:master Sep 16, 2021
@claudio-benfatto claudio-benfatto deleted the fix_print_cleartext_credentials branch September 16, 2021 07:29
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