Skip to content

WIP: Update ContainerRegistry code to use ORAS.NET#1961

Draft
adityapatwardhan wants to merge 6 commits intomasterfrom
MoveToOras
Draft

WIP: Update ContainerRegistry code to use ORAS.NET#1961
adityapatwardhan wants to merge 6 commits intomasterfrom
MoveToOras

Conversation

@adityapatwardhan
Copy link
Member

@adityapatwardhan adityapatwardhan commented Mar 11, 2026

PR Summary

This pull request introduces significant updates to modernize the codebase by migrating from .NET Framework 4.7.2 to .NET 8.0, adds new dependencies for improved functionality, and refines group policy enforcement to be Windows-specific. Additionally, it introduces a new credential provider for ORAS registry integration and makes several code improvements for robustness and maintainability.

Migration to .NET 8.0 and Dependency Updates:

  • Changed build scripts and project files (build.ps1, doBuild.ps1, Microsoft.PowerShell.PSResourceGet.csproj) to target net8.0 instead of net472, updated language version to 12.0, and added/updated dependencies including OrasProject.Oras, Microsoft.Extensions.Caching.Memory, and others. [1] [2] [3] [4] [5]

Group Policy Enforcement Improvements:

  • Refactored group policy repository enforcement checks to only apply on Windows, using OperatingSystem.IsWindows() and [SupportedOSPlatform("windows")] attributes, ensuring cross-platform compatibility and avoiding unnecessary checks on non-Windows systems. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]

ORAS Registry Integration:

  • Added a new file PSResourceGetCredentialProvider.cs implementing the ICredentialProvider interface for ORAS registry authentication, supporting SecretManagement, Azure Identity, and anonymous access, and handling AAD-to-ACR token exchanges.
  • Added OrasProject.Oras NuGet package as a dependency in the project file.

Code Robustness and Maintenance:

  • Improved registry value handling in GroupPolicyRepositoryEnforcement to check for null or empty values before conversion, preventing potential exceptions.
  • Changed exception handling in InstallHelper.cs to rethrow exceptions without losing stack trace.

Minor Improvements:

  • Changed PSScriptMetadata constructor to accept an explicit GUID instead of generating a new one when not provided.
  • Added missing using NuGet.Protocol.Core.Types; in RepositorySettings.cs for completeness.

PR Context

PR Checklist

@adityapatwardhan adityapatwardhan marked this pull request as draft March 11, 2026 22:26
@adityapatwardhan
Copy link
Member Author

adityapatwardhan commented Mar 11, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 11, 2026

Azure Pipelines successfully started running 1 pipeline(s).

@adityapatwardhan
Copy link
Member Author

adityapatwardhan commented Mar 11, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 11, 2026

Azure Pipelines successfully started running 1 pipeline(s).

@adityapatwardhan
Copy link
Member Author

adityapatwardhan commented Mar 11, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 11, 2026

Azure Pipelines successfully started running 1 pipeline(s).

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 11, 2026
View reviewed changes
src/code/PSResourceGetCredentialProvider.cs
? string.Format(RefreshTokenRequestBodyNoTenantTemplate, _registryHost, aadAccessToken)
: string.Format(RefreshTokenRequestBodyTemplate, _registryHost, tenantId, aadAccessToken);

using var content = new StringContent(requestBody, System.Text.Encoding.UTF8, "application/x-www-form-urlencoded");

Check warning

Code scanning / CodeQL

Information exposure through transmitted data Medium

This data transmitted to the user depends on
sensitive information
Loading
.
This data transmitted to the user depends on
sensitive information
Loading
.

Copilot Autofix

AI 2 days ago

In general, the right fix is not to stop sending the access token to the token-exchange service (that’s necessary) but to (1) ensure it is only ever transmitted over a secure channel to the expected host, and (2) make sure it is never reflected back to the user via error messages or logs. This aligns with the guideline “avoid transmitting passwords or exceptions to the user”: we continue to transmit the token to the auth service, but we prevent accidental disclosure elsewhere and explicitly constrain the transmission to HTTPS.

For this specific code, the best minimally invasive fix is:

  1. Validate exchangeUrl before sending the HTTP request in ExchangeForAcrRefreshTokenAsync:
    • Parse exchangeUrl as a Uri.
    • Ensure its scheme is https; if not, throw or return null with a safe warning, without ever sending the sensitive requestBody.
    • Optionally, we can also ensure the host matches the expected registry host domain, but we can do that without changing functionality by at least enforcing HTTPS.
  2. Keep the existing behavior of using aadAccessToken in the request body; we are only adding a guard to ensure that this sensitive value is not sent over an insecure protocol.
  3. Avoid introducing any logs that include the token value; the current code does not log it, so we leave logging as-is.

All required changes are localized to ExchangeForAcrRefreshTokenAsync in src/code/PSResourceGetCredentialProvider.cs. No changes are needed in Utils.cs, and we can rely on System.Uri, which is already part of the BCL and requires no new imports.

Concretely:

  • In ExchangeForAcrRefreshTokenAsync, right after string exchangeUrl = ..., add code to create a Uri from exchangeUrl and verify uri.Scheme == Uri.UriSchemeHttps. If not, throw an InvalidOperationException (or return null) before constructing requestBody or sending it.
  • This makes CodeQL recognize that any transmission of the tainted value only happens over HTTPS and mitigates accidental exposure via non-secure channels.
Suggested changeset 1
src/code/PSResourceGetCredentialProvider.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/code/PSResourceGetCredentialProvider.cs b/src/code/PSResourceGetCredentialProvider.cs
--- a/src/code/PSResourceGetCredentialProvider.cs
+++ b/src/code/PSResourceGetCredentialProvider.cs
@@ -145,6 +145,14 @@
         private async Task<string> ExchangeForAcrRefreshTokenAsync(string aadAccessToken, string tenantId, CancellationToken cancellationToken)
         {
             string exchangeUrl = string.Format(OAuthExchangeUrlTemplate, _registryHost);
+
+            // Ensure that we only transmit the access token over a secure (HTTPS) channel.
+            if (!Uri.TryCreate(exchangeUrl, UriKind.Absolute, out Uri exchangeUri) ||
+                !string.Equals(exchangeUri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase))
+            {
+                throw new InvalidOperationException($"Exchange URL must use HTTPS. Actual URL: {exchangeUrl}");
+            }
+
             string requestBody = string.IsNullOrEmpty(tenantId)
                 ? string.Format(RefreshTokenRequestBodyNoTenantTemplate, _registryHost, aadAccessToken)
                 : string.Format(RefreshTokenRequestBodyTemplate, _registryHost, tenantId, aadAccessToken);
EOF
@@ -145,6 +145,14 @@
private async Task<string> ExchangeForAcrRefreshTokenAsync(string aadAccessToken, string tenantId, CancellationToken cancellationToken)
{
string exchangeUrl = string.Format(OAuthExchangeUrlTemplate, _registryHost);

// Ensure that we only transmit the access token over a secure (HTTPS) channel.
if (!Uri.TryCreate(exchangeUrl, UriKind.Absolute, out Uri exchangeUri) ||
!string.Equals(exchangeUri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase))
{
throw new InvalidOperationException($"Exchange URL must use HTTPS. Actual URL: {exchangeUrl}");
}

string requestBody = string.IsNullOrEmpty(tenantId)
? string.Format(RefreshTokenRequestBodyNoTenantTemplate, _registryHost, aadAccessToken)
: string.Format(RefreshTokenRequestBodyTemplate, _registryHost, tenantId, aadAccessToken);
Copilot is powered by AI and may make mistakes. Always verify output.
@adityapatwardhan
Copy link
Member Author

adityapatwardhan commented Mar 12, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 12, 2026

Azure Pipelines successfully started running 1 pipeline(s).

@adityapatwardhan
Copy link
Member Author

adityapatwardhan commented Mar 13, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 13, 2026

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anamnavi anamnavi Awaiting requested review from anamnavi anamnavi is a code owner

@alerickson alerickson Awaiting requested review from alerickson alerickson is a code owner

@SydneyhSmith SydneyhSmith Awaiting requested review from SydneyhSmith SydneyhSmith is a code owner

@shammu1 shammu1 Awaiting requested review from shammu1 shammu1 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@adityapatwardhan