-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add support for authenticating with Entra (AAD) with a certificate instead of client secret #32
Conversation
…Entra (AAD) with a certificate instead of client secret. For more information, please see README updates.
Sorry for the delay here. Just as I was getting around to this some very valid points came up in #33 and I was thinking that this might all get folded into a V3. OTOH, you might not want to take that leap, and might prefer this to go into V2 ASAP. Any preference? On the PR itself, a quick glance through looks excellent - I very much appreciate the time and attention to detail with documentation updates, tests and even a version bump! Thank you so much for the submission. |
|
||
def client_assertion_claims(tenant_id, client_id) | ||
{ | ||
'aud' => "https://login.microsoftonline.com/#{tenant_id}/oauth2/v2.0/token", |
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.
It won't work in case of multitenant Microsoft Entra ID app .
If tenant_id is not provided aud clam can be set as :
'aud' => 'https://login.microsoftonline.com/common/oauth2/v2.0/token'
| `client_secret` | **Mandatory for client secret flow.** Client secret for the 'application' (integration) configured on the Azure side. Found via the Azure UI. Don't give this if using client assertion flow. | | ||
| `certificate_path` | **Mandatory for client assertion flow.** Don't give this if using a client secret instead of client assertion. This should be the filepath to a PKCS#12 file. | | ||
| `base_azure_url` | Location of Azure login page, for specialised requirements; default is `OmniAuth::Strategies::AzureActivedirectoryV2::BASE_AZURE_URL` (at the time of writing, this is `https://login.microsoftonline.com`). | | ||
| `tenant_id` | **Mandatory for client assertion flow.** _Azure_ tenant ID for multi-tenanted use. Default is `common`. Forms part of the Azure OAuth URL - `{base}/{tenant_id}/oauth2/v2.0/...` | |
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.
Not necessary , in case of multi-tenant Entra ID app , common can be used in stead of tenant_id as an aud clam in JWT
@pond - Thanks for the reply! I forgot to check back earlier. I'm fine either way, but I currently have this code awkwardly monkeypatched in two different services in my prod environment, so I would probably prefer to have it merged in v2 if that's fine with you. I see @Ayyoub-Ka 's comments about this not working for multi-tenant setups, which is fair, but I probably won't want to add support for that myself because my environment is single-tenant and I don't have a way to test the changes. I can update the documentation if you like, though. |
OK, merged, but due to logistics issues on our side, this is going to have to be in a V3 with rename for Entra. Totally understand if you prefer to stay on your monkey patch. Hoped to do it in V2, but there's just no available time to maintain an old V2 branch at this point. |
So, sorry about that, hopefully V3 on release will be easy for you to use; either way, thank you for the great submission. |
@pond No worries, thanks! I'm fine to wait. |
This PR adds support for the client assertion Entra Oauth2 flow, which uses a certificate instead of client secret for authenticating the app with Entra. You can see this documentation for more details about how this flow works:
https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth2-auth-code-flow#request-an-access-token-with-a-certificate-credential
I have these changes working in my production environment and thought it would be nice to make them available to others, so I'm opening a PR to merge them upstream.