Fix | Reenable SqlBulkCopy in least-privilege environments#4306
Open
edwardneal wants to merge 4 commits into
Open
Fix | Reenable SqlBulkCopy in least-privilege environments#4306edwardneal wants to merge 4 commits into
edwardneal wants to merge 4 commits into
Conversation
* Add an UnescapedName property to DatabaseObject and use it in XEventsTracingTest. * Create ServerLogin primitive. * Create DatabaseUser primitive.
This makes DatabaseUser simpler to work with. It can store a reference to the database the user should be created in, and the class can handle switching to/from that database.
Fix bug by checking HAS_PERMS_BY_NAME over the sys.all_columns view. This is safe on all supported platforms (including Synapse Analytics dedicated and shared pools - documentation is inconsistent.) Regression tests create a low-privileged login and user in on-premise environments, then use those credentials to perform a SqlBulkCopy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SqlBulkCopy has traditionally only normally required SELECT and INSERT permissions over the table being copied to (src). However, recent additions to the class also required SELECT permissions over the
[sys].[all_columns]metadata view. These permissions are present by default but are sometimes removed as part of SQL Server hardening.This change to the permissions contract was unintentional, the failure of SqlBulkCopy in least-privilege environments which only grant SELECT & INSERT permissions was a bug. This PR resolves the bug by guarding all access to the metadata view behind a simple
HAS_PERMS_BY_NAMEcheck.As a result of this change, least-privilege environments will be unable to use the newer features introduced in #3677 and #3590. I think this is unavoidable - there's no way to implement the features safely without access to the metadata view.
HAS_PERMS_BY_NAMEis documented here as being supported in SQL Server, Azure SQL, managed instances and Fabric. It doesn't document support for two platforms: Azure Synapse Analytics, and APS/PDW. However, other documentation confirms that Azure Synapse Serverless pools support this function, and testing against a dedicated pool confirms that this does too. Furthermore, the APS/PDW 2016 release notes explicitly document the introduction of this function.I've chosen to use this approach because wrapping metadata access in a
TRY/CATCHblock won't work - whenXACT_ABORTis enabled, even a caught exception will result in the wrapping transaction being doomed, and Synapse doesn't allow this option to be temporarily turned off.Issues
Fixes #4293.
Testing
Existing SqlBulkCopy tests continue to pass. I've added two new tests which build a low-privileged environment, then run a bulk copy inside it. They verify that the copy succeeds, but also that the newer alias-based functionality is unavailable.
As part of this, I've added
ServerLoginandDatabaseUserprimitives. The latter primitive highlighted the need for some form of state (the database in which we create the user) to be made available at the point of creation, so I've added this.This widened test infrastructure and more complex testing accounts for about 3/4 of the changes in this PR.
Could someone start a CI run please?