Tests | Remove hardcoded credentials from ManualTests

[edwardneal]

This PR mops up almost all of the remaining hardcoded credentials, and does three things:

• Removes references to CertificateUtility.CreateCertificate, which always used a hardcoded certificate.
• Forces the tests to dynamically generate keys in Azure Key Vault.
• Removes references to CertificateUtilityWin in CspProviderExt, then the class itself.

Removal of hardcoded credentials

The removal of CertificateUtility.CreateCertificate is pretty uncontroversial. However, the AKVTest.TestRoundTripWithAKVAndCertStoreProvider test tries to round-trip encrypted column data, encrypting it with the hardcoded certificate and decrypting it with the Azure Key Vault key. I've thus treated this key as another static credential and removed it.

It's worth noting that the CoreCryptoTests class loads hardcoded credentials from TCECryptoNativeBaselineRsa.txt. I've not touched this because I don't know what to do with it. It isn't testing any SqlClient-specific functionality, just .NET's ability to decrypt text which is encrypted by native code. I'm not sure whether this is necessary any more though: we already test this implicitly with the end-to-end AE tests. There's no modification required here - just a choice on whether to keep or delete the test entirely.

CspProviderExt tests

One partially-related change was to remove CertificateUtilityWin. This was only ever used by the CspProviderExt tests, which used RSACryptoServiceProvider to generate a key in a specific CSP and encrypt/decrypt data with it. This consisted of three tests:

1. TestKeysFromCertificatesCreatedWithMultipleCryptoProviders, which looked for all CSPs with a name containing "RSA and AES", then generated a key in that CSP and tested the Always Encrypted provider with it.
2. TestRoundTripWithCSPAndCertStoreProvider, which generated a certificate and tried to round-trip between the certificate-based AE provider and the CSP-based AE provider. XUnit was already being told to skip this test.
3. TestEncryptDecryptWithCSP, which did exactly the same thing as test 1, but with a hard-coded CSP of "Microsoft Enhanced RSA and AES Cryptographic Provider".

Tests 1 and 3 are actually identical. "Microsoft Enhanced RSA and AES Cryptographic Provider" is the only CSP which fits the criteria for Test 1. I've thus eliminated test 1, and modified test 3 to run against all matching CSPs.

Of the remaining two tests, TestEncryptDecryptWithCSP would run a PowerShell script to generate a certificate, then extract the private key and use this as the CSP. It didn't need to do this, it could simply instantiate an RSACryptoServiceProvider directly and use that. I've thus eliminated the certificate generation. The round-trip test did need the certificate though, so I've just switched it to use the same certificate generation fixture as the rest of the tests. It still fails if I force it to run, but it fails with the same error as before. Once these changes were done, CertificateUtilityWin was no longer referenced and could be deleted wholesale.

The CspProviderExt/CertificateUtilityWin changes comprise around half of the changes. If it's easier to review, I can split them into a separate PR.

[benrr101]

Absolutely fantastic! Just a few changes I'd like to see made, but otherwise I fully support these changes!

[benrr101]

as far as I can tell this variable isn't being used to do anything other than indicate that the token has been acquired. If that's true, I'd recommend either making it a bool or checking _authority/_resource for null as the check.

But I'm also wondering, can we store the access token itself? Or does that need to be re-acquired each time the call to this method is made? If we can store the access token, we could use _accessToken as the value to check here (plus we could get rid of _authority and _resource.

[edwardneal]

I agree. AKVUrl was used all over the place for a static AKV credential, this was the last referenced location. It's now gone!

I'm not sure about the impact that caching the access token could have on test coverage - only the first test to run would ever be able to exercise the case where the AKV access token can't be retrieved. I think it'd be better to explicitly add a TokenCredential derivative which throws and run some of the AKV tests based on that, then revisit any caching.

[benrr101]

Fantastic, thank you for addressing the comments :D

[benrr101]

/azp run

[edwardneal]

Thanks - I've just resolved both comments.

Won't the AE tests only run against a branch in this repo?

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[benrr101]

@edwardneal ... uhhh good question. I don't think it will... :/
How about if the CI I just kicked off is successful, I can push your branch to dotnet/SqlClient and kick off a build from there to do a one-off verification.

I'm quickly getting tired of these rules that prevent us from running all our tests against external branches 😠

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.18%. Comparing base (0be2756) to head (23214ce).
Report is 16 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (23214ce). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (0be2756)	HEAD (23214ce)
addons	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3090      +/-   ##
==========================================
- Coverage   72.81%   66.18%   -6.64%     
==========================================
  Files         282      279       -3     
  Lines       59112    58827     -285     
==========================================
- Hits        43045    38936    -4109     
- Misses      16067    19891    +3824     

Flag	Coverage Δ
addons	?
netcore	69.18% <ø> (-6.35%)	⬇️
netfx	64.87% <ø> (-6.28%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[edwardneal]

Thanks @benrr101, that's fine by me

[benrr101]

Consider this my approval ... closing and rebuilding as #3204

Rereading my original plan I have no idea what I meant... So let's just follow the pattern that was working before.