Added support for using password files instead of just passwords

[r-a303931]

This pull requests provides the ability to specify a file with the password needed for connecting to a data source. This is useful for environments where it is desirable to open source or publish the configuration, but not possible to do currently due to the inclusion of a database password.

It will try to use the password file option whenever possible, but will gracefully fall back to using the original password option in the case of the file not being possible to read from.

[lucko]

I'd rather see this implemented as a system property check instead of an additional file, similar to the server option:

https://github.com/lucko/LuckPerms/blob/979c80d9eeabd45b107401c119f77cf0e3a3081f/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java#L90-L92

Something like....

diff --git a/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java b/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
--- a/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
+++ b/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
@@ -541,11 +541,16 @@
         int connectionTimeout = c.getInteger("data.pool-settings.connection-timeout", 5000);
         Map<String, String> props = ImmutableMap.copyOf(c.getStringMap("data.pool-settings.properties", ImmutableMap.of()));
 
+        String password = c.getString("data.password", null);
+        if (password != null && password.equals("load-from-system-property")) {
+            password = System.getProperty("luckperms.database.password", null);
+        }
+
         return new StorageCredentials(
                 c.getString("data.address", null),
                 c.getString("data.database", null),
                 c.getString("data.username", null),
-                c.getString("data.password", null),
+                password,
                 maxPoolSize, minIdle, maxLifetime, keepAliveTime, connectionTimeout, props
         );
     }));

What do you think? :)

[r-a303931]

That could work. I am confused about how this could be done securely, however; my understanding is that to set a system property in such a way, there are two possible paths:

1. Use the command line interface and specify it as an argument (e.g. java -Dluckperms.database.password=toor server.jar)
   • This is problematic because the password is visible through inspection of the process (on Linux, cat /proc/$PID/cmdline; on Windows, with procexp64.exe from SysInternals)
2. Use java.util.Properties, load properties from a file, and then overwrite the system properties to make this property available globally
   • This leads back to the issue of needing to have a file that is available somewhere else

What about the ability to select between either a file or a system property? Something like:

diff --git a/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java b/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
index 1c4a2720..63694f11 100644
--- a/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
+++ b/common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java
@@ -541,11 +546,40 @@ public final class ConfigKeys {
         int connectionTimeout = c.getInteger("data.pool-settings.connection-timeout", 5000);
         Map<String, String> props = ImmutableMap.copyOf(c.getStringMap("data.pool-settings.properties", ImmutableMap.of()));

+        String password = c.getString("data.password", null);
+        if (password != null && password.startsWith("load-from-system-property")) {
+            String[] parts = password.split(":");
+            if (parts.length > 0) {
+                password = System.getProperty(parts[1].trim(), null);
+            } else {
+                password = System.getProperty("luckperms.database.password", null);
+            }
+        } else if (password != null && password.startsWith("load-from-local-file")) {
+            String[] parts = password.split(":");
+            if (parts.length > 0) {
+                try {
+                    Charset encoding = Charset.defaultCharset();
+                    byte[] passwordBytes = Files.readAllBytes(Paths.get(parts[1].trim()));
+                    // Database passwords probably do not intend to have newlines, but when editing files in most editors
+                    // (e.g., vi(m), VS Code, etc) a new line will automatically be added for POSIX compatibility.
+                    // This takes that into consideration
+                    password = new String(passwordBytes, encoding).trim();
+                } catch (IOException ioException) {
+                    LoggerFactory.getLogger(ConfigKeys.class).error("Load from local file failed.");
+                    ioException.printStackTrace();
+                    password = null;
+                }
+            } else {
+                LoggerFactory.getLogger(ConfigKeys.class).error("Load from local file specified for database password, but no file path provided.");
+                password = null;
+            }
+        }
+
         return new StorageCredentials(
                 c.getString("data.address", null),
                 c.getString("data.database", null),
                 c.getString("data.username", null),
-                c.getString("data.password", null),
+                password,
                 maxPoolSize, minIdle, maxLifetime, keepAliveTime, connectionTimeout, props
         );
     }));

[lucko]

I don't think security of the system properties approach is an issue - if another process is able to read that information then it can most likely read the config file from the filesystem too.

[lucko]

My aim here is to keep the diff relatively small considering the feature is particularly niche.

[lucko]

For the reasons stated above I'm going to close this PR for now - thanks for your efforts though :)

If you'd like to continue this further please let me know and I will happily re-open.