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

Drop support for application keys #127

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

Mr0grog
Copy link
Collaborator

@Mr0grog Mr0grog commented Nov 11, 2024

Every so often folks run into trouble by mixing up API and application keys. Back in #118, I added more documentation and error messaging to help clarify potential issues, but I also noticed that the application keys don’t actually seem to be needed.

I did some testing and it turns out they definitely are not needed for submitting metrics, distributions, events, or logs (we don’t do the last two, but there is an open issue about supporting events). As far as I can tell, there’s no way for Datadog users to tell what application key something was submitted with, so they really don’t seem to have any value at all for our current use cases. Given the narrow goals of this library, we probably don’t ever want to support more complicated stuff that we would need application keys for, either.

Support was originally added by @gert-fresh back in 2015/2016 in PR #7. There’s not much description there, but I think it was because this library used to configure a single, globally available Datadog client, so a developer using this library might also want to use that client to do other things requiring an application key. That’s no longer how things work here today (each reporter instance has its own client, and those clients are not available to user code). Assuming I understand that correctly, the original use case is obsolete.

⚠️ Removing this feature is actually more work than keeping it (see code for handling different numbers of arguments and logging deprecations), but I want to remove it because its existence encourages users to include an unnecessary credential in their apps, which increases their security risks. Maybe it’s not worth worrying about, though?


Total side note: I haven’t touched this package in a while, but a few recent issues brought my attention back to it, so I thought I’d do some work on old issues and ideas that have been kicking around here. I figure I’ll work on a bunch of updates that might deprecate some things (like this, or #125) in the next week or so, and then cut another release at the start of next year that actually removes all the deprecations and cleans things out. Maybe we require a more recent version of Node.js then, too, so we can update some of our dev tooling (much of which has moved on to requiring Node.js 18+, which we only require 12+ here).

@Mr0grog Mr0grog force-pushed the drop-dangerous-unused-credentials branch from bc9290e to 04b0243 Compare November 18, 2024 00:28
@Mr0grog
Copy link
Collaborator Author

Mr0grog commented Nov 18, 2024

Rebased to merge cleanly on top of main.

@Mr0grog Mr0grog force-pushed the drop-dangerous-unused-credentials branch from 0c3938f to 787aee4 Compare November 20, 2024 07:52
Application keys are not actually needed for submitting metrics or distributions (or events or logs, in case we ever wind up supporting that), so specifying them risks exposing a potentially more powerful credential for no real reason. When support for them was originally added, this library used a single, globally configured Datadog client, so it made sense to allow configuring it with an app key in case a developer using this library *also* wanted to use that global API client for other things. That's no longer the case (the client is not accessible from outside this library), so there's no reason to continue to support this, which might encourage dangerous use.
@Mr0grog Mr0grog force-pushed the drop-dangerous-unused-credentials branch from 787aee4 to f1d9183 Compare November 28, 2024 23:31
@Mr0grog Mr0grog merged commit 488adc7 into main Nov 28, 2024
10 checks passed
@Mr0grog Mr0grog deleted the drop-dangerous-unused-credentials branch November 28, 2024 23:36
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.

1 participant