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

Sqlserver input: require authentication method to be specified #9388

Merged
merged 3 commits into from Jul 7, 2021

Conversation

reimda
Copy link
Contributor

@reimda reimda commented Jun 16, 2021

The sqlserver input plugin uses windows authentication (aka integrated security) when there is no username and password in the connection string.

#8822 added support for AAD authentication but only when there is no password in the connection string. Since the new check for no password happens before the previous connection logic, this causes windows authentication to be unavailable, which is a regression.

This PR adds a setting to enable AAD authentication. This allows use of windows authentication as before. It's backward compatible because the default of the new setting is the old behavior. New users who want to use AAD will to opt in to the new auth behavior by changing the setting to "AAD".

resolves #9362

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 16, 2021
@reimda
Copy link
Contributor Author

reimda commented Jun 16, 2021

@cerilewis could you test the PR build once CI passes?

@avinash-nigam what do you think of this solution?

@aslgithub
Copy link

@reimda I've found this via #9362
If its any help I've tested the telgraf-tiger artifact build and can confirm this build fixes the issue.

I've a thought if it helps in any way, normally in my experience whilst no user or password is provided in the connection string for Windows Integrated Authentication, normally the setting Integrated Security=true would be set to indicate the intention e.g.
"Server=localhost;Port=1433;Integrated Security=true;log=1;",

@cerilewis
Copy link

@reimda I can confirm that using the config that failed previously with 1.19.0rc0 now works with the tiger build above.
Thank you for addressing this.

@aslgithub
Copy link

@srebhan Tagging you in on this to see if you can help merge this please?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

The code looks good to me, however, I think we should give the "default" auth method a more descriptive name e.g. "credentials" or "connection params".

@srebhan srebhan added area/sqlserver fix pr to fix corresponding bug regression something that used to work, but is now broken and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jun 30, 2021
@srebhan srebhan self-assigned this Jun 30, 2021
@reimda reimda requested review from srebhan and sspaink June 30, 2021 21:55
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jul 2, 2021
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

This looks good, but you may want to consider adding a test to ensure future changes don't break support for this, but that's optional here and not required to merge. just let us know if you want to wait on merging.

@reimda reimda merged commit 537ac63 into master Jul 7, 2021
@reimda reimda deleted the sqlserver-auth-method branch July 7, 2021 19:09
reimda added a commit that referenced this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. regression something that used to work, but is now broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server input no longer functions correctly with Windows Integrated authentication
6 participants