SqlServerConnection should use a more robust way of cloning the connection

[divega]

Currently in SqlServerConnection.CreateMasterConnection() we just create a new connection with a modified connection string in order to get a connection to the master database. But we know that SqlConnection.Open() will by default strip out the password from the connection string so by the time we get the connection string it might have already been mangled.

There is also SqlCredentials which was added in recent versions of SqlClient as an alternative to having credentials embedded in the connection string.

Incidentally ((ICloneable)sqlConnection).Clone() can handle all of this, but the functionality isn't available in CoreCLR.

[rowanmiller]

This was already tracked by https://github.com/aspnet/EntityFramework/issues/1406 but the info here is more detailed so we can probably close the other one

[divega]

Ah! @ajcvickers you pick which one you want to close.

[rowanmiller]

Need to start by driving ability to clone in CoreCLR

[divega]

We have followed up with the SqlClient folks and a connection clone method won't be part of SqlClient in the RTM time frame.

We should start using it as soon as it is available though. Until then CreateMasterConnection() will any time credentials are required that are not included in the connection string.

Removing myself and the release from the issue so that we can decide in triage how to track it.

[divega]

SqlConnection now implements ICloneable. If we can test that credentials stripped out from the connection string does not affect the cloned connection then we are not blocked anymore. Otherwise we should file an issue in SqlClient.

cc @saurabh500, @ajcvickers

[ajcvickers]

There are complications around doing this in our current code:

• Sometimes we have the connection string only, not an existing connection, in which case we don't want to clone at all
• Even after we clone, we would need to manipulate the connection to now point to master. This might always/sometimes fail, especially if the connection is open.

Given that this isn't really a major issue in EF Core I propose we leave the code as-is for now and only revisit if there is customer feedback.

[jnm2]

We're using SqlConnection.Credential and struggling with this. Would you take a fix?

What is the recommended workaround in the meantime? Putting the username and password in the connection string and setting PersistSecurityInfo = true?

[jnm2]

It looks like User ID and Password do work without PersistSecurityInfo.

What if connectionStringBuilder.ConnectionString was replaced with new SqlConnection(connectionStringBuilder.ConnectionString, DbConnection.Credential)?

https://github.com/dotnet/efcore/blob/d4cd3360c4f101986ffa3ffc8004fa1195b56466/src/EFCore.SqlServer/Storage/Internal/SqlServerConnection.cs#L56-L68

[ajcvickers]

@jnm2 It would be good to understand what are you doing that causes you to hit this issue?

[jnm2]

This is a desktop tool. When creating a new project, the user enters an instance name and optionally SQL credentials and the name of a SQL database to create to store the project in. The UI password entry control provides a SecureString instance which we attach to the SqlConnection with NetworkCredential.

To keep things simple, we're creating and passing around a Func<SqlConnection> connectionFactory which returns new SqlConnection(builtConnectionString, networkCredential). We were hoping to use context.Database.MigrateAsync. The class which implements database management uses this code to create a new database:

using var connection = connectionFactory.Invoke();
using var context = new FooContext(connection);

await context.Database.MigrateAsync(cancellationToken).ConfigureAwait(false);

// ...
// (further initialization and data import specified by the user)

MigrateAsync throws a SqlException with the message Login failed for user ''. The call stack showed that the uncaught SqlException was thrown inside the _databaseCreator.CreateAsync call:

https://github.com/dotnet/efcore/blob/d4cd3360c4f101986ffa3ffc8004fa1195b56466/src/EFCore.Relational/Migrations/Internal/Migrator.cs#L138-L149

SqlServerConnection.CreateMasterConnection disregards the SqlConnection.Credential property, and that looks like the sole problem.

[ajcvickers]

@jnm2 A couple of things. First, SecureString is not recommended anymore: see https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md

Second, I suspect that the best approach here is to not use EF to create the database instance. Connecting to master is one of those fragile things that can easily be broken by different SQL and client permissions and settings. Instead, I would recommend creating an empty database instance using a connection to master that you create appropriately. MigrateAsync is designed to treat such as database as new and run all the rest of the schema creation for you. EF doesn't need to create to master for that.

So:

• Create the empty database
• Then call Migrate on it

(Also, your scenario is just about the only situation where I approve of running Migrate in code. 😉)

[jnm2]

That helps, thank you!