npgsql / npgsql

Npgsql is the .NET data provider for PostgreSQL.
http://www.npgsql.org
PostgreSQL License
3.25k stars 822 forks source link

DataSource usage with password rotation #5720

Open madiganz opened 3 months ago

madiganz commented 3 months ago

I'm in the process of migrating a service from EF 6 to 8 and I'm working on using the NgpsqlDataSource as it's the property way in EF 7 and above and so I can remove the obsolete NpgsqlConnection.GlobalTypeMapper code. The currently implementation uses a service registered as a singleton through DI that gets the entire connection string from AWS Secrets Manager. This service is used in the OnConfiguring method of the DbContext to set and update the password if there is a connection error and it needs to retry

// In Startup.cs
services.AddSingleton<IDatabaseSecretsManager<PostgresCredentials>, PostgresSecretsManager>();
services.AddDbContextFactory<DbContext>();

// In DbContext.cs
protected override void OnConfiguring(DbContextOptionsBuilder options)
{
    options
        .UseNpgsql(dbSecretsManager.DbConnectionString().Result, npgsqlOptions =>
        {
            npgsqlOptions
                .ExecutionStrategy(_ => new PostgresCredentialsRotationExecutionStrategy(this, dbSecretsManager));
        })
        .UseSnakeCaseNamingConvention()
        .UseExceptionProcessor();
}

First thing is I tried to replace the execution strategy with UsePeriodicPasswordProvider and it just doesn't do exactly what I want. For one thing, it only updates the password and not the entire connection string. More importantly, it doesn't actually seem to update the connections. My attempt is to set password wrong in AWS Secrets Manager, start up the service, query the API and while it's attempting to connect I change the password in Secrets Manager to the correct one and it still does not work. My guess is that even though the DataSource is changing, the same DbContext is being used and it's still using the old values from the DataSource.

I have tried setting everything up in Startup.cs, but that doesn't work because UsePeriodicPasswordProvider doesn't seem to meet the requirements of true failure retry and PostgresCredentialsRotationExecutionStrategy needs the context passed in so it can manually update the connection string. I have tried passing NgpsqlDataSource as a singleton through DI and that seems to generally work, but setting it as the connection string in .UseNpgsql(datasource, npgsqlOptions => ... seems to mess up the execution strategy.

The following is how I've tried to set it up as a singleton. It's also worth noting that the MapEnum calls do not seem to work as I'm getting the following error: "Reading as 'MyEnum' is not supported for fields having DataTypeName 'public.my_enum'".

services.AddSingleton(sp =>
{
    var dbSecretsManager = sp.GetRequiredService<IDatabaseSecretsManager<PostgresCredentials>>();
    var dataSourceBuilder = new NpgsqlDataSourceBuilder(dbSecretsManager.DbConnectionString().Result);
    dataSourceBuilder.MapEnum<MyEnum>();
    return dataSourceBuilder.Build();
});

Is there a recommended way to do something like this? I find it hard to believe there is no good documentation on how to set this up with ASP.NET in a production environment fetching credentials from an outside source that isn't a config file.

roji commented 3 months ago

For one thing, it only updates the password and not the entire connection string.

Can you provide more info on this? AFAIK the "secret" part that gets rotated only goes into the Password portion of the connection string - I'm not sure why you'd need to change the entire connection string as the application is running.

More importantly, it doesn't actually seem to update the connections. My attempt is to set password wrong in AWS Secrets Manager, start up the service, query the API and while it's attempting to connect I change the password in Secrets Manager to the correct one and it still does not work. My guess is that even though the DataSource is changing, the same DbContext is being used and it's still using the old values from the DataSource.

I'm not really following here.

The way PostgreSQL works in general, is that the password (or secret) is only used for authentication while a physical connection is opened; once a connection is already open, the password is no longer needed. If you change the password while your application is running, and some connections have already been established, then those connections will continue to be used with no change; the newly-rotated password will only be used if and when a new physical connection needs to be established. That's very much by design - there's definitely no desire (or possibility) for all existing physical connections to suddenly be closed/broken just because the secret has been rotated.

But if I misunderstood your question, please let me know and provide a bit more context on the problem you're seeing.

I have tried setting everything up in Startup.cs, but that doesn't work because UsePeriodicPasswordProvider doesn't seem to meet the requirements of true failure retry and PostgresCredentialsRotationExecutionStrategy needs the context passed in so it can manually update the connection string. I have tried passing NgpsqlDataSource as a singleton through DI and that seems to generally work, but setting it as the connection string in .UseNpgsql(datasource, npgsqlOptions => ... seems to mess up the execution strategy.

Passing in an NpgsqlDataSource as a singleton to UseNpgsql is generally what you want to do; as for the rest, you're not providing enough details on what's going wrote (how does this "mess up the execution strategy"?). It's also really important to understand that the password rotation is a lower-level Npgsql feature, which has nothing to do with EF; whereas the execution strategy is an EF feature that has nothing to do with the password rotation. You seem to be expecting some sort of interaction between the two which I'm currently not understanding.

Can you please put together a minimal, runnable code sample that shows your actual code, and shows the exact errors/problems you're running up against?

madiganz commented 3 months ago

Can you provide more info on this? AFAIK the "secret" part that gets rotated only goes into the Password portion of the connection string - I'm not sure why you'd need to change the entire connection string as the application is running.

Let's ignore the mention of needing to update the connection string. I think understand the problem based on the password rotation is fine.

The way PostgreSQL works in general, is that the password (or secret) is only used for authentication while a physical connection is opened; once a connection is already open, the password is no longer needed. If you change the password while your application is running, and some connections have already been established, then those connections will continue to be used with no change; the newly-rotated password will only be used if and when a new physical connection needs to be established. That's very much by design - there's definitely no desire (or possibility) for all existing physical connections to suddenly be closed/broken just because the secret has been rotated.

I can work on getting you a minimal runnable code sample, but in the meantime let's see if maybe the problem is simple or it's just a lack of understandin. As mentioned above, the database context is currently being registered as a factory using

services.AddDbContextFactory<DbContext>();

The password is being rotated every month in AWS. The problem I'm seeing is when the password is rotated (or just wrong and switched to correct for testing), the current connection does not update it's password for the exact reasons you mention above. Is the database context factory re-using the bad connections? Should it completely close a connection when it errors out due to a password problem so that a user can get a new healthy connection? Is this something I should keep using the execution strategy to handle? The current execution strategy checks whether the error is about a connection failure and then resets the connection string which leads to a retry attempt succeeding.

protected override void OnRetry()
{
    var connectionString = this.dbSecretsManager.DbConnectionString().Result;
    this.Dependencies.CurrentContext.Context.Database.SetConnectionString(connectionString);
    base.OnRetry();
}
madiganz commented 3 months ago

I may have been mistaken. It looks like if the OnRetry logic is changed to use dataSource.Password = dbSecretsManager.Password(); it seems to work as expected. Unless I am misunderstanding something, it seems like this is the better logic for retrying failed passwords instead of using the npgsql password rotation. I can put together some minimal code for advisement if you would like as I would like to know the best practice for this.

roji commented 3 months ago

The problem I'm seeing is when the password is rotated (or just wrong and switched to correct for testing), the current connection does not update it's password for the exact reasons you mention above. Is the database context factory re-using the bad connections?

Once again, in PostgreSQL, when a password is rotated, current connections do not become "bad" or broken in any way - they continue working. The new (rotated) password is only needed for when you establish a new physical connection, i.e. because there are no more idle ones in the pool. Therefore, there's nothing for the EF execution to handle; physical connection which were established before the password was rotated will continue working fine, and new physical connections should use the new (rotated) password.

looks like if the OnRetry logic is changed to use dataSource.Password = dbSecretsManager.Password(); it seems to work as expected. Unless I am misunderstanding something, it seems like this is the better logic for retrying failed passwords instead of using the npgsql password rotation.

Setting dataSource.Password is just an alternative way of rotating the password at the Npgsql level - it is identical to configuring the password rotation callback on NpgsqlDataSourceBuilder, the only difference is that dataSource.Password is a more "push"-like API (the application manually rotates the password based on its own timing/whatever), as opposed to the callback API which is a more "pull"-like API (where Npgsql itself calls the callback).

In any case, if things are working for you with dataSource.Password, that should be fine.

madiganz commented 3 months ago

Once again, in PostgreSQL, when a password is rotated, current connections do not become "bad" or broken in any way - they continue working. The new (rotated) password is only needed for when you establish a new physical connection, i.e. because there are no more idle ones in the pool. Therefore, there's nothing for the EF execution to handle; physical connection which were established before the password was rotated will continue working fine, and new physical connections should use the new (rotated) password.

What I am referring to is mainly just the retry of a failed connection instead of just returning an error to the user. Correct me if I'm wrong but the only way to actually retry a connection attempt is to use an execution strategy like I mentioned.

Setting dataSource.Password is just an alternative way of rotating the password at the Npgsql level - it is identical to configuring the password rotation callback on NpgsqlDataSourceBuilder, the only difference is that dataSource.Password is a more "push"-like API (the application manually rotates the password based on its own timing/whatever), as opposed to the callback API which is a more "pull"-like API (where Npgsql itself calls the callback). In any case, if things are working for you with dataSource.Password, that should be fine.

I'd rather use the password rotation callback on NpgsqlDataSourceBuilder if possible. How would this be properly synced with the timing different of when the database credentials are actually being rotated. Even if I set the check interval to 5 minutes and the password rotates every month there could still be just under 5 minutes when the password is wrong?

madiganz commented 2 months ago

@roji just curious if you had any final suggestion about this point

How would this be properly synced with the timing different of when the database credentials are actually being rotated. Even if I set the check interval to 5 minutes and the password rotates every month there could still be just under 5 minutes when the password is wrong?

NinoFloris commented 2 months ago

How would this be properly synced with the timing different of when the database credentials are actually being rotated. Even if I set the check interval to 5 minutes and the password rotates every month there could still be just under 5 minutes when the password is wrong?

@madiganz if you know exactly when the password is changed you could use UsePasswordProvider instead. It would allow you full control over providing the new password the moment it is changed. There is probably still a tiny race between the moment it was changed in the the database and the notification or timestamp (or some other mechanism) but that should be < 1s.