Skip to content

Avoid using jackson method for parsing YAML from any URL in RootCredentialsSet #2543

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.

Sign up for GitHub

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

Merged
dimas-b merged 2 commits into from
Sep 15, 2025
Merged

Avoid using jackson method for parsing YAML from any URL in RootCredentialsSet #2543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fdc127
Avoid using jackson method for parsing YAML from any URL in RootCrede…
dimas-b Sep 10, 2025
dfe2b58
review: switch to URI
dimas-b Sep 12, 2025
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
30 changes: 24 additions & 6 deletions ...-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,13 @@
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import jakarta.annotation.Nullable;
import java.net.URL;
import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -98,7 +102,10 @@ static RootCredentialsSet fromList(List<String> credentialsList) {
}

/**
* Parse credentials set from any URL containing a valid YAML or JSON credentials file.
* Parse credentials set from any file or JRE resource containing a valid YAML or JSON credentials
* file.
*
* <p>Note: HTTP and other remote URLs are not allowed.
*
* <p>The expected YAML format is:
*
Expand Down Expand Up @@ -130,18 +137,29 @@ static RootCredentialsSet fromList(List<String> credentialsList) {
* }
* </pre>
*/
static RootCredentialsSet fromUrl(URL url) {
static RootCredentialsSet fromUri(URI uri) {
Preconditions.checkNotNull(uri);
Preconditions.checkArgument(
Strings.isNullOrEmpty(uri.getHost()),
"Remote URIs are not allowed for RootCredentialsSet: %s",
uri);
Comment on lines +140 to +145
Copy link
Member

@snazy snazy Sep 15, 2025

Choose a reason for hiding this comment

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

Actually thinking a bit more about this, you can still get a "host" via a jar URI like jar:http://foo.bar/... or all the file scheme "specialties". But that's rather a question what source locations we want to allow.

For the scope of this PR, the change as it is looks fine.

try (InputStream is = uri.toURL().openStream()) {
return fromInputStream(is);
} catch (Exception e) {
throw new IllegalArgumentException("Failed to read credentials from " + uri, e);
}
}

private static RootCredentialsSet fromInputStream(InputStream in) throws IOException {
YAMLFactory factory = new YAMLFactory();
ObjectMapper mapper = new ObjectMapper(factory).configure(FAIL_ON_UNKNOWN_PROPERTIES, false);
try (var parser = factory.createParser(url)) {
try (var parser = factory.createParser(in)) {
var values = mapper.readValues(parser, RootCredentialsSet.class);
var builder = ImmutableRootCredentialsSet.builder();
while (values.hasNext()) {
builder.putAllCredentials(values.next().credentials());
}
return builder.build();
} catch (Exception e) {
throw new IllegalArgumentException("Failed to read credentials file: " + url, e);
}
}

Expand Down
22 changes: 14 additions & 8 deletions ...e/src/test/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.net.URISyntaxException;
import java.net.URL;
import java.util.List;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -89,25 +90,29 @@ void getSecretsValidSystemProperty() {
}

@Test
void getSecretsValidJson() {
void getSecretsValidJson() throws URISyntaxException {
URL resource = getClass().getResource("credentials.json");
RootCredentialsSet set = RootCredentialsSet.fromUrl(resource);
assertThat(resource).isNotNull();
RootCredentialsSet set = RootCredentialsSet.fromUri(resource.toURI());
assertCredentials(set);
}

@Test
void getSecretsValidYaml() {
void getSecretsValidYaml() throws URISyntaxException {
URL resource = getClass().getResource("credentials.yaml");
RootCredentialsSet set = RootCredentialsSet.fromUrl(resource);
assertThat(resource).isNotNull();

RootCredentialsSet set = RootCredentialsSet.fromUri(resource.toURI());
assertCredentials(set);
}

@Test
void getSecretsInvalidJson() {
URL resource = getClass().getResource("credentials-invalid.json");
assertThatThrownBy(() -> RootCredentialsSet.fromUrl(resource))
assertThat(resource).isNotNull();
assertThatThrownBy(() -> RootCredentialsSet.fromUri(resource.toURI()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Failed to read credentials file")
.hasMessageContaining("Failed to read credentials")
.rootCause()
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining(
Expand All @@ -117,9 +122,10 @@ void getSecretsInvalidJson() {
@Test
void getSecretsInvalidYaml() {
URL resource = getClass().getResource("credentials-invalid.yaml");
assertThatThrownBy(() -> RootCredentialsSet.fromUrl(resource))
assertThat(resource).isNotNull();
assertThatThrownBy(() -> RootCredentialsSet.fromUri(resource.toURI()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Failed to read credentials file")
.hasMessageContaining("Failed to read credentials")
.rootCause()
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining(
Expand Down
3 changes: 1 addition & 2 deletions runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ public Integer call() {
List<String> realms; // TODO Iterable

if (inputOptions.fileOptions != null) {
rootCredentialsSet =
RootCredentialsSet.fromUrl(inputOptions.fileOptions.file.toUri().toURL());
rootCredentialsSet = RootCredentialsSet.fromUri(inputOptions.fileOptions.file.toUri());
realms = rootCredentialsSet.credentials().keySet().stream().toList();
} else {
realms = inputOptions.stdinOptions.realms;
Expand Down
2 changes: 1 addition & 1 deletion runtime/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public void testBootstrapFromInvalidFile(QuarkusMainLauncher launcher) {
LaunchResult result = launcher.launch("bootstrap", "-f", "/non/existing/file");
assertThat(result.exitCode()).isEqualTo(EXIT_CODE_BOOTSTRAP_ERROR);
assertThat(result.getErrorOutput())
.contains("Failed to read credentials file: file:/non/existing/file")
.contains("Failed to read credentials from file:///non/existing/file")
.contains("Bootstrap encountered errors during operation.");
}

Expand Down