Supply password via callback (to support AWS IAM Authentication)

[williamdenton]

Background

When using Postgres in AWS there is an option to authenticate using IAM. This generates a time-limited password based off the assumed role/access key your service is using. This means that when using AWS postgres (RDS) you dont need to have static passwords. Generated passwords are good for authentication for 15 minutes, once a connection is open it doesn't matter is the password has expired.

Using this presents some problems in Npgsql.

1. A new connection string must be generated at least every 15 minutes
2. Npgsql Connection pools are based on the connection string, a new password will create a new pool
3. Npgsql connection pools are never removed once created (but a pool may have zero connections). But if configured to have minimum of 1 problems could arise

npgsql/src/Npgsql/PoolManager.cs

Line 26 in 191643e

// Note that pools never get removed. _pools is strictly append-only.

So ideally we need to have access to a "fresh" password when ever opening a connection, while keeping the same connection pool.

Work around

Using Passfile on the connection string and keeping a file up-to-date on the filesystem with a currently valid password might work. This isn't ideal as you have to write the password to disk so npgsql can read it back in. You could have a different process writing the file, but most likely it would be the app itself managing this file. It's a little bit rube-goldberg.

Proposal

Add the option of a delegate to NpgsqlConnection to generate a password on demand. This delegate would be used in the NpgsqlConnector when opening a connection the same way as the current logic works for plaintext authentication in the GetPassword() on line 56 below. We can then leave the password out of the connection string allowing pooling to continue working how it currently does.

npgsql/src/Npgsql/NpgsqlConnector.Auth.cs

Lines 54 to 66 in 191643e

	async Task AuthenticateCleartext(bool async)
	{
	var passwd = GetPassword();
	if (passwd == null)
	throw new NpgsqlException("No password has been provided but the backend requires one (in cleartext)");
	var encoded = new byte[Encoding.UTF8.GetByteCount(passwd) + 1];
	Encoding.UTF8.GetBytes(passwd, 0, passwd.Length, encoded, 0);
	await WritePassword(encoded, async);
	await Flush(async);
	Expect<AuthenticationRequestMessage>(await ReadMessage(async));
	}

An additional step in this GetPassword() function to use the delegate in this method would be fairly non-invasive

npgsql/src/Npgsql/NpgsqlConnector.Auth.cs

Lines 369 to 384 in 191643e

	string? GetPassword()
	{
	var passwd = Settings.Password;
	if (passwd != null)
	return passwd;
	// No password was provided. Attempt to pull the password from the pgpass file.
	var matchingEntry = PgPassFile.Load(Settings.Passfile)?.GetFirstMatchingEntry(Settings.Host, Settings.Port, Settings.Database, Settings.Username);
	if (matchingEntry != null)
	{
	Log.Trace("Taking password from pgpass file");
	return matchingEntry.Password;
	}
	return null;
	}

The delegate could be either of these

delegate string GetPassword(string host, string port, string database, string username);

delegate string GetPassword(NpgsqlConnectionStringBuilder settings);

Prior Art for this approach

Npgsql already exposes some delegates on the NpgsqlConnection that are passed down to the connector, so there is prior art for this kind of behaviour

npgsql/src/Npgsql/NpgsqlConnection.cs

Lines 717 to 723 in 7b8e117

	/// <summary>
	/// Selects the local Secure Sockets Layer (SSL) certificate used for authentication.
	/// </summary>
	/// <remarks>
	/// See <see href="https://msdn.microsoft.com/en-us/library/system.net.security.localcertificateselectioncallback(v=vs.110).aspx"/>
	/// </remarks>
	public ProvideClientCertificatesCallback? ProvideClientCertificatesCallback { get; set; }

npgsql/src/Npgsql/NpgsqlConnector.cs

Lines 245 to 252 in 191643e

	internal NpgsqlConnector(NpgsqlConnection connection)
	: this(connection.Settings, connection.OriginalConnectionString)
	{
	Connection = connection;
	Connection.Connector = this;
	ProvideClientCertificatesCallback = Connection.ProvideClientCertificatesCallback;
	UserCertificateValidationCallback = Connection.UserCertificateValidationCallback;
	}

[YohDeadfall]

Yep, I think it would be the right way to move forward.

[williamdenton]

Cool. I'll make a pr for further discussion

[roji]

Makes sense to me too - as you wrote it's similar to the delegates we already have to support certificate-based authentication.

I'd go with the second overload (which accepts NpgsqlConnectionStringBuilder). One complication is that the username isn't always simply specified inside the builder (see NpgsqlConnector.GetUsername()), so we should probably pass it to the delegate separately as well.

[williamdenton]

If the username is non trivial then maybe having explicit params is better

delegate string GetPassword(string host, string port, string database, string username);

This is essentially replicating the password file format

npgsql/src/Npgsql/PgPassFile.cs

Lines 60 to 61 in 191643e

	internal Entry? GetFirstMatchingEntry(string? host = null, int? port = null, string? database = null, string? username = null)
	=> Entries.FirstOrDefault(entry => entry.IsMatch(host, port, database, username));

[roji]

In theory someone may want to take into account other connection parameters beyond host/port/database/username. But I agree this is quite far-fetched, we can go with the broken-down version.

[williamdenton]

Also passing NpgsqlConnectionStringBuilder gives the caller the opportunity to mutate it, and given this call is happening in the connector after pooling has been decided i can foresee some very strange behaviours. What if they set the password on the builder as well as returning it? That means we would have to be defensive in cloning it, at which point it seems heavy handed to be passing the builder.

[roji]

This is in theory true, although the user who passed us the connection string in the first place is the same one which wired the callback - so I don't see too much reason to worry about this. But regardless, it seems fine to go ahead with the method accepting the different values instead.

[williamdenton]

Just checking if you saw the PR i added here #2502

[YohDeadfall]

I saw it but haven't reviewed it yet.

[bgrainger]

@roji Can you please double-check this PR to the AWS docs for accuracy? awsdocs/amazon-rds-user-guide#149

[roji]

Done, thanks @bgrainger!