-
Notifications
You must be signed in to change notification settings - Fork 38
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
Clear text conn type #20
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isoos
requested changes
Nov 29, 2021
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, a few changes needed, and also please run dart format
on the sources.
isoos
approved these changes
Nov 30, 2021
Thank you, published as 2.4.3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds the option for clear text password authentication.
Fixes connections to Azure Database for PostgreSQL.
I created a new boolean parameter at the time of creating the connection called allowClearTextPassword default to false to enable this kind of authentication for the connection.
According to https://www.postgresql.org/docs/11/protocol-flow.html#id-1.10.5.7.3 flow, this patch just returns the password to the server when requested a clear text password after a connection attempt.
For the structure of the message I didn't quite got the grasp of the byte formatting, but replicated the structure used for md5 as it follows a similar message flow. Need review on this to guarantee it is working. Local testing showed no problem.
TODO 1: I could not come up with tests, as there should be a SQL server running with clear text password request configuration I cannot replicate. Please advise on whether such server exists or how could I write the necessary tests. I have successfully tested the changes under our environment, for both queries and transactions of every type.
TODO 2: There may be security concerns about using clear text passwords I am not aware of. Right now we have an open ticket with Microsoft regarding this issue. Note that in this environment, the connections are performed within a private cloud cluster and encrypted, thus making the password encryption non necessary.