Skip to content

Keystore improvements. #17404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mashhurs wants to merge 1 commit into from
Closed

Keystore improvements. #17404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions logstash-core/src/main/java/org/logstash/execution/AbstractPipelineExt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ private AbstractPipelineExt initialize(final ThreadContext context,
}
}
boolean supportEscapes = getSetting(context, "config.support_escapes").isTrue();
try (ConfigVariableExpander cve = new ConfigVariableExpander(getSecretStore(context), EnvironmentVariableProvider.defaultProvider())) {
try (ConfigVariableExpander cve = new ConfigVariableExpander(getSecretStore(context),
EnvironmentVariableProvider.defaultProvider())) {
lir = ConfigCompiler.configToPipelineIR(configParts, supportEscapes, cve);
} catch (InvalidIRException iirex) {
throw new IllegalArgumentException(iirex);
Expand Down Expand Up @@ -842,15 +843,28 @@ protected final boolean hasSetting(final ThreadContext context, final String nam
}

protected SecretStore getSecretStore(final ThreadContext context) {
String keystoreFile = hasSetting(context, "keystore.file")
Copy link
Member

@yaauie yaauie Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be an illegal state for hasSetting(context, "keystore.file") to return false -- it would mean that the setting hasn't been registered (not simply that a value was not provided by the user).

? getSetting(context, "keystore.file").asJavaString()
Copy link
Member

@yaauie yaauie Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we send IRubyObject#asJavaString() to nil, we get a TypeError: nil is not a string. This effectively already safeguards against a nil/null value (although it could be done in a more clear way).

: null;
String keystoreClassname = hasSetting(context, "keystore.classname")
? getSetting(context, "keystore.classname").asJavaString()
: null;
return (keystoreFile != null && keystoreClassname != null)
Copy link
Member

@yaauie yaauie Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither keystoreFile nor keystoreClassname can actually be null here.

? SecretStoreExt.getIfExists(keystoreFile, keystoreClassname)
: null;
final String keystoreFile = safelyGetSettingValueAsString(context, "keystore.file");
final String keystoreClassname = safelyGetSettingValueAsString(context, "keystore.classname");
if (keystoreFile == null && keystoreClassname == null) {
// explicitly set keystore and classname null
return null;
}

if (keystoreFile == null | keystoreClassname == null) {
throw new IllegalStateException("Setting `keystore.file` requires `keystore.classname`, or vice versa");
}
Comment on lines +848 to +855
Copy link
Member

@yaauie yaauie Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the mix of bitwise logic and conditional logic works, but is surprising to me.

I would also rather have an else-if chain to provide specific error messages instead of grouping them up.

Suggested change
if (keystoreFile == null && keystoreClassname == null) {
// explicitly set keystore and classname null
return null;
}
if (keystoreFile == null | keystoreClassname == null) {
throw new IllegalStateException("Setting `keystore.file` requires `keystore.classname`, or vice versa");
}
if (keystoreFile == null && keystoreClassname == null) {
// explicitly set keystore and classname null
return null;
} else if (keystoreFile == null) {
throw new IllegalStateException("Setting `keystore.file` is required when `keystore.classname` is provided");
} else if (keystoreClassname == null) {
throw new IllegalStateException("Setting `keystore.classname` is required when `keystore.file` is provided");
}

But separately and possibly more importantly, both keystore.file and keystore.classname are registered settings, so #hasSetting() will always return true for either of them, and the existing code will have already thrown a TypeError if either of them are null.

return SecretStoreExt.getIfExists(keystoreFile, keystoreClassname);
}

private String safelyGetSettingValueAsString(final ThreadContext context, final String settingName) {
final boolean hasKeystoreFileSetting = hasSetting(context, settingName);
if (hasKeystoreFileSetting) {
final IRubyObject keystoreFileSettingValue = getSetting(context, settingName);
if (!keystoreFileSettingValue.isNil()) {
return keystoreFileSettingValue.asJavaString();
}
}
return null;
}

private AbstractNamespacedMetricExt getDlqMetric(final ThreadContext context) {
Expand Down
16 changes: 10 additions & 6 deletions logstash-core/src/main/java/org/logstash/secret/store/SecretStoreExt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,20 @@ public class SecretStoreExt {

private static final SecretStoreFactory SECRET_STORE_FACTORY = SecretStoreFactory.fromEnvironment();

public static SecureConfig getConfig(String keystoreFile, String keystoreClassname) {
public static SecureConfig getConfig(final String keystoreFile, final String keystoreClassname) {
return getSecureConfig(RubyUtil.RUBY.getENV(), keystoreFile, keystoreClassname);
}

private static SecureConfig getSecureConfig(RubyHash env, String file, String classname) {
private static SecureConfig getSecureConfig(final RubyHash env, final String file, final String classname) {
String keystorePass = (String) env.get("LOGSTASH_KEYSTORE_PASS");
return getSecureConfig(file, keystorePass, classname);
}

private static SecureConfig getSecureConfig(String keystoreFile, String keystorePass, String keystoreClassname) {
private static SecureConfig getSecureConfig(final String keystoreFile, final String keystorePass, final String keystoreClassname) {
if (keystoreFile == null || keystoreClassname == null) {
throw new IllegalArgumentException("`keystore.file` and `keystore.classname` cannot be null");
}
Comment on lines +44 to +46
Copy link
Member

@yaauie yaauie Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: while we know that the current path to these variables are the settings keystore.file and keystore.classname, I'm wary of making the exception reference those settings here -- we don't know the future uses of the secretstore.

Suggested change
if (keystoreFile == null || keystoreClassname == null) {
throw new IllegalArgumentException("`keystore.file` and `keystore.classname` cannot be null");
}
Objects.requireNonNull(keystoreFile, "keystoreFile");
Objects.requireNonNull(keystoreClassname, "keystoreClassname");


SecureConfig sc = new SecureConfig();
sc.add("keystore.file", keystoreFile.toCharArray());
if (keystorePass != null) {
Expand All @@ -50,18 +54,18 @@ private static SecureConfig getSecureConfig(String keystoreFile, String keystore
return sc;
}

public static boolean exists(String keystoreFile, String keystoreClassname) {
public static boolean exists(final String keystoreFile, final String keystoreClassname) {
return SECRET_STORE_FACTORY.exists(getConfig(keystoreFile, keystoreClassname));
}

public static SecretStore getIfExists(String keystoreFile, String keystoreClassname) {
public static SecretStore getIfExists(final String keystoreFile, final String keystoreClassname) {
SecureConfig sc = getConfig(keystoreFile, keystoreClassname);
return SECRET_STORE_FACTORY.exists(sc)
? SECRET_STORE_FACTORY.load(sc)
: null;
}

public static SecretIdentifier getStoreId(String id) {
public static SecretIdentifier getStoreId(final String id) {
return new SecretIdentifier(id);
}
}