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.01% 18.02% +0.01%
- Complexity 16474 16621 +147
============================================
Files 5976 6024 +48
Lines 537858 542229 +4371
Branches 66049 66458 +409
============================================
+ Hits 96882 97742 +860
- Misses 430052 433464 +3412
- Partials 10924 11023 +99
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.
| protected String idToken = null; | ||
|
|
There was a problem hiding this comment.
idToken is stored as an instance field. Since provider beans are typically singletons, this shared mutable state can leak between concurrent login attempts. Please avoid storing per-request tokens on the provider instance; keep them local to the method or in a request/session-scoped context.
| 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. |
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