Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1190] feat(server-common): Support custom filters #1191

Merged
merged 9 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ protected void configure() {

Servlet servlet = new ServletContainer(config);
server.addServlet(servlet, "/iceberg/*");
server.addCustomFilters("/iceberg/*");
server.addFilter(new AuthenticationFilter(), "/iceberg/*");
}

Expand Down
48 changes: 48 additions & 0 deletions docs/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,51 @@ If you want to use the command `curl`, you can follow the commands:
openssl x509 -inform der -in $JAVA_HOME/localhost.crt -out certificate.pem
curl -v -X GET --cacert ./certificate.pem -H "Accept: application/vnd.gravitino.v1+json" -H "Content-Type: application/json" https://localhost:8433/api/version
```

## Custom filters
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to move doc to GravitinoServer Configuration because Custom filters is general and not limited to safety

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to move this part to server configurations, not here. Since user can configure other filters like redirect, it is not purely a security related feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.


Gravitino supports custom filters to implement the user specified logic to satisfy different safety needs.

### Gravitino server's configuration
| Configuration item | Description | Default value | Required | Since version |
|---------------------------------------------|------------------------------------------------------------------|---------------|-----------|---------------|
| `gravitino.server.webserver.customFilters` | Comma separated list of filter class names to apply to the APIs. | `` | Yes | 0.4.0 |
The filter should be a standard javax servlet Filter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an empty line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Filter parameters can also be specified in the configuration, by setting config entries of the form `gravitino.server.webserver.<class name of filter>.param.<param name>=<value>`

For example:
```text
gravitino.server.webserver.customFilters=com.test.filter1
gravitino.server.webserver.com.test.filter1.param.name1=foo
gravitino.server.webserver.com.test.filter1.param.name2=ba
```

### Iceberg REST service's configuration
| Configuration item | Description | Default value | Required | Since version |
|-----------------------------------------------------|--------------------------------------------------------------------|---------------|-----------|---------------|
| `gravitino.auxService.iceberg-rest.customFilters` | Comma separated list of filter class names to apply to the APIs. | `` | Yes | 0.4.0 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default value `` and it's a required value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Require value means that the value can't be null. It's ok to use `` as the value. It means that we don't use any filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same question, Required means user must fillup this field or the program couldn't startup as I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required means that this field can't be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the same question, Required means user must fillup this field or the program couldn't startup as I understand.

According to your standard, gravitino.auxService.iceberg-rest.catalog-backend isn't a required field. But you choose it as a required field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Following the previous document specification, we use "(none)" to indicate no default value.

JDBC catalog:
image

Hive catalog:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see other documents follow this standard. I will change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1214 I raised another pr to fix the comments in other places.
If this config option has default value, this config option is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

The filter should be a standard javax servlet Filter.
Filter parameters can also be specified in the configuration, by setting config entries of the form `gravitino.auxService.iceberg-rest.<class name of filter>.param.<param name>=<value>`
Copy link
Contributor

Choose a reason for hiding this comment

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

The key seems to be too long, can we shorten it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refer to Spark's way. I have no better idea.


For example:
```text
gravitino.auxService.iceberg-rest.customFilters=com.test.filter1
gravitino.auxService.iceberg-rest.com.test.filter1.param.name1=foo
gravitino.auxService.iceberg-rest.com.test.filter1.param.name2=ba
```

### Example

If you want to use a cross-origin filter for the Gravitino server, you can follow the steps.

```shell
wget https://repo1.maven.org/maven2/org/eclipse/jetty/jetty-servlets/9.4.51.v20230217/jetty-servlets-9.4.51.v20230217.jar
cp jetty-servlets-9.4.51.v20230217.jar <gravitno home>/libs
Copy link
Contributor

Choose a reason for hiding this comment

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

gravitno home -> gravitino home

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

```

You can refer to the [Configurations](gravitino-server-config.md) and append the configurations to the conf/gravitino.conf.

```text
gravitino.server.webserver.customFilters=org.eclipse.jetty.servlets.CrossOriginFilter
gravitino.server.webserver.org.eclipse.jetty.servlets.CrossOriginFilter.param.allowedOrigins=*
```
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.EnumSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;
Expand Down Expand Up @@ -104,7 +105,7 @@ public synchronized void initialize(
StringUtils.isNotBlank(serverConfig.getTrustStorePath()),
"If enables the authentication of the client, must set trustStorePath");
Preconditions.checkArgument(
StringUtils.isNotBlank(serverConfig.getTrustStorePasword()),
StringUtils.isNotBlank(serverConfig.getTrustStorePassword()),
"If enables the authentication of the client, must set trustStorePassword");
}
ServerConnector httpsConnector =
Expand All @@ -123,7 +124,7 @@ public synchronized void initialize(
serverConfig.getSupportedAlgorithms(),
serverConfig.isEnableClientAuth(),
serverConfig.getTrustStorePath(),
serverConfig.getTrustStorePasword(),
serverConfig.getTrustStorePassword(),
serverConfig.getTrustStoreType());
server.addConnector(httpsConnector);
} else {
Expand Down Expand Up @@ -433,4 +434,19 @@ public Thread run() {
public ThreadPool getThreadPool() {
return server.getThreadPool();
}

public void addCustomFilters(String pathSpec) {
for (String filterName : serverConfig.getCustomFilters()) {
if (StringUtils.isBlank(filterName)) {
continue;
}
FilterHolder filterHolder = new FilterHolder();
filterHolder.setClassName(filterName);
for (Map.Entry<String, String> entry :
serverConfig.getAllWithPrefix(String.format("%s.param.", filterName)).entrySet()) {
filterHolder.setInitParameter(entry.getKey(), entry.getValue());
}
servletContextHandler.addFilter(filterHolder, pathSpec, EnumSet.allOf(DispatcherType.class));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,13 @@ public final class JettyServerConfig {
.stringConf()
.createWithDefault("JKS");

public static final ConfigEntry<String> CUSTOM_FILTERS =
new ConfigBuilder("customFilters")
.doc("Comma separated list of filter class names to apply to the APIs")
.version("0.4.0")
.stringConf()
.createWithDefault("");

private final String host;

private final int httpPort;
Expand Down Expand Up @@ -199,7 +206,9 @@ public final class JettyServerConfig {
private final Set<String> enableCipherAlgorithms;
private final boolean enableClientAuth;
private final String trustStorePath;
private final String trustStorePasword;
private final String trustStorePassword;

private final Set<String> customFilters;
private final String trustStoreType;
private final Config internalConfig;

Expand Down Expand Up @@ -245,8 +254,11 @@ private JettyServerConfig(Map<String, String> configs) {
Sets.newHashSet(internalConfig.get(ENABLE_CIPHER_ALGORITHMS).split(SPLITTER)));
this.enableClientAuth = internalConfig.get(ENABLE_CLIENT_AUTH);
this.trustStorePath = internalConfig.get(SSL_TRUST_STORE_PATH);
this.trustStorePasword = internalConfig.get(SSL_TRUST_STORE_PASSWORD);
this.trustStorePassword = internalConfig.get(SSL_TRUST_STORE_PASSWORD);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An unrelated typo. Just fix by the way.

this.trustStoreType = internalConfig.get(SSL_TRUST_STORE_TYPE);
this.customFilters =
Collections.unmodifiableSet(
Sets.newHashSet(internalConfig.get(CUSTOM_FILTERS).split(SPLITTER)));
}

public static JettyServerConfig fromConfig(Config config, String prefix) {
Expand Down Expand Up @@ -330,14 +342,18 @@ public String getTrustStorePath() {
return trustStorePath;
}

public String getTrustStorePasword() {
return trustStorePasword;
public String getTrustStorePassword() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An unrelated typo. Just fix by the way.

return trustStorePassword;
}

public String getTrustStoreType() {
return trustStoreType;
}

public Map<String, String> getAllWithPrefix(String prefix) {
return internalConfig.getConfigsWithPrefix(prefix);
}

private SSLContext getDefaultSSLContext() {
try {
return SSLContext.getDefault();
Expand Down Expand Up @@ -366,6 +382,10 @@ public Set<String> getSupportedAlgorithms() {
return supportedAlgorithms;
}

public Set<String> getCustomFilters() {
return customFilters;
}

@VisibleForTesting
Set<String> getSupportedCipherSuites() {
SSLContext context =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ protected void configure() {
server.addServlet(servlet, "/api/*");
Servlet configServlet = new ConfigServlet(serverConfig);
server.addServlet(configServlet, "/configs");
server.addCustomFilters("/api/*");
server.addFilter(new VersioningFilter(), "/api/*");
server.addFilter(new AuthenticationFilter(), "/api/*");
}
Expand Down
Loading