You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dimas-b (via GitHub)" <gi...@apache.org> on 2025/09/29 17:34:30 UTC
[PR] Enforce that credentials are vended when requested [polaris]
dimas-b opened a new pull request, #2711:
URL: https://github.com/apache/polaris/pull/2711
This is a follow-up change to #2672 and #2589
* Fix a big in `delegationModes` parameter propagation in `createTableStaged()`
* Add checks (leading to 400) that credentials are vended when requested.
* The check is disabled if `SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION` is set
* Disable credential vending tests in file-based catalog since the "FILE" storage integration code never vends any credentials. These tests are still executed under `PolarisRestCatalogMinIOIT`
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392075495
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
AWS S3 may be used without STS (user's choice).
How about `This can happen with FILE and S3 storage if STS is not available or disabled (not permitted).`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395885700
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
I think this error will be wrapped in to an error SubScopedResult and finally be thrown as `UnprocessableEntityException` in the `StorageCredentialCache` line 155. Just check if this is intended
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -69,6 +73,7 @@ static StorageCredentialCacheKey of(
allowedListAction,
allowedReadLocations,
allowedWriteLocations,
- refreshCredentialsEndpoint);
+ refreshCredentialsEndpoint,
+ credentialsRequired);
Review Comment:
[non-blocker] I think this implies some extra cache miss for azure and gcp where `credentialsRequired` does not make any difference.
Do we have some general use-case of the `credentialsRequired` on gcp and azure, such that we could unify the credential existence check at `storageCredentialCache`, such that the persistence/credentialVendor just do its best effort to generate an access config, and let the cache api to decide whether the result is valid?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2403468798
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,13 +804,22 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
- responseBuilder.addAllConfig(credentialConfig);
- responseBuilder.addCredential(
- ImmutableCredential.builder()
- .prefix(tableMetadata.location())
- .config(credentialConfig)
- .build());
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (!credentialConfig.isEmpty()) {
+ responseBuilder.addAllConfig(credentialConfig);
+ responseBuilder.addCredential(
+ ImmutableCredential.builder()
+ .prefix(tableMetadata.location())
+ .config(credentialConfig)
+ .build());
+ } else {
+ Boolean skipCredIndirection =
+ realmConfig.getConfig(FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION);
+ Preconditions.checkArgument(
+ !accessConfig.supportsCredentialVending() || skipCredIndirection,
+ "Credential vending was requested for table %s, but no credentials are available",
+ tableIdentifier);
Review Comment:
Sounds reasonable to me.
TBH, I do not like `AccessConfig.supportsCredentialVending()`, but it seemed to be the simplest way to make the check without breaking other things :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2403085055
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,13 +804,22 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
- responseBuilder.addAllConfig(credentialConfig);
- responseBuilder.addCredential(
- ImmutableCredential.builder()
- .prefix(tableMetadata.location())
- .config(credentialConfig)
- .build());
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (!credentialConfig.isEmpty()) {
+ responseBuilder.addAllConfig(credentialConfig);
+ responseBuilder.addCredential(
+ ImmutableCredential.builder()
+ .prefix(tableMetadata.location())
+ .config(credentialConfig)
+ .build());
+ } else {
+ Boolean skipCredIndirection =
+ realmConfig.getConfig(FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION);
+ Preconditions.checkArgument(
+ !accessConfig.supportsCredentialVending() || skipCredIndirection,
+ "Credential vending was requested for table %s, but no credentials are available",
+ tableIdentifier);
Review Comment:
WDYT of moving this check to `getAccessConfig` /`FileIOUtil.refreshAccessConfig` in the later refactoring PR: https://github.com/apache/polaris/pull/2736, and we probably do not need the additional `supportsCredentialVending` field in AccessConfig, just adding an additional arg to let caller decide whether to require the credential to be vended.
But I think that should be in a follow-up and after the 1.2.0 cut to avoid additional noise on the release. If I understand correctly, we do not offer backward compatibility of these inner interface. So we could do this if needed in the next release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2393292992
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
A changelog entry is not needed for the reworked changeset... WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389589911
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
We should put the check within ` if (shouldUseSts(storageConfig)) { ... }`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392075495
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
AWS S3 may be used without STS (admin's choice).
How about `This can happen with FILE and S3 storage if STS is not available or disabled (not permitted).`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395921412
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -69,6 +73,7 @@ static StorageCredentialCacheKey of(
allowedListAction,
allowedReadLocations,
allowedWriteLocations,
- refreshCredentialsEndpoint);
+ refreshCredentialsEndpoint,
+ credentialsRequired);
Review Comment:
Good point about cache :thinking: Thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2396430234
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
I'm reworking the impl. again... it will be 400 / `IllegalArgumentException` at the REST level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2396270160
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
Sorry to clarify, I think this is ok since 422 implies the same error will occur again if client retries, which is the current case. But in the PR description
> Add checks to AwsCredentialsStorageIntegration (leading to 400) that S3 storage credentials are vended when requested.
It says this check will lead to 400. So just point out in case it is intended to be 400 : )
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
Sorry to clarify, I think this is ok since 422 implies the same error will occur again if client retries, which is the current case. But in the PR description
> Add checks to AwsCredentialsStorageIntegration (leading to 400) that S3 storage credentials are vended when requested.
says this check will lead to 400. So just point out in case it is intended to be 400 : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2393292269
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogFileIntegrationTest.java:
##########
@@ -51,4 +53,18 @@ private static String stripTrailingSlash(String uri) {
? uri.substring(0, uri.length() - 1)
: uri;
}
+
+ @Test
+ @Override
+ @Disabled // Polaris does not provide credentials for FILE storage
+ public void testLoadCredentials() {
+ super.testLoadCredentials();
+ }
Review Comment:
This is no longer disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389829679
##########
client/python/integration_tests/conftest.py:
##########
@@ -276,7 +282,6 @@ def test_pyiceberg_catalog(
"warehouse": test_catalog.name,
"credential": f"{test_principal.principal.client_id}:{test_principal.credentials.client_secret.get_secret_value()}",
"scope": "PRINCIPAL_ROLE:ALL",
- "header.X-Iceberg-Access-Delegation": "vended-credentials",
Review Comment:
Cf. https://github.com/apache/iceberg-python/blob/pyiceberg-0.10.0/pyiceberg/catalog/rest/__init__.py#L487
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389585398
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
`AwsCredentialsStorageIntegration.getSubscopedCreds()` may be called when STS is disabled (so no credentials will be vended). It's a valid use case for non-AWS S3 storage. Clients still need non-credential config from the "storage integration" code (e.g. endpoints).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392082152
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
SG ! for my understanding, when is the case when its actual S3 but we don't credential vending ? is it for case where the client already has credentials to work on storage, but then would they sending the header, or we just want admins to have this knob
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392075495
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
AWS S3 may be used without STS (user's choice).
How about `This can happen with FILE and S3 storage if STS is not available or disabled (not permitted).`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395930146
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
On the java client side it shows up as `org.apache.iceberg.exceptions.RESTException: Unable to process: Failed to get subscoped credentials: Credential vending was requested, but STS is not available`. Is that reasonable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389579477
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
Thanks @dimas-b for the change. But I think this is not the right place to check. `SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION` is mainly for POC or test env, where we allow Polaris to skip credential vending no matter what clients ask.
I believe the right place to check is in `AwsCredentialsStorageIntegration::getSubscopedCreds()`. Clients ask for credential vending, but this storage doesn't have the capability to do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2396430234
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
Good catch - thx! I'm reworking the impl. again... it will be 400 / `IllegalArgumentException` at the REST level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2402652975
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -108,6 +108,14 @@ public abstract AccessConfig getSubscopedCreds(
@Nonnull Set<PolarisStorageActions> actions,
@Nonnull Set<String> locations);
+ /**
+ * @param credentialsRequired if {@code true} indicates that the caller requires the returned
+ * {@link AccessConfig} to have storage credentials; if {@code false} the returned {@link
+ * AccessConfig} may or may not contain credentials.
+ */
+ public void validateCredentials(
+ @Nonnull AccessConfig accessConfig, boolean credentialsRequired) {}
Review Comment:
Thanks for catching this and sorry about leftovers from previous code revisions :sweat: ... removed.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -2093,6 +2093,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant(
PolarisStorageIntegration<PolarisStorageConfigurationInfo> storageIntegration =
ms.loadPolarisStorageIntegrationInCurrentTxn(callCtx, reloadedEntity.getEntity());
+ storageIntegration.config();
Review Comment:
removed
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -40,7 +40,7 @@ public PolarisStorageIntegration(T config, String identifierOrId) {
this.integrationIdentifierOrId = identifierOrId;
}
- protected T config() {
Review Comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #2711:
URL: https://github.com/apache/polaris/pull/2711#issuecomment-3358809239
I revamped this PR again :sweat_smile: Since there were too many "undo" changes, I squashed and force-pushed. Sorry about losing change history, but I think this revision is has the smallest "diff" so far :wink: Please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #2711:
URL: https://github.com/apache/polaris/pull/2711#issuecomment-3354428045
Reworked this PR completely. Please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395937598
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
In the REST response it is indeed a 422 with type = `UnprocessableEntityException`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #2711:
URL: https://github.com/apache/polaris/pull/2711#issuecomment-3353238721
It looks like the ripple effects of validating the vended credentials are available at the REST API levels are too wide.
I'll reset this PR and take a different approach (as @flyrain suggested).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2393293260
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
Updated. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2393291838
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException {
}
}
+ @Test
+ public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException {
Review Comment:
`ALLOW_EMPTY_VENDED_CREDENTIALS` is gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2403468798
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,13 +804,22 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
- responseBuilder.addAllConfig(credentialConfig);
- responseBuilder.addCredential(
- ImmutableCredential.builder()
- .prefix(tableMetadata.location())
- .config(credentialConfig)
- .build());
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (!credentialConfig.isEmpty()) {
+ responseBuilder.addAllConfig(credentialConfig);
+ responseBuilder.addCredential(
+ ImmutableCredential.builder()
+ .prefix(tableMetadata.location())
+ .config(credentialConfig)
+ .build());
+ } else {
+ Boolean skipCredIndirection =
+ realmConfig.getConfig(FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION);
+ Preconditions.checkArgument(
+ !accessConfig.supportsCredentialVending() || skipCredIndirection,
+ "Credential vending was requested for table %s, but no credentials are available",
+ tableIdentifier);
Review Comment:
Sounds reasonable to me.
TBH, I do not like `AccessConfig.supportsCredentialVending()`, but it seemed to be the simplest way to make the check without breaking other thinks :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #2711:
URL: https://github.com/apache/polaris/pull/2711
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395949330
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -69,6 +73,7 @@ static StorageCredentialCacheKey of(
allowedListAction,
allowedReadLocations,
allowedWriteLocations,
- refreshCredentialsEndpoint);
+ refreshCredentialsEndpoint,
+ credentialsRequired);
Review Comment:
> Do we have some general use-case of the credentialsRequired on gcp and azure
My first attempts in this PR was do enforce credential presence close to the REST level, but it proved to have too wide a ripple effect because many use cases (including PyIceberg) request credential vending by default and that breaks on FILE storage.
I'll see if I can refactor the code to maintain cache validity, but still delegate config validation to storage integrations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389868245
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
```suggestion
no credentials). This can happen with FILE and S3 compatible storage that does not have STS. Clients should normally be updated
```
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogFileIntegrationTest.java:
##########
@@ -51,4 +53,18 @@ private static String stripTrailingSlash(String uri) {
? uri.substring(0, uri.length() - 1)
: uri;
}
+
+ @Test
+ @Override
+ @Disabled // Polaris does not provide credentials for FILE storage
+ public void testLoadCredentials() {
+ super.testLoadCredentials();
+ }
Review Comment:
should we run these test with the config to allow empty creds on ?
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException {
}
}
+ @Test
+ public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException {
Review Comment:
[optional] is there a way to run these test without relying on the defaults ? just incase we plan defaults this would break.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2396464376
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -70,19 +71,25 @@ public AwsCredentialsStorageIntegration(
/** {@inheritDoc} */
@Override
- public AccessConfig getSubscopedCreds(
+ public AccessConfig getAccessConfig(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ boolean credentialsRequired) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
AccessConfig.Builder accessConfig = AccessConfig.builder();
- if (shouldUseSts(storageConfig)) {
+ boolean shouldUseSts = shouldUseSts(storageConfig);
+ Preconditions.checkArgument(
Review Comment:
Re-implemented. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395921412
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -69,6 +73,7 @@ static StorageCredentialCacheKey of(
allowedListAction,
allowedReadLocations,
allowedWriteLocations,
- refreshCredentialsEndpoint);
+ refreshCredentialsEndpoint,
+ credentialsRequired);
Review Comment:
Good point about cache :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392089139
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException {
}
}
+ @Test
+ public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException {
Review Comment:
It's not about config defaults in this case, it's about MinIO being the only backend available in OSS now that can operate without STS so we can only test vended credential enforcement on MinIO.
The `ALLOW_EMPTY_VENDED_CREDENTIALS` setting is really for users to restore old Polaris behaviour if the enforcement is problematic for them.
If we change the `ALLOW_EMPTY_VENDED_CREDENTIALS` default, this test will have to override that config locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392089139
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException {
}
}
+ @Test
+ public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException {
Review Comment:
It's not about config defaults in this case, it's about MinIO being the only backend available in OSS now that can operate without STS so we can only test vended credential enforcement on MinIO.
The `ALLOW_EMPTY_VENDED_CREDENTIALS` setting is really for users to restore old Polaris behaviour if the enforcement is problematic for them.
If we change the `ALLOW_EMPTY_VENDED_CREDENTIALS` default, this test will have to re-enable it locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392308516
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -301,6 +302,62 @@ public void testInternalEndpoints() throws IOException {
}
}
+ @Test
+ public void testCreateTableFailureWithCredentialVendingWithoutSts() throws IOException {
Review Comment:
I'll set this in the test explicitly to make it independent of defaults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2400019423
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -40,7 +40,7 @@ public PolarisStorageIntegration(T config, String identifierOrId) {
this.integrationIdentifierOrId = identifierOrId;
}
- protected T config() {
Review Comment:
Seems to be unrelated change?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -108,6 +108,14 @@ public abstract AccessConfig getSubscopedCreds(
@Nonnull Set<PolarisStorageActions> actions,
@Nonnull Set<String> locations);
+ /**
+ * @param credentialsRequired if {@code true} indicates that the caller requires the returned
+ * {@link AccessConfig} to have storage credentials; if {@code false} the returned {@link
+ * AccessConfig} may or may not contain credentials.
+ */
+ public void validateCredentials(
+ @Nonnull AccessConfig accessConfig, boolean credentialsRequired) {}
Review Comment:
Seems this is not used?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -2093,6 +2093,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant(
PolarisStorageIntegration<PolarisStorageConfigurationInfo> storageIntegration =
ms.loadPolarisStorageIntegrationInCurrentTxn(callCtx, reloadedEntity.getEntity());
+ storageIntegration.config();
Review Comment:
I think we could remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that S3 credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2395949330
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -69,6 +73,7 @@ static StorageCredentialCacheKey of(
allowedListAction,
allowedReadLocations,
allowedWriteLocations,
- refreshCredentialsEndpoint);
+ refreshCredentialsEndpoint,
+ credentialsRequired);
Review Comment:
> Do we have some general use-case of the credentialsRequired on gcp and azure
My first attempt in this PR was do enforce credential presence close to the REST level, but it proved to have too wide a ripple effect because many use cases (including PyIceberg) request credential vending by default and that breaks on FILE storage.
I'll see if I can refactor the code to maintain cache validity, but still delegate config validation to storage integrations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389585398
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
`AwsCredentialsStorageIntegration.getSubscopedCreds()` may be called when STS is disable (so no credentials will be vended). It's a valid use case for non-AWS S3 storage. Clients still need non-credential config from the "storage integration" code (e.g. endpoints).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389589911
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
We should put the check within ` if (shouldUseSts(storageConfig)) { ... }`, sorry, the method `shouldUseSts()` itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2389659628
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -804,7 +804,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
credentialDelegation.getAccessConfig(
tableIdentifier, tableMetadata, actions, refreshCredentialsEndpoint);
Map<String, String> credentialConfig = accessConfig.credentials();
- if (!credentialConfig.isEmpty() && delegationModes.contains(VENDED_CREDENTIALS)) {
+ if (delegationModes.contains(VENDED_CREDENTIALS)) {
Review Comment:
@flyrain : could you propose pseudo-code (or java example) for this check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392082152
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
SG ! for my understanding, when is the case when its actual S3 but we don't credential vending ? is it for case where the client already has credentials to work on storage, but then would they be sending the header ? or we just want admins to have this knob
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enforce that credentials are vended when requested [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2711:
URL: https://github.com/apache/polaris/pull/2711#discussion_r2392407912
##########
CHANGELOG.md:
##########
@@ -34,6 +34,11 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Amazon RDS plugin enabled, this allows polaris to connect to AWS Aurora PostgreSQL using IAM authentication.
- The EclipseLink Persistence implementation has been deprecated since 1.0.0 and will be completely removed
in 1.3.0 or in 2.0.0 (whichever happens earlier).
+- Client requests for vended credentials will fail by default if no credentials are available (they used to return
+ no credentials). This can happen with FILE and S3 storage that does not have STS. Clients should normally be updated
Review Comment:
Admins sometimes put restrictions on various policies / permissions even in AWS where STS endpoints are available.
Clients should not request vended credentials when they are known to be unavailable. The extra check is basically to make this kind of error more visible, I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org