Fix windows encoding issues (opensearch-project#2206)

Adds spotbugs [1] to detect internalization before they are added to the codebase, also fixed several encoding bugs that impact windows users. [1] https://spotbugs.readthedocs.io/en/stable/index.html Closes opensearch-project#2194 Signed-off-by: Peter Nied <[email protected]>
stephen-crawford · Dec 2, 2022 · 6dd5188 · 6dd5188
1 parent f7b0cb2
commit 6dd5188
Show file tree

Hide file tree

Showing 9 changed files with 26 additions and 11 deletions.
diff --git a/build.gradle b/build.gradle
@@ -42,6 +42,7 @@ plugins {
     id "nebula.ospackage" version "9.0.0"
     id "com.google.osdetector" version "1.7.0"
     id "org.gradle.test-retry" version "1.3.1"
+    id "com.github.spotbugs" version "5.0.13"
 }
 import org.gradle.crypto.checksum.Checksum
 
@@ -223,6 +224,13 @@ testsJar {
     libsDirName = '.'
 }
 
+spotbugs {
+    includeFilter = file('spotbugs-include.xml')
+}
+
+spotbugsTest {
+    enabled = false
+}
 
 test {
     maxParallelForks = 3

diff --git a/spotbugs-include.xml b/spotbugs-include.xml
@@ -0,0 +1,5 @@
+<FindBugsFilter>
+    <Match>
+        <Bug category="I18N" />
+    </Match>
+</FindBugsFilter>
diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java b/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
@@ -18,6 +18,7 @@
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
 import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
@@ -155,7 +156,7 @@ private AuthTokenProcessorAction.Response handleImpl(RestRequest restRequest, Re
             SettingsException {
         if (token_log.isDebugEnabled()) {
             try {
-                token_log.debug("SAMLResponse for {}\n{}", samlRequestId, new String(Util.base64decoder(samlResponseBase64), "UTF-8"));
+                token_log.debug("SAMLResponse for {}\n{}", samlRequestId, new String(Util.base64decoder(samlResponseBase64), StandardCharsets.UTF_8));
             } catch (Exception e) {
                 token_log.warn(
                         "SAMLResponse for {} cannot be decoded from base64\n{}",

diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java
@@ -454,7 +454,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
                     try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, originalResult.internalSourceRef(), XContentType.JSON)) {
                         Object base64 = parser.map().values().iterator().next();
                         if(base64 instanceof String) {
-                            originalSource = (new String(BaseEncoding.base64().decode((String) base64)));
+                            originalSource = (new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8));
                         } else {
                             originalSource = XContentHelper.convertToJson(originalResult.internalSourceRef(), false, XContentType.JSON);
                         }
@@ -465,7 +465,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
                     try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, currentIndex.source(), XContentType.JSON)) {
                         Object base64 = parser.map().values().iterator().next();
                         if(base64 instanceof String) {
-                            currentSource = (new String(BaseEncoding.base64().decode((String) base64)));
+                            currentSource = new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8);
                         } else {
                             currentSource = XContentHelper.convertToJson(currentIndex.source(), false, XContentType.JSON);
                         }
@@ -492,7 +492,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
                 try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, currentIndex.source(), XContentType.JSON)) {
                    Object base64 = parser.map().values().iterator().next();
                    if(base64 instanceof String) {
-                       msg.addSecurityConfigContentToRequestBody(new String(BaseEncoding.base64().decode((String) base64)), id);
+                       msg.addSecurityConfigContentToRequestBody(new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8), id);
                    } else {
                        msg.addSecurityConfigTupleToRequestBody(new Tuple<XContentType, BytesReference>(XContentType.JSON, currentIndex.source()), id);
                    }

diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java b/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java
@@ -31,6 +31,7 @@
 package org.opensearch.security.configuration;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
@@ -275,7 +276,7 @@ private SecurityDynamicConfiguration<?> toConfig(GetResponse singleGetResponse,
 
             parser.nextToken();
 
-            final String jsonAsString = SecurityUtils.replaceEnvVars(new String(parser.binaryValue()), settings);
+            final String jsonAsString = SecurityUtils.replaceEnvVars(new String(parser.binaryValue(), StandardCharsets.UTF_8), settings);
             final JsonNode jsonNode = DefaultObjectMapper.readTree(jsonAsString);
             int configVersion = 1;
 

diff --git a/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java b/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java
@@ -28,6 +28,7 @@
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.File;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.LinkOption;
 import java.nio.file.Path;

diff --git a/src/main/java/org/opensearch/security/support/ConfigHelper.java b/src/main/java/org/opensearch/security/support/ConfigHelper.java
@@ -35,6 +35,7 @@
 import java.io.IOException;
 import java.io.Reader;
 import java.io.StringReader;
+import java.nio.charset.StandardCharsets;
 
 import org.opensearch.security.securityconf.impl.Meta;
 import org.apache.logging.log4j.Logger;
@@ -96,7 +97,7 @@ public static void uploadFile(Client tc, String filepath, String index, CType cT
     public static Reader createFileOrStringReader(CType cType, int configVersion, String filepath, boolean populateEmptyIfFileMissing) throws Exception {
         Reader reader;
         if (!populateEmptyIfFileMissing || new File(filepath).exists()) {
-            reader = new FileReader(filepath);
+            reader = new FileReader(filepath, StandardCharsets.UTF_8);
         } else {
             reader = new StringReader(createEmptySdcYaml(cType, configVersion));
         }
@@ -148,7 +149,7 @@ public static <T> SecurityDynamicConfiguration<T> fromYamlReader(Reader yamlRead
     }
 
     public static <T> SecurityDynamicConfiguration<T> fromYamlFile(String filepath, CType ctype, int version, long seqNo, long primaryTerm) throws IOException {
-        return fromYamlReader(new FileReader(filepath), ctype, version, seqNo, primaryTerm);
+        return fromYamlReader(new FileReader(filepath, StandardCharsets.UTF_8), ctype, version, seqNo, primaryTerm);
     }
 
     public static <T> SecurityDynamicConfiguration<T> fromYamlString(String yamlString, CType ctype, int version, long seqNo, long primaryTerm) throws IOException {

diff --git a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java
@@ -911,8 +911,8 @@ private static boolean retrieveFile(final Client tc, final String filepath, fina
 
         }
 
-        System.out.println("Will retrieve '"+type+"/" +id+"' into "+filepath+" "+(legacy?"(legacy mode)":""));
-        try (Writer writer = new FileWriter(filepath)) {
+        System.out.println("Will retrieve '"+"/" +id+"' into "+filepath+" "+(legacy?"(legacy mode)":""));
+        try (Writer writer = new FileWriter(filepath, StandardCharsets.UTF_8)) {
 
             final GetResponse response = tc.get(new GetRequest(index).type(type).id(id).refresh(true).realtime(false)).actionGet();
 

diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java
@@ -52,7 +52,6 @@
 import org.opensearch.common.xcontent.XContentType;
 import org.junit.Assert;
 import org.junit.Assume;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse;
@@ -343,7 +342,6 @@ public void testSingle() throws Exception {
     }
 
     @Test
-    @Ignore // https://github.com/opensearch-project/security/issues/2194
     public void testSpecialUsernames() throws Exception {
 
         setup();