libre.sh/keycloak-scim
3
0
Fork 0
Code Issues 15 Pull requests Releases 1 Packages Activity Actions

fix: adding weak validation of spi options (#31737)

Browse source 
closes: #27298

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
This commit is contained in:
Steven Hawkins 2024-08-08 08:21:24 -04:00 committed by GitHub
parent a7c71dc0bc
commit 10fae5de7a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 54 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

40
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java
View file

@ -375,6 +375,8 @@ public final class Picocli {
disabledMappers.addAll(PropertyMappers.getDisabledRuntimeMappers().values());
}
checkSpiOptions(options, ignoredBuildTime, ignoredRunTime);
for (OptionCategory category : abstractCommand.getOptionCategories()) {
List<PropertyMapper<?>> mappers = new ArrayList<>(disabledMappers);
Optional.ofNullable(PropertyMappers.getRuntimeMappers().get(category)).ifPresent(mappers::addAll);
@ -427,10 +429,10 @@ public final class Picocli {
Logger logger = Logger.getLogger(Picocli.class); // logger can't be instantiated in a class field
if (!ignoredBuildTime.isEmpty()) {
logger.warn(format("The following build time non-cli options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n",
logger.warn(format("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n",
String.join(", ", ignoredBuildTime)));
} else if (!ignoredRunTime.isEmpty()) {
logger.warn(format("The following run time non-cli options were found, but will be ignored during build time: %s\n",
logger.warn(format("The following run time options were found, but will be ignored during build time: %s\n",
String.join(", ", ignoredRunTime)));
}
@ -449,6 +451,36 @@ public final class Picocli {
}
}
private static void checkSpiOptions(IncludeOptions options, final List<String> ignoredBuildTime,
final List<String> ignoredRunTime) {
String kcSpiPrefix = NS_KEYCLOAK_PREFIX + "spi";
for (String key : Configuration.getConfig().getPropertyNames()) {
if (!key.startsWith(kcSpiPrefix)) {
continue;
}
boolean buildTimeOption = key.endsWith("-provider") || key.endsWith("-provider-default") || key.endsWith("-enabled");
ConfigValue configValue = Configuration.getConfigValue(key);
String configValueStr = configValue.getValue();
// don't consider missing or anything below standard env properties
if (configValueStr == null || configValue.getConfigSourceOrdinal() < 300) {
continue;
}
if (!options.includeBuildTime) {
if (buildTimeOption) {
String currentValue = getRawPersistedProperty(key).orElse(null);
if (!configValueStr.equals(currentValue)) {
ignoredBuildTime.add(key);
}
}
} else if (!buildTimeOption) {
ignoredRunTime.add(key);
}
}
}
private static void handleDeprecated(Set<String> deprecatedInUse, PropertyMapper<?> mapper, String configValue,
DeprecatedMetadata metadata) {
Set<String> deprecatedValuesInUse = new HashSet<>();
@ -855,11 +887,11 @@ public final class Picocli {
// change this once we are able to obtain properties from providers
List<String> args = new ArrayList<>();
ConfigArgsConfigSource.parseConfigArgs(List.of(rawArgs), (arg, value) -> {
if (!arg.startsWith("--spi") && !arg.startsWith("-D")) {
if (!arg.startsWith(ConfigArgsConfigSource.SPI_OPTION_PREFIX) && !arg.startsWith("-D")) {
args.add(arg + "=" + value);
}
}, arg -> {
if (arg.startsWith("--spi")) {
if (arg.startsWith(ConfigArgsConfigSource.SPI_OPTION_PREFIX)) {
throw new PropertyException(format("spi argument %s requires a value.", arg));
}
if (!arg.startsWith("-D")) {

4
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java
View file

@ -47,6 +47,8 @@ import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers;
*/
public class ConfigArgsConfigSource extends PropertiesConfigSource {
public static final String SPI_OPTION_PREFIX = "--spi";
public static final Set<String> SHORT_OPTIONS_ACCEPTING_VALUE = Set.of(Main.PROFILE_SHORT_NAME, Main.CONFIG_FILE_SHORT_NAME);
public static final String CLI_ARGS = "kc.config.args";
@ -156,7 +158,7 @@ public class ConfigArgsConfigSource extends PropertiesConfigSource {
// the weaknesses here:
// - needs to know all of the short name options that accept a value
// - does not know all of the picocli parsing rules. picocli will accept -cffile, and short option grouping - that's not accounted for
if (mapper != null || SHORT_OPTIONS_ACCEPTING_VALUE.contains(key) || arg.startsWith("--spi")) {
if (mapper != null || SHORT_OPTIONS_ACCEPTING_VALUE.contains(key) || arg.startsWith(SPI_OPTION_PREFIX)) {
i++; // consume next as a value to the key
value = args.get(i);
} else {

16
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/StartCommandDistTest.java vendored
View file

@ -52,6 +52,20 @@ public class StartCommandDistTest {
() -> "The Output:\n" + result.getErrorOutput() + "doesn't contains the expected string.");
}
@Test
@Launch({ "build", "--spi-events-listener-jboss-logging-success-level=debug" })
void warnSpiRuntimeAtBuildtime(LaunchResult result) {
assertTrue(result.getOutput().contains("The following run time options were found, but will be ignored during build time: kc.spi-events-listener-jboss-logging-success-level"),
() -> "The Output:\n" + result.getOutput() + "doesn't contains the expected string.");
}
@Test
@Launch({ "start", "--optimized", "--http-enabled=true", "--hostname-strict=false", "--spi-events-listener-jboss-logging-enabled=false" })
void warnSpiBuildtimeAtRuntime(LaunchResult result) {
assertTrue(result.getOutput().contains("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.spi-events-listener-jboss-logging-enabled"),
() -> "The Output:\n" + result.getOutput() + "doesn't contains the expected string.");
}
@Test
@Launch({ "--profile=dev", "start" })
void failUsingDevProfile(LaunchResult result) {
@ -175,7 +189,7 @@ public class StartCommandDistTest {
cliResult.assertBuild();
dist.setEnvVar("KC_DB", "postgres");
cliResult = dist.run("start", "--optimized", "--hostname=localhost", "--http-enabled=true");
cliResult.assertMessage("The following build time non-cli options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.db");
cliResult.assertMessage("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: kc.db");
}
@Test