[#41] feat(server): Add https support for Jetty server #860

qqqttt123 · 2023-11-28T09:02:42Z

What changes were proposed in this pull request?

I add a https connector referring to the implement of Apache Livy(incubating).

Why are the changes needed?

Fix: #41

Does this PR introduce any user-facing change?

Yes, I add the documents.

How was this patch tested?

Manual test.

I run the command

# generate the key store
cd $JAVA_HOME
bin/keytool -genkeypair  -alias localhost \
        -keyalg RSA -keysize 4096 -keypass localhost \
        -sigalg SHA256withRSA \
        -keystore localhost.jks -storetype JKS -storepass localhost \
        -dname "cn=localhost,ou=localhost,o=localhost,l=beijing,st=beijing,c=cn" \
        -validity 36500
# generate the certificate
bin/keytool -export -alias localhost -keystore localhost.jks -file  localhost.crt -storepass localhost

# import the certificate
bin/keytool -import -alias localhost -keystore jre/lib/security/cacerts -file localhost.crt -storepass changeit -noprompt

# server's configuration
gravitino.server.webserver.host localhost
gravitino.server.webserver.httpsEnable true
gravitino.server.webserver.keyStorePath ${JAVA_HOME}/localhost.jks (Configuration doesn't support to resolve environment variable, so you should replace ${JAVA_HOME} with the actual value)
gravitino.server.webserver.keyStorePassword localhost
gravitino.server.webserver.managerPassword localhost

# user GravitinoClient to call the version api
String uri = "https://localhost:8443";
GravitinoClient client = GravitinoClient.builder(uri).build();
GravitinoVersion gravitinoVersion = client.getVersion();
# The result is successful.

github-actions · 2023-11-29T09:17:14Z

Code Coverage Report

Overall Project	65.78% `-0.42%`	🟢
Files changed	59.82%	🔴

Module	Coverage
server-common	66.57% `-10.82%`	🔴

Files

Module	File	Coverage
server-common	JettyServerConfig.java	92.69% `-6.36%`	🟢
	JettyServer.java	46.23% `-21.82%`	🔴

yuqi1129 · 2023-11-29T12:35:56Z

docs/security.md

+### Gravitino server's configuration
+| Configuration item                            | Description                                   | Default value | Since version |
+|-----------------------------------------------|-----------------------------------------------|---------------|---------------|
+| `gravitino.server.webserver.httpsEnable`      | Enables https                                 | `false`       | 0.3.0         |


Shouldn't we use https as default?

HTTPS will make deployment more complex. So we don't choose to enable the feature by default. Many other systems made a similar choice.

I would suggest changing to "gravitino.server.webserver.enableHttps"

mchades

Usually, once HTTPS is set up, most services will configure redirection to redirect all HTTP traffic to HTTPS to ensure the security of communication. Should we support this ability?

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java

mchades · 2023-11-30T09:54:05Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java

+          serverConfig.getHost(),
+          serverConfig.getHttpPort(),
+          serverConfig.getHttpsPort());
+    } else {


user can not start services on both https and http at the same time?

Livy chooses http or https. I follow the suggestion. Thanks.

OK, but should we tell the user in the document that if gravitino.server.webserver.httpsPort=true then all configurations about HTTP(such as HTTP port) will be invalidated?

I also have a concern about the convenience and compatibility of migration:

If the user wants to enable HTTPS after running the service on HTTP for a period of time, for your current implementation, they must complete all HTTP client refactoring before switching. It's a big matter for online service

What's your suggestion?

I suggest supporting both HTTP and HTTPS.

mchades · 2023-11-30T09:54:30Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java

+              serverConfig.getHost(),
+              serverConfig.getHttpPort(),
+              serverConfig.getHttpsPort());
+        } else {


qqqttt123 · 2023-11-30T10:10:34Z

Usually, once HTTPS is set up, most services will configure redirection to redirect all HTTP traffic to HTTPS to ensure the security of communication. Should we support this ability?

Could you give me some examples?

mchades · 2023-11-30T12:09:45Z

Usually, once HTTPS is set up, most services will configure redirection to redirect all HTTP traffic to HTTPS to ensure the security of communication. Should we support this ability?

Could you give me some examples?

I'm referring to the web service, such as http://github.com, http://google.com, http://apple.com

diqiu50 · 2023-11-30T12:25:57Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java

+    connector.setReuseAddress(true);
+    return connector;
+  }
+


How to handle exceptions on https initialize. Would it be easy for the user to understand if the original exception is thrown?

What exception should we handle?

diqiu50

Missing integration tests.

qqqttt123 · 2023-11-30T12:35:28Z

Missing integration tests.

Integration tests may be difficult. Some steps need human interaction.

qqqttt123 · 2023-11-30T13:25:00Z

Missing integration tests.

I find I can add the option -no-prompt to skip human interaction. I will have a try to add a integration test.

docs/security.md

FANNG1 · 2023-12-01T07:06:27Z

docs/security.md

+Because HTTPS will protect the header of request from smuggling and HTTPS will be safer.
+Both Gravitino server and Iceberg rest service can configure HTTPS.
+
+### Gravitino server's configuration


Gravitino server and Iceberg REST server have it's configuration doc, I wonder whether it's proper to put security configuration here.

It's ok for us not to add these configuration options in those documents. Spark also follows this document style.

I think you should add more doc example about how to configure https.

qqqttt123 · 2023-12-01T11:28:33Z

Add a warning log if we enable OAuth and HTTP.

qqqttt123 · 2023-12-02T06:46:19Z

Add a warning log if we enable OAuth and HTTP.

Fixed.

qqqttt123 · 2023-12-02T06:48:03Z

For documents, we just add some simple configuration options. I will polish the document in the next pr.

qqqttt123 · 2023-12-02T06:59:14Z

I have addressed the comments. @jerryshao Could you review this pr if you have time?

jerryshao

Have you tested with Gravitino Client and curl. As I remembered there will be some settings in the client side if client side auth is enable.

Also some other configurations like keystore type, trustStore, trustStorePassword, trustStoreType, algorithm probably are also required, can you please check the Spark's code SSLOptions.

jerryshao · 2023-12-04T02:16:42Z

docs/security.md

+### Gravitino server's configuration
+| Configuration item                            | Description                                   | Default value | Since version |
+|-----------------------------------------------|-----------------------------------------------|---------------|---------------|
+| `gravitino.server.webserver.httpsEnable`      | Enables https                                 | `false`       | 0.3.0         |


I would suggest changing to "gravitino.server.webserver.enableHttps"

jerryshao · 2023-12-04T02:19:09Z

docs/security.md

+### Iceberg REST service's configuration
+| Configuration item                                   | Description                                   | Default value | Since version |
+|------------------------------------------------------|-----------------------------------------------|---------------|---------------|
+| `gravitino.auxService.iceberg-rest.httpsEnable`      | Enables https                                 | `false`       | 0.3.0         |


I will fix.

jerryshao · 2023-12-04T02:25:11Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/AuthenticatorUtil.java

+
+  private AuthenticatorUtil() {}
+
+  public static boolean isEnableOAuth2() {


Basically not just oauth2, all the token based auth should be with https, otherwise it has a risk of token leakage.

OK, I got it.

qqqttt123 · 2023-12-04T02:37:52Z

Have you tested with Gravitino Client and curl. As I remembered there will be some settings in the client side if client side auth is enable.

Also some other configurations like keystore type, trustStore, trustStorePassword, trustStoreType, algorithm probably are also required, can you please check the Spark's code SSLOptions.

I have tested by GravitinoClient.

jerryshao · 2023-12-04T02:42:48Z

Have you tested with Gravitino Client and curl. As I remembered there will be some settings in the client side if client side auth is enable.
Also some other configurations like keystore type, trustStore, trustStorePassword, trustStoreType, algorithm probably are also required, can you please check the Spark's code SSLOptions.

I have tested by GravitinoClient.

My point is the second sentence.

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

jerryshao · 2023-12-04T12:09:15Z

docs/security.md

+| `gravitino.server.webserver.keyStorePassword`       | Password to the key store                                  | ``            | 0.3.0         |
+| `gravitino.server.webserver.keyStoreType`           | The type to the key store                                  | `JKS`         | 0.3.0         |
+| `gravitino.server.webserver.managerPassword`        | Manager password to the key store                          | ``            | 0.3.0         |
+| `gravitino.server.webserver.tlsProtocol`            | TLS protocol to use. The protocol must be supported by JVM | none          | 0.3.0         |


How do you configure this? and algorithm, it would be better to add more docs about it.

OK, I will add more description referring the doc of Spark.

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java

jerryshao · 2023-12-04T12:18:51Z

I remembered that you have a sever side check for auth and HTTP, why do you remove that check?

qqqttt123 · 2023-12-04T12:24:22Z

I remembered that you have a sever side check for auth and HTTP, why do you remove that check?

Because we choose simpleAuth by default, the server always use the authentication mechanism, simple or OAuth. The server always faces the risk of token data leak.

jerryshao · 2023-12-04T14:43:02Z

docs/security.md

+
+About `tlsProtocol`, the reference list of protocols can be found in the "Additional JSSE Standard Names" section of the Java security guide. The list for Java 8 can be found at <a href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#jssenames">this</a>.
+About `enableCipherAlgorithms`, the reference list of protocols can be found in the "JSSE Cipher Suite Names" section of the Java security guide. The list for Java 8 can be found at
+<a href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">this</a>


Why do you use html links rather than markdown style?

Spark chooses this style. I will change it to markdown style. Fixed.

Also are you going to add more security doc here in this PR, or you will split into a doc PR?

I will split into a doc PR.

jerryshao · 2023-12-05T02:19:10Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+          .doc("Password to the key store")
+          .version("0.3.0")
+          .stringConf()
+          .createWithDefault("");


I think all these must-setting configurations should have no default empty string as value, right? Using default value here is a bit misleading.

Yes, but they are must-setting configurations, it's weird if we use createWithOptional. It will report a error if we choose to use createWithDefault(null). So I choose to use empty value here.

I see, maybe we need to add a method in ConfigEntry called create to support configurations with no default value. @qqqttt123 can you please create an issue to track this?

Created #950

qqqttt123 marked this pull request as draft November 28, 2023 09:02

qqqttt123 changed the title ~~[#41] server: Add https support for Jetty Server~~ [#41] feat(server): Add https support for Jetty Server Nov 28, 2023

[#41] server: Add https support for Jetty Server

d2455cd

qqqttt123 requested review from jerryshao, FANNG1 and yuqi1129 November 29, 2023 12:16

qqqttt123 marked this pull request as ready for review November 29, 2023 12:16

yuqi1129 reviewed Nov 29, 2023

View reviewed changes

qqqttt123 requested review from xunliu, diqiu50, mchades and Clearvive November 29, 2023 13:12

qqqttt123 changed the title ~~[#41] feat(server): Add https support for Jetty Server~~ [#41] feat(server): Add https support for Jetty server Nov 29, 2023

mchades reviewed Nov 30, 2023

View reviewed changes

Heng Qin added 2 commits November 30, 2023 18:26

Address comments

3244424

fix comments

8db1af3

qqqttt123 closed this Nov 30, 2023

qqqttt123 reopened this Nov 30, 2023

diqiu50 reviewed Nov 30, 2023

View reviewed changes

FANNG1 reviewed Dec 1, 2023

View reviewed changes

docs/security.md Outdated Show resolved Hide resolved

FANNG1 reviewed Dec 1, 2023

View reviewed changes

Address comments

0627f52

jerryshao reviewed Dec 4, 2023

View reviewed changes

Heng Qin added 3 commits December 4, 2023 11:04

fix

a9bc4ff

fix

a5b36c4

fix

25e78fb

qqqttt123 commented Dec 4, 2023

View reviewed changes

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java Show resolved Hide resolved

fix

f2b46f2

qqqttt123 requested a review from jerryshao December 4, 2023 11:27

fix

bbf3760

jerryshao reviewed Dec 4, 2023

View reviewed changes

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServer.java Show resolved Hide resolved

Heng Qin added 2 commits December 4, 2023 20:31

fix

3f083d4

fix

3666dbc

jerryshao reviewed Dec 4, 2023

View reviewed changes

Fix markdown

9dc2780

jerryshao reviewed Dec 5, 2023

View reviewed changes

jerryshao approved these changes Dec 5, 2023

View reviewed changes

jerryshao merged commit 68af8d6 into apache:main Dec 5, 2023
2 checks passed


		private AuthenticatorUtil() {}

		public static boolean isEnableOAuth2() {

[#41] feat(server): Add https support for Jetty server #860

[#41] feat(server): Add https support for Jetty server #860

Conversation

qqqttt123 commented Nov 28, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented Nov 29, 2023 • edited Loading

Code Coverage Report

Choose a reason for hiding this comment

qqqttt123 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Nov 30, 2023

mchades commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diqiu50 left a comment

Choose a reason for hiding this comment

qqqttt123 commented Nov 30, 2023

qqqttt123 commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

qqqttt123 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Dec 1, 2023

qqqttt123 commented Dec 2, 2023

qqqttt123 commented Dec 2, 2023

qqqttt123 commented Dec 2, 2023

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Dec 4, 2023

jerryshao commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Dec 4, 2023

qqqttt123 commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2023 •

edited

Loading

qqqttt123 Nov 29, 2023 •

edited

Loading

qqqttt123 Nov 30, 2023 •

edited

Loading

mchades commented Nov 30, 2023 •

edited

Loading

qqqttt123 commented Nov 30, 2023 •

edited

Loading

qqqttt123 Dec 1, 2023 •

edited

Loading

jerryshao commented Dec 4, 2023 •

edited

Loading

qqqttt123 Dec 5, 2023 •

edited

Loading