Require explicit user-consent to enable HadoopFileIO #1532
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |||||
| */ | ||||||
| package org.apache.polaris.service.quarkus.config; | ||||||
|
|
||||||
| import static java.lang.String.format; | ||||||
|
|
||||||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||||||
| import jakarta.enterprise.context.ApplicationScoped; | ||||||
| import jakarta.enterprise.event.Observes; | ||||||
| import jakarta.enterprise.event.Startup; | ||||||
|
|
@@ -27,12 +30,15 @@ | |||||
| import java.nio.charset.StandardCharsets; | ||||||
| import java.util.ArrayList; | ||||||
| import java.util.List; | ||||||
| import org.apache.polaris.core.config.FeatureConfiguration; | ||||||
| import org.apache.polaris.core.config.ProductionReadinessCheck; | ||||||
| import org.apache.polaris.core.config.ProductionReadinessCheck.Error; | ||||||
| import org.apache.polaris.core.persistence.MetaStoreManagerFactory; | ||||||
| import org.apache.polaris.service.auth.AuthenticationRealmConfiguration.TokenBrokerConfiguration.RSAKeyPairConfiguration; | ||||||
| import org.apache.polaris.service.auth.AuthenticationRealmConfiguration.TokenBrokerConfiguration.SymmetricKeyConfiguration; | ||||||
| import org.apache.polaris.service.auth.AuthenticationType; | ||||||
| import org.apache.polaris.service.catalog.validation.IcebergPropertiesValidation; | ||||||
| import org.apache.polaris.service.config.FeaturesConfiguration; | ||||||
| import org.apache.polaris.service.context.DefaultRealmContextResolver; | ||||||
| import org.apache.polaris.service.context.RealmContextResolver; | ||||||
| import org.apache.polaris.service.context.TestRealmContextResolver; | ||||||
|
|
@@ -44,6 +50,7 @@ | |||||
| import org.eclipse.microprofile.config.ConfigValue; | ||||||
| import org.slf4j.Logger; | ||||||
| import org.slf4j.LoggerFactory; | ||||||
| import org.slf4j.event.Level; | ||||||
|
|
||||||
| @ApplicationScoped | ||||||
| public class ProductionReadinessChecks { | ||||||
|
|
@@ -57,26 +64,53 @@ public class ProductionReadinessChecks { | |||||
| */ | ||||||
| private static final String WARNING_SIGN_UTF_8 = "\u0000\u26A0\uFE0F"; | ||||||
|
|
||||||
| private static final String SEVERE_SIGN_UTF_8 = "\u0000\uD83D\uDED1"; | ||||||
|
There was a problem hiding this comment. These changes seem pretty unnecessary There was a problem hiding this comment. I do think that there are nuances in what's a warning and what's severe. There was a problem hiding this comment. That may be so, but I don't think this change is necessary to There was a problem hiding this comment. @eric-maynard I am not sure what the ask is here. Would you rather have the UTF-8 icon and/or the log message implemented in a separate PR? Or do you mean that the PR title should be a bit more general? There was a problem hiding this comment. Sure, I think a separate PR for these changes could be a good idea. If this PR really is a bugfix, let's not couple unnecessary changes with it. There was a problem hiding this comment. Let's be mindful on the overhead that too fine-grained PR would cause though... The changes in this PR are cohesive to one another. And it is quite small. So it makes sense to submit the changes in a single PR. Splitting a PR into many tiny PRs would prevent reviewers from understanding the big picture and would not have any functional merit. There was a problem hiding this comment. As I think the changes are "related", I think it's fair to do the changes in one PR (even if it's just "display/rendering" messages). There was a problem hiding this comment. It's definitely fair, it's only a question of necessity. If there really is an urgent 1.0-blocking bugfix to be made, must we include these changes here? Are these logging changes really 1.0-blocking? |
||||||
|
|
||||||
| /** A simple warning sign displayed when the character set is not UTF-8. */ | ||||||
| private static final String WARNING_SIGN_PLAIN = "!!!"; | ||||||
|
|
||||||
| private static final String SEVERE_SIGN_PLAIN = "***STOP***"; | ||||||
|
|
||||||
| public void warnOnFailedChecks( | ||||||
| @Observes Startup event, | ||||||
| Instance<ProductionReadinessCheck> checks, | ||||||
| QuarkusReadinessConfiguration config) { | ||||||
| List<Error> errors = checks.stream().flatMap(check -> check.getErrors().stream()).toList(); | ||||||
| if (!errors.isEmpty()) { | ||||||
| var utf8 = Charset.defaultCharset().equals(StandardCharsets.UTF_8); | ||||||
| var warning = utf8 ? WARNING_SIGN_UTF_8 : WARNING_SIGN_PLAIN; | ||||||
| var severe = utf8 ? SEVERE_SIGN_UTF_8 : SEVERE_SIGN_PLAIN; | ||||||
| var hasSevere = errors.stream().anyMatch(Error::severe); | ||||||
| LOGGER | ||||||
| .makeLoggingEventBuilder(hasSevere ? Level.ERROR : Level.WARN) | ||||||
| .log( | ||||||
| "{} Production readiness checks failed! Check the warnings below.", | ||||||
| hasSevere ? severe : warning); | ||||||
| errors.forEach( | ||||||
| error -> | ||||||
| LOGGER | ||||||
| .makeLoggingEventBuilder(error.severe() ? Level.ERROR : Level.WARN) | ||||||
| .log( | ||||||
| "- {} {} Offending configuration option: '{}'.", | ||||||
| error.severe() ? severe : warning, | ||||||
| error.message(), | ||||||
| error.offendingProperty())); | ||||||
| LOGGER | ||||||
| .makeLoggingEventBuilder(hasSevere ? Level.ERROR : Level.WARN) | ||||||
| .log( | ||||||
| "Refer to https://polaris.apache.org/in-dev/unreleased/configuring-polaris-for-production for more information."); | ||||||
|
|
||||||
| if (hasSevere) { | ||||||
| if (!config.ignoreSevereIssues()) { | ||||||
| throw new IllegalStateException( | ||||||
|
There was a problem hiding this comment. nit: the current method is named
Maybe add a |
||||||
| "Severe production readiness issues detected, startup aborted!"); | ||||||
| } | ||||||
| LOGGER.error( | ||||||
| "{} severe production readiness issues detected, but user explicitly requested startup by setting " | ||||||
| + "polaris.readiness.ignore-severe-issues=true and accepts the risk of denial-of-service, " | ||||||
| + "data-loss, corruption and others !", | ||||||
| severe); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -176,4 +210,71 @@ public ProductionReadinessCheck checkPolarisEventListener( | |||||
| private static String authRealmSegment(String realm) { | ||||||
| return realm.equals(QuarkusAuthenticationConfiguration.DEFAULT_REALM_KEY) ? "" : realm + "."; | ||||||
| } | ||||||
|
|
||||||
| @Produces | ||||||
| public ProductionReadinessCheck checkInsecureStorageSettings( | ||||||
| FeaturesConfiguration featureConfiguration) { | ||||||
| var insecure = FeatureConfiguration.ALLOW_INSECURE_STORAGE_TYPES; | ||||||
|
|
||||||
| var errors = new ArrayList<Error>(); | ||||||
| if (Boolean.parseBoolean(featureConfiguration.defaults().get(insecure.key))) { | ||||||
| errors.add( | ||||||
| Error.ofSevere( | ||||||
| "Must not enable a configuration that exposes known and severe security risks: " | ||||||
| + insecure.description, | ||||||
| format("polaris.features.\"%s\"", insecure.key))); | ||||||
| } | ||||||
|
|
||||||
| featureConfiguration | ||||||
| .realmOverrides() | ||||||
| .forEach( | ||||||
| (realmId, overrides) -> { | ||||||
| if (Boolean.parseBoolean(overrides.overrides().get(insecure.key))) { | ||||||
| errors.add( | ||||||
| Error.ofSevere( | ||||||
| "Must not enable a configuration that exposes known and severe security risks: " | ||||||
| + insecure.description, | ||||||
| format( | ||||||
| "polaris.features.realm-overrides.\"%s\".overrides.\"%s\"", | ||||||
| realmId, insecure.key))); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| var storageTypes = FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES; | ||||||
| var mapper = new ObjectMapper(); | ||||||
| var defaults = featureConfiguration.parseDefaults(mapper); | ||||||
| var realmOverrides = featureConfiguration.parseRealmOverrides(mapper); | ||||||
| @SuppressWarnings("unchecked") | ||||||
| var supported = (List<String>) defaults.getOrDefault(storageTypes.key, List.of()); | ||||||
| supported.stream() | ||||||
| .filter(n -> !IcebergPropertiesValidation.safeStorageType(n)) | ||||||
| .forEach( | ||||||
| t -> | ||||||
| errors.add( | ||||||
| Error.ofSevere( | ||||||
| format( | ||||||
| "The storage type '%s' is considered insecure and to expose the service to severe security ricks!", | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| t), | ||||||
| format("polaris.features.\"%s\"", storageTypes.key)))); | ||||||
| realmOverrides.forEach( | ||||||
| (realmId, overrides) -> { | ||||||
| @SuppressWarnings("unchecked") | ||||||
| var s = (List<String>) overrides.getOrDefault(storageTypes.key, List.of()); | ||||||
| s.stream() | ||||||
| .filter(n -> !IcebergPropertiesValidation.safeStorageType(n)) | ||||||
| .forEach( | ||||||
| t -> | ||||||
| errors.add( | ||||||
| Error.ofSevere( | ||||||
| format( | ||||||
| "The storage type '%s' is considered insecure and to expose the service to severe security ricks!", | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| t), | ||||||
| format( | ||||||
| "polaris.features.realm-overrides.\"%s\".overrides.\"%s\"", | ||||||
| realmId, storageTypes.key)))); | ||||||
| }); | ||||||
| return errors.isEmpty() | ||||||
| ? ProductionReadinessCheck.OK | ||||||
| : ProductionReadinessCheck.of(errors.toArray(new Error[0])); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.polaris.service.quarkus.config; | ||
|
|
||
| import io.quarkus.runtime.annotations.StaticInitSafe; | ||
| import io.smallrye.config.ConfigMapping; | ||
| import io.smallrye.config.WithDefault; | ||
|
|
||
| @StaticInitSafe | ||
| @ConfigMapping(prefix = "polaris.readiness") | ||
| public interface QuarkusReadinessConfiguration { | ||
|
|
||
| /** | ||
| * Setting this to {@code true} means that Polaris will start up even if severe security risks | ||
| * have been detected, accepting the risk of denial-of-service, data-loss, corruption and other | ||
| * risks. | ||
| */ | ||
| @WithDefault("false") | ||
| boolean ignoreSevereIssues(); | ||
|
There was a problem hiding this comment. Besides what's already been pointed out about the validity of the FileIO check done here, this is not a good config name. What is a "severe issue"? Does it mean that we don't ignore mild issues? Are there super-severe issues? There was a problem hiding this comment. @eric-maynard "Severe" is a standard term to denote the most important information. See java.util.logging.Level for an example of how it is used in the context of logs. There was a problem hiding this comment. Having a log level of SEVERE and a config called "ignore severe issues" are completely different things. Is the implication that this config would disable failure in all cases where we use the SEVERE logging level? Even if that is the case, my questions stand. Does this config not apply to less-severe issues? What guidelines will qualify an issue as severe (necessitating a SEVERE log)? Have we gone back and audited all "issues" to verify none should be considered SEVERE? What even is an "issue"? There was a problem hiding this comment. @eric-maynard I don't know how to answer your questions. They seem related to a much bigger scope than the scope of this PR. Especially the audit part. We could extend the logic that in subsequent PRs, so that more cases are covered, if it is what you have in mind. I guess my question to you is : how can we move this PR forward? There was a problem hiding this comment. I am not sure it lays out a path, that's the thing. What value would be created out of splitting this PR into smaller PRs? It could also possibly be a topic for a ML discussion if there are PR guidelines that are not explicitly written in There was a problem hiding this comment. We should aim for small independent code changes whenever possible. Just because two things work well together doesn't mean they should be in the same Pull request. As a generic guide, 1 "feature" per PR, small is better than bigger, independent changes should stay independent. IMHO, if the title can't cover the scope of the changes then the PR should be broken up. I think we have several independent things here as noted in the PR description. Ideally for me even refactors go in separate pull-requests. We've been a bit lax on this recently as we have adopted code from other projects but I don't think we should get in the habit. The general benefits of this approach are for downstream integrators which can cherry-pick changes easier, and reviewers will have an easier time since net small changes are much easier to understand and usually easier to come to agreement on. (Bisecting also becomes much easier). If PR's can only be understood of a larger context, then grouping them in a project is usually the right thing to do or explaining the context in a design doc. I think this pr itself does a great job of mapping out the independent changes in a bulleted list. IMHO, each of those points could probably approached independently since I think we already have consensus on at least some of the Items. There was a problem hiding this comment. It's not about PR guidelines -- there are a bunch of issues with the code changes proposed in this PR that I've highlighted in various comments. Even the questions asked in this thread are not resolved. I still don't know what a "severe issue" is. Breaking up the change could allow us to move forward with some of the less controversial aspects of the change, but if you're opposed to that I'm also happy to continue hashing out these issues here. There was a problem hiding this comment. If I generally agree with the last comments, specifically in this PR, I consider the changes "related": several/rendering messages is related to clearly inform the users about allowing insecure file types. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure when my comment got dropped but this still looks redundant with the existing config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I informs users who may not be aware of code-level intricacies that certain storage types are considered insecure. Without it, a user may naively assume that adding something to the list of storage types at run time is "good enough". That may be true for some types, but not for others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can probably improve the granularity of this check, but I'd suggest to do that in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me,
ALLOW_INSECURE_STORAGE_TYPES = falseseems to imply that Polaris is somehow guaranteeing that no insecure File IO implementations are allowed, which I don't think is something the service can guarantee after this change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a user facing property, we need to be consistent (hard to change later).
I think this property is good enough as it's generic to any storage type (not only
FILE).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that true? What qualifies a storage type as insecure?
If the answer is "the storage type can use HadoopFileIO", then all 4 storage types are insecure.