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

Add support for authenticating with Entra (AAD) with a certificate instead of client secret #32

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

juliaducey
Copy link

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.

…Entra (AAD) with a certificate instead of client secret.

For more information, please see README updates.
@pond
Copy link
Member

pond commented Sep 5, 2024

@juliaducey

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",
Copy link

@Ayyoub-Ka Ayyoub-Ka Sep 6, 2024

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/...` |

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

@juliaducey
Copy link
Author

@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.

@pond pond merged commit 2b40a51 into RIPAGlobal:master Oct 16, 2024
4 checks passed
@pond
Copy link
Member

pond commented Oct 16, 2024

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.

@pond
Copy link
Member

pond commented Oct 16, 2024

So, sorry about that, hopefully V3 on release will be easy for you to use; either way, thank you for the great submission.

@juliaducey
Copy link
Author

@pond No worries, thanks! I'm fine to wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants