Cleans up sysouts and replaces with logger statements where necessary

Signed-off-by: Darshit Chanpura <[email protected]>

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

@@ -98,8 +98,7 @@ public Void run() {
                         }
                     } catch (Throwable e) {
                         log.error("Unable to enable krb_debug due to ", e);
-                        System.err.println("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
-                        System.out.println("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
+                        log.debug("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
                     }
 
                     System.setProperty(KrbConstants.USE_SUBJECT_CREDS_ONLY_PROP, "false");

src/main/java/org/opensearch/security/auditlog/sink/DebugSink.java

@@ -11,11 +11,15 @@
 
 package org.opensearch.security.auditlog.sink;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.security.auditlog.impl.AuditMessage;
 
 public final class DebugSink extends AuditLogSink {
 
+    final Logger log = LogManager.getLogger(DebugSink.class);
+
     public DebugSink(String name, Settings settings, AuditLogSink fallbackSink) {
         super(name, settings, null, fallbackSink);
     }
@@ -27,7 +31,7 @@ public boolean isHandlingBackpressure() {
 
     @Override
     public boolean doStore(final AuditMessage msg) {
-        System.out.println("AUDIT_LOG: " + msg.toPrettyString());
+        log.info("AUDIT_LOG: " + msg.toPrettyString());
         return true;
     }
 

src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java

@@ -244,8 +244,6 @@ public void onChange(Map<CType, SecurityDynamicConfiguration<?>> typeToConfig) {
             log.debug("Static roles loaded ({})", staticRoles.getCEntries().size());
 
             if (actionGroups.containsAny(staticActionGroups)) {
-                System.out.println("static: " + actionGroups.getCEntries());
-                System.out.println("Static Action Groups:" + staticActionGroups.getCEntries());
                 throw new StaticResourceException("Cannot override static action groups");
             }
             if (!actionGroups.add(staticActionGroups) && !staticActionGroups.getCEntries().isEmpty()) {

src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java

@@ -158,8 +158,6 @@ public Object run() {
             final String renegoMsg =
                 "Client side initiated TLS renegotiation enabled. This can open a vulnerablity for DoS attacks through client side initiated TLS renegotiation.";
             log.warn(renegoMsg);
-            System.out.println(renegoMsg);
-            System.err.println(renegoMsg);
         } else {
             if (!rejectClientInitiatedRenegotiation) {
 
@@ -225,8 +223,6 @@ public Object run() {
 
         if (!httpSSLEnabled && !transportSSLEnabled) {
             log.error("SSL not activated for http and/or transport.");
-            System.out.println("SSL not activated for http and/or transport.");
-            System.err.println("SSL not activated for http and/or transport.");
         }
 
         if (ExternalSecurityKeyStore.hasExternalSslContext(settings)) {

src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java

@@ -25,6 +25,8 @@
 import org.apache.commons.cli.Option;
 import org.apache.commons.cli.Options;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.xcontent.XContentType;
 import org.opensearch.core.xcontent.ToXContent;
@@ -44,6 +46,7 @@ public class AuditConfigMigrater {
     private static final CommandLineParser parser = new DefaultParser();
 
     public static void main(String[] args) {
+        final Logger log = LogManager.getLogger(this.getClass());
         options.addOption(
             Option.builder("s")
                 .argName("source")
@@ -91,7 +94,7 @@ public static void main(String[] args) {
             final String opensearchOutput = sanitizeFilePath(line.getOptionValue("oed", "."), OPENSEARCH_AUDIT_FILTERED_YML);
 
             // create settings builder
-            System.out.println("Using source opensearch.yml file from path " + source);
+            log.info("Using source opensearch.yml file from path " + source);
             final Settings.Builder settingsBuilder = Settings.builder().loadFromPath(Paths.get(source));
 
             // create audit config
@@ -106,10 +109,10 @@ public static void main(String[] args) {
             DefaultObjectMapper.YAML_MAPPER.writeValue(new File(auditOutput), result);
 
             // remove all deprecated values opensearch.yml
-            System.out.println("Looking for deprecated keys in " + source);
+            log.info("Looking for deprecated keys in " + source);
             AuditConfig.DEPRECATED_KEYS.forEach(key -> {
                 if (settingsBuilder.get(key) != null) {
-                    System.out.println(" " + key);
+                    log.info(" " + key);
                 }
                 settingsBuilder.remove(key);
             });
@@ -123,16 +126,15 @@ public static void main(String[] args) {
                 builder.close();
             }
 
-            System.out.println("Generated " + AUDIT_YML + " is available at path " + auditOutput);
-            System.out.println(
+            log.info("Generated " + AUDIT_YML + " is available at path " + auditOutput);
+            log.info(
                 "Generated "
                     + OPENSEARCH_AUDIT_FILTERED_YML
                     + " is available at path "
                     + opensearchOutput
                     + " Please remove the deprecated keys from your opensearch.yml or replace with the generated file after reviewing."
             );
         } catch (final Exception e) {
-            e.printStackTrace();
             formatter.printHelp("audit_config_migrater.sh", options, true);
             System.exit(-1);
         }

src/main/java/org/opensearch/security/tools/Migrater.java

@@ -27,6 +27,8 @@
 import org.apache.commons.cli.Option;
 import org.apache.commons.cli.Options;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.common.collect.Tuple;
 import org.opensearch.security.DefaultObjectMapper;
 import org.opensearch.security.securityconf.Migration;
@@ -39,8 +41,9 @@
 
 public class Migrater {
 
-    public static void main(final String[] args) {
+    protected static final Logger log = LogManager.getLogger(Migrater.class);
 
+    public static void main(final String[] args) {
         final Options options = new Options();
         final HelpFormatter formatter = new HelpFormatter();
         options.addOption(
@@ -60,7 +63,7 @@ public static void main(final String[] args) {
                 }
             }
         } catch (final Exception exp) {
-            System.err.println("Parsing failed.  Reason: " + exp.getMessage());
+            log.error("Parsing failed.  Reason: " + exp.getMessage());
             formatter.printHelp("migrater.sh", options, true);
         }
 
@@ -69,12 +72,12 @@ public static void main(final String[] args) {
 
     public static boolean migrateDirectory(File dir, boolean backup) {
         if (!dir.exists()) {
-            System.out.println(dir.getAbsolutePath() + " does not exist");
+            log.info(dir.getAbsolutePath() + " does not exist");
             return false;
         }
 
         if (!dir.isDirectory()) {
-            System.out.println(dir.getAbsolutePath() + " is not a directory");
+            log.info(dir.getAbsolutePath() + " is not a directory");
             return false;
         }
 
@@ -93,12 +96,12 @@ public static boolean migrateFile(File file, CType cType, boolean backup) {
         final String absolutePath = file.getAbsolutePath();
         // NODESDN is newer type and supports populating empty content if file is missing
         if (!file.exists() && cType != CType.NODESDN) {
-            System.out.println("Skip " + absolutePath + " because it does not exist");
+            log.info("Skip " + absolutePath + " because it does not exist");
             return false;
         }
 
         if (!file.isFile()) {
-            System.out.println("Skip " + absolutePath + " because it is a directory or a special file");
+            log.info("Skip " + absolutePath + " because it is a directory or a special file");
             return false;
         }
 
@@ -169,7 +172,7 @@ public static boolean migrateFile(File file, CType cType, boolean backup) {
                 return backupAndWrite(file, val, backup);
             }
         } catch (Exception e) {
-            System.out.println("Can not migrate " + file + " due to " + e);
+            log.error("Can not migrate " + file + " due to " + e);
         }
 
         return false;
@@ -178,19 +181,18 @@ public static boolean migrateFile(File file, CType cType, boolean backup) {
     private static boolean backupAndWrite(File file, SecurityDynamicConfiguration<?> val, boolean backup) {
         try {
             if (val == null) {
-                System.out.println("NULL object for " + file.getAbsolutePath());
+                log.info("NULL object for " + file.getAbsolutePath());
                 return false;
             }
             if (backup && file.exists()) {
                 Files.copy(file, new File(file.getParent(), file.getName() + ".bck6"));
             }
             DefaultObjectMapper.YAML_MAPPER.writeValue(file, val);
-            System.out.println("Migrated (as " + val.getCType() + ") " + file.getAbsolutePath());
+            log.info("Migrated (as " + val.getCType() + ") " + file.getAbsolutePath());
             return true;
         } catch (Exception e) {
-            System.out.println("Unable to write " + file.getAbsolutePath() + ". This is unexpected and we will abort migration.");
-            System.out.println("    Details: " + e.getMessage());
-            e.printStackTrace();
+            log.error("Unable to write " + file.getAbsolutePath() + ". This is unexpected and we will abort migration.");
+            log.error("    Details: " + e.getMessage());
         }
 
         return false;

src/test/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java

@@ -314,7 +314,6 @@ public void testNbfInSkew() throws Exception {
 
         long expiringDate = 20 + System.currentTimeMillis() / 1000;
         long notBeforeDate = 5 + System.currentTimeMillis() / 1000;
-        ;
 
         AuthCredentials creds = jwtAuth.extractCredentials(
             new FakeRestRequest(

src/test/java/com/amazon/dlic/auth/ldap/LdapBackendIntegTest.java

@@ -53,7 +53,6 @@ protected String getResourceFolder() {
     public void testIntegLdapAuthenticationSSL() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("jacksonm", "secret")).getStatusCode());
@@ -63,7 +62,6 @@ public void testIntegLdapAuthenticationSSL() throws Exception {
     public void testIntegLdapAuthenticationSSLFail() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("wrong", "wrong")).getStatusCode());
@@ -87,7 +85,7 @@ public void testAttributesWithImpersonation() throws Exception {
                 encodeBasicHeader("spock", "spocksecret")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
+
         Assert.assertTrue(res.getBody().contains("ldap.dn"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.entryDN"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.subschemaSubentry"));

src/test/java/com/amazon/dlic/auth/ldap/LdapBackendTest.java

@@ -1081,7 +1081,6 @@ public void testMultiCn() throws Exception {
         );
         Assert.assertNotNull(user);
         Assert.assertEquals("cn=cabc,ou=people,o=TEST", user.getName());
-        System.out.println(user.getUserEntry().getAttribute("cn"));
     }
 
     @AfterClass

src/test/java/com/amazon/dlic/auth/ldap2/LdapBackendIntegTest2.java

@@ -53,7 +53,6 @@ protected String getResourceFolder() {
     public void testIntegLdapAuthenticationSSL() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config_ldap2.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("jacksonm", "secret")).getStatusCode());
@@ -63,7 +62,6 @@ public void testIntegLdapAuthenticationSSL() throws Exception {
     public void testIntegLdapAuthenticationSSLFail() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config_ldap2.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("wrong", "wrong")).getStatusCode());
@@ -87,7 +85,7 @@ public void testAttributesWithImpersonation() throws Exception {
                 encodeBasicHeader("spock", "spocksecret")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
+
         Assert.assertTrue(res.getBody().contains("ldap.dn"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.entryDN"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.subschemaSubentry"));

src/test/java/org/opensearch/security/AggregationTests.java

@@ -114,7 +114,7 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("nagilum", "nagilum")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
+
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");
@@ -134,7 +134,7 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("nagilum", "nagilum")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
+
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");
@@ -154,7 +154,7 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("worf", "worf")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
+
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");

src/test/java/org/opensearch/security/ConfigTests.java

@@ -58,30 +58,25 @@ public void testEmptyConfig() throws Exception {
     @Test
     public void testMigrate() throws Exception {
 
+        // TODO add a better way to test it rather than adding sysouts
         Tuple<SecurityDynamicConfiguration<RoleV7>, SecurityDynamicConfiguration<TenantV7>> rolesResult = Migration.migrateRoles(
             (SecurityDynamicConfiguration<RoleV6>) load("./legacy/securityconfig_v6/roles.yml", CType.ROLES),
             (SecurityDynamicConfiguration<RoleMappingsV6>) load("./legacy/securityconfig_v6/roles_mapping.yml", CType.ROLESMAPPING)
         );
 
-        System.out.println(Strings.toString(XContentType.JSON, rolesResult.v2(), true, false));
-        System.out.println(Strings.toString(XContentType.JSON, rolesResult.v1(), true, false));
-
         SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsResult = Migration.migrateActionGroups(
             (SecurityDynamicConfiguration<ActionGroupsV6>) load("./legacy/securityconfig_v6/action_groups.yml", CType.ACTIONGROUPS)
         );
-        System.out.println(Strings.toString(XContentType.JSON, actionGroupsResult, true, false));
         SecurityDynamicConfiguration<ConfigV7> configResult = Migration.migrateConfig(
             (SecurityDynamicConfiguration<ConfigV6>) load("./legacy/securityconfig_v6/config.yml", CType.CONFIG)
         );
-        System.out.println(Strings.toString(XContentType.JSON, configResult, true, false));
         SecurityDynamicConfiguration<InternalUserV7> internalUsersResult = Migration.migrateInternalUsers(
             (SecurityDynamicConfiguration<InternalUserV6>) load("./legacy/securityconfig_v6/internal_users.yml", CType.INTERNALUSERS)
         );
-        System.out.println(Strings.toString(XContentType.JSON, internalUsersResult, true, false));
         SecurityDynamicConfiguration<RoleMappingsV7> rolemappingsResult = Migration.migrateRoleMappings(
             (SecurityDynamicConfiguration<RoleMappingsV6>) load("./legacy/securityconfig_v6/roles_mapping.yml", CType.ROLESMAPPING)
         );
-        System.out.println(Strings.toString(XContentType.JSON, rolemappingsResult, true, false));
+
     }
 
     @Test
@@ -110,14 +105,12 @@ private void check(String file, CType cType) throws Exception {
         final String adjustedFilePath = SingleClusterTest.TEST_RESOURCE_RELATIVE_PATH + file;
         JsonNode jsonNode = YAML.readTree(Files.readString(new File(adjustedFilePath).toPath(), StandardCharsets.UTF_8));
         int configVersion = 1;
-        System.out.println("%%%%%%%% THIS IS A LINE OF INTEREST %%%%%%%");
+
         if (jsonNode.get("_meta") != null) {
             Assert.assertEquals(jsonNode.get("_meta").get("type").asText(), cType.toLCString());
             configVersion = jsonNode.get("_meta").get("config_version").asInt();
         }
 
-        System.out.println("%%%%%%%% THIS IS A LINE OF INTEREST: CONFIG VERSION: " + configVersion + "%%%%%%%");
-
         SecurityDynamicConfiguration<?> dc = load(file, cType);
         Assert.assertNotNull(dc);
         // Assert.assertTrue(dc.getCEntries().size() > 0);
@@ -132,12 +125,10 @@ private SecurityDynamicConfiguration<?> load(String file, CType cType) throws Ex
         JsonNode jsonNode = YAML.readTree(Files.readString(new File(adjustedFilePath).toPath(), StandardCharsets.UTF_8));
         int configVersion = 1;
 
-        System.out.println("%%%%%%%% THIS IS A LINE OF INTEREST LOAD: CONFIG VERSION: %%%%%%%");
         if (jsonNode.get("_meta") != null) {
             Assert.assertEquals(jsonNode.get("_meta").get("type").asText(), cType.toLCString());
             configVersion = jsonNode.get("_meta").get("config_version").asInt();
         }
-        System.out.println("%%%%%%%% THIS IS A LINE OF INTEREST: CONFIG VERSION: " + configVersion + "%%%%%%%");
         return SecurityDynamicConfiguration.fromNode(jsonNode, cType, configVersion, 0, 0);
     }
 }