johanclasson / vso-agent-tasks

Build and Release Tasks for Visual Studio Online and Team Foundation Server
MIT License
20 stars 16 forks source link

DbUpMigration - Support for Azure AD access tokens #71

Open jarpsimoes opened 3 years ago

jarpsimoes commented 3 years ago

Hi guys,

That plugin does not support authentication on SQL Databases with service principal. That issue occurs because DBUp used it's before 4.3.* version. That's an limitation in a lot of projects.

johanclasson commented 3 years ago

Only upgrading the DbUp version would not be enough since we also need to actively make DbUp use AzureServiceTokenProvider to set the sql connection access token.

For example using connection strings like Server=myserver.database.windows.net; Database=MyDb;, and setting an appropriate AzureServicesAuthConnectionString environment variable (ref.).

Pseudo c# code:

var useAzureSqlIntegratedSecurity = !(connectionString.Contains("Password=") ||
                                      connectionString.Contains("Integrated Security=") ||
                                      connectionString.Contains("Trusted_Connection="));
var upgrader =
    DeployChanges.To
        .SqlDatabase(connectionString, null, useAzureSqlIntegratedSecurity)

We can also consider to make an input field in the task where the AzureServiceTokenProvider connection string can be specifield.

peterbud commented 2 years ago

There is a DbUp fork which supports Azure AD access tokens: https://github.com/chriswill/DbUpReboot

Would it be possible to also include the DLLs from this forked version as well in the vso task and use it with a configuration setting to enable that authentication method?

johanclassoncab commented 2 years ago

It is my understanding that this is also supported by the original DbUp. What use case do you have in mind?

peterbud commented 2 years ago

I don't think that DbUp supports token based authentication (like if you have a DevOps pipeline executed by a service principal) or any of the new AAD authentications: https://github.com/DbUp/dbup-sqlserver/issues/15

techarch commented 2 years ago

Also Microsoft now recommends migrating from System.Data.SqlClient to Microsoft.Data.SqlClient. PS: our teams love DbUp, and now that we're starting to migrate to Azure SQL this enhancement is becoming quite key to our success as user-assigned managed identities make it possible to be completely password-less.

johanclasson commented 2 years ago

I get that System.Data.SqlClient is deprecated, but that is an implementation detail of DbUp. It is not my impression that the DbUp repo is dead, and would therefore not like to complicate things by using a fork instead.

Besides, I know for certain that the following code works:

...
DeployChanges.To
        .SqlDatabase("Server=my-serves.database.windows.net; Database=MyDb;", null, true)
...

DbUp 4.5.0 use AzureServiceTokenProvider (also deprecated) to get the token (ref.).

For configuration you set the AzureServicesAuthConnectionString environment variable according to this.

I still think that the best solution is what I wrote earlier:

var useAzureSqlIntegratedSecurity = !(connectionString.Contains("Password=") ||
                                      connectionString.Contains("Integrated Security=") ||
                                      connectionString.Contains("Trusted_Connection="));
var upgrader =
    DeployChanges.To
        .SqlDatabase(connectionString, null, useAzureSqlIntegratedSecurity)

And possibly making it easier to pass in either client id or client id + password + tenant id as task parameters. That way it would work with managed identities, az cli identity, and regular service principals.

lacsilva commented 2 years ago

Thank you so much for working on this. Any idea when the improvements above will be released?