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

[Core] Update the Main Credential regex to support #3509

Merged
merged 4 commits into from Sep 8, 2017

Conversation

nmuesch
Copy link
Contributor

@nmuesch nmuesch commented Sep 7, 2017

multiple types of API input in datadog.conf

Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.

What does this PR do?

Modified the regex used when checking for the API Key in datadog.conf or other locations. Currently we only support the syntax api_key: <KEY> However, this doesn't allow for a space before the : or using api_key = <KEY>

Motivation

Currently the API key isn't being fully redacted in the flare if it doesn't use the aforementioned format. It seems like K8s Agents use the = syntax by default.

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Performed a series of manual tests locally based on the above syntaxes.

Additional Notes

Anything else we should know when reviewing?

With this PR, the flare will be unsuccessful if the key is <YOUR_API_KEY_HERE> with a seemingly helpful message saying the API key is invalid. So the flare will not be sent at all, but the user sees this message.

Initially attempted to remove lambdas to make it a bit cleaner, however this doesn't work for the case where there are multiple API keys, comma delimited. We would need another function to be called in the replacement piece that split the matched group into segments.

multiple types of API input in datadog.conf
@@ -94,11 +94,11 @@ class Flare(object):
]
MAIN_CREDENTIALS = [
CredentialPattern(
re.compile('^\s*api_key:( *\w+(\w{5}) ?,?)+$'),
re.compile('^\s*api_key\s*[=, :]( *\w+(\w{5}) ?,?)+$'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this piece of code is overly complex, your patch is good but I'd make it a little bit simpler.

  1. we can change the regex to only capture in a group the last 5 digits of the api key
  2. instead of nesting maps and lambdas, proceed with a string replace using a backreference to the group from above

the following piece of code should do the same thing (untested):

CredentialPattern(
  re.compile(r'^\s*api_key\s*[=, :]\s*\w{27}(\w{5})$'),
  r'api_key: ***************************\1',
  'api_key'
)

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 definitely agree here. Removing the lambdas and just replacing everything but the last 5 chars does indeed do the trick. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it doesn't work for comma separated API keys, left more details in the notes based on our discussions

@xvello xvello added this to the 5.17.1 milestone Sep 7, 2017
@xvello
Copy link
Contributor

xvello commented Sep 7, 2017

Thanks @nmuesch, that new format is introduced in docker-dd-agent 12.0.5160+ 's new entrypoint. It would be great to add to 5.17.1.

@nmuesch nmuesch changed the title Update the Main Credential regex to support [Core] Update the Main Credential regex to support Sep 8, 2017
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

let's merge this now for 5.17.1, we can simplify the code if we have the opportunity in the future, but at least this change doesn't make the code more complex.

@olivielpeau olivielpeau merged commit fbe5964 into master Sep 8, 2017
@olivielpeau olivielpeau deleted the nick/fix_api_key_regex branch September 8, 2017 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants