-
Notifications
You must be signed in to change notification settings - Fork 22
Use JWTs with sync-cli #434
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
Conversation
heyronhay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one suggestion in the comments, but also, we don't log the JWT token anywhere, do we? To prevent it from exposed. I guess this applies to API/APP keys as well.
|
|
||
| destination_auth = {} | ||
| if k := kwargs.get("destination_api_key"): | ||
| # JWT takes precedence over API keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we output a message somewhere if JWT and API keys are specified that the JWT overrides it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added on lines 147-152.
datadog_sync/utils/configuration.py
Outdated
| if (kwargs.get("source_jwt") and kwargs.get("source_api_key")) or ( | ||
| kwargs.get("destination_jwt") and kwargs.get("destination_api_key") | ||
| ): | ||
| logger.warning("Both a JWT and an API key were found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Both a JWT and an API key were found." -> "Both a JWT and an API key were found, JWT will take precedence" or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops yeah they need to know what is going on, updated
What does this PR do?
Allows forwarding JWTs.
Description of the Change
This allows for using JWTs instead of app/api keys.