Cosmos extension/client secret credential support#242
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds support for authenticating to Cosmos DB using an Azure AD Service Principal (client secret) alongside the existing ConnectionString and RBAC modes. New settings, validation rules, client construction logic, and unit tests are introduced.
Changes:
- New
UseServicePrincipalAuth,TenantId,ClientId,ClientSecretproperties onCosmosSettingsBasewith associated validation rules and a mutual-exclusion check againstUseRbacAuth. CosmosExtensionServices.CreateClientconstructs aCosmosClientwithClientSecretCredential(and optional Key Vault encryption) when Service Principal auth is enabled.- Unit tests covering missing Service Principal configuration and a fully-configured success case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension/CosmosSettingsBase.cs | Adds Service Principal settings and validation rules. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension/CosmosExtensionServices.cs | Adds Service Principal branch to client creation, with optional client-side encryption. |
| Extensions/Cosmos/Cosmos.DataTransfer.CosmosExtension.UnitTests/CosmosSinkSettingsTests.cs | Adds validation tests for Service Principal auth (missing fields and success cases). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Hi @philnach - Could you have a look at this PR and consider it? |
|
@copilot , if dmt is hosted in azure with userbac would dmt just work if the.machine has a managed identity with access to cosmosdb? |
|
Hi @thucnguyen77, thanks for the contribution and for explaining the use case. Before we go further on the implementation, I want to make sure we're not adding code that duplicates an existing capability. The current
So with no code changes, you can already do this against a Cosmos account that only allows AAD/RBAC: $env:AZURE_TENANT_ID = "<your-tenant-id>"
$env:AZURE_CLIENT_ID = "<your-sp-app-id>"
$env:AZURE_CLIENT_SECRET = "<your-sp-secret>"
dmt.exe --settings migrationsettings.json…with this existing settings shape: {
"Sink": "Cosmos-nosql",
"SinkSettings": {
"UseRbacAuth": true,
"AccountEndpoint": "https://<account>.documents.azure.com:443/",
"Database": "myDb",
"Container": "myContainer"
}
}The credential chain also transparently falls through to:
So A few questions to help us decide the right path forward:
If there is a concrete scenario the env-var path can't cover, happy to keep iterating — but in that case I'd want us to (a) treat Thanks again really appreciate you taking the time to file an issue and propose a fix with tests! |
|
Hi @philnach , thanks very much for your review and feedback. Please find my answers to your questions below.
This is exactly the scenario I am in. The source and target databases are in two different Azure tenants. And the migration tool is supposed to run in a shared server that has access to both tenants. As this is a shared server, we would minimised the footprint, so adding environment variables is not ideal.
I am ware of the
So with what discussed above, I am hoping that the proposed fix I am more than happy to fine-tune the fix further, or to leave with you for further update to the PR where suitable. Cheers, |
|
Hi @thucnguyen77, thanks for the detailed answer — that scenario makes complete sense and you're right, it's one that A couple of things I'd like to land before merge — happy for you to take these on, or I can pick them up if you'd prefer: Required
Recommended (would simplify the diff)
Nits
If you'd like, I can take items 1–2 (README + warning) in a follow-up commit on your branch after you're done with the code refactor, or you can roll them in — your call. Once those land I think we're in good shape to merge. Thanks again for sticking with this and for the clear explanation of the cross-tenant scenario. That's the kind of context that makes it easy to say yes. |
|
Hi @philnach, Thanks very much for your feedback, and your suggestion 'Fold the SP path into So I went ahead and updated the code according to your suggestion, including the support for client certificate as well. Please help review again. With the Extensions/Cosmos/README.md update, if you are happy with the code, I am hoping you could help take it up accordingly using your writing style, just to make it consistent. Kind regards, |
philnach
left a comment
philnach
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround @thucnguyen77 — the refactor to fold service-principal under UseRbacAuth reads really well and is much cleaner than carrying a parallel flag. A few specific things I liked:
- The
tokenCredential ??= new DefaultAzureCredential(...)fallback pattern collapses the previous duplicated encryption branch nicely. - Good call adding
ClientCertificatePath/ClientCertificatePasswordalongsideClientSecret— that gives security-conscious users a non-secret path. - The certificate construction correctly picks the password vs. no-password overloads of
ClientCertificateCredential. - Test coverage for the new validation matrix (partial SP info, secret-only, cert-only) is solid.
- Mirroring tests into the source-side suite is the right instinct.
I have two blocking items, one I'd really like to see, and a few nits. Happy to take on the README update separately as previously discussed — once the items below are addressed I'll approve.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| cosmosClient = new CosmosClient(settings.AccountEndpoint, tokenCredential, clientOptions); | ||
| if (!string.IsNullOrEmpty(settings.ClientSecret)) | ||
| { | ||
| logger.LogWarning("ClientSecret is configured in settings. Ensure this configuration file is not committed to source control. Consider injecting via environment variables, command-line args (--{Section}:ClientSecret=...), or User Secrets instead.", |
|
@thucnguyen77, thanks again for the contribution and patience here. Copilot is diffidently adding more work, so I decided to help out. I pushed a follow-up commit to help close out the remaining review noise and keep this moving. Take a look and let me know if you have any feedback. What I updated
Why I added this
How I validated
|
|
I pushed a cleanup commit that addresses the new actionable items from the latest Copilot pass on head What I fixed
Validation
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
Extensions/Cosmos/README.md:62
- README states
AllowBulkExecutiondefaults tofalseand can be toggled, butCreateClientcurrently hard-codesCosmosClientOptions.AllowBulkExecution = true, so the documented setting/default is inaccurate. Either fix the code to respectAllowBulkExecution, or update the documentation to match actual behavior.
- `InitClientEncryption` (default: `false`): Enable Always Encrypted feature.
- `LimitToEndpoint` (default: `false`): Restrict client to endpoint (see CosmosClientOptions.LimitToEndpoint).
- `DisableSslValidation` (default: `false`): Disable SSL certificate validation (for local dev only; not for production).
- `AllowBulkExecution` (default: `false`): Enable bulk execution for optimized performance. Warning: may affect consistency and error handling.
Source and sink require settings used to locate and access the Cosmos DB account. This can be done in one of three ways:
- Using a `ConnectionString` that includes an AccountEndpoint and AccountKey
- Using RBAC (Role Based Access Control) by setting `UseRbacAuth` to true and specifying `AccountEndpoint` and optionally `EnableInteractiveCredentials` to prompt the user to log in to Azure if default credentials are not available. See ([migrate-passwordless](https://learn.microsoft.com/azure/cosmos-db/nosql/migrate-passwordless?tabs=sign-in-azure-cli%2Cdotnet%2Cazure-portal-create%2Cazure-portal-associate%2Capp-service-identity) for how to configure Cosmos DB for passwordless access.
- Using RBAC with explicit service principal credentials by setting `UseRbacAuth` to true and specifying `AccountEndpoint`, `TenantId`, `ClientId`, and either `ClientSecret` or `ClientCertificatePath`.
> **Security warning**: `ClientSecret` is plaintext in settings files. Do not commit secrets to source control. Prefer runtime injection through .NET configuration providers such as environment variables, command-line args (for example `--SourceSettings:ClientSecret=...` / `--SinkSettings:ClientSecret=...`), or User Secrets.
> **Implementation note**: The RBAC service principal path now uses explicit credential-selection logic with additional validation and error wrapping. This was added to make auth-path behavior deterministic and testable, avoid unmanaged certificate object lifetime handling in the tool process, and surface actionable messages when certificate/credential configuration is invalid.
### Bulk Execution
The extension supports bulk execution for Cosmos DB operations. When the `AllowBulkExecution` setting is set to `true`, operations such as bulk inserts and updates are optimized for performance. Use with caution, as bulk execution may affect consistency and error handling. Default is `false`.
| if (!string.IsNullOrEmpty(settings.TenantId) && !string.IsNullOrEmpty(settings.ClientId)) | ||
| { | ||
| if (!string.IsNullOrEmpty(settings.ClientSecret)) | ||
| { | ||
| return TokenCredentialSelection.ClientSecretCredential; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(settings.ClientCertificatePath)) |
| public bool UseRbacAuth { get; set; } | ||
| public string? AccountEndpoint { get; set; } | ||
| public bool EnableInteractiveCredentials { get; set; } | ||
| public bool InitClientEncryption { get; set; } = false; | ||
| public string? TenantId { get; set; } | ||
| public string? ClientId { get; set; } | ||
| public string? ClientSecret { get; set; } | ||
| public string? ClientCertificatePath { get; set; } | ||
| public string? ClientCertificatePassword { get; set; } |
| using Azure.Identity; | ||
| using Microsoft.Extensions.Logging; | ||
| using Moq; | ||
| using System.Security.Cryptography; | ||
| using System.Security.Cryptography.X509Certificates; |
| CosmosClient? cosmosClient; | ||
| if (settings.UseRbacAuth) | ||
| { | ||
| TokenCredential tokenCredential = new DefaultAzureCredential(includeInteractiveCredentials: settings.EnableInteractiveCredentials); | ||
| var tokenCredential = CreateRbacTokenCredential(settings, logger); | ||
|
|
||
| if(settings.InitClientEncryption) | ||
| { | ||
| var keyResolver = new KeyResolver(tokenCredential); | ||
| cosmosClient = new CosmosClient(settings.AccountEndpoint, tokenCredential, clientOptions) | ||
| .WithEncryption(keyResolver, KeyEncryptionKeyResolverName.AzureKeyVault); | ||
| } | ||
| else | ||
| { | ||
| cosmosClient = new CosmosClient(settings.AccountEndpoint, tokenCredential, clientOptions); | ||
| } | ||
| cosmosClient = settings.InitClientEncryption |
Hi @philnach, Thanks very much for taking it on further. It has been hectic for me at work recently so did not have time to work on this. Hope the PR is now in good shape for code-merge with the main goals now achieved. I am more than happy to fix any issue related to this feature post-release. Regards, |
This pull request adds support for explicit service principal credentials within the existing
UseRbacAuthauthentication mode for the Cosmos extension. In addition to default credential-chain behavior, users can now provideTenantId+ClientIdand eitherClientSecretorClientCertificatePath(with optionalClientCertificatePassword) to authenticate source and sink independently, including cross-tenant migration scenarios.The PR also adds validation for service principal configuration combinations, conflict checks for mixed auth inputs, and unit tests for validation and credential-selection behavior.
Service Principal Authentication Support
CosmosSettingsBasefor Service Principal authentication:UseServicePrincipalAuth,TenantId,ClientId, andClientSecret.CreateClientmethod inCosmosExtensionServicesto create aCosmosClientusing Service Principal credentials whenUseRbacAuthis enabled, including support for client-side encryption with Key Vault integration.Validation Enhancements
Validatemethod inCosmosSettingsBaseto handle Service Principal authentication, ensuring required fields are present, preventing conflicting authentication modes, and updating error messages accordingly.Unit Testing
CosmosSinkSettingsTeststo verify validation errors are correctly reported for missing Service Principal configuration and that valid configurations pass validation.