Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests | Remove hardcoded credentials from ManualTests #3204

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Mar 6, 2025

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.

One test implied that DataTestUtility.AKVUrl would point to an RSA key which aligned with the certificate's private key. Switching this to dynamically generate the key in places.
These were mostly related to generating CSP keys.
* Reorder properties and constructors
* Move AEConnectionStringProviderWithCspParameters to its own file
* Tweak to the AKV token acquisition
Redundant bracket, alphabetised the ManualTesting csproj
@benrr101 benrr101 added the Area\Tests Issues that are targeted to tests or test projects label Mar 6, 2025
@benrr101 benrr101 requested a review from a team March 6, 2025 18:40
@benrr101
Copy link
Contributor Author

benrr101 commented Mar 7, 2025

@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
Copy link
Contributor

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
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@benrr101
Copy link
Contributor Author

@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 :)

@paulmedynski paulmedynski self-assigned this Mar 21, 2025
benrr101 and others added 2 commits March 27, 2025 12:19
…/TestFixtures/SQLSetupStrategy.cs


Let's try @edwardneal's idea

Co-authored-by: Edward Neal <[email protected]>
…/TestFixtures/SQLSetupStrategy.cs

Co-authored-by: Edward Neal <[email protected]>
@paulmedynski paulmedynski added this to the 6.1-preview1 milestone Apr 2, 2025
@mdaigle mdaigle self-assigned this Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Tests Issues that are targeted to tests or test projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants