From 62b9c3d5f8698f4b64804524321844bd741e125c Mon Sep 17 00:00:00 2001 From: "Michael J. Lyons (XBOX)" Date: Wed, 17 Jul 2024 14:04:42 -0700 Subject: [PATCH 1/5] Add method for sending X5C --- VERSION | 2 +- .../Authentication/MicrosoftAuthentication.cs | 15 ++++++++++++++- .../Microsoft.AzureRepos/AzureDevOpsConstants.cs | 2 ++ .../AzureReposHostProvider.cs | 8 ++++++++ 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index 27c7c7b9d..3a6d2147d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.5.1.0 +2.5.1.1 diff --git a/src/shared/Core/Authentication/MicrosoftAuthentication.cs b/src/shared/Core/Authentication/MicrosoftAuthentication.cs index b39cc1a73..c64ce4b9c 100644 --- a/src/shared/Core/Authentication/MicrosoftAuthentication.cs +++ b/src/shared/Core/Authentication/MicrosoftAuthentication.cs @@ -92,6 +92,11 @@ public class ServicePrincipalIdentity /// If both and are set, the certificate will be used. /// public string ClientSecret { get; set; } + + /// + /// Whether the authentication should send X5C + /// + public bool SendX5C { get; set; } } public interface IMicrosoftAuthenticationResult @@ -269,7 +274,15 @@ public async Task GetTokenForServicePrincipalAsy try { - AuthenticationResult result = await app.AcquireTokenForClient(scopes).ExecuteAsync(); + var tokenBuilder = app.AcquireTokenForClient(scopes); + + if (sp.SendX5C) + { + tokenBuilder = tokenBuilder.WithSendX5C(true); + } + + AuthenticationResult result = await tokenBuilder.ExecuteAsync(); + return new MsalResult(result); } catch (Exception ex) diff --git a/src/shared/Microsoft.AzureRepos/AzureDevOpsConstants.cs b/src/shared/Microsoft.AzureRepos/AzureDevOpsConstants.cs index c46f08c33..a282d4eff 100644 --- a/src/shared/Microsoft.AzureRepos/AzureDevOpsConstants.cs +++ b/src/shared/Microsoft.AzureRepos/AzureDevOpsConstants.cs @@ -44,6 +44,7 @@ public static class EnvironmentVariables public const string ServicePrincipalId = "GCM_AZREPOS_SERVICE_PRINCIPAL"; public const string ServicePrincipalSecret = "GCM_AZREPOS_SP_SECRET"; public const string ServicePrincipalCertificateThumbprint = "GCM_AZREPOS_SP_CERT_THUMBPRINT"; + public const string ServicePrincipalCertificateSendX5C = "GCM_AZREPOS_SP_CERT_SEND_X5C"; public const string ManagedIdentity = "GCM_AZREPOS_MANAGEDIDENTITY"; } @@ -59,6 +60,7 @@ public static class Credential public const string ServicePrincipal = "azreposServicePrincipal"; public const string ServicePrincipalSecret = "azreposServicePrincipalSecret"; public const string ServicePrincipalCertificateThumbprint = "azreposServicePrincipalCertificateThumbprint"; + public const string ServicePrincipalCertificateSendX5C = "azreposServicePrincipalCertificateSendX5C"; public const string ManagedIdentity = "azreposManagedIdentity"; } } diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index 55b1449d7..cdbf16133 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -549,6 +549,14 @@ private bool UseServicePrincipal(out ServicePrincipalIdentity sp) if (hasCertThumbprint) { + bool hasX5CSetting = _context.Settings.TryGetSetting( + AzureDevOpsConstants.EnvironmentVariables.ServicePrincipalCertificateSendX5C, + Constants.GitConfiguration.Credential.SectionName, + AzureDevOpsConstants.GitConfiguration.Credential.ServicePrincipalCertificateSendX5C, + out string certHasX5C); + + sp.SendX5C = !hasX5CSetting || certHasX5C == "false"; + X509Certificate2 cert = X509Utils.GetCertificateByThumbprint(certThumbprint); if (cert is null) { From ece43792a8335393a3611d1e9ad529edd7b7501c Mon Sep 17 00:00:00 2001 From: "Michael J. Lyons (XBOX)" Date: Thu, 18 Jul 2024 06:18:56 -0700 Subject: [PATCH 2/5] Debugging updates --- .../Core/Authentication/MicrosoftAuthentication.cs | 12 +++--------- .../Microsoft.AzureRepos/AzureReposHostProvider.cs | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/shared/Core/Authentication/MicrosoftAuthentication.cs b/src/shared/Core/Authentication/MicrosoftAuthentication.cs index c64ce4b9c..12bccf5fe 100644 --- a/src/shared/Core/Authentication/MicrosoftAuthentication.cs +++ b/src/shared/Core/Authentication/MicrosoftAuthentication.cs @@ -274,20 +274,14 @@ public async Task GetTokenForServicePrincipalAsy try { - var tokenBuilder = app.AcquireTokenForClient(scopes); - - if (sp.SendX5C) - { - tokenBuilder = tokenBuilder.WithSendX5C(true); - } - - AuthenticationResult result = await tokenBuilder.ExecuteAsync(); + Context.Trace.WriteLine($"Sending with X5C: '{sp.SendX5C}'."); + AuthenticationResult result = await app.AcquireTokenForClient(scopes).WithSendX5C(sp.SendX5C).ExecuteAsync();; return new MsalResult(result); } catch (Exception ex) { - Context.Trace.WriteLine($"Failed to acquire token for service principal '{sp.TenantId}/{sp.TenantId}'."); + Context.Trace.WriteLine($"Failed to acquire token for service principal '{sp.TenantId}/{sp.Id}'."); Context.Trace.WriteException(ex); throw; } diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index cdbf16133..9bd6c72a1 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -555,7 +555,7 @@ private bool UseServicePrincipal(out ServicePrincipalIdentity sp) AzureDevOpsConstants.GitConfiguration.Credential.ServicePrincipalCertificateSendX5C, out string certHasX5C); - sp.SendX5C = !hasX5CSetting || certHasX5C == "false"; + sp.SendX5C = !hasX5CSetting || certHasX5C != "false"; X509Certificate2 cert = X509Utils.GetCertificateByThumbprint(certThumbprint); if (cert is null) From 52c1be973c5182e336e50599ee0377143760e466 Mon Sep 17 00:00:00 2001 From: "Michael J. Lyons (XBOX)" Date: Thu, 18 Jul 2024 16:51:16 -0700 Subject: [PATCH 3/5] Revert version change --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 3a6d2147d..27c7c7b9d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.5.1.1 +2.5.1.0 From 55d626240ff4b427a04082f4e33a5e34e0d46ed8 Mon Sep 17 00:00:00 2001 From: "Michael J. Lyons (XBOX)" Date: Thu, 18 Jul 2024 17:50:09 -0700 Subject: [PATCH 4/5] Apply code review feedback to use more appropriate helper functions in parsing a boolean config option --- src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index 9bd6c72a1..1d5c649d0 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -549,13 +549,11 @@ private bool UseServicePrincipal(out ServicePrincipalIdentity sp) if (hasCertThumbprint) { - bool hasX5CSetting = _context.Settings.TryGetSetting( + sp.SendX5C = _context.Settings.TryGetSetting( AzureDevOpsConstants.EnvironmentVariables.ServicePrincipalCertificateSendX5C, Constants.GitConfiguration.Credential.SectionName, AzureDevOpsConstants.GitConfiguration.Credential.ServicePrincipalCertificateSendX5C, - out string certHasX5C); - - sp.SendX5C = !hasX5CSetting || certHasX5C != "false"; + out string certHasX5CStr) && certHasX5CStr.ToBooleanyOrDefault(false); X509Certificate2 cert = X509Utils.GetCertificateByThumbprint(certThumbprint); if (cert is null) From ab05752af4e3709c9059f9d00f1502ebfe9084f6 Mon Sep 17 00:00:00 2001 From: "Michael J. Lyons (XBOX)" Date: Thu, 18 Jul 2024 17:50:46 -0700 Subject: [PATCH 5/5] Update configuration and environment documentation to document the new x5c MSAL claim option --- docs/azrepos-misp.md | 3 +++ docs/configuration.md | 22 ++++++++++++++++++++++ docs/environment.md | 28 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/docs/azrepos-misp.md b/docs/azrepos-misp.md index 60a3c3e2b..9d11dbfa6 100644 --- a/docs/azrepos-misp.md +++ b/docs/azrepos-misp.md @@ -108,6 +108,7 @@ Type|Git Configuration|Environment Variable -|-|- Client Secret|[`credential.azreposServicePrincipalSecret`][gcm-sp-secret-config]|[`GCM_AZREPOS_SP_SECRET`][gcm-sp-secret-env] Certificate|[`credential.azreposServicePrincipalCertificateThumbprint`][gcm-sp-cert-config]|[`GCM_AZREPOS_SP_CERT_THUMBPRINT`][gcm-sp-cert-env] +Send X5C|[`credential.azreposServicePrincipalCertificateSendX5C`][gcm-sp-cert-x5c-config]|[`GCM_AZREPOS_SP_CERT_SEND_X5C`][gcm-sp-cert-x5c-env] The value for these options should be the client secret or the thumbrint of the certificate that is associated with the Service Principal. @@ -126,4 +127,6 @@ current user or the local machine. [gcm-sp-secret-config]: https://gh.io/gcm/config#credentialazreposserviceprincipalsecret [gcm-sp-secret-env]: https://gh.io/gcm/env#GCM_AZREPOS_SP_SECRET [gcm-sp-cert-config]: https://gh.io/gcm/config#credentialazreposserviceprincipalcertificatethumbprint +[gcm-sp-cert-x5c-config]: https://gh.io/gcm/config#credentialazreposserviceprincipalcertificatesendx5c [gcm-sp-cert-env]: https://gh.io/gcm/env#GCM_AZREPOS_SP_CERT_THUMBPRINT +[gcm-sp-cert-x5c-env]: https://gh.io/gcm/env#GCM_AZREPOS_SP_CERT_SEND_X5C diff --git a/docs/configuration.md b/docs/configuration.md index 2439c9297..f6993dfd8 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -858,6 +858,7 @@ You must also set at least one authentication mechanism if you set this value: - [credential.azreposServicePrincipalSecret][credential-azrepos-sp-secret] - [credential.azreposServicePrincipalCertificateThumbprint][credential-azrepos-sp-cert-thumbprint] +- [credential.azreposServicePrincipalCertificateSendX5C][credential-azrepos-sp-cert-x5c] For more information about service principals, see the Azure DevOps [documentation][azrepos-sp-mid]. @@ -904,6 +905,25 @@ git config --global credential.azreposServicePrincipalCertificateThumbprint "9b6 --- +### credential.azreposServicePrincipalCertificateSendX5C + +When using a certificate for [service principal][service-principal] authentication, this configuration +specifies whether the X5C claim should be should be sent to the STS. Sending the x5c +enables application developers to achieve easy certificate rollover in Azure AD: +this method will send the public certificate to Azure AD along with the token request, +so that Azure AD can use it to validate the subject name based on a trusted issuer +policy. This saves the application admin from the need to explicitly manage the +certificate rollover. For details see [https://aka.ms/msal-net-sni](https://aka.ms/msal-net-sni). + +#### Example + +```shell +git config --global credential.azreposServicePrincipalCertificateSendX5C true +``` +**Also see: [GCM_AZREPOS_SP_CERT_SEND_X5C][gcm-azrepos-sp-cert-x5c]** + +--- + ### trace2.normalTarget Turns on Trace2 Normal Format tracing - see [Git's Trace2 Normal Format @@ -1034,6 +1054,8 @@ Defaults to disabled. [credential-azrepos-sp]: #credentialazreposserviceprincipal [credential-azrepos-sp-secret]: #credentialazreposserviceprincipalsecret [credential-azrepos-sp-cert-thumbprint]: #credentialazreposserviceprincipalcertificatethumbprint +[credential-azrepos-sp-cert-x5c]: #credentialazreposserviceprincipalcertificatesendx5c [gcm-azrepos-service-principal]: environment.md#GCM_AZREPOS_SERVICE_PRINCIPAL [gcm-azrepos-sp-secret]: environment.md#GCM_AZREPOS_SP_SECRET [gcm-azrepos-sp-cert-thumbprint]: environment.md#GCM_AZREPOS_SP_CERT_THUMBPRINT +[gcm-azrepos-sp-cert-x5c]: environment.md#GCM_AZREPOS_SP_CERT_SEND_X5C diff --git a/docs/environment.md b/docs/environment.md index 18f3f05fe..edda0d714 100644 --- a/docs/environment.md +++ b/docs/environment.md @@ -1039,6 +1039,32 @@ export GCM_AZREPOS_SP_CERT_THUMBPRINT="9b6555292e4ea21cbc2ebd23e66e2f91ebbe92dc" --- +### GCM_AZREPOS_SP_CERT_SEND_X5C + +When using a certificate for service principal authentication, this configuration +specifies whether the X5C claim should be should be sent to the STS. Sending the x5c +enables application developers to achieve easy certificate rollover in Azure AD: +this method will send the public certificate to Azure AD along with the token request, +so that Azure AD can use it to validate the subject name based on a trusted issuer +policy. This saves the application admin from the need to explicitly manage the +certificate rollover. For details see [https://aka.ms/msal-net-sni](https://aka.ms/msal-net-sni). + +#### Windows + +```batch +SET GCM_AZREPOS_SP_CERT_SEND_X5C="true" +``` + +#### macOS/Linux + +```bash +export GCM_AZREPOS_SP_CERT_SEND_X5C="true" +``` + +**Also see: [credential.azreposServicePrincipalCertificateSendX5C][credential-azrepos-sp-cert-x5c]** + +--- + ### GIT_TRACE2 Turns on Trace2 Normal Format tracing - see [Git's Trace2 Normal Format @@ -1184,6 +1210,8 @@ Defaults to disabled. [gcm-azrepos-sp]: #gcm_azrepos_service_principal [gcm-azrepos-sp-secret]: #gcm_azrepos_sp_secret [gcm-azrepos-sp-cert-thumbprint]: #gcm_azrepos_sp_cert_thumbprint +[gcm-azrepos-sp-cert-x5c]: #gcm_azrepos_sp_cert_send_x5c [credential-azrepos-sp]: configuration.md#credentialazreposserviceprincipal [credential-azrepos-sp-secret]: configuration.md#credentialazreposserviceprincipalsecret [credential-azrepos-sp-cert-thumbprint]: configuration.md#credentialazreposserviceprincipalcertificatethumbprint +[credential-azrepos-sp-cert-x5c]: configuration.md#credentialazreposserviceprincipalcertificatesendx5c