Tests | Remove hardcoded credentials from ManualTests

[benrr101]

This is a remake of #3090 so that we can get all of the CI to run, original description follows

---

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]

@edwardneal I could use a wee bit of help figuring out the errors - it looks to me that a table doesn't exist, so we can't delete the entries in it before running the test. Since we're not getting any errors creating the tables, I suspect we're dropping the table somewhere between tests. But I can't seem to repro it locally (although that could be an issue with my setup not being the same as test machine).

I've also kicked the failing tests again to see if maybe it's transient. But since it's consistent across all the enclave tests I suspect it isn't transient.

[edwardneal]

Sorry for the delay here @benrr101, I've just been able to get to look at this.

The fixture here is PlatformSpecificTestContext. This instantiates a SQLSetupStrategyCertStoreProvider, which calls the base SQLSetupStrategy - CreateTables, followed by SetupDatabase.

The effect of CreateTables which we care about is that it populates the TrustedMasterKeyPathsTestTable and returns it; the caller adds this to the databaseObjects field.

SetupDatabase iterates over each of these objects, calling their Create method. Then, it calls InsertSampleData, which wipes the table and adds a new record to it.

The net effect is that we create a new table, then immediately insert sample data into it. This fails, so the test fixture can't be instantiated and every test which uses the fixture fails. I'm not able to reproduce it either.

I'd like to compare the logs of a few CI runs - could you retrigger this please?

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[benrr101]

@edwardneal Ok, that's mostly what I suspected that it was an issue with the database setup steps causing the fixture to fail to be instantiated. Stuff like that working locally but not on the test machine suggests to me some kind of race condition, but that's just wild speculation. As per your request, I've kicked off another CI run. Let me know if you need more - I appreciate you looking into this :)

[benrr101]

@edwardneal Any further thoughts on this one?

[benrr101]

@edwardneal thanks again for looking into the failures and dealing with the annoying back-n-forth of trying to resolve the issues. Let's see what this round does.

@paulmedynski, @mdaigle do you have anything to add regarding the net8/net9 test behavior? I'm kinda wondering if we need to make sure that the test projects are defined to target the same frameworks as the projects themselves.

[saurabh500]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[edwardneal]

CI has timed out again, but I was able to see the cause in the realtime logs a few hours ago. It looks like we're not receiving any response from 10.0.0.4 and 10.0.0.5 prior to running the tests. Is the AE Enclave test environment healthy?

[saurabh500]

@cheenamalhotra does this have to do with the VM decommissioning?

@edwardneal there are test infrastructure issues we are trying to iron out.

[David-Engel]

10.0.0.5 is the one that was deallocated due to Azure capacity pressure, yes. It let me start it back up so I've re-run failed jobs for this PR.

[edwardneal]

Thanks - I can see that most of these tests are passing now (with the exception of Azure Key Vault - I'll try to reproduce that locally.)

[paulmedynski]

This is a large review, and I've only skimmed it so far. I will need a subject-matter expert to walk me through most of it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.28%. Comparing base (cc23e04) to head (02470f2).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3204      +/-   ##
==========================================
- Coverage   67.16%   65.28%   -1.89%     
==========================================
  Files         300      300              
  Lines       65640    65640              
==========================================
- Hits        44087    42851    -1236     
- Misses      21553    22789    +1236     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	68.50% <ø> (-3.80%)	⬇️
netfx	66.68% <ø> (+1.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.

[paulmedynski]

Clearing my 'Request changes' review, but not approving. In the interest of getting this merged, other team members with a better understanding of the changes should review for approval.

No changes necessary after discussion.

[benrr101]

@paulmedynski - I can't review this on myself, so that means we've only got you, @mdaigle, @samsharma2700, and @cheenamalhotra who can review it.

[paulmedynski]

It all makes sense to me, with a few comments.

[benrr101]

@paulmedynski I addressed most of the comments you added, some a bit more heavily-handed (heavy-handidly?) than others. I also merged main into it b/c I couldn't build without a fix we added a looong time ago.