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

ANCHOR-403 Refactor TOML Exception obfuscation for URL/Parsing to Toml Functions our of NetUtil #1051

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d4f087b
ANCHOR-403-reece-SSRF-metadata
reecexlm Aug 5, 2023
2d74b54
use assertThrows
reecexlm Aug 6, 2023
c797ff5
additional cleanup
reecexlm Aug 6, 2023
30c7943
remove custom exception
reecexlm Aug 8, 2023
93acffc
make sure that the log event is actually captured by adding a verific…
reecexlm Aug 9, 2023
05c167d
temporary setting toml to url to test
reecexlm Aug 9, 2023
eb31590
just testing. ignore
reecexlm Aug 9, 2023
4e9c934
try other file
reecexlm Aug 9, 2023
1c2ed0d
patch toml parser as well
reecexlm Aug 9, 2023
e73c208
replace comment
reecexlm Aug 9, 2023
8da260c
revert
reecexlm Aug 9, 2023
19c84e5
lint
reecexlm Aug 9, 2023
3c6f9b0
debugF
reecexlm Aug 9, 2023
807ff46
revert config changes
reecexlm Aug 10, 2023
33369dd
revert defaults
reecexlm Aug 10, 2023
574e8c7
[ANCHOR-403] Prevent SSRF through SEP-1 TOML redirect (#1043)
reecexlm Aug 10, 2023
90d9550
bump version to 2.1.3
reecexlm Aug 10, 2023
4bd7244
spotless
reecexlm Aug 10, 2023
be182c3
log the actual exception
reecexlm Aug 10, 2023
f272904
fix comment
reecexlm Aug 10, 2023
ea1cd61
Merge branch 'develop' into ANCHOR-403-reece-SDP001-aws-metadata
reecexlm Aug 10, 2023
eaf3224
don't repeat exceptions at each layer
reecexlm Aug 11, 2023
7288802
Merge remote-tracking branch 'refs/remotes/origin/ANCHOR-403-reece-SD…
reecexlm Aug 11, 2023
7a4cec4
refactor sep1helper
reecexlm Aug 11, 2023
d726f77
addressing review comments
reecexlm Aug 11, 2023
5ec4ad3
update tests
reecexlm Aug 11, 2023
90c584b
fix assert
reecexlm Aug 11, 2023
10e6562
log exception
reecexlm Aug 11, 2023
482057e
add some docs to netutil and reduce unnecessary logging
reecexlm Aug 11, 2023
56ece29
Merge branch 'main' into ANCHOR-403-reece-SDP001-aws-metadata
reecexlm Aug 11, 2023
a622baf
rebased on main
reecexlm Aug 11, 2023
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: 1 addition & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ subprojects {

allprojects {
group = "org.stellar.anchor-sdk"
version = "2.1.2"
version = "2.1.3"

tasks.jar {
manifest {
Expand Down
18 changes: 14 additions & 4 deletions core/src/main/java/org/stellar/anchor/sep1/Sep1Service.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.stellar.anchor.sep1;

import static org.stellar.anchor.util.Log.debug;
import static org.stellar.anchor.util.Log.debugF;
import static org.stellar.anchor.util.MetricConstants.SEP1_TOML_ACCESSED;

Expand All @@ -27,10 +26,10 @@ public class Sep1Service {
*/
public Sep1Service(Sep1Config sep1Config) throws IOException, InvalidConfigException {
if (sep1Config.isEnabled()) {
debug("sep1Config:", sep1Config);
debugF("sep1Config: {}", sep1Config);
switch (sep1Config.getType()) {
case STRING:
debug("reading stellar.toml from config[sep1.toml.value]");
debugF("reading stellar.toml from config[sep1.toml.value]");
tomlValue = sep1Config.getValue();
break;
case FILE:
Expand All @@ -39,7 +38,7 @@ public Sep1Service(Sep1Config sep1Config) throws IOException, InvalidConfigExcep
break;
case URL:
debugF("reading stellar.toml from {}", sep1Config.getValue());
tomlValue = NetUtil.fetch(sep1Config.getValue());
tomlValue = fetchTomlFromURL(sep1Config.getValue());
break;
default:
throw new InvalidConfigException(
Expand All @@ -50,6 +49,17 @@ public Sep1Service(Sep1Config sep1Config) throws IOException, InvalidConfigExcep
}
}

private String fetchTomlFromURL(String url) throws IOException {
philipliu marked this conversation as resolved.
Show resolved Hide resolved
try {
return NetUtil.fetch(url);
} catch (IOException e) {
// Obfuscate the message and rethrow
String obfuscatedMessage = String.format("Unable to fetch data from %s", url);
debugF(e.toString()); // Log the obfuscated message using the debugF method
throw new IOException(obfuscatedMessage); // Preserve the original exception as the cause
}
}

public String getStellarToml() {
sep1TomlAccessedCounter.increment();
return tomlValue;
Expand Down
27 changes: 23 additions & 4 deletions core/src/main/java/org/stellar/anchor/util/NetUtil.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.stellar.anchor.util;

import static org.stellar.anchor.util.Log.debugF;
import static org.stellar.anchor.util.StringHelper.isEmpty;

import io.jsonwebtoken.lang.Strings;
Expand All @@ -13,12 +14,30 @@
import okhttp3.Response;

public class NetUtil {

public static String fetch(String url) throws IOException {
Request request = OkHttpUtil.buildGetRequest(url);
Response response = getCall(request).execute();

if (response.body() == null) return "";
return Objects.requireNonNull(response.body()).string();
try {
Request request = OkHttpUtil.buildGetRequest(url);
Response response = getCall(request).execute();
String message =
String.format("NetUtil:fetch of %s unsuccessful. response: %s", url, response.toString());
philipliu marked this conversation as resolved.
Show resolved Hide resolved

// Check if response was unsuccessful (ie not status code 2xx) and throw IOException
if (!response.isSuccessful()) {
debugF(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I guess if we're going to throw the exception with the same message, there's no need to log it.

Copy link
Contributor Author

@reecexlm reecexlm Aug 11, 2023

Choose a reason for hiding this comment

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

i appreciate the nit! ah yea, that was left over from when i was obfuscating here. I also don't need to catch/re-throw anymore. i will add some documentation for the fn as well so that its clear that fetch expects a response and successful return code.

throw new IOException(message);
}

if (response.body() == null) {
debugF(message);
throw new IOException(message);
}
philipliu marked this conversation as resolved.
Show resolved Hide resolved
return Objects.requireNonNull(response.body()).string();
} catch (IOException e) {
debugF(e.toString());
throw e;
}
}

@SuppressWarnings("BooleanMethodIsAlwaysInverted")
Expand Down
27 changes: 23 additions & 4 deletions core/src/main/java/org/stellar/anchor/util/Sep1Helper.java
Original file line number Diff line number Diff line change
@@ -1,24 +1,43 @@
package org.stellar.anchor.util;

import static org.stellar.anchor.util.Log.*;

import com.moandjiezana.toml.Toml;
import java.io.IOException;
import java.net.URL;
import org.stellar.anchor.api.exception.InvalidConfigException;

public class Sep1Helper {
public static TomlContent readToml(String url) throws IOException {
return new TomlContent(new URL(url));
}

public static TomlContent parse(String tomlString) {
return new TomlContent(tomlString);
public static TomlContent parse(String tomlString) throws InvalidConfigException {
try {
return new TomlContent(tomlString);
} catch (Exception e) {
// Obfuscate the message and rethrow
String obfuscatedMessage = "Failed to parse TOML content. Invalid Config.";
debugF(e.toString()); // Log the exception
throw new InvalidConfigException(
obfuscatedMessage); // Preserve the original exception as the cause
}
}

public static class TomlContent {
private final Toml toml;

TomlContent(URL url) throws IOException {
String tomlValue = NetUtil.fetch(url.toString());
toml = new Toml().read(tomlValue);
philipliu marked this conversation as resolved.
Show resolved Hide resolved
try {
String tomlValue = NetUtil.fetch(url.toString());
toml = new Toml().read(tomlValue);
} catch (IOException e) {
// Obfuscate the message and rethrow
String obfuscatedMessage =
String.format("An error occurred while fetching the TOML from %s", url);
Log.error(e.toString());
throw new IOException(obfuscatedMessage); // Preserve the original exception as the cause
}
}

TomlContent(String tomlString) {
Expand Down
25 changes: 19 additions & 6 deletions core/src/test/kotlin/org/stellar/anchor/util/NetUtilTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ package org.stellar.anchor.util

import io.mockk.*
import io.mockk.impl.annotations.MockK
import java.io.IOException
import java.net.MalformedURLException
import okhttp3.Response
import okhttp3.ResponseBody
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Assertions.*
import org.junit.jupiter.api.Assertions.assertThrows
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
Expand Down Expand Up @@ -36,31 +38,42 @@ internal class NetUtilTest {
}

@Test
fun `test fetch()`() {
fun `test fetch successful response`() {
mockkStatic(NetUtil::class)
every { NetUtil.getCall(any()) } returns mockCall
every { mockCall.execute() } returns mockResponse
every { mockResponse.isSuccessful } returns true
every { mockResponse.body } returns mockResponseBody
every { mockResponseBody.string() } returns "result"

val result = NetUtil.fetch("http://hello")
assert(result.equals("result"))
assertEquals("result", result)

verify {
NetUtil.getCall(any())
mockCall.execute()
}
}

@Test
fun `test fetch() throws exception`() {
fun `test fetch unsuccessful response`() {
mockkStatic(NetUtil::class)
every { NetUtil.getCall(any()) } returns mockCall
every { mockCall.execute() } returns mockResponse
every { mockResponse.isSuccessful } returns false

assertThrows(IOException::class.java) { NetUtil.fetch("http://hello") }
}

@Test
fun `test fetch null response body`() {
mockkStatic(NetUtil::class)
every { NetUtil.getCall(any()) } returns mockCall
every { mockCall.execute() } returns mockResponse
every { mockResponse.isSuccessful } returns true
every { mockResponse.body } returns null
every { mockResponseBody.string() } returns "result"

val result = NetUtil.fetch("http://hello")
assert(result.equals(""))
assertThrows(IOException::class.java) { NetUtil.fetch("http://hello") }
}

@Test
Expand Down
75 changes: 75 additions & 0 deletions core/src/test/kotlin/org/stellar/anchor/util/Sep1HelperTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package org.stellar.anchor.util

import java.io.IOException
import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Assertions.*
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.stellar.anchor.api.exception.InvalidConfigException

class Sep1HelperTest {
val stellarToml =
"""
ACCOUNTS = [ "GCSGSR6KQQ5BP2FXVPWRL6SWPUSFWLVONLIBJZUKTVQB5FYJFVL6XOXE" ]
VERSION = "0.1.0"
SIGNING_KEY = "GBDYDBJKQBJK4GY4V7FAONSFF2IBJSKNTBYJ65F5KCGBY2BIGPGGLJOH"
NETWORK_PASSPHRASE = "Test SDF Network ; September 2015"

WEB_AUTH_ENDPOINT = "http://localhost:8080/auth"
KYC_SERVER = "http://localhost:8080/sep12"
TRANSFER_SERVER_SEP0024 = "http://localhost:8080/sep24"
DIRECT_PAYMENT_SERVER = "http://localhost:8080/sep31"
ANCHOR_QUOTE_SERVER = "http://localhost:8080/sep38"

[[CURRENCIES]]
code = "SRT"
issuer = "GCDNJUBQSX7AJWLJACMJ7I4BC3Z47BQUTMHEICZLE6MU4KQBRYG5JY6B"
status = "test"
is_asset_anchored = false
anchor_asset_type = "other"
desc = "A fake anchored asset to use with this example anchor server."

[DOCUMENTATION]
ORG_NAME = "Stellar Development Foundation"
ORG_URL = "https://stellar.org"
ORG_DESCRIPTION = "SEP 24 reference server."
ORG_KEYBASE = "stellar.public"
ORG_TWITTER = "StellarOrg"
ORG_GITHUB = "stellar"
"""
.trimIndent()

private val mockWebServer = MockWebServer()

@BeforeEach
fun setup() {
mockWebServer.start()
}

@AfterEach
fun teardown() {
mockWebServer.shutdown()
}

@Test
fun `test Read Toml with IOException`() {
// Enqueue a response with an HTTP error status code
mockWebServer.enqueue(MockResponse().setResponseCode(500))
val exception =
assertThrows(IOException::class.java) {
Sep1Helper.readToml(mockWebServer.url("/").toString())
}
// You may need to adjust the assertion based on the specific behavior of Sep1Helper
assertTrue(exception.message!!.contains("An error occurred while fetching the TOML from"))
}

@Test
fun `test Parse Invalid Toml String`() {
val invalidTomlString = "key = value" // An invalid TOML string without quotes
val exception =
assertThrows(InvalidConfigException::class.java) { Sep1Helper.parse(invalidTomlString) }
assertEquals("Failed to parse TOML content. Invalid Config.", exception.message)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.springframework.validation.Errors;
import org.springframework.validation.ValidationUtils;
import org.springframework.validation.Validator;
import org.stellar.anchor.api.exception.InvalidConfigException;
import org.stellar.anchor.config.Sep1Config;
import org.stellar.anchor.util.NetUtil;
import org.stellar.anchor.util.Sep1Helper;
Expand Down Expand Up @@ -59,12 +60,11 @@ void validateTomlTypeAndValue(PropertySep1Config config, Errors errors) {
case STRING:
try {
Sep1Helper.parse(config.getToml().getValue());
} catch (IllegalStateException isex) {
} catch (InvalidConfigException e) {
errors.rejectValue(
"value",
"sep1-toml-value-string-invalid",
String.format(
"sep1.toml.value does not contain a valid TOML. %s", isex.getMessage()));
String.format("sep1.toml.value does not contain a valid TOML. %s", e.getMessage()));
}
break;
case URL:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.stellar.anchor.platform.controller.sep;

import java.io.IOException;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
Expand Down Expand Up @@ -40,7 +41,7 @@ public RedirectView landingPage() throws SepNotFoundException {
@RequestMapping(
value = "/.well-known/stellar.toml",
method = {RequestMethod.GET, RequestMethod.OPTIONS})
public ResponseEntity<String> getToml() throws SepNotFoundException {
public ResponseEntity<String> getToml() throws IOException, SepNotFoundException {
if (!sep1Config.isEnabled()) {
throw new SepNotFoundException("Not Found");
}
Expand Down
Loading
Loading