-
Notifications
You must be signed in to change notification settings - Fork 48
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
authn: Generalize support to ~any OIDC/OAuth2 IdP, not just AWS Cognito #731
Conversation
17097dd
to
68849a3
Compare
@victorlin Tagged you for review specifically since you volunteered to be involved in this work. |
@victorlin Some notes on testing. I've tested this against our testing env (e.g. using AWS Cognito) and also against an Azure AD I set up for the purpose (somewhat matching what CDC is using). For Azure AD, I used a JSON file like so: {
"OIDC_IDP_URL": "https://login.microsoftonline.com/0ce9e8dc-e009-4cb4-8512-7989bd6906a8/v2.0",
"OAUTH2_CLIENT_ID": "c3d6647f-dccc-4a85-a2bb-fb8fbc7524a9",
"OAUTH2_CLIENT_SECRET": "SECRET FROM AZURE AD APP REGISTRATION",
"OAUTH2_CLI_CLIENT_ID": "REQUIRED; NOT YET SORTED OUT; WILL BE PART OF FUTURE NEXTSTRAIN CLI CONFIGURATION TOO",
"OIDC_USERNAME_CLAIM": "preferred_username",
"OIDC_GROUPS_CLAIM": "roles",
"OIDC_IAT_BACKDATED_BY": 300,
"COGNITO_USER_POOL_ID": "REQUIRED; NOT YET SORTED OUT",
} and then set @victorlin You can login to the Azure portal as azure-admin@nextstrain.org (creds in 1Password). From there, you can generate another client secret to use (or I can share the one I generated) under "App registration", find the app/client, click the secrets link, add a new one. Once you have the nextstrain.org server running locally, you can try logging in with test-user@nextstraintesting.onmicrosoft.com (password in 1Password). I can also walk you thru all of this. I should verify our testing env still works with this PR, as there've been some changes since my last check. |
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.
- Tested using AWS Cognito with the testing user pool. I used the same commands as in Add RESTful API for managing Groups members #581 (review) with
s/COGNITO_CLI_CLIENT_ID/OAUTH2_CLI_CLIENT_ID/
. - Tested using AAD with the existing app registration (via a new client secret) and a user that I created for myself and added to
test-private/owners
. I tested everything mentioned here except token renewal.
"COGNITO_CLIENT_ID": "6qiojrhr8tibt0f6hphnm1osp1", | ||
"COGNITO_CLI_CLIENT_ID": "9opa27o74f4jsq8g4a34e1mqr", | ||
"COGNITO_USER_POOL_ID": "us-east-1_zqpCrjM7I" | ||
"OIDC_IDP_URL": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_zqpCrjM7I", |
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's interesting how in AWS, there is a clear distinction between IAM users/groups and Cognito users/groups, while in Azure with the current setup, it's all under AAD. I wonder if this has implications for organizations that manages Azure resources with AAD users.
Did you look into something like Azure AD B2C? I haven't used it personally, but it looks to be quite different from AAD, made clearer by name with the recent rebranding.
I'm asking these questions, but I don't think we should ponder too much because this seems to be working well for CDC's use case.
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.
I didn't look into Azure AD B2C, but looking now, yes, it's much more akin to Cognito. e.g. conceptually we could swap out our usage of Cognito for Azure AD B2C, but we wouldn't swap Cognito for Azure AD/Entra ID.
In CDC's case, they want to use their existing user directory in Azure AD and since they're also hosting the application themselves, it's simplest to authenticate against Azure AD directly. While they could set up an Azure AD B2C instance just for this app and then invite specific users from their Azure AD into their Azure AD B2C, it would just be more layers of indirection for not any functional gain. The gain would be more administrative/organizational, e.g. if for some reason they didn't want to directly authn against Azure AD or if they wanted a harder adminstrative split.
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.
Maybe to sum that up: There's a lot of ways to do user directories for authn purposes, but no one right way; it's highly dependent on the needs/context/where the users already are.
OIDC is OpenID Connect 1.0, which is an identity/authentication protocol layered on top of OAuth 2.0's authorization protocol. AWS Cognito implements OIDC/OAuth2 but our authn code hardcoded some assumptions about Cognito specifically. Undo that and parameterize and generalize the code to work (in theory) with other OIDC identity providers (IdPs). In practice, some additional changes may be necessary for specific other IdPs, but as-is I can get this generalized authn code to work against a test Azure AD IdP. Outside of authn, there are still some other bits of the codebase which require Cognito. Those will be addressed in subsequent work. This work is motivated by CDC AMD's efforts to host a copy of nextstrain.org internally in order to avail themselves of Groups internally. Related-to: <nextstrain/private#94>
The backdating must be a fixed duration, which is what I've observed with Azure AD (300s) and other IdPs. Backdating is sometimes applied to be more lenient with clients that have a slow clock (i.e. who otherwise might see a correct iat as in the future and reject the token). Without accounting for backdating, our staleBefore marker can cause a temporary deauthentication and repeated renewal attempts as token renewal requests "work" but produce a token with an iat that's still older than the staleBefore.
9644d8c
to
e84c672
Compare
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.
(non-blocking)
This file still has many references to Cognito in comments and variable names even though it is generalized for more than that. I'd think to replace most references to "Cognito" with "authentication service", and "Cognito group" with "authentication group" or something along those lines.
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.
Yeah... there's also other bits elsewhere (code, comments, documentation) which assume Cognito. For example, the documentation in docs/infrastructure.md
is about our specific setup, not the general infra required. My thinking was not to excise Cognito entirely, but to just make it possible to swap it out. I think it's reasonable for the codebase to generally assume Cognito for now. We can see how this goes over time and do the work to more fully excise it later.
OIDC is OpenID Connect 1.0, which is an identity/authentication protocol layered on top of OAuth 2.0's authorization protocol. AWS Cognito implements OIDC/OAuth2 but our authn code hardcoded some assumptions about Cognito specifically. Undo that and parameterize and generalize the code to work (in theory) with other OIDC identity providers (IdPs). In practice, some additional changes may be necessary for specific other IdPs, but as-is I can get this generalized authn code to work against a test Azure AD IdP.
Outside of authn, there are still some other bits of the codebase which require Cognito. Those will be addressed in subsequent work.
This work is motivated by CDC AMD's efforts to host a copy of nextstrain.org internally in order to avail themselves of Groups internally.
Related-to: https://github.com/nextstrain/private/issues/94
Checklist