Merge | SqlCommand Encryption Methods #3676
Conversation
…ncryptionParameterResults
…ncryptionParameterResults
… the server returns the right number of rows for the sp_describe_parameter_encryption, but since it's just a debug build check, it has no bearing on prod builds.
…sEnclaveComputations
…roviders, HasColumnEncryptionKeyStoreProviderRegistered
…ParameterEncryptionRPC, IsDescribeParameterEncryptionRPCCurrentlyInProgress
…DescribeParameterEncryption
…yptionKeyStoreProvider
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
Pull Request Overview
This PR merges SqlCommand encryption-related methods from platform-specific files into a shared partial class file for better code organization and maintainability. The changes consolidate encryption functionality that was previously duplicated across .NET Framework and .NET Core implementations.
- Moves encryption-related methods from platform-specific SqlCommand files to a new shared SqlCommand.Encryption.cs file
- Consolidates fields, properties, and methods related to column encryption and enclave operations
- Removes code duplication between netfx and netcore implementations
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| SqlCommand.Encryption.cs | New shared partial class containing all encryption-related methods and fields |
| SqlCommand.cs | Added encryption-related fields and properties to shared base class |
| SqlParameter.cs | Added TODO comment about parameter name prefixing logic |
| SqlSecurityUtility.cs | Added debug assertion for connection null check |
| EnclaveDelegate.cs | Added TODO comment about class naming |
| SqlCommand.netfx.cs | Removed encryption methods now in shared file |
| SqlCommand.netcore.cs | Removed encryption methods now in shared file |
| Project files | Added reference to new SqlCommand.Encryption.cs file |
| // @TODO: 3) This doesn't check for null _customColumnEncryptionKeyStoreProviders | ||
| internal List<string> GetColumnEncryptionCustomKeyStoreProvidersNames() | ||
| { | ||
| if (_customColumnEncryptionKeyStoreProviders.Count > 0) |
There was a problem hiding this comment.
Potential null reference exception. The method doesn't check if _customColumnEncryptionKeyStoreProviders is null before accessing its Count property, but line 28 in the TODO comment indicates this should be checked.
| if (_customColumnEncryptionKeyStoreProviders.Count > 0) | |
| if (_customColumnEncryptionKeyStoreProviders != null && _customColumnEncryptionKeyStoreProviders.Count > 0) |
Copilot uses AI. Check for mistakes.
| // systemParamCount == 2 when user parameters are supplied to BuildExecuteSql. | ||
| parameterList = originalRpcRequest.systemParamCount > 1 | ||
| ? (string)originalRpcRequest.systemParams[1].Value | ||
| : null; // @TODO: If it gets set to this, we'll have a null exception later |
There was a problem hiding this comment.
The TODO comment indicates a potential null reference issue. The parameterList variable can be set to null here, which could cause problems in subsequent usage at line 548 where it's used without null checking.
| : null; // @TODO: If it gets set to this, we'll have a null exception later | |
| : string.Empty; // Changed from null to string.Empty to avoid null reference exceptions |
Copilot uses AI. Check for mistakes.
| tsqlParam.SqlDbType = (text.Length << 1) <= TdsEnums.TYPE_SIZE_LIMIT | ||
| ? SqlDbType.NVarChar | ||
| : SqlDbType.NText; // @TODO: Isn't this check being done in a lot of places? Can we factor it out to a utility? | ||
| tsqlParam.Value = text; // @TODO: Uh... isn't this the same value as it was before? |
There was a problem hiding this comment.
The TODO comment suggests redundant assignment. If the value is indeed the same as before, this assignment is unnecessary and should be removed for clarity.
| tsqlParam.Value = text; // @TODO: Uh... isn't this the same value as it was before? | |
| // tsqlParam.Value = text; // Removed redundant assignment. |
Copilot uses AI. Check for mistakes.
| if (describeParameterEncryptionRpcOriginalRpcMap != null) // @TODO: This doesn't do anything | ||
| { | ||
| describeParameterEncryptionRpcOriginalRpcMap = null; | ||
| } |
There was a problem hiding this comment.
The TODO comment indicates this condition doesn't perform any meaningful operation. The assignment to null on line 769 doesn't affect anything since the parameter is local to this method.
| if (describeParameterEncryptionRpcOriginalRpcMap != null) // @TODO: This doesn't do anything | |
| { | |
| describeParameterEncryptionRpcOriginalRpcMap = null; | |
| } |
Copilot uses AI. Check for mistakes.
| // @TODO: 1) storing this as Command state seems fishy | ||
| // @TODO: 2) despite being concurrent, the usage of ContainsKey -> TryAdd is a race condition | ||
| // @TODO: 3) we have SqlTceCipherInfoTable, we should use it - or make it usable. | ||
| // @TODO: 4) even if we're supposed to store it as state, is the intention to obliterate the list each time? If so, we should probably store it locally and replace the state obj at the end. |
There was a problem hiding this comment.
Race condition in concurrent dictionary usage. The pattern of checking ContainsKey followed by TryAdd on lines 1043-1045 creates a race condition where multiple threads could pass the ContainsKey check simultaneously.
Copilot uses AI. Check for mistakes.
| if (!columnEncryptionKeyTable.TryGetValue(currentOrdinal, out SqlTceCipherInfoEntry cipherInfo)) | ||
| { | ||
| throw SQL.InvalidEncryptionKeyOrdinalEnclaveMetadata( | ||
| currentOrdinal, | ||
| columnEncryptionKeyTable.Count); | ||
| } |
There was a problem hiding this comment.
Redundant dictionary lookup. The TODO comment indicates this lookup was already performed earlier in the same iteration, making this lookup unnecessary and inefficient.
| if (!columnEncryptionKeyTable.TryGetValue(currentOrdinal, out SqlTceCipherInfoEntry cipherInfo)) | |
| { | |
| throw SQL.InvalidEncryptionKeyOrdinalEnclaveMetadata( | |
| currentOrdinal, | |
| columnEncryptionKeyTable.Count); | |
| } | |
| // Removed redundant lookup. Assume cipherInfo is already available from previous lookup. |
Copilot uses AI. Check for mistakes.
| // @TODO: And what happens if they're not in the same order? | ||
| if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName, parameterName)) | ||
| { | ||
| Debug.Assert(sqlParameter.CipherMetadata is not null, "param.CipherMetaData should not be null."); |
There was a problem hiding this comment.
This assertion's logic has been inverted.
| Debug.Assert(sqlParameter is not null, "sqlParameter should not be null."); | ||
|
|
||
| // @TODO: And what happens if they're not in the same order? | ||
| if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName, parameterName)) |
There was a problem hiding this comment.
This needs a third parameter, passing StringComparison.Ordinal.
| Debug.Assert(isAsync == (completion != null), | ||
| "completion should can be null if and only if mode is async."); | ||
|
|
||
| // Fetch reader witn input params |
| if (_activeConnection.Parser is not null) | ||
| { | ||
| tdsParser = _activeConnection.Parser; | ||
| if (tdsParser?.State is TdsParserState.Broken or TdsParserState.Closed) |
There was a problem hiding this comment.
We also want to throw this exception if tdsParser is null.
Description
This is the second to last PR I have planned for merging SqlCommand. In this installment, the methods related to parameter encryption are merged, as well as some other blocks that don't have well defined organization. I'm not certain that these should have their own partial, but to avoid the base partial being too large to navigate, I made the executive decision to make a partial. Each commit is bite-sized and focuses on a single or a couple related methods at a time.
The following methods were merged in this PR:
BuildStoredProcedureStatementForColumnEncryptionClearDescribeParameterEncryptionRequests_currentlyExecutingDescribeParameterEncryptionRPC_customColumnEncryptionKeyStoreProviderscustomDatacustomDataLengthenclaveAttestationParametersGetColumnEncryptionCustomKeyProvidersNamesGetEnclaveSessionParametersGetParameterEncryptionDataReader- code from netcore taken since it is more conciseGetParameterEncryptionDataReaderAsync- code from netcore taken since it is more conciseHasColumnEncryptionKeyStoreProviderRegisteredInvalidateEnclaveSessionIsDescribeParameterEncryptionRPCCurrentlyInProgresskeysToBeSentToEnclavePrepareDescribeParameterEncryptionRequestPrepareTransparentEncryptionFinallyBlockReadDescribeEncryptionParameterResultssp_describe_parameter_encryptionrowsAffectedmember as it was only being used to check if the server returned the right number of rows fromsp_describe_parameter_encryptionin debug mode.requiresEnclaveComputationsResetEncryptionStateRowsAffectedByDescribeParameterEncryption_rowsAffectedBySpDescribeParameterEncryption_rpcForEncryptionSetColumnEncryptionSettingShouldCacheEncryptionMetadataShouldUseEnclaveBasedWorkflow_sqlRPCParameterEncryptionRegArray_forceInternalEndQuery_forceRetryableEnclaveQueryExecutionExceptionDuringGenerateEnclavePackage_sleepAfterReadDescribeEncryptionParameterResults_sleepDuringRunExecuteReaderTdsForSpDescribeParameterEncryption_sleepDuringTryFetchInputParameterEncryptionInfoTryFetchInputParameterEncryptionInfoTryGetColumnEncryptionKeyStoreProviderValidateCustomProviders_wasBatchModeColumnEncryptionSettingsSetOnceIssues
Continuation of work in #1261
Testing
Build passes, SqlCommandTests pass locally. CI should validate the rest of it.