Skip to content

[improvement](s3) Default S3 access to HTTPS #65379

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

Open
0AyanamiRei wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[improvement](s3) Default S3 access to HTTPS #65379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
732e201
[improvement](s3) Default S3 access to HTTPS
0AyanamiRei Jul 8, 2026
f91eea2
[improvement](s3) Normalize scheme-less S3 endpoints to HTTPS
0AyanamiRei Jul 10, 2026
bc79ba6
Merge branch 'master' into improvement/s3-default-https
0AyanamiRei Jul 10, 2026
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 be/src/common/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,7 +1594,7 @@ DEFINE_Validator(paimon_file_system_scheme_mappings,
DEFINE_mInt32(thrift_client_open_num_tries, "1");

// http scheme in S3Client to use. E.g. http or https
DEFINE_String(s3_client_http_scheme, "http");
DEFINE_String(s3_client_http_scheme, "https");
DEFINE_Validator(s3_client_http_scheme, [](const std::string& config) -> bool {
return config == "http" || config == "https";
});
Expand Down
2 changes: 1 addition & 1 deletion cloud/src/common/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ CONF_String(priority_networks, "");
CONF_Bool(enable_cluster_name_check, "false");

// http scheme in S3Client to use. E.g. http or https
CONF_String(s3_client_http_scheme, "http");
CONF_String(s3_client_http_scheme, "https");
CONF_Validator(s3_client_http_scheme, [](const std::string& config) -> bool {
return config == "http" || config == "https";
});
Expand Down
29 changes: 9 additions & 20 deletions fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
package org.apache.doris.catalog;

import org.apache.doris.common.DdlException;
import org.apache.doris.common.credentials.CloudCredentialWithEndpoint;
import org.apache.doris.common.proc.BaseProcResult;
import org.apache.doris.common.util.DatasourcePrintableMap;
import org.apache.doris.common.util.S3Util;
import org.apache.doris.datasource.property.storage.S3Properties;
import org.apache.doris.filesystem.UploadPartResult;
import org.apache.doris.filesystem.spi.ObjFileSystem;
Expand Down Expand Up @@ -103,12 +103,9 @@ protected void setProperties(ImmutableMap<String, String> newProperties) throws
}

// the endpoint for ping need add uri scheme.
String pingEndpoint = properties.get(S3Properties.ENDPOINT);
if (!pingEndpoint.startsWith("http://") && !pingEndpoint.startsWith("https://")) {
pingEndpoint = "http://" + properties.get(S3Properties.ENDPOINT);
properties.put(S3Properties.ENDPOINT, pingEndpoint);
properties.put(S3Properties.Env.ENDPOINT, pingEndpoint);
}
String pingEndpoint = S3Util.buildEndpointUrl(properties.get(S3Properties.ENDPOINT));
properties.put(S3Properties.ENDPOINT, pingEndpoint);
properties.put(S3Properties.Env.ENDPOINT, pingEndpoint);
String region = S3Properties.getRegionOfEndpoint(pingEndpoint);
properties.putIfAbsent(S3Properties.REGION, region);

Expand Down Expand Up @@ -232,6 +229,11 @@ public void modifyProperties(Map<String, String> properties) throws DdlException
}
// compatible with old version, Need convert if modified properties map uses old properties.
S3Properties.convertToStdProperties(properties);
if (properties.containsKey(S3Properties.ENDPOINT)) {
String endpoint = S3Util.buildEndpointUrl(properties.get(S3Properties.ENDPOINT));

@github-actions github-actions Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildEndpointUrl also runs for empty updates after convertToStdProperties, so ALTER RESOURCE ... SET ("AWS_ENDPOINT" = "") (or "s3.endpoint" = "") is no longer ignored by the existing replaceIfEffectiveValue guard. buildEndpointUrl("") returns the non-empty string https://, and with s3_validity_check=false this gets written to both endpoint keys, leaving the resource with an invalid endpoint instead of preserving the old value. Please skip blank endpoint values before normalization, or validate/reject them before they can be persisted.

properties.put(S3Properties.ENDPOINT, endpoint);
properties.put(S3Properties.Env.ENDPOINT, endpoint);
}
boolean needCheck = isNeedCheck(properties);
if (LOG.isDebugEnabled()) {
LOG.debug("s3 info need check validity : {}", needCheck);
Expand Down Expand Up @@ -278,18 +280,6 @@ public void modifyProperties(Map<String, String> properties) throws DdlException
super.modifyProperties(properties);
}

private CloudCredentialWithEndpoint getS3PingCredentials(Map<String, String> properties) {
String ak = properties.getOrDefault(S3Properties.ACCESS_KEY, this.properties.get(S3Properties.ACCESS_KEY));
String sk = properties.getOrDefault(S3Properties.SECRET_KEY, this.properties.get(S3Properties.SECRET_KEY));
String token = properties.getOrDefault(S3Properties.SESSION_TOKEN,
this.properties.get(S3Properties.SESSION_TOKEN));
String endpoint = properties.getOrDefault(S3Properties.ENDPOINT, this.properties.get(S3Properties.ENDPOINT));
String pingEndpoint = "http://" + endpoint;
String region = S3Properties.getRegionOfEndpoint(pingEndpoint);
properties.putIfAbsent(S3Properties.REGION, region);
return new CloudCredentialWithEndpoint(pingEndpoint, region, ak, sk, token);
}

private boolean isNeedCheck(Map<String, String> newProperties) {
boolean needCheck = !this.properties.containsKey(S3Properties.VALIDITY_CHECK)
|| Boolean.parseBoolean(this.properties.get(S3Properties.VALIDITY_CHECK));
Expand Down Expand Up @@ -328,4 +318,3 @@ protected void getProcNodeData(BaseProcResult result) {
readUnlock();
}
}

13 changes: 8 additions & 5 deletions fe/fe-core/src/main/java/org/apache/doris/common/util/S3Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,7 @@ public static String extendGlobNumberRange(String pathPattern) {
public static void validateAndTestEndpoint(String endpoint) throws UserException {
HttpURLConnection connection = null;
try {
String urlStr = endpoint;
// Add default protocol if not specified
if (!endpoint.startsWith("http://") && !endpoint.startsWith("https://")) {
urlStr = "http://" + endpoint;
}
String urlStr = buildEndpointUrl(endpoint);
endpoint = endpoint.replaceFirst("^http://", "");
endpoint = endpoint.replaceFirst("^https://", "");
List<String> whiteList = new ArrayList<>(Arrays.asList(Config.s3_load_endpoint_white_list));
Expand Down Expand Up @@ -434,6 +430,13 @@ public static void validateAndTestEndpoint(String endpoint) throws UserException
}
}

public static String buildEndpointUrl(String endpoint) {
if (endpoint.contains("://")) {
return endpoint;
}
return "https://" + endpoint;

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This only changes the load/TVF endpoint validation path, but other scheme-less object-storage endpoints still keep the old HTTP behavior. For example, S3Resource.setProperties() still rewrites and persists scheme-less resources/vaults as http://..., CreateStageCommand.tryConnect() still validates external stages with http:// + endpoint, DLFCatalog.initializeFileIO() still prepends http:// before building its S3 client, and the S3/Minio catalog connectivity tester passes a scheme-less endpoint straight to URI.create(endpoint). Those paths do not go through this helper, so users of CREATE RESOURCE, S3 storage vaults, external stages, DLF Iceberg catalogs, or catalog connectivity checks can still miss the new HTTPS default after this PR. Please route these parallel paths through the same HTTPS-default normalization while preserving explicit http://, and update the tests that currently pin or miss the old behavior.

}

/**
* Check if a path pattern is deterministic, meaning all file paths can be determined
* without listing. A pattern is deterministic if it contains no true wildcard characters
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void testFeConnection() throws Exception {
String endpoint = properties.getEndpoint();

try (S3Client client = S3Util.buildS3Client(
URI.create(endpoint),
URI.create(S3Util.buildEndpointUrl(endpoint)),
properties.getRegion(),
Boolean.parseBoolean(properties.getUsePathStyle()),
properties.getAwsCredentialsProvider())) {
Expand Down
5 changes: 1 addition & 4 deletions fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/dlf/DLFCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,7 @@ protected FileIO initializeFileIO(Map<String, String> properties, Configuration
boolean isUsePathStyle = Boolean.parseBoolean(ossProperties.getUsePathStyle());
// s3 file io just supports s3-like endpoint
String s3Endpoint = endpoint.replace("oss-" + region, "s3.oss-" + region);
if (!s3Endpoint.contains("://")) {
s3Endpoint = "http://" + s3Endpoint;
}
URI endpointUri = URI.create(s3Endpoint);
URI endpointUri = URI.create(S3Util.buildEndpointUrl(s3Endpoint));
FileIO io = new S3FileIO(() -> S3Util.buildS3Client(endpointUri, region, credential, isUsePathStyle));
io.initialize(properties);
return io;
Expand Down
3 changes: 2 additions & 1 deletion ...-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.doris.common.FeNameFormat;
import org.apache.doris.common.InternalErrorCode;
import org.apache.doris.common.UserException;
import org.apache.doris.common.util.S3Util;
import org.apache.doris.datasource.property.storage.StorageProperties;
import org.apache.doris.filesystem.spi.ObjFileSystem;
import org.apache.doris.fs.FileSystemFactory;
Expand Down Expand Up @@ -145,7 +146,7 @@ private void checkObjectStorageInfo() throws UserException {
private void tryConnect(String endpoint) throws Exception {
HttpURLConnection connection = null;
try {
String urlStr = "http://" + endpoint;
String urlStr = S3Util.buildEndpointUrl(endpoint);
// TODO: Server-Side Request Forgery Check is still need?
URL url = new URL(urlStr);
SecurityChecker.getInstance().startSSRFChecking(urlStr);
Expand Down
4 changes: 3 additions & 1 deletion ...ore/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.apache.doris.cloud.proto.Cloud.ObjectStoreInfoPB.Provider;
import org.apache.doris.cloud.proto.Cloud.StagePB.StageAccessType;
import org.apache.doris.common.util.FileFormatConstants;
import org.apache.doris.common.util.S3Util;
import org.apache.doris.nereids.exceptions.AnalysisException;

import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -155,7 +156,8 @@ public boolean isDryRun() {
* getObjectStoreInfoPB
*/
public ObjectStoreInfoPB getObjectStoreInfoPB() {
ObjectStoreInfoPB.Builder builder = ObjectStoreInfoPB.newBuilder().setEndpoint(properties.get(ENDPOINT))
ObjectStoreInfoPB.Builder builder = ObjectStoreInfoPB.newBuilder()
.setEndpoint(S3Util.buildEndpointUrl(properties.get(ENDPOINT)))
.setRegion(properties.get(REGION)).setBucket(properties.get(BUCKET)).setPrefix(properties.get(PREFIX))
.setProvider(Provider.valueOf(properties.get(PROVIDER).toUpperCase()));
if (getAccessType() == StageAccessType.AKSK) {
Expand Down
37 changes: 34 additions & 3 deletions fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,9 @@ public void testSerialization() throws Exception {
Assert.assertEquals("s3_1", rS3Resource1.getName());
Assert.assertEquals("s3_2", rS3Resource2.getName());

Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ENDPOINT), "http://aaa");
Assert.assertEquals("https://aaa", rS3Resource2.getProperty(S3Properties.ENDPOINT));
Assert.assertEquals("https://aaa",
S3Properties.getObjStoreInfoPB(rS3Resource2.getCopiedProperties()).getEndpoint());
Assert.assertEquals(rS3Resource2.getProperty(S3Properties.REGION), "bbb");
Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ROOT_PATH), "/path/to/root");
Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ACCESS_KEY), "xxx");
Expand Down Expand Up @@ -226,11 +228,24 @@ public void testModifyProperties() throws Exception {
Map<String, String> modify = new HashMap<>();
modify.put("s3.access_key", "aaa");
s3Resource.modifyProperties(modify);

modify.clear();
modify.put(S3Properties.ENDPOINT, "new-endpoint");
s3Resource.modifyProperties(modify);
Assert.assertEquals("https://new-endpoint", s3Resource.getProperty(S3Properties.ENDPOINT));
Assert.assertEquals("https://new-endpoint", s3Resource.getProperty(S3Properties.Env.ENDPOINT));
Assert.assertEquals("https://new-endpoint",
S3Properties.getObjStoreInfoPB(s3Resource.getCopiedProperties()).getEndpoint());

modify.clear();
modify.put(S3Properties.Env.ENDPOINT, "http://other-endpoint");
s3Resource.modifyProperties(modify);
Assert.assertEquals("http://other-endpoint", s3Resource.getProperty(S3Properties.ENDPOINT));
Assert.assertEquals("http://other-endpoint", s3Resource.getProperty(S3Properties.Env.ENDPOINT));
}

@Test
public void testHttpScheme() throws DdlException {
// if https:// is set, it should be replaced with http://
ImmutableMap<String, String> properties = ImmutableMap.of(
"AWS_ENDPOINT", "https://aaa",
"AWS_REGION", "bbb",
Expand All @@ -242,7 +257,23 @@ public void testHttpScheme() throws DdlException {
);
S3Resource s3Resource = new S3Resource("s3_2");
s3Resource.setProperties(properties);
Assert.assertEquals(s3Resource.getProperty(S3Properties.ENDPOINT), "https://aaa");
Assert.assertEquals("https://aaa", s3Resource.getProperty(S3Properties.ENDPOINT));
}

@Test
public void testExplicitHttpScheme() throws DdlException {
ImmutableMap<String, String> properties = ImmutableMap.of(
"AWS_ENDPOINT", "http://aaa",
"AWS_REGION", "bbb",
"AWS_ROOT_PATH", "/path/to/root",
"AWS_ACCESS_KEY", "xxx",
"AWS_SECRET_KEY", "yyy",
"AWS_BUCKET", "test-bucket",
"s3_validity_check", "false"
);
S3Resource s3Resource = new S3Resource("s3_2");
s3Resource.setProperties(properties);
Assert.assertEquals("http://aaa", s3Resource.getProperty(S3Properties.ENDPOINT));
}

@Test
Expand Down
31 changes: 30 additions & 1 deletion fe/fe-core/src/test/java/org/apache/doris/common/util/S3UtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,36 @@

public class S3UtilTest {

@Test
public void testBuildEndpointUrlDefaultsToHttps() {
Assert.assertEquals("https://s3.us-east-1.amazonaws.com",
S3Util.buildEndpointUrl("s3.us-east-1.amazonaws.com"));
}

@Test
public void testBuildEndpointUrlKeepsExplicitHttp() {
Assert.assertEquals("http://127.0.0.1:9000",
S3Util.buildEndpointUrl("http://127.0.0.1:9000"));
}

@Test
public void testBuildEndpointUrlKeepsExplicitHttps() {
Assert.assertEquals("https://s3.us-east-1.amazonaws.com",
S3Util.buildEndpointUrl("https://s3.us-east-1.amazonaws.com"));
}

@Test
public void testBuildEndpointUrlKeepsUppercaseScheme() {
Assert.assertEquals("HTTP://127.0.0.1:9000",
S3Util.buildEndpointUrl("HTTP://127.0.0.1:9000"));
}

@Test
public void testBuildEndpointUrlKeepsExistingScheme() {
Assert.assertEquals("ftp://127.0.0.1:21",
S3Util.buildEndpointUrl("ftp://127.0.0.1:21"));
}

@Test
public void testExtendGlobNumberRange_simpleRange() {
// Test simple range expansion {1..3}
Expand Down Expand Up @@ -457,4 +487,3 @@ public void testExpandBracketPatterns_malformedBracket() {
Assert.assertEquals("file[abc.csv", S3Util.expandBracketPatterns("file[abc.csv"));
}
}

11 changes: 9 additions & 2 deletions ...e/src/test/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,20 @@ public void testAnalyzeStorageProperties() {
CreateStageCommand command3 = getCreateStageCommand(sql);
ObjectStoreInfoPB objectStoreInfoPB = command3.getStageProperties().getObjectStoreInfoPB();

Assertions.assertEquals("cos.ap-beijing.myqcloud.com", objectStoreInfoPB.getEndpoint());
Assertions.assertEquals("https://cos.ap-beijing.myqcloud.com", objectStoreInfoPB.getEndpoint());
Assertions.assertEquals("ap-beijing", objectStoreInfoPB.getRegion());
Assertions.assertEquals("tmp-bucket", objectStoreInfoPB.getBucket());
Assertions.assertEquals("tmp_prefix", objectStoreInfoPB.getPrefix());
Assertions.assertEquals("tmp_ak", objectStoreInfoPB.getAk());
Assertions.assertEquals("tmp_sk", objectStoreInfoPB.getSk());
Assertions.assertEquals(Provider.COS, objectStoreInfoPB.getProvider());

sql = "create stage if not exists ex_stage_1 "
+ OBJ_INFO.replace("cos.ap-beijing.myqcloud.com", "http://cos.ap-beijing.myqcloud.com") + ")";
ObjectStoreInfoPB explicitHttpObjectStoreInfoPB =
getCreateStageCommand(sql).getStageProperties().getObjectStoreInfoPB();
Assertions.assertEquals("http://cos.ap-beijing.myqcloud.com",
explicitHttpObjectStoreInfoPB.getEndpoint());
}

@Test
Expand Down Expand Up @@ -292,7 +299,7 @@ public void testStagePB() throws Exception {
Assertions.assertEquals("ex_stage_1", stagePB.getName());
Assertions.assertTrue(StringUtils.isNotBlank(stagePB.getStageId()));
ObjectStoreInfoPB objInfo = stagePB.getObjInfo();
Assertions.assertEquals("cos.ap-beijing.myqcloud.com", objInfo.getEndpoint());
Assertions.assertEquals("https://cos.ap-beijing.myqcloud.com", objInfo.getEndpoint());
Map<String, String> propertiesMap = stagePB.getPropertiesMap();
Assertions.assertEquals(4, propertiesMap.size());
Assertions.assertEquals("csv", propertiesMap.get("default.file.type"));
Expand Down
Loading