Redact operational info from server info by michalvavrik · Pull Request #51190 · keycloak/keycloak

michalvavrik · 2026-07-27T10:49:28Z

Closes Server info exposes database operational details to view-realm administrators #51139

Copilot

Pull request overview

Redacts sensitive database connection details from server info unless the caller can manage the master realm.

Changes:

Filters database URL and username from provider operational information.
Centralizes operational-information key constants.
Adds permission-focused integration coverage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`ServerInfoPermissionsTest.java`	Tests database detail visibility by permission level.
`ServerInfoAdminResource.java`	Applies permission-based redaction.
`ServerInfoAwareProviderFactory.java`	Defines shared database key constants.
`QuarkusJpaConnectionProviderFactory.java`	Uses shared constants.
`DefaultJpaConnectionProviderFactory.java`	Uses shared constants.

michalvavrik · 2026-07-27T10:57:06Z

Added couple of reviewers, anyone of them that happens to have a time will do. Thanks

michalvavrik · 2026-07-27T12:10:03Z

PR CI failures seem unrelated, I found already reported issue: #51191.

vmuzikar · 2026-07-27T16:07:27Z

 @Extension(name = KeycloakOpenAPI.Profiles.ADMIN , value = "")
 public class ServerInfoAdminResource {

+    private static final Set<String> SENSITIVE_OPERATIONAL_INFO_KEYS = Set.of(OPERATIONAL_INFO_DATABASE_URL, OPERATIONAL_INFO_DATABASE_USER);


I'd like to see this rather driven by the ServerInfoAwareProviderFactory than hardcoded here. This would make it more explicit, and also would work better for extensions.

We could add a new method to ServerInfoAwareProviderFactory, something like getOperationalInfoSensitiveKeys.

Or, maybe even introduce a breaking change and make it secure by default by having "allowlist" instead of "denylist": getOperationalInfoNonSensitiveKeys.

Or, maybe even introduce a breaking change and make it secure by default by having "allowlist" instead of "denylist": getOperationalInfoNonSensitiveKeys.

I inspected all keys I could find before opening this PR, this is edge case, I don't think it is necessary. I'll provide getOperationalInfoSensitiveKeys as suggested. If you prefer the breaking change, np, just let me know.

TBH I can't link suggested names to the description, e.g. getOperationalInfoNonSensitiveKeys doesn't seem like breaking change. I'll just do something and you can simply comment again when you see it.

Done. Check and just comment on the current code if you want it written differently. Thanks

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

michalvavrik · 2026-07-27T20:03:32Z

+    default Map<String, String> getNonSensitiveOperationalInfo() {
+        return getOperationalInfo();


I don't understand this comment, there is no provider that I can see which returns them. I already had this on REST-layer and it was requested that I don't do it there. If some provider needs adjustments, I am sure maintainers of that provider will act...

But nice to know that Copilot agrees with me in regards of the REST layer :-)

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

server-spi-private/src/main/java/org/keycloak/provider/ServerInfoAwareProviderFactory.java:45

This default is fail-open: factories that already implement this SPI (including custom JPA providers) but do not override the new method return their full map, so databaseUrl and databaseUser remain exposed to delegated admins. Filter the known sensitive keys in the default (or centrally in the REST resource) so the authorization boundary does not depend on every provider opting in.

    default Map<String, String> getNonSensitiveOperationalInfo() {
        return getOperationalInfo();
    }

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

shawkins · 2026-07-28T16:30:40Z

Just wanted to double check - is there a case to be made that non-master admins should not be able to see any operational info? None of this information is realm specific and leaks configuration details they may otherwise not know or need to know. Some of it should now also be available via metrics also.

And does this issue / PR need to be linked under #48961 ?

michalvavrik · 2026-07-28T17:06:26Z

Just wanted to double check - is there a case to be made that non-master admins should not be able to see any operational info?

Thanks @shawkins. Maybe this is more question for others. I thought about this before but how can I tell without intention being written down somewhere.

Existing code counts on non-admin (e.g. master) realm admins to see it. You can see cpu info / memory info / system info are not shown to them, just system info version. At the very least it would be a breaking change?

None of this information is realm specific and leaks configuration details they may otherwise not know or need to know. Some of it should now also be available via metrics also.

I agree, none of this is realm-specific.

And does this issue / PR need to be linked under #48961 ?

Whatever works for you. My intention was to fix the weakness (the linked issue), which is a subset of that enhancement request.

ahus1

Hello @michalvavrik - thank you for the PR.

While I see that the PR follows the original issue by the letter when it made showing this information conditionally (if I understand this correctly).

I suppose this code could be simplified by just removing those two items completely, all the time, for everyone. It would make this a lot simpler; and might even need fewer tests for that.

BTW, removing this information is not a breaking change. If you want to be extra safe, one could add it to the notable changes; but I'm ok to skip that part.

I'm adding this as a comment, and not "requesting changes", as this is the area of the cloud native team.

michalvavrik · 2026-07-29T07:30:40Z

I suppose this code could be simplified by just removing those two items completely, all the time, for everyone. It would make this a lot simpler; and might even need fewer tests for that.

I am fine with this proposal and if that is consensus among other reviewers as well, I am happy to follow it.

BTW, removing this information is not a breaking change

@ahus1 if non-master admin saw this information and now they don't, based on what information do we know no user needs it? Can you provide some information that I don't have or maybe clarify your thoughts? It feels natural to me to follow the principle of least privilege and try to avoid issuing master-admin when possible.

michalvavrik · 2026-07-29T15:03:22Z

It seems that I am alone thinking that it would be a breaking change, let me just move forward with @ahus1 and @shawkins proposal (hints), so that we can move forward with this one. Thanks for the feedback.

closes: keycloak#51139 Signed-off-by: Michal Vavřík <dev@michalvavrik.net>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/base/src/test/java/org/keycloak/tests/admin/ServerInfoPermissionsTest.java:94

The PR title and #51139 scope redaction to callers without server-level permissions, but these assertions require the fields to be absent for master-admin and a master-realm manage-realm administrator, while never exercising the reported delegated VIEW_REALM caller. This codifies the source-level removal for all administrators and unnecessarily drops existing operational details from authorized server admins; preserve them for authorized callers and test redaction with the delegated role instead.

        // databaseUrl and databaseUser are never exposed in server info
        assertDatabaseConnectionInfoNotExposed(clients.get("master-admin").serverInfo().getInfo());
        assertDatabaseConnectionInfoNotExposed(clients.get("master-admin-" + AdminRoles.MANAGE_REALM).serverInfo().getInfo());

michalvavrik · 2026-07-29T23:08:28Z

I pushed suggested changes. Frankly, I am not convinced, but it probably doesn't matter, in the end whatever addresses this weakness will work as long as reviewers believe it is correct. Little recap:

@michalvavrik (1st version - my preference) - drop db url and username in REST layer
@vmuzikar (2nd version - my interpretation of comments) - make ServerInfoAwareProviderFactory sensitivity aware and drop on the same fields on the factory level
@shawkins (didn't address) I think suggested to drop operation info for non-master admins
@ahus1 (current version) suggested to drop db url and username for everyone

if someone has another suggestion, please shout

Copilot AI review requested due to automatic review settings July 27, 2026 10:49

michalvavrik requested review from a team as code owners July 27, 2026 10:49

keycloak-github-bot Bot added the team/cloud-native label Jul 27, 2026

Copilot started reviewing on behalf of michalvavrik July 27, 2026 10:50 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

michalvavrik force-pushed the dev-auto/keycloak-51139 branch 2 times, most recently from 9174f08 to 50dc575 Compare July 27, 2026 10:53

michalvavrik requested review from Pepo48, edewit, shawkins and vmuzikar July 27, 2026 10:56

edewit previously approved these changes Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 27, 2026 16:00

vmuzikar force-pushed the dev-auto/keycloak-51139 branch from 50dc575 to f64954d Compare July 27, 2026 16:00

vmuzikar reviewed Jul 27, 2026

View reviewed changes

Copilot started reviewing on behalf of vmuzikar July 27, 2026 16:09 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 27, 2026 19:59

michalvavrik dismissed edewit’s stale review via 0328bfb July 27, 2026 19:59

michalvavrik force-pushed the dev-auto/keycloak-51139 branch from f64954d to 0328bfb Compare July 27, 2026 19:59

Copilot started reviewing on behalf of michalvavrik July 27, 2026 19:59 View session

michalvavrik force-pushed the dev-auto/keycloak-51139 branch from 0328bfb to a3df209 Compare July 27, 2026 20:01

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 27, 2026 20:01

Copilot started reviewing on behalf of michalvavrik July 27, 2026 20:02 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread tests/base/src/test/java/org/keycloak/tests/admin/ServerInfoPermissionsTest.java Outdated

michalvavrik requested a review from vmuzikar July 27, 2026 20:05

Copilot AI review requested due to automatic review settings July 27, 2026 20:10

michalvavrik force-pushed the dev-auto/keycloak-51139 branch from a3df209 to f0fb804 Compare July 27, 2026 20:10

Copilot started reviewing on behalf of michalvavrik July 27, 2026 20:10 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

ahus1 reviewed Jul 29, 2026

View reviewed changes

fix(services): remove db user and url from server info

33d9dd0

closes: keycloak#51139 Signed-off-by: Michal Vavřík <dev@michalvavrik.net>

Copilot AI review requested due to automatic review settings July 29, 2026 23:04

michalvavrik force-pushed the dev-auto/keycloak-51139 branch from f0fb804 to 33d9dd0 Compare July 29, 2026 23:04

Copilot started reviewing on behalf of michalvavrik July 29, 2026 23:04 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

michalvavrik changed the title ~~Redact databaseUrl and databaseUser from server info for users without master-admin permissions~~ Redact operational info from server info for users without master-admin permissions Jul 30, 2026

michalvavrik changed the title ~~Redact operational info from server info for users without master-admin permissions~~ Redact operational info from server info Jul 30, 2026

		default Map<String, String> getNonSensitiveOperationalInfo() {
		return getOperationalInfo();

Uh oh!

Conversation

michalvavrik commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

michalvavrik commented Jul 27, 2026

Uh oh!

michalvavrik commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

shawkins commented Jul 28, 2026

Uh oh!

michalvavrik commented Jul 28, 2026

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

michalvavrik commented Jul 29, 2026

Uh oh!

michalvavrik commented Jul 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

michalvavrik commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michalvavrik commented Jul 27, 2026 •

edited

Loading

michalvavrik commented Jul 29, 2026 •

edited

Loading