jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.54k stars 4.02k forks source link

Insecure and misleading default realm config in jhipster-realm.json #20147

Closed yelhouti closed 1 year ago

yelhouti commented 1 year ago
Overview of the issue

The default jhipster-realm.json comes with some defaults that are outdated and make it hard to use for multi-tenancy. Moreover secrets are exported as *** which creates false sense of security as would expect a lot of deployments in production to keep them that way making them vulnerable for attacks.

Motivation for or Use Case

I would like to be able to easily rename the realm in jhipster-realm.json And be able to regenerate all secrets so that importing the realm in production be secure by default.

I guide with all this would be great in my opinion.

Reproduce the error

Import the default realm.

Check that it can be accessed using client-secret: ********** (10 asterisks)

Edit: There is also a client named "internal" specifically created by jhipster (which make it misleading) used only by micro-services (but always present in all configs), that in my opinion should be removed when not required. it's secret two internal should be regenerated and documented for change before production.

Suggest a Fix
  1. recreate the realm/roles... manually
  2. export the realm using, the script below and whenever and upgrade is made
    docker exec docker_keycloak_1 /opt/keycloak/bin/kc.sh export --realm jhipster --users=realm_file --file /tmp/jhipster-realm.json
    docker cp docker_keycloak_1:/tmp/jhipster-realm.json ./src/main/docker/realm-config/jhipster-realm.json
  3. Document what should change before moving to production.
  4. Optional: Document what should change for multi tenancy. (not all jhipster references should be changed, roles/claims for example should still be called jhipster) but realm name, redirectURIs... should change.
JHipster Version(s)

7.9.3

Tcharl commented 1 year ago

Agrees, but it should be mentioned in the project readme.

I'm also not a big fan of the 'internal' client, which gives the same identity to all MS, meaning that you can't restrict or add claims, roles, to specific ms. I would prefer to get on client per microservice

vishal423 commented 1 year ago

Moreover secrets are exported as *** which creates false sense of security as would expect a lot of deployments in production to keep them that way making them vulnerable for attacks.

Export as asterisk is the default behaviour and not in the scope of this project. I remember seeing few occurrences in the project generated realm file (mostly for the default Keycloak clients) and I think we can easily fix that with some random value during project generation.

I would like to be able to easily rename the realm in jhipster-realm.json

You can use offline tools to rename to your preferred value.

Do you want to propose a PR with fix for the asterisk issue?

yelhouti commented 1 year ago

Export as asterisk is the default behaviour and not in the scope of this project.

That is actually not true when export from the CLI and the the web interface.

You can use offline tools to rename to your preferred value.

Like which tools ? The issues with the current the jhipster-realm.json for me are:

  1. The ID of the realm is jhipster id should be an UUID to match the default behavior of keycloak.
  2. the realm file contains a lot of references to references to jhipster that mean different things. When creating a new Tenant Some should stay JHipster (like the client_scope) but others should change (like the realm_name) separating the two would make it easier for the next person.

Also you didn't address my main issues (which maybe I didn't fully address to avoid compromising already deployed apps that imported the file) but all apps, use the same KeyPair to sign tokens, meaning that I could generated a token in my instance and use it in yours if you imported the realm directly.

I already did the job and I am willing to share my work on what needs to be done, here is some code and some documentation I produced (some of it should be in the README in my opinion) some should be changed in the code.

public void createRealm(String id, String name, String displayName) throws JsonProcessingException {
        // replace all occurrences of demo by the new realm name (slug) to fix returns URIs...
        String newRealmConfig = demoRealmConfig.replaceAll("demo", name);
        // change uuids to avoid duplicate key constraints for clients, roles...
        // also replace all occurrences of the same value with the same new value, to keep relationships working
        for (String uuid : uuids) {
            newRealmConfig = newRealmConfig.replaceAll(uuid, UUID.randomUUID().toString());
        }
        RealmRepresentation realmRepresentation = mapper.readValue(newRealmConfig, RealmRepresentation.class);
        // cleanup key pairs and other secrets to be regenerated by keycloak
        if (realmRepresentation.getComponents().containsKey("org.keycloak.keys.KeyProvider")) {
            realmRepresentation.getComponents().remove("org.keycloak.keys.KeyProvider");
        } else {
            log.warn(
                "Keycloak seems to be storing secrets differently, ensure that tokens are generated using different keys for different realms"
            );
        }
        // cleanup client secrets to be regenerated by keycloak
        realmRepresentation.getClients().forEach(clientRepresentation -> clientRepresentation.setSecret(null));
        realmRepresentation.setId(id);
        realmRepresentation.setRealm(name);
        realmRepresentation.setDisplayName(displayName);
        // remove jhipster clients not used by our app
        List<String> clientsToRemove = Arrays.asList("jhipster-control-center", "internal");
        realmRepresentation.getClients().removeIf(c -> clientsToRemove.contains(c.getClientId()));
        clientsToRemove.forEach(client -> realmRepresentation.getRoles().getClient().remove(client));
        // remove default users
        realmRepresentation.getUsers().clear();
        // fix redirect uris
        ClientRepresentation webAppClient = realmRepresentation
            .getClients()
            .stream()
            .filter(c -> c.getClientId().equals(clientId))
            .findFirst()
            .get();
        String redirectUri = redirectUriTemplate.replace("{name}", name);
        webAppClient.setRedirectUris(Arrays.asList(redirectUri));
        // TODO removed for brievety setup email if configured in spring
        // fix csp
        realmRepresentation
            .getBrowserSecurityHeaders()
            .put(
                "contentSecurityPolicy",
                "frame-src 'self' " + redirectUri + "; frame-ancestors 'self' " + redirectUri + "; object-src 'none';"
            );
        keycloak.realms().create(realmRepresentation);
    }

But for the code to work, the realmName... should be changed to demo, or even better the app name.

demo-realm.json was generated from jhipster-realm.json by:

  1. generating a new id for the realm and replacing when it is used in all the file (containerId=jhipster)
  2. renaming "jhipster" with "demo" for the realm name and all redirect URIs, but not for roles, scopes...

I hope this helps the next person

vishal423 commented 1 year ago

Export as asterisk is the default behaviour and not in the scope of this project.

That is actually not true when export from the CLI and the the web interface.

I don't understand your intent. As I said before, if you see an asterisk in the checked-in realm configurations, then, we should correct that with some random UUID. How does export come into picture?

You can use offline tools to rename to your preferred value.

Like which tools ? The issues with the current the jhipster-realm.json for me are:

I was referring to Text/IDE tools and shouldn't take more than couple of mins. Generating an application named realm is a good to have.

Also you didn't address my main issues (which maybe I didn't fully address to avoid compromising already deployed apps that imported the file) but all apps, use the same KeyPair to sign tokens, meaning that I could generated a token in my instance and use it in yours if you imported the realm directly.

Please note the generated realm is not production hardened. You probably don't want to go live with admin/admin credentials :) I don't see any explicit settings in realm configurations about KeyPair. Since we reuses the same image as provided by Keycloak team, did you check with them and what's their recommendation?

vishal423 commented 1 year ago

btw, I don't like this programmatic approach as it would make the future maintenance a costly operation.