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

Dynamic Passwords in NpgsqlConnection #2502

Merged
merged 13 commits into from Jun 29, 2019

Conversation

Projects
None yet
4 participants
@williamdenton
Copy link
Contributor

commented Jun 21, 2019

See #2500

williamdenton added some commits Jun 21, 2019

Add GetPassword delegate to Connection and pass down to Connector
New connections will execute the delegate to obtain a password. This allows the use of dynamic passwords or tokens such as those used by Amazon RDS IAM Authentication.
#2500
Username can be derived from connection string or another source
It's possible that the username is not directly taken from the connection string, so flow the computed username down so it is available in the delegate.
#2500
@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

The critical design decision here is to not allow fallback to the password specified on the connection string or to a PassFile. I think that if you are defining the delegate and you want to return null that is your responsibility to handle. Falling back into other authentication methods would be confusing in this case i believe

@YohDeadfall
Copy link
Member

left a comment

Good start!

Show resolved Hide resolved src/Npgsql/NpgsqlConnection.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnection.cs Outdated
@roji
Copy link
Member

left a comment

Looks good! See below comments in addition to @YohDeadfall:

  • I don't really see the reason to call this a "dynamic" password - it's just a callback-based mechanism.
  • Is there a specific reason to ignore Password on the connection string if a delegate is provided? Why not have the delegate just be a fallback when the password isn't specified, just like PgPass is handled? I could imagine scenarios where some fallback delegate is wired up, but a user may want to provide a password manually...
Show resolved Hide resolved src/Npgsql/NpgsqlConnection.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
Show resolved Hide resolved test/Npgsql.Tests/ConnectionTests.cs Outdated
Show resolved Hide resolved test/Npgsql.Tests/ConnectionTests.cs Outdated

@roji roji assigned williamdenton and unassigned williamdenton Jun 26, 2019

williamdenton added some commits Jun 27, 2019

Rename password delegate inline with prior art
This aligns this property with the existing delegates ProvideClientCertificatesCallback and RemoteCertificateValidationCallback
#2500
Show resolved Hide resolved doc/security.md Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs
Show resolved Hide resolved test/Npgsql.Tests/ConnectionTests.cs Outdated
Show resolved Hide resolved test/Npgsql.Tests/ConnectionTests.cs Outdated
Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
@roji
Copy link
Member

left a comment

Looking very close, thanks for your work. Let me know if anything is unclear.

Show resolved Hide resolved src/Npgsql/NpgsqlConnector.Auth.cs Outdated
@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

no problem, these tweaks are all very easy

williamdenton added some commits Jun 29, 2019

@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

let me know if you would like all these commits rebased and squashed to have a sane linear history, rather than showing the journey we came on

@roji

roji approved these changes Jun 29, 2019

Copy link
Member

left a comment

I'm good to merge. @YohDeadfall can you please give this a quick review?

@roji

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

@williamdenton yeah please squash this to a single commit in preparation for merge.

@YohDeadfall
Copy link
Member

left a comment

Looks good except a few nits which I can't comment in the code (Windows Phone Edge is no more supported supported by GitHub), so will write my comments here.

  • In line 385 in the GetPassword method GetDynamicPassword mentioned, but there is no such delegate.
  • In the same method in line 392 there is a naming issue too. Probably you want to say Obtaining password using {nameof(GetPasswordCallback)} delegate failed.?

williamdenton added some commits Jun 29, 2019

@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@roji do you want a single commit, or two or three with the critical steps split out.

For example we have made some changes to the passfile implementation that were done as we're in the vicinity, not directly related to this issue.

@YohDeadfall

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

Personally I prefer to separate fixes from features to keep things clear, but as I know Shay likes to squash everything into a single commit.

@roji roji merged commit e600e7e into npgsql:dev Jun 29, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

roji added a commit that referenced this pull request Jun 29, 2019

Add password callback to NpgsqlConnection (#2502)
New connections will execute the delegate to obtain a password.
This allows the use of dynamic passwords or tokens such as those used by Amazon RDS IAM Authentication.

Fixes #2500
@roji

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

I think things here are related and small enough to make a single commit reasonable... Merged via d78f3d6.

@williamdenton thanks for your contribution! It's a nice new feature for Npgsql, hope we'll see more from you in the future :)

@YohDeadfall

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

As I said, Shay likes single commit changes (:

Yep, thank you! And it's great to see documentation changes.

@roji

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

@YohDeadfall knows me too well :)

@austindrenski
Copy link
Member

left a comment

A bit late to the party, but awesome to see this merged. Thanks for your contribution! 🎉

@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

When will this likely make it into a nuget? Guessing it's in the 4.1 release.

I might prepare a blog post to go with this.

@austindrenski

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

When will this likely make it into a nuget? Guessing it's in the 4.1 release.

It'll definitely be part of the 4.1 release, which I would expect to see this calendar year. (@roji may have a more specific timeline in mind.) Of course, in the meantime, it should already be available from the npgsql-unstable feed.

I might prepare a blog post to go with this.

Please let us know if you do! We've linked to related blog posts in past release notes, so I'm sure we could take a look at whatever you put together.

@YohDeadfall

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

(@roji may have a more specific timeline in mind.)

Currently I'm a blocker here because of unfinished work on type handlers.

@roji

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

The idea is to more or less synchronize the next release (which we may end up calling 5.0) with .NET Core 3.0, which is due this year.

williamdenton added a commit to williamdenton/Npgsql.EntityFrameworkCore.PostgreSQL that referenced this pull request Jul 2, 2019

@williamdenton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

which we may end up calling 5.0

Semver wise 0af8c1d is a breaking change which means this isn't 4.1 - its definitely 5.x.

@roji

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Oh, there are other more important ones for sure :)

williamdenton added a commit to williamdenton/Npgsql.EntityFrameworkCore.PostgreSQL that referenced this pull request Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.