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

feat(inputs.influxdb_listener): Add token based authentication #13610

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Jul 12, 2023

resolves #9445

This PR adds token based authentication to the InfluxDB v1 listener to complement the authentication methods supported. This is especially relevant to get a forward compatibility with v2.x.

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jul 12, 2023
@srebhan srebhan added area/influxdb ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Jul 12, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

@srebhan thanks for this PR and looking at this feature request.

Looking at the original request, the user seems to be asking for "token" authentication. When I think about token authentication, especially in reference to v2, I think of the generated string API tokens, not a JWT token.

Effectively I was thinking we would have a a token value just like we have in the influxdb_v2 output.

Was that not your understanding?

Thanks!

@srebhan
Copy link
Contributor Author

srebhan commented Jul 13, 2023

@powersj I talked to Geoffrey yesterday and he pointed me to the 1.8 server code. I reproduced what is there. The downside of a fixed token is that it cannot have an expiry date and token without an expiry won't work with InfluxDB 1.8...

@powersj
Copy link
Contributor

powersj commented Jul 13, 2023

That is not my understanding of reading the feature request. The background is a user is transitioning from v1 to v2 and wants that process to go more smoothly. The current behavior lists:

Telegraf InfluxDB Listener v1 input plugin only supports username/password not tokens.

They want the v1 input plugin to support tokens which is a v2 auth mechanism. Looking further down they use the go client library as an example:

Use the form username:password for an authentication token. Example: my-user:my-password. Use an empty string ("") if the server doesn't require authentication.

The JWT tokens are a legacy 1.8 auth mechanism, which is why I still think the correct item is allowing a username/password via a token value?

@srebhan
Copy link
Contributor Author

srebhan commented Jul 14, 2023

Sorry, but isn't this what I added in line 116? What am I missing here? I just added all documented auth mechanisms as per documentation, so you can use Authentication: Basic <encoded user/password> , Authentication: Token <user>:<password> and Authentication: Bearer <token>...

@powersj
Copy link
Contributor

powersj commented Jul 14, 2023

Sorry, but isn't this what I added in line 116? What am I missing here?

If I was a user who wanted to use backwards compatibility, again I was expecting a token parameter in the sample config itself, again similar to the client libraries.

You are re-purposing the basic auth username and password configuration fields. To someone coming from v2 that is not obvious that they can use that as a token backwards comparability. At the very least you need to update the docs.

@powersj
Copy link
Contributor

powersj commented Jul 14, 2023

I've gone through this again and realized my comment may have come across a little harsh - my point is that the code is fine, rather we have not guided the user to the fact that this is even possibility.

Updating the sample config section near basic auth username/password to state that these can be set for users using compatibility mode via "username:password" is all that is needed to land this.

@srebhan
Copy link
Contributor Author

srebhan commented Jul 21, 2023

@powersj no worries about the comment, I don't classify this as harsh, you have to try harder! ;-) Will add a comment as soon as possible to the sample-config.

@telegraf-tiger
Copy link
Contributor

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you!

@powersj powersj merged commit fe84675 into influxdata:master Jul 24, 2023
26 checks passed
@github-actions github-actions bot added this to the v1.28.0 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/influxdb feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telegraf to accept tokens on v1 InfluxDB input plugins
2 participants