search

serilog-mssql / serilog-sinks-mssqlserver

A Serilog sink that writes events to Microsoft SQL Server and Azure SQL
Apache License 2.0
278 stars 148 forks source link

Removing deprecated Microsoft.Azure.Services.AppAuthentication nuget package and replacing it by Azure.Identity #410

Closed azerios closed 2 years ago

azerios commented 2 years ago

Hello,

The IT security team of my company will not go live if the Microsoft.Azure.Services.AppAuthentication nuget package is used as reference.

The goal of this PR is to replace it by Azure.Identity without causing any regression.

If this fix isn't enough, feel free to do it on another branch and closed mine.

Again, without this critical security change, it's one year lost for my team.

This is link to https://github.com/serilog-mssql/serilog-sinks-mssqlserver/issues/400

Regards

ckadluba commented 2 years ago

It looks good, thank you for the contribution.

Since you updated the nuget references for the .NET Core 3.1 and .NET Framework 4.7.2 targets, were you able to test the change with both targets?

Also, it there is a restriction now for other targets that were not changed (.NET Framework 4.6.2, .NET Standard, please document this in the README.md).

Also, there seem to be problems with unit tests for the .NET Core 3.1 target. Can you please take a look at those? Let us know if you need any help with that.

azerios commented 2 years ago

I don't have any tenant by my own, I took the sample from the migration guide. I want to try my changes before the end of the week with an existing tenant.

Regards

ckadluba commented 2 years ago

Thank you. I will leave this PR open until you could test it with Azure.

azerios commented 2 years ago

Dear,

My company provide me a tenantId and I was able to test my code, that why I made this last commit.

Both tests failed because I provide a valid tenantId : image

And the accessToken object : image

@ckadluba Is it possible after the merge of these changes to generate a pre-release version to be able to generate a report on Veracode with a better security rating.

Regards

ckadluba commented 2 years ago

There are still failing tests. Are you able to fix them?

azerios commented 2 years ago

The exception returned is hard coded to make all tests to pass because I have a different exception type on my machine.

Assert.Throws() Failure Expected: typeof(Azure.Identity.CredentialUnavailableException) Actual: typeof(Azure.Identity.AuthenticationFailedException)

ckadluba commented 2 years ago

Please try to find out why the exception is different in your environment from the test when it runs on the build agent. We should not change lib code to silence a failing test of which we do not know why it fails. It might indicate a real bug.

azerios commented 2 years ago

I ran this command in Powershell as Admin : Invoke-WebRequest -Uri https://aka.ms/installazurecliwindows -OutFile .\AzureCLI.msi; Start-Process msiexec.exe -Wait -ArgumentList '/I AzureCLI.msi /quiet'; rm .\AzureCLI.msi

And install the last version of Powershell 7 : https://docs.microsoft.com/en-us/powershell/scripting/install/installing-powershell-on-windows?view=powershell-7.2

I'm still working on fixing unit tests

ckadluba commented 2 years ago

I think this PR needs a little bit more work. I hope I find some time soon to test this on Azure myself. If I understand this correctly, you were able to test this successfully on your Azure tenant. Correct?

The unexcepted AuthenticationFailedException exception on the failing tests is easily explained. On the GitHub agent running the tests there is no active Azure login session. Therefore the Azure SDK method throws this exception.

But maybe we can remove those tests and some others because when using system assigned managed identities as supported by this sink, if I understand it correctly, the DefaultAzureCredential class can be used without any options set. This would mean that the ctor parameters azureServiceTokenProviderResource and tenantId and fields _azureServiceTokenProviderResource and _tenantId should be removed from the class AzureManagedServiceAuthenticator. This also makes makes some tests obsolete.

Please remove the mentioned parameters and fields from AzureManagedServiceAuthenticator.

ckadluba commented 2 years ago

We are currently upgrading SqlClient in PR https://github.com/serilog-mssql/serilog-sinks-mssqlserver/pull/418 and will remove the obsolete MS AppAuthentication soon and use SqlClient auth capabilities instead. That will also obsolete this PR because the newer SqlClient version will itself depend on Azure.Identity which makes it unnecessary for MSSQL sink to depend on Azure.Identity.

Thanks a lot for your input and research.