NUWCDIVNPT / stig-manager

An API and client for managing STIG assessments
Other
112 stars 29 forks source link

Adjust client auth implementation to accommodate Azure AD-flavored OIDC #666

Closed cd-rite closed 2 years ago

cd-rite commented 2 years ago

Adjustments to work with Azure AD OIDC:

ingallsk-saic commented 2 years ago

We're running into an issue integrating STIG Manager with AD FS due to the opaque refresh tokens. Since it's opaque, the client fails to parse the token and ends up immediately requesting a new auth token. The updated auth token lacks the scopes field (all other fields are present), which causes the client to fail to load.

To work around the issue, we tried setting STIGMAN_CLIENT_REFRESH_DISABLED=true based on the config below, but that environment variable doesn't seem be to referenced in the OIDC refresh code. https://github.com/NUWCDIVNPT/stig-manager/blob/34b2993584e6ed1b12699d8f4139bf9b9bf395d4/api/source/utils/config.js#L28

For testing, we've manually disabled use of the refresh token by skipping it's check after authentication succeeds, which other than the refresh capabilities, seems to function properly.

Is the missing scope field in the updated auth token related to the opaque refresh token issue?

csmig commented 2 years ago

It probably is related, perhaps the token request that follows the refresh token parse failure is missing the scope query param.

In any case, that may be academic because, as you've noticed, we are currently not handling an opaque or missing refresh token properly. Last year we exposed the envvar STIGMAN_CLIENT_REFRESH_DISABLED as the way we would handle these scenarios. But as you've also noticed, it seems the implementation is incomplete. This issue is high priority for us and your report is much appreciated.

For the record, is this the code block you've skipped in the client that allows it to load and function properly? https://github.com/NUWCDIVNPT/stig-manager/blob/34b2993584e6ed1b12699d8f4139bf9b9bf395d4/client/src/js/init.js#L18-L28

ingallsk-saic commented 2 years ago

Yeah, that's correct. We just skipped that if statement, so the function is effectively empty.

csmig commented 2 years ago

@s-ingk If you are able, please fetch branch refresh-token-handling in this repo and run from that code. I've reworked the refresh token handling and tested against an Okta OIDC Provider that generates an opaque refresh token. I don't have access to an ADFS, so can't directly test against that.

In this implementation, when the refresh token cannot be parsed as a JWT a refresh will be attempted 60 seconds before the access token expires. If the refresh token can be parsed, a refresh will be attempted 60 seconds before the refresh token expires. In either case, you'll want to set STIGMAN_CLIENT_REFRESH_DISABLED to false (or just unset the envvar) for the refresh to actually be scheduled.

I have not addressed the other Azure AD specific issue (prepending an Application ID URI to each scope) so not ready to open a PR just yet. But I'm curious if ADFS will refresh properly with just this fix. Thanks for the assistance.

ingallsk-saic commented 2 years ago

Sorry for the delay, we tested both the refresh-token-handling branch and v1.2.10. Both work until the access token expires, but the access token returned using the refresh token doesn't work.

Due to the complexity of the ADFS setup, and other issues we've encountered with ADFS itself, we're no longer considering using ADFS and are instead moving to Keycloak.

csmig commented 2 years ago

Thanks for the report, it is interesting.

I see that RFC 6749 defines an optional scope parameter during refresh. Perhaps ADFS is incorrectly implementing this optional field. The RFC says if the scope parameter is missing, the value "is treated as equal to the scope originally granted by the resource owner". The OIDC Providers I've tested (Keycloak, Azure AD and Okta) will refresh their access tokens with the same scopes as the original, and I do not currently send them this optional scope parameter.

Keycloak is a fine choice for OIDC support, by the way...