Skip to content
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.

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

Fix windows encoding issues #2206

Merged
merged 3 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ jobs:
-x spotlessCheck
-x checkstyleMain
-x checkstyleTest
-x spotbugsMain
peternied marked this conversation as resolved.
Show resolved Hide resolved

- name: Coverage
uses: codecov/codecov-action@v1
Expand Down Expand Up @@ -80,6 +81,7 @@ jobs:
-x spotlessCheck
-x checkstyleMain
-x checkstyleTest
-x spotbugsMain

backward-compatibility:
runs-on: ubuntu-latest
Expand Down
15 changes: 15 additions & 0 deletions .github/workflows/code-hygiene.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 9 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -85,6 +86,14 @@ spotless {
}
}

spotbugs {
includeFilter = file('spotbugs-include.xml')
}

spotbugsTest {
enabled = false
}

java.sourceCompatibility = JavaVersion.VERSION_11
java.targetCompatibility = JavaVersion.VERSION_11

Expand Down
5 changes: 5 additions & 0 deletions spotbugs-include.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<FindBugsFilter>
<Match>
<Bug category="I18N" />
</Match>
</FindBugsFilter>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this should be added as a separate PR or is fine to include it in this one even though it seems not directly related to the parsing issue--maybe it is and I am wrong?

Copy link
Member

Choose a reason for hiding this comment

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

This is helpful to find instances where the code is relying on the default encoding. See https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html?highlight=i18n#dm-reliance-on-default-encoding-dm-default-encoding

Copy link
Member Author

@peternied peternied Oct 31, 2022

Choose a reason for hiding this comment

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

I did not add more rules because we are in violation of many different rules, but I didn't see any types that would create platform specific issues.

If you'd like to see the full list, delete line 3 and run the gradle task, you'll see hundreds of things!

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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{}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
peternied marked this conversation as resolved.
Show resolved Hide resolved
} else {
originalSource = XContentHelper.convertToJson(originalResult.internalSourceRef(), false, XContentType.JSON);
}
Expand All @@ -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);
}
Expand All @@ -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, BytesReference>(XContentType.JSON, currentIndex.source()), id);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -646,7 +647,7 @@ private boolean areSameCerts(final X509Certificate[] currentX509Certs, final X50

final Function<? super X509Certificate, String> 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<String> currentCertSignatureSet = Arrays.stream(currentX509Certs)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -143,7 +144,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 0 additions & 2 deletions src/test/java/org/opensearch/security/IntegrationTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down