-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Support managed identities and service principals #1372
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6676986
x509: add utils to find certs by thumbprint
mjcheetham b627044
msauth: rename GetTokenAsync to GetTokenForUserAsync
mjcheetham 89b099e
msauth: abstract token cache init helpers
mjcheetham 6a90c36
msauth: add support for service principal auth
mjcheetham bfa87db
msauth: add support for managed identity
mjcheetham f00c859
msauth: add MSAL app token cache support for CCAs
mjcheetham aafbda4
azrepos: support service principals and managed IDs
mjcheetham eff4ea6
azrepos: add tests of MID and SP get credential
mjcheetham File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
x509: add utils to find certs by thumbprint
- Loading branch information
commit 667698614ff6fb854bccb37e0c7fc904bd439d28
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| using System.Security.Cryptography.X509Certificates; | ||
|
|
||
| namespace GitCredentialManager; | ||
|
|
||
| public static class X509Utils | ||
| { | ||
| public static X509Certificate2 GetCertificateByThumbprint(string thumbprint) | ||
| { | ||
| foreach (var location in new[]{StoreLocation.CurrentUser, StoreLocation.LocalMachine}) | ||
| { | ||
| using var store = new X509Store(StoreName.My, location); | ||
| store.Open(OpenFlags.ReadOnly); | ||
|
|
||
| X509Certificate2Collection certs = store.Certificates.Find(X509FindType.FindByThumbprint, thumbprint, false); | ||
| if (certs.Count > 0) | ||
| { | ||
| return certs[0]; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_THUMBPRINTto specify a thumbprint.We could add other options like
..CertificatePath/.._CERT_PATHand..CertificateName/.._CERT_NAMEin the future (unless you think it's useful from the get go!) :)