You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/03/24 16:52:57 UTC
[GitHub] [zookeeper] nkalmar opened a new pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
nkalmar opened a new pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652
Initial commit to allow adminServer to only serve HTTPS requests
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#discussion_r600686645
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java
##########
@@ -140,10 +143,16 @@ public JettyAdminServer(
sslContextFactory.setTrustStore(trustStore);
sslContextFactory.setTrustStorePassword(certAuthPassword);
- connector = new ServerConnector(
- server,
- new UnifiedConnectionFactory(sslContextFactory, HttpVersion.fromVersion(httpVersion).asString()),
- new HttpConnectionFactory(config));
+ if(forceHTTPS) {
Review comment:
you have a checkstyle violation here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#discussion_r601169559
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/admin/JettyAdminServerTest.java
##########
@@ -234,6 +236,35 @@ public void testQuorum() throws Exception {
"waiting for server 2 down");
}
+ @Test
+ public void testForceHttpsPortUnificationEnabled() throws Exception {
+ testForceHttps(true);
+ }
+
+ @Test
+ public void testForceHttpsPortUnificationDisabled() throws Exception {
+ testForceHttps(false);
+ }
+
+ private void testForceHttps(boolean portUnification) throws Exception {
+ System.setProperty("zookeeper.admin.forceHTTPS", "true");
+ System.setProperty("zookeeper.admin.portUnification", String.valueOf(portUnification));
+ boolean httpsPassed = false;
+
+ JettyAdminServer server = new JettyAdminServer();
+ try {
+ server.start();
+ queryAdminServer(String.format(HTTPS_URL_FORMAT, jettyAdminPort), true);
+ httpsPassed = true;
+ queryAdminServer(String.format(URL_FORMAT, jettyAdminPort), false);
Review comment:
Thanks, true that, I had that fail but got lost after the refactor! Good catch (I even had a spotbugs failure of unused import of Assert.fail, and I didn't realize my mistake, removed the import instead. duh...)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#discussion_r601136349
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/admin/JettyAdminServerTest.java
##########
@@ -234,6 +236,35 @@ public void testQuorum() throws Exception {
"waiting for server 2 down");
}
+ @Test
+ public void testForceHttpsPortUnificationEnabled() throws Exception {
+ testForceHttps(true);
+ }
+
+ @Test
+ public void testForceHttpsPortUnificationDisabled() throws Exception {
+ testForceHttps(false);
+ }
+
+ private void testForceHttps(boolean portUnification) throws Exception {
+ System.setProperty("zookeeper.admin.forceHTTPS", "true");
+ System.setProperty("zookeeper.admin.portUnification", String.valueOf(portUnification));
+ boolean httpsPassed = false;
+
+ JettyAdminServer server = new JettyAdminServer();
+ try {
+ server.start();
+ queryAdminServer(String.format(HTTPS_URL_FORMAT, jettyAdminPort), true);
+ httpsPassed = true;
+ queryAdminServer(String.format(URL_FORMAT, jettyAdminPort), false);
Review comment:
I think currently the test will pass always, even if the http call succeeds.
I think you should add a line here (after the http call), like:
`fail("http call should have been failed when forceHTTPS=true");`
to actually assert that the http call fails, the https is enforced and we jumped to the `catch` block.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar edited a comment on pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar edited a comment on pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#issuecomment-806548198
Thank you @ztzg , about 3.6... I did the cherry-pick and accidentally committed to apache 3.6 branch instead of mine, so, consider it done I guess. Sorry about that... I will see that the commit did not break anything.
Edit: commit seems good, spotbugs fails but from a different commit (quorumpeertest has a whitespace after a function start "(". It is not caused by my commit.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg closed pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
ztzg closed pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#issuecomment-812854781
> Thank you @ztzg , about 3.6... I did the cherry-pick and accidentally committed to apache 3.6 branch instead of mine, so, consider it done I guess. Sorry about that... I will see that the commit did not break anything.
:)
> Edit: commit seems good, spotbugs fails but from a different commit (quorumpeertest has a whitespace after a function start "(". It is not caused by my commit.
Right; that has been fixed by @arshadmohammad while preparing 3.6.3. Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#discussion_r600675894
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java
##########
@@ -35,11 +35,7 @@
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.security.ConstraintMapping;
import org.eclipse.jetty.security.ConstraintSecurityHandler;
-import org.eclipse.jetty.server.HttpConfiguration;
-import org.eclipse.jetty.server.HttpConnectionFactory;
-import org.eclipse.jetty.server.SecureRequestCustomizer;
-import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.*;
Review comment:
Yeah, I see, gonna fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#issuecomment-806045487
Thanks for the review @symat , yes, I was planning to add unit tests, this was just an initial commit to see what folks think about the implementation (mainly to sticking to one port only). I will do the unit tests shortly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#issuecomment-806548198
Thank you @ztzg , about 3.6... I did the cherry-pick and accidentally committed to apache 3.6 branch instead of mine, so, consider it done I guess. Sorry about that... I will see that the commit did not break anything.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#issuecomment-806450206
Also, looking at the code "forceHTTPS" seems funky, I will change it to "forceHttps". Taking a quick look, all caps is not used for HTTPS in configs.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1652: ZOOKEEPER-4259 - Allow AdminServer to force https
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1652:
URL: https://github.com/apache/zookeeper/pull/1652#discussion_r600676791
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -2025,6 +2025,15 @@ Both subsystems need to have sufficient amount of threads to achieve peak read t
#### AdminServer configuration
+**New in 3.7.1:** The following
+options are used to configure the [AdminServer](#sc_adminserver).
+
+* *admin.forceHTTPS* :
+ (Java system property: **zookeeper.admin.disableHTTP**)
Review comment:
typo, first I wanted to call it disableHTTP
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org