Adding config for keystore types, switching tls to native implementation, and adding authorization for server-broker tls channel #7653

jasperjiaguo · 2021-10-27T23:47:44Z

Description

Add missing functionality for netty tls truststore/keystore type, so that JKS/PKCS12 keystore can load properly.
Switch TLS to native implementation (https://netty.io/wiki/forked-tomcat-native.html). Native method brings less overhead for encryption/decryption.
Add authorization endpoint for broker-server netty tls channel. The authorization is performed on server side after handshake completion of the broker-server channel, which can be used for server to check broker's certificate.

We have validated the functionalities of above changes internally using production data and queries.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
No

Does this PR fix a zero-downtime upgrade introduced earlier?
No

Does this PR otherwise need attention when creating release notes? Things to consider:
Yes

Release Notes

Moved org.apache.pinot.server.api.access.AllowAllAccessFactory to org.apache.pinot.server.access.AllowAllAccessFactory
important Note that if your current installation explicitly configured access control factory to use org.apache.pinot.server.api.access.AllowAllAccessFactory, please change it to org.apache.pinot.server.access.AllowAllAccessFactory.
Adding the following configs so that keystore/truststore of different types(JKS/PKCS12/...) can load properly

pinot-controller
controller.tls.keystore.type
controller.tls.truststore.type
pinot-broker
pinot.broker.tls.keystore.type
pinot.broker.tls.truststore.type
pinot-server
pinot.server.tls.keystore.type
pinot.server.tls.truststore.type

codecov-commenter · 2021-10-28T00:16:05Z

Codecov Report

Merging #7653 (1a603e4) into master (c594796) will decrease coverage by 6.41%.
The diff coverage is 13.04%.

@@             Coverage Diff              @@
##             master    #7653      +/-   ##
============================================
- Coverage     71.47%   65.06%   -6.42%     
+ Complexity     4033     4029       -4     
============================================
  Files          1581     1536      -45     
  Lines         80419    78634    -1785     
  Branches      11950    11752     -198     
============================================
- Hits          57483    51160    -6323     
- Misses        19049    23815    +4766     
+ Partials       3887     3659     -228

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`68.52% <13.04%> (-0.11%)`	⬇️
unittests2	`14.57% <0.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rg/apache/pinot/core/transport/ServerChannels.java	`75.47% <0.00%> (-16.84%)`	⬇️
...ava/org/apache/pinot/core/transport/TlsConfig.java	`3.84% <0.00%> (-78.51%)`	⬇️
...main/java/org/apache/pinot/core/util/TlsUtils.java	`0.00% <0.00%> (-72.47%)`	⬇️
...che/pinot/server/access/AllowAllAccessFactory.java	`80.00% <0.00%> (ø)`
...apache/pinot/spi/ingestion/batch/spec/TlsSpec.java	`0.00% <0.00%> (ø)`
...va/org/apache/pinot/spi/utils/CommonConstants.java	`26.31% <ø> (ø)`
...e/pinot/core/transport/InstanceRequestHandler.java	`56.96% <37.50%> (-4.15%)`	⬇️
...a/org/apache/pinot/core/transport/QueryServer.java	`53.19% <50.00%> (-17.27%)`	⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 372 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c594796...1a603e4. Read the comment docs.

Jackie-Jiang

@apucher Can you please take a look at this PR?

Jackie-Jiang · 2021-10-28T23:54:07Z

...ot-driver/src/main/java/org/apache/pinot/connector/presto/PinotScatterGatherQueryClient.java

@@ -73,8 +73,7 @@ public boolean isRetriable() {
    }
  }

-  public static class PinotException
-      extends RuntimeException {
+  public static class PinotException extends RuntimeException {


(code format) Please apply the Pinot Code Style: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide

apucher

Hi @jasperjiaguo, very much appreciate contributions on pinot security.

I have some initial questions, and I'd appreciate if you could provide some additional context.

Also, would you mind adding a unit test for different keystore formats, or blank values for your new tls property?

...ot-driver/src/main/java/org/apache/pinot/connector/presto/PinotScatterGatherQueryClient.java

pinot-core/pom.xml

pinot-core/src/main/java/org/apache/pinot/core/transport/QueryServer.java

pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java

pinot-core/src/main/java/org/apache/pinot/server/access/AccessControl.java

apucher · 2021-10-29T02:06:05Z

pinot-core/src/main/java/org/apache/pinot/server/access/AllowAllAccessFactory.java

@@ -16,13 +16,19 @@
 * specific language governing permissions and limitations
 * under the License.
 */
-package org.apache.pinot.server.api.access;
+package org.apache.pinot.server.access;


this looks like it could cause serious breakage to existing pinot installations, since access control is a string-configured property with fully-qualified class name

@apucher In BaseServerStarter we have
String accessControlFactoryClass = _serverConf.getProperty(Server.ACCESS_CONTROL_FACTORY_CLASS, Server.DEFAULT_ACCESS_CONTROL_FACTORY_CLASS);
where I have changed DEFAULT_ACCESS_CONTROL_FACTORY_CLASS to org.apache.pinot.server.access.AllowAllAccessFactory, and left ACCESS_CONTROL_FACTORY_CLASS unchanged (pinot.server.admin.access.control.factory.class).
So if the current installation is using default (not configuring pinot.server.admin.access.control.factory.class) or configuring it to customized class there should be no problem.

IIUC, the only case that will cause problem is someone explicitly configured pinot.server.admin.access.control.factory.class to org.apache.pinot.server.api.access.AllowAllAccessFactory. Do you know any use case configured in this way?

At least, I would specifically call this out in the PR's release notes.

I'm aware of at least one installation that explicitly sets AllowAll... as part of startup scripts.

Sure, added in the release notes.

why are we changing the package names?

We moved the access control classes from pinot-server to pinot-core since they are used in QueryServer of pinot-core to enable broker-server channel authorization. There will be circular dependency if we don't change the package name.

siddharthteotia · 2021-11-05T20:17:19Z

@apucher , can you please take a look again. Would like to get this merged if no more comments

apucher

Thank you for the updates.

Would you mind still adding the unit test for a different keystore type?
It doesn't just serve robustness but similarly as documentation for future developers.

Additionally, I'm not completely clear on how the application layer auth for broker and server would look like. Is this cert- or token-based? Here, too, a minimal unit test could help guide future generations

apucher · 2021-11-05T20:53:46Z

you could probably add to BasicAuthTlsRealtimeIntegrationTest to cover this

kishoreg · 2021-11-08T08:07:30Z

Its not clear why we are changing the package name for AllowAllAccessFactory

jasperjiaguo · 2021-11-08T19:55:27Z

Its not clear why we are changing the package name for AllowAllAccessFactory

Hello @kishoreg, we moved the access control classes from pinot-server to pinot-core since they are used in QueryServer of pinot-core to enable broker-server channel authorization. There will be circular dependency if we don't change the package name.

jasperjiaguo · 2021-11-09T00:40:39Z

Thank you for the updates.

Would you mind still adding the unit test for a different keystore type? It doesn't just serve robustness but similarly as documentation for future developers.

The test for the keystore type is done. I renamed the original tlstest.jks to tlstes.p12 as it is internally a P12 keystore. I think now JAVA uses PKCS12 instead of JKS for default keystore type. Generated a new JKS keystore for robustness test.

The authZ is cert based and it extracts X509 cert from the ChannelHandlerContext in userEventTriggered

…tomcat-native.html) 2. Add access control for broker-server netty tls channel 3. Add missing functionality for netty tls truststore/keystore types 4. Fix a few issues, add config for ssl provider, remove the key existence check for fields having default values 5. Fix keystore naming, add keystore test for JKS 6. Adding test case for server AccessControl

apucher

good from my side. thank you for adding the cert auth sample. this should make extending this easier for future generations.

@kishoreg @Jackie-Jiang anything still open from your side?

…tomcat-native.html) (apache#7653) 2. Add access control for broker-server netty tls channel 3. Add missing functionality for netty tls truststore/keystore types 4. Fix a few issues, add config for ssl provider, remove the key existence check for fields having default values 5. Fix keystore naming, add keystore test for JKS 6. Adding test case for server AccessControl

Jackie-Jiang reviewed Oct 28, 2021

View reviewed changes

apucher reviewed Oct 29, 2021

View reviewed changes

jasperjiaguo force-pushed the tls_config_read_netty_native_auth branch from 32d1130 to 4c27b9c Compare November 3, 2021 00:43

jasperjiaguo requested a review from apucher November 3, 2021 01:02

jasperjiaguo force-pushed the tls_config_read_netty_native_auth branch from cb1ad17 to 96dc8ab Compare November 5, 2021 20:20

apucher reviewed Nov 5, 2021

View reviewed changes

jasperjiaguo force-pushed the tls_config_read_netty_native_auth branch 2 times, most recently from 0c5fd26 to 7230b67 Compare November 8, 2021 07:57

jasperjiaguo force-pushed the tls_config_read_netty_native_auth branch 2 times, most recently from c852378 to 33a391e Compare November 11, 2021 20:16

jasperjiaguo force-pushed the tls_config_read_netty_native_auth branch from b16f950 to 2d0c907 Compare November 11, 2021 20:31

jasperjiaguo requested a review from apucher November 12, 2021 00:23

apucher approved these changes Nov 12, 2021

View reviewed changes

siddharthteotia approved these changes Nov 12, 2021

View reviewed changes

siddharthteotia merged commit 4b2d31b into apache:master Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding config for keystore types, switching tls to native implementation, and adding authorization for server-broker tls channel #7653

Adding config for keystore types, switching tls to native implementation, and adding authorization for server-broker tls channel #7653

jasperjiaguo commented Oct 27, 2021 •

edited

Loading

codecov-commenter commented Oct 28, 2021 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Oct 28, 2021

jasperjiaguo Nov 1, 2021

apucher left a comment

apucher Oct 29, 2021

jasperjiaguo Nov 2, 2021 •

edited

Loading

apucher Nov 5, 2021

jasperjiaguo Nov 5, 2021

kishoreg Nov 8, 2021

jasperjiaguo Nov 11, 2021

jasperjiaguo Nov 11, 2021

siddharthteotia commented Nov 5, 2021

apucher left a comment

apucher commented Nov 5, 2021

kishoreg commented Nov 8, 2021

jasperjiaguo commented Nov 8, 2021 •

edited

Loading

jasperjiaguo commented Nov 9, 2021 •

edited

Loading

apucher left a comment

Adding config for keystore types, switching tls to native implementation, and adding authorization for server-broker tls channel #7653

Adding config for keystore types, switching tls to native implementation, and adding authorization for server-broker tls channel #7653

Conversation

jasperjiaguo commented Oct 27, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

codecov-commenter commented Oct 28, 2021 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasperjiaguo Nov 2, 2021 • 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

siddharthteotia commented Nov 5, 2021

apucher left a comment

Choose a reason for hiding this comment

apucher commented Nov 5, 2021

kishoreg commented Nov 8, 2021

jasperjiaguo commented Nov 8, 2021 • edited Loading

jasperjiaguo commented Nov 9, 2021 • edited Loading

apucher left a comment

Choose a reason for hiding this comment

jasperjiaguo commented Oct 27, 2021 •

edited

Loading

codecov-commenter commented Oct 28, 2021 •

edited

Loading

jasperjiaguo Nov 2, 2021 •

edited

Loading

jasperjiaguo commented Nov 8, 2021 •

edited

Loading

jasperjiaguo commented Nov 9, 2021 •

edited

Loading