Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support managed identities and service principals #1372

Merged
merged 8 commits into from
Aug 16, 2023

Conversation

mjcheetham
Copy link
Collaborator

@mjcheetham mjcheetham commented Aug 7, 2023

Add support for both managed identities and service principals for Azure Repos authentication.

Service principals are the "service account" equivalent to normal "users" (user principals). Rather than a password+MFA, service principals (SP) authenticate either with a 'secret' or a certificate - we support both.

Managed identities (MI) are an evolution of service principals whereby the secret/certificate are hidden and managed by Azure. There are two types of managed identities: system-assigned and user-assigned. System-assigned are tied to a particular VM or resource, whereas user-assigned can be shared between VMs/resources.

Azure DevOps recently learned to support SPs and MIs as identities that can be members of orgs and projects. Note that this only applies to AAD-connected Azure DevOps orgs.

This functionality is most compelling for automated scenarios, like CI machines, that need access to Azure Repos.

Fixes #1313

@mjcheetham mjcheetham added host:azure-repos Specific to the Azure Repos (Azure DevOps, VSTS) host provider auth:microsoft Specific to Microsoft AAD/MSA authentication enhancement New feature or request labels Aug 7, 2023
@mjcheetham
Copy link
Collaborator Author

@bgavrilMS would you or someone in your team be able to have a brief look over the Service Principal and Managed Identity parts in MicrosoftAuthentication.cs? Thank you!

#endregion

#region Helpers

private async Task RegisterTokenCacheAsync(IPublicClientApplication app, ITrace2 trace2)
private async Task RegisterTokenCacheAsync(IClientApplicationBase app, ITrace2 trace2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For client_credentials, the tokens are placed in app.AppTokenCache. We recommend not mixing AppTokenCache and UserTokenCache, i.e. just use another file for app token cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot! I had overlooked the difference between AppTokenCache and UserTokenCache.

I don't know of any app token cache that we could share (like we do with other Microsoft developer tools for the user cache). We can just store in our ~/.gcm data directory for now.


public static class X509Utils
{
public static X509Certificate2 GetCertificateByThumbprint(string thumbprint)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a slight more battle tested way to load certs. I think this is fine for loading by thumbprint, but if loading by name you should exclude expired certs and load the most fresh.

https://github.com/AzureAD/microsoft-identity-web/blob/96323e40bc6e6610192461f7b1dfaf4c2605ff21/src/Microsoft.Identity.Web.Certificate/CertificateLoaderHelper.cs#L55

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a compelling argument for allowing people to specify something other than a thumbprint (like the cert name, or file path)? Are there common situations where the thumbprint would not be known?

I guess at certificate renewal a new thumbprint would be created, but the certificate would have the same name?

Right now we only have credential.azreposServicePrincipalCertificateThumbprint / GCM_AZREPOS_SP_CERT_THUMBPRINT to specify a thumbprint.

We could add other options like ..CertificatePath / .._CERT_PATH and ..CertificateName / .._CERT_NAME in the future (unless you think it's useful from the get go!) :)

Copy link

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that you serialize cca.AppTokenCache for client_credentials.

@mjcheetham
Copy link
Collaborator Author

Ensure that you serialize cca.AppTokenCache for client_credentials.

Thanks for spotting this! I've pushed an update that should now correctly serialise the AppTokenCache and UserTokenCaches separately. Had a question on if there's any known shared AppTokensCaches, akin to the VS/dev-tools one for user tokens.

Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Just a few nitpicky comments about documentation/trace statements.

src/shared/Core/Authentication/MicrosoftAuthentication.cs Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/environment.md Outdated Show resolved Hide resolved
docs/environment.md Outdated Show resolved Hide resolved
Rename the lone GetToken method to clarify that this is for user
principals (regular user identities). This is in preparation for adding
extra principal types including service principals, and managed
identities.

Also add some XML doc comments to the method.
Refactor the token cache helper methods to allow us to re-use the
existing cache registration logic with a different ITokenCache and
StorageCreationProperties.

This will be useful when we later introduce a confidential client
application (for service principals) that needs a different cache
location, and uses the AppTokenCache, rather than the User one.
Add support for acquiring a token for a service principal.
Either a client secret or certificate can be used to authenticate (the
latter being preferred).
Add support for obtaining an access token using either the
system-assigned and a user-assigned managed identity.
Add app token cache support for confidential client applications
(service principals). This is a different cache than the one for user
tokens that is used by public client applications (for normal users).

We do not know of any other app token cache that we can share with
currently, so we just use our own in the GCM data directory.
Allow a service principal or managed identity to be used to
authenticate against Azure Repos. Required information for
service principals is specified in Git config or environment
variables, as is the ID for a managed identity.
Add tests of the `GetCredentialAsync` method on the
`AzureReposHostProvider` using managed identity and service principal.
@mjcheetham mjcheetham merged commit 488aa48 into git-ecosystem:main Aug 16, 2023
8 checks passed
@mjcheetham mjcheetham deleted the midsp branch August 16, 2023 21:55
mjcheetham added a commit that referenced this pull request Nov 1, 2023
**Changes:**

- Add support for managed identity and service principals in Azure Repos
(#1372)
- Support universal Gitea OAuth app configuration (#1442)
- Set default generic OAuth redirect URI value (#1444)
- Drop WPF helpers on Windows (#1417)
- Add software rendering override for Windows (#1445, #1453)
- Recognise GitLab hosts via WWW-Authenticate header (#1428)
- Recognise Bitbucket hosts via WWW-Authenticate header (#1441)
- Support GitHub Gist remote URLs (#1402)
- Update to Avalonia 11.x (#1383)
- Documentation updates (#1416)
- Drop unnecessary .NET Framework-specific code (#1447)
- Updates to release process (#1386, #1381)
- Update code signing certificates (#1431)
mjcheetham added a commit that referenced this pull request Nov 1, 2023
**Changes:**

_Since 2.4.0:_

- Fix macOS ARM64 tarball contents (#1458)

_Since 2.3.x:_

- Add support for managed identity and service principals in Azure Repos
(#1372)
- Support universal Gitea OAuth app configuration (#1442)
- Set default generic OAuth redirect URI value (#1444)
- Drop WPF helpers on Windows (#1417)
- Add software rendering override for Windows (#1445, #1453)
- Recognise GitLab hosts via WWW-Authenticate header (#1428)
- Recognise Bitbucket hosts via WWW-Authenticate header (#1441)
- Support GitHub Gist remote URLs (#1402)
- Update to Avalonia 11.x (#1383)
- Documentation updates (#1416)
- Drop unnecessary .NET Framework-specific code (#1447)
- Updates to release process (#1386, #1381)
- Update code signing certificates (#1431)
@bayu889900

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth:microsoft Specific to Microsoft AAD/MSA authentication enhancement New feature or request host:azure-repos Specific to the Azure Repos (Azure DevOps, VSTS) host provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Azure managed identity authentication with AzureDevOps repos
5 participants