fix: removing EnvConfigSource keys that are not intended - #51291
fix: removing EnvConfigSource keys that are not intended#51291shawkins wants to merge 6 commits into
Conversation
closes: keycloak#51113 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
and adding a upgrade note Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steven Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
Pull request overview
Fixes #51113 by separating Keycloak environment variables from Quarkus environment handling and masking SPI values in show-config.
Changes:
- Filters Keycloak-specific environment variables from Quarkus configuration.
- Masks all SPI option values in
show-config. - Adds regression tests and upgrade documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
KcEnvConfigSource.java |
Splits Keycloak and Quarkus environment sources. |
Configuration.java |
Manually registers default configuration sources. |
ShowConfig.java |
Masks SPI configuration values. |
PicocliTest.java |
Tests SPI secret masking. |
changes-26_8_0.adoc |
Documents the masking change. |
| .withSources(new DotEnvConfigSourceProvider() | ||
| .getConfigSources(Thread.currentThread().getContextClassLoader())) |
There was a problem hiding this comment.
This is not an issue for two reasons. 1. all spi options are now masked. 2. .env files are not a documented configsource, it is not expected that they will be used.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java:279
- The negative-only assertion can also pass if this valid SPI property is accidentally dropped from
show-config, so it does not verify the documented “mask” behavior. Assert the property is still emitted with the mask as well as checking that the secret is absent.
NonRunningPicocli nonRunningPicocli = pseudoLaunch("show-config");
assertThat(nonRunningPicocli.getOutString(), not(containsString("custom-secret")));
and adding a upgrade note Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-26_8_0.adoc
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/documentation/upgrading/topics/changes/changes-26_8_0.adoc:57
- This repeats the release-note section already added at lines 41–43, causing the same change to appear twice. Remove this second section.
=== `show-conifg` will mask all SPI option values
The `show-config` command will mask all SPI option values. The command is not currently aware of provider factory configuration property metadata, so it cannot determine if a property is marked as `isSecret`.
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
Should be ready for a review. Given that we are now masking all SPI options the logic to change how the env properties are discovered could be reverted - if it seems too risky to take responsibility for this in our code. |
@shawkins, I'm not sure the env filtering can be safely reverted. The SPI masking catches So I guess it's not redundant, but rather complementary. |
That is correct, the spi masking would need to check for both prefixes. |
| if (property.startsWith(MicroProfileConfigProvider.SPI_PREFIX)) { | ||
| // could be marked as ProviderConfigProperty.isSecret, so the simplest option for now | ||
| // is to just mask all direct usage of spi options. | ||
| // the most straight-forward alternative is to move show-config to be run after the quarkus start |
There was a problem hiding this comment.
@shawkins, correct. To be specific, if we decide to revert the changes in KcEnvConfigSource.java and Configuration.java, I would adjust:
| // the most straight-forward alternative is to move show-config to be run after the quarkus start | |
| if (property.startsWith(MicroProfileConfigProvider.SPI_PREFIX) | |
| || property.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX + "spi.")) { |
During the review yesterday, I ran the added tests with this change on top and everything seemed to work.
There was a problem hiding this comment.
Correct, I'm not saying that we have to take this approach, just that it's an option if anyone feels the changes to the Configuration creation seem too risky.
closes: #51113