Add keycloak OAuth provider#13033
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13033 +/- ##
============================================
+ Coverage 18.03% 18.05% +0.01%
- Complexity 16648 16667 +19
============================================
Files 6035 6036 +1
Lines 542487 542630 +143
Branches 66477 66489 +12
============================================
+ Hits 97854 97959 +105
- Misses 433612 433642 +30
- Partials 11021 11029 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Keycloak as an additional OAuth2/OIDC provider by extending the OAuth provider model/API/schema and exposing new provider URL fields in the UI, along with a new Keycloak authenticator implementation and tests.
Changes:
- Add
authorizeurlandtokenurlfields end-to-end (DB schema, VO/response objects, API commands, and UI config/i18n). - Register a new
KeycloakOAuth2Providerand include it in the default provider order. - Update GitHub token exchange to use a configurable token URL instead of a hardcoded endpoint.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/auth/Login.vue | Adds Keycloak social login button and consumes authorizeurl from listOauthProvider. |
| ui/src/config/section/config.js | Extends OAuth provider UI config to edit/display authorizeurl/tokenurl and adds keycloak to provider options. |
| ui/public/locales/en.json | Adds UI labels for Authorize URL and Token URL. |
| ui/public/assets/keycloak.svg | Adds Keycloak icon asset for the login UI. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java | Introduces unit tests for the new Keycloak provider. |
| plugins/user-authenticators/oauth2/src/main/resources/META-INF/cloudstack/oauth2/spring-oauth2-context.xml | Registers Keycloak provider bean and adds it to default ordering. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java | Adds authorizeUrl and tokenUrl columns/fields to the OAuth provider entity. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java | Implements Keycloak OAuth2/OIDC flow using token endpoint + ID token parsing. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java | Minor refactor/cleanup and fixes a format string. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java | Switches GitHub token endpoint to DB-configured tokenUrl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java | Extends API response to include authorizeurl and tokenurl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java | Adds new parameters and returns them in response. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java | Adds new parameters and validates Keycloak requires the URLs. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java | Includes new URL fields in list responses. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java | Plumbs new URL fields through register/update persistence. |
| plugins/user-authenticators/oauth2/pom.xml | Adds CXF JOSE dependency used for JWT parsing. |
| engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql | Adds authorize_url and token_url columns to cloud.oauth_provider. |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds API constants for authorizeurl and tokenurl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void validateIdToken(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
| } | ||
|
|
||
| private String obtainEmail(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
|
|
||
| return (String) claims.getClaim("email"); |
There was a problem hiding this comment.
The ID token is parsed with JwsJwtCompactConsumer but its signature is never verified (and the tests even use {"alg":"none"} tokens). This allows token forgery. Please perform proper OIDC validation: verify JWS signature against the provider's JWKS, and validate standard claims (exp/iat, iss, aud, nonce) before trusting the email claim.
There was a problem hiding this comment.
I agree with this comment. The fact is that the token is never checked in Google / Github. Should we introduce it ?
There was a problem hiding this comment.
@tazouxme , it makes sense but some conflicting considerations:
- How much effort will this be?
- How likely is an exploit and what is the potential damage?
My gut says this is a security feature we might want and not a direct vulnerability (as people can decide not to use a certain provider). Note that my gut has been known to be wrong.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17513 |
|
thanks for the contribution @tazouxme , looks useful. Please have a look at co-pilot’s comments and the pre-commit and license check failures. |
|
Looks very useful feature. Thanks for Contributing @tazouxme Will test it out, I had previously tested saml authentication with keycloak https://kiranchavala.in/blog/cloudstack-integration-with-keycloak/ |
|
Thanks for your positive feedback. There's only a valid point from Copilot regarding the Token validation. Signature should be validated but currently not done for Google / Github. Should this be introduced ? |
If you feel like implementing it, your efforts are appreciated. I think it can be a separate PR. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class KeycloakOAuth2Provider extends AdapterBase implements UserOAuth2Authenticator { | ||
|
|
||
| protected String idToken = null; | ||
|
|
||
| @Inject | ||
| OauthProviderDao oauthProviderDao; |
There was a problem hiding this comment.
idToken is stored as an instance field on the provider bean. Since these authenticators are created as singleton Spring beans by default, this cached token can be shared across concurrent login requests and leak/mix authentication state between users. Avoid storing per-request auth state in fields; keep the token in local variables and pass it through methods instead.
| JsonObject json = JsonParser.parseString(body).getAsJsonObject(); | ||
| String idToken = json.get("id_token").getAsString(); | ||
| validateIdToken(idToken, provider); | ||
|
|
There was a problem hiding this comment.
The token endpoint response is assumed to always contain an id_token. If Keycloak is configured to not return an ID token (or an error response body is returned with HTTP 200), json.get("id_token") will be null and this will throw a NullPointerException. Please validate the JSON response (presence/type of id_token) and raise a clear authentication error when it’s missing.
| String idToken = json.get("id_token").getAsString(); | ||
| validateIdToken(idToken, provider); | ||
|
|
||
| this.idToken = idToken; |
There was a problem hiding this comment.
The local variable String idToken = ... shadows the idToken field, which is easy to misread and can lead to subtle bugs when modifying this code. Use a different local name (e.g., fetchedIdToken) or eliminate the field entirely (see concurrency concern).
| String idToken = json.get("id_token").getAsString(); | |
| validateIdToken(idToken, provider); | |
| this.idToken = idToken; | |
| String fetchedIdToken = json.get("id_token").getAsString(); | |
| validateIdToken(fetchedIdToken, provider); | |
| this.idToken = fetchedIdToken; |
| private void validateIdToken(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
| } | ||
|
|
||
| private String obtainEmail(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
|
|
||
| return (String) claims.getClaim("email"); |
There was a problem hiding this comment.
validateIdToken/obtainEmail parse the JWT but never verify its JWS signature or validate standard OIDC claims (e.g., exp, iss, nonce). This means a forged/unsigned token could be accepted as long as it contains the expected aud/email fields. Please implement proper ID token verification (signature validation via JWKS and required claim checks) before trusting the email claim.
|
|
||
| @Override | ||
| public String getUserEmailAddress() throws CloudRuntimeException { | ||
| return null; |
There was a problem hiding this comment.
getUserEmailAddress() is currently unimplemented and always returns null. Since it is part of the UserOAuth2Authenticator contract, either implement it (e.g., derive the email from the cached token) or throw a clear exception indicating it’s unsupported to avoid confusing null propagation.
| return null; | |
| if (StringUtils.isBlank(idToken)) { | |
| throw new CloudRuntimeException("User email address is unavailable because no ID token is cached"); | |
| } | |
| OauthProviderVO provider = oauthProviderDao.findByProvider(getName()); | |
| if (provider == null) { | |
| throw new CloudAuthenticationException("Keycloak provider is not registered, so user email address cannot be retrieved"); | |
| } | |
| return obtainEmail(idToken, provider); |
| class="auth-btn github-auth" | ||
| style="height: 38px; width: 185px; padding: 0; margin-bottom: 5px;" > | ||
| <img src="/assets/github.svg" style="width: 32px; padding: 5px" /> | ||
| <img src="/assets/github.svg" alt="Google" style="width: 32px; padding: 5px" /> |
There was a problem hiding this comment.
The GitHub button icon has an incorrect alt text (currently "Google"), which harms accessibility/screen readers. Update the alt text to describe the GitHub icon.
| <img src="/assets/github.svg" alt="Google" style="width: 32px; padding: 5px" /> | |
| <img src="/assets/github.svg" alt="GitHub" style="width: 32px; padding: 5px" /> |
| <img src="/assets/google.svg" alt="Github" style="width: 32px; padding: 5px" /> | ||
| <a-typography-text>Sign in with Google</a-typography-text> |
There was a problem hiding this comment.
The Google button icon has an incorrect alt text (currently "Github"), which harms accessibility/screen readers. Update the alt text to describe the Google icon.
Description
Add Keycloak as a new OAuth provider. This PR adds two new fields in the UI and in the DB:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Manual testing using local Keycloak instance with basic Realm