From a3133fa995cc7ad3184c70ea7c24dc098ad3497f Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 2 Nov 2022 10:56:12 -0500 Subject: [PATCH] Fix windows encoding issues (#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 https://github.com/opensearch-project/security/issues/2194 Signed-off-by: Peter Nied Signed-off-by: Stephen Crawford --- .github/workflows/ci.yml | 2 ++ .github/workflows/code-hygiene.yml | 15 +++++++++++++++ build.gradle | 9 +++++++++ spotbugs-include.xml | 5 +++++ .../auth/http/saml/AuthTokenProcessorHandler.java | 3 ++- .../security/auditlog/impl/AbstractAuditLog.java | 6 +++--- .../ConfigurationLoaderSecurity7.java | 3 ++- .../security/ssl/DefaultSecurityKeyStore.java | 3 ++- .../opensearch/security/support/ConfigHelper.java | 5 +++-- .../opensearch/security/tools/SecurityAdmin.java | 2 +- .../org/opensearch/security/IntegrationTests.java | 2 -- 11 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 spotbugs-include.xml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ad10298a61..3e7fc78987 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,6 +34,7 @@ jobs: -x spotlessCheck -x checkstyleMain -x checkstyleTest + -x spotbugsMain - name: Coverage uses: codecov/codecov-action@v1 @@ -80,6 +81,7 @@ jobs: -x spotlessCheck -x checkstyleMain -x checkstyleTest + -x spotbugsMain backward-compatibility: runs-on: ubuntu-latest diff --git a/.github/workflows/code-hygiene.yml b/.github/workflows/code-hygiene.yml index 6691ee8a15..f078cb2b56 100644 --- a/.github/workflows/code-hygiene.yml +++ b/.github/workflows/code-hygiene.yml @@ -42,3 +42,18 @@ jobs: - uses: gradle/gradle-build-action@v2 with: arguments: checkstyleMain checkstyleTest + + spotbugs: + runs-on: ubuntu-latest + name: Spotbugs scan + steps: + - uses: actions/checkout@v2 + + - uses: actions/setup-java@v2 + with: + distribution: temurin # Temurin is a distribution of adoptium + java-version: 11 + + - uses: gradle/gradle-build-action@v2 + with: + arguments: spotbugsMain diff --git a/build.gradle b/build.gradle index 4d2a94b624..bafa7de4ac 100644 --- a/build.gradle +++ b/build.gradle @@ -57,6 +57,7 @@ plugins { id 'nebula.ospackage' version "8.3.0" id "org.gradle.test-retry" version "1.3.1" id 'eclipse' + id "com.github.spotbugs" version "5.0.13" } allprojects { @@ -85,6 +86,14 @@ spotless { } } +spotbugs { + includeFilter = file('spotbugs-include.xml') +} + +spotbugsTest { + enabled = false +} + java.sourceCompatibility = JavaVersion.VERSION_11 java.targetCompatibility = JavaVersion.VERSION_11 diff --git a/spotbugs-include.xml b/spotbugs-include.xml new file mode 100644 index 0000000000..dd6062700d --- /dev/null +++ b/spotbugs-include.xml @@ -0,0 +1,5 @@ + + + + + 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 index cb9f6e9a8b..4ab5673adb 100644 --- a/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java +++ b/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java @@ -14,6 +14,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; @@ -152,7 +153,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 index d6f59028fa..3ebabb14bb 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java @@ -419,7 +419,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); } @@ -430,7 +430,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); } @@ -457,7 +457,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.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 index e7b759aefd..785a923bf8 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java @@ -27,6 +27,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; @@ -253,7 +254,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 index 8562ea20e7..d186b26869 100644 --- a/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java +++ b/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java @@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; @@ -646,7 +647,7 @@ private boolean areSameCerts(final X509Certificate[] currentX509Certs, final X50 final Function certificateSignature = cert -> { final byte[] signature = cert !=null && cert.getSignature() != null ? cert.getSignature() : null; - return new String(signature); + return new String(signature, StandardCharsets.UTF_8); }; final Set currentCertSignatureSet = Arrays.stream(currentX509Certs) diff --git a/src/main/java/org/opensearch/security/support/ConfigHelper.java b/src/main/java/org/opensearch/security/support/ConfigHelper.java index a12a8d5614..23f93e3672 100644 --- a/src/main/java/org/opensearch/security/support/ConfigHelper.java +++ b/src/main/java/org/opensearch/security/support/ConfigHelper.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.Reader; import java.io.StringReader; +import java.nio.charset.StandardCharsets; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -91,7 +92,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)); } @@ -143,7 +144,7 @@ public static SecurityDynamicConfiguration fromYamlReader(Reader yamlRead } public static SecurityDynamicConfiguration 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 SecurityDynamicConfiguration 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 index 4e89fd32de..ee4d8e72d7 100644 --- a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java +++ b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java @@ -896,7 +896,7 @@ private static boolean retrieveFile(final RestHighLevelClient restHighLevelClien } System.out.println("Will retrieve '"+"/" +id+"' into "+filepath+" "+(legacy?"(legacy mode)":"")); - try (Writer writer = new FileWriter(filepath)) { + try (Writer writer = new FileWriter(filepath, StandardCharsets.UTF_8)) { final GetResponse response = restHighLevelClient.get(new GetRequest(index).id(id).refresh(true).realtime(false), RequestOptions.DEFAULT); diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index 246fca03d9..226551a5ae 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/src/test/java/org/opensearch/security/IntegrationTests.java @@ -34,7 +34,6 @@ import org.apache.hc.core5.http.message.BasicHeader; import org.junit.Assert; import org.junit.Assume; -import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; @@ -270,7 +269,6 @@ public void testSingle() throws Exception { } @Test - @Ignore // https://github.com/opensearch-project/security/issues/2194 public void testSpecialUsernames() throws Exception { setup();