Create SSL context in constructor of ChannelInitializer #3550

massakam · 2019-02-08T13:11:38Z

Motivation

To use TLS, it is necessary to read a private key and certificate files to create an SSL context. Currently, Pulsar reads these files every time a new TLS session is established.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarChannelInitializer.java

Lines 51 to 67 in a62633b

    
           protected void initChannel(SocketChannel ch) throws Exception { 
        
               ServiceConfiguration serviceConfig = pulsar.getConfiguration(); 
        
               if (enableTLS) { 
        
                   SslContext sslCtx = SecurityUtility.createNettySslContextForServer( 
        
                           serviceConfig.isTlsAllowInsecureConnection(), serviceConfig.getTlsTrustCertsFilePath(), 
        
                           serviceConfig.getTlsCertificateFilePath(), serviceConfig.getTlsKeyFilePath(), 
        
                           serviceConfig.getTlsCiphers(), serviceConfig.getTlsProtocols(), 
        
                           serviceConfig.isTlsRequireTrustedClientCertOnConnect()); 
        
                   ch.pipeline().addLast(TLS_HANDLER, sslCtx.newHandler(ch.alloc())); 
        
                   ch.pipeline().addLast("ByteBufPairEncoder", ByteBufPair.COPYING_ENCODER); 
        
               } else { 
        
                   ch.pipeline().addLast("ByteBufPairEncoder", ByteBufPair.ENCODER); 
        
               } 
        
               ch.pipeline().addLast("frameDecoder", new LengthFieldBasedFrameDecoder(PulsarDecoder.MaxFrameSize, 0, 4, 0, 4)); 
        
               ch.pipeline().addLast("handler", new ServerCnx(pulsar)); 
        
           }

Therefore, when many TLS sessions are started at the same time, the load on the broker increases exponentially. This becomes pronounced if the size of the trusted certificate file is large.

Modifications

Currently, a SSL context is created in initChannel methods of subclasses of ChannelInitializer.
Moving that process to the constructors eliminate the need to read private key and certificates every time.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as TlsProducerConsumerTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (no)

massakam · 2019-02-08T14:52:57Z

rerun java8 tests
rerun integration tests

merlimat

Great catch! 👍

merlimat · 2019-02-08T17:49:04Z

run java8 tests
run integration tests

merlimat · 2019-02-08T17:50:07Z

@massakam There appears to be a legitimate test failure:

2019-02-08\T\16:14:30.563 [ERROR] setup(org.apache.pulsar.io.debezium.PulsarDatabaseHistoryTest)  Time elapsed: 2.757 s  <<< FAILURE!
org.apache.pulsar.broker.PulsarServerException: java.security.KeyManagementException: Certificate loading error
	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:448)
	at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.startBroker(MockedPulsarServiceBaseTest.java:212)
	at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.startBroker(MockedPulsarServiceBaseTest.java:202)
	at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.init(MockedPulsarServiceBaseTest.java:147)
	at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.internalSetup(MockedPulsarServiceBaseTest.java:116)
	at org.apache.pulsar.io.debezium.PulsarDatabaseHistoryTest.setup(PulsarDatabaseHistoryTest.java:54)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:59)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:451)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:222)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:516)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:707)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:979)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:125)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109)
	at org.testng.TestRunner.privateRun(TestRunner.java:648)
	at org.testng.TestRunner.run(TestRunner.java:505)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:455)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415)
	at org.testng.SuiteRunner.run(SuiteRunner.java:364)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1187)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1116)
	at org.testng.TestNG.runSuites(TestNG.java:1028)
	at org.testng.TestNG.run(TestNG.java:996)
	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:99)
	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:379)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:340)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:125)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:413)
Caused by: java.security.KeyManagementException: Certificate loading error
	at org.apache.pulsar.common.util.SecurityUtility.loadCertificatesFromPemFile(SecurityUtility.java:179)
	at org.apache.pulsar.common.util.SecurityUtility.createNettySslContextForServer(SecurityUtility.java:106)
	at org.apache.pulsar.broker.service.PulsarChannelInitializer.<init>(PulsarChannelInitializer.java:48)
	at org.apache.pulsar.broker.service.BrokerService.start(BrokerService.java:307)
	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:366)
	at org.apache.pulsar.broker.PulsarService$$EnhancerByMockitoWithCGLIB$$f71c0598.CGLIB$start$42(<generated>)
	at org.apache.pulsar.broker.PulsarService$$EnhancerByMockitoWithCGLIB$$f71c0598$$FastClassByMockitoWithCGLIB$$2d95e201.invoke(<generated>)
	at org.mockito.cglib.proxy.MethodProxy.invokeSuper(MethodProxy.java:216)
	at org.powermock.api.mockito.repackaged.DelegatingMockitoMethodProxy.invokeSuper(DelegatingMockitoMethodProxy.java:20)
	at org.mockito.internal.invocation.realmethod.DefaultRealMethod.invoke(DefaultRealMethod.java:21)
	at org.mockito.internal.invocation.realmethod.CleanTraceRealMethod.invoke(CleanTraceRealMethod.java:30)
	at org.mockito.internal.invocation.InvocationImpl.callRealMethod(InvocationImpl.java:112)
	at org.mockito.internal.stubbing.answers.CallsRealMethods.answer(CallsRealMethods.java:41)
	at org.mockito.internal.handler.MockHandlerImpl.handle(MockHandlerImpl.java:93)
	at org.mockito.internal.handler.NullResultGuardian.handle(NullResultGuardian.java:29)
	at org.mockito.internal.handler.InvocationNotifierHandler.handle(InvocationNotifierHandler.java:38)
	at org.powermock.api.mockito.repackaged.MethodInterceptorFilter.intercept(MethodInterceptorFilter.java:60)
	at org.apache.pulsar.broker.PulsarService$$EnhancerByMockitoWithCGLIB$$f71c0598.start(<generated>)
	... 38 more
Caused by: java.io.FileNotFoundException: ../pulsar-broker/src/test/resources/certificate/server.crt (No such file or directory)
	at java.io.FileInputStream.open0(Native Method)
	at java.io.FileInputStream.open(FileInputStream.java:195)
	at java.io.FileInputStream.<init>(FileInputStream.java:138)
	at java.io.FileInputStream.<init>(FileInputStream.java:93)
	at org.apache.pulsar.common.util.SecurityUtility.loadCertificatesFromPemFile(SecurityUtility.java:174)
	... 55 more

2019-02-08\T\16:14:30.921 [INFO] 
2019-02-08\T\16:14:30.921 [INFO] Results:
2019-02-08\T\16:14:30.921 [INFO] 
2019-02-08\T\16:14:30.921 [ERROR] Failures: 
2019-02-08\T\16:14:30.921 [ERROR] org.apache.pulsar.io.debezium.PulsarDatabaseHistoryTest.setup(org.apache.pulsar.io.debezium.PulsarDatabaseHistoryTest)
2019-02-08\T\16:14:30.921 [ERROR]   Run 1: PulsarDatabaseHistoryTest.setup:54->MockedPulsarServiceBaseTest.internalSetup:116->MockedPulsarServiceBaseTest.init:147->MockedPulsarServiceBaseTest.startBroker:202->MockedPulsarServiceBaseTest.startBroker:212 » PulsarServer

massakam · 2019-02-09T15:54:20Z

@merlimat With this modification, broker now loads a private key and certificates at startup. Therefore, if brokerServicePortTls is set but tlsKeyFilePath and tlsCertificateFilePath are not set, the broker fails to start.

Initially, I modified MockedPulsarServiceBaseTest to set a private key and a certificate, but it seems that the test classes in other submodules that inherit this class fail to read those files.

So deleted setting of the ports to be used for TLS from MockedPulsarServiceBaseTest and set them only in the tests that need to enable TLS.

rdhabalia · 2019-02-09T19:12:23Z

@massakam @merlimat what will happen when certs and keys are rotating regularly ? earlier, on every new cnx if broker was reading certs then it can read new certs but now, do we have to restart broker?

massakam · 2019-02-10T02:09:09Z

@rdhabalia

do we have to restart broker?

Yes, new certificates will not be loaded until the broker is restarted.

merlimat · 2019-02-10T02:12:34Z

@rdhabalia Good point. Maybe we could either cache for some configurable time (eg: 1hour) or monitor the file for changes

rdhabalia · 2019-02-11T18:00:16Z

Maybe we could either cache for some configurable time (eg: 1hour) or monitor the file for changes

Yes, we can fix it with that alternative. I will make the change to fix it. 👍

jai1 · 2019-02-11T18:10:11Z

https://github.com/apache/pulsar/pull/3568/files - this PR should do what Rajan suggested.

…

On Mon, Feb 11, 2019 at 10:00 AM Rajan Dhabalia ***@***.***> wrote: Maybe we could either cache for some configurable time (eg: 1hour) or monitor the file for changes Yes, we can fix it with that alternative. I will make the change to fix it. 👍 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3550 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIQh_HR2SR3xy3Tl5QiPs8Q2ORvL0iLOks5vMa-3gaJpZM4avlQr> .

massakam added the type/performance label Feb 8, 2019

massakam added this to the 2.3.0 milestone Feb 8, 2019

massakam self-assigned this Feb 8, 2019

merlimat approved these changes Feb 8, 2019

View reviewed changes

Masahiro Sakamoto added 2 commits February 9, 2019 21:34

Create SSL context in constructor of ChannelInitializer

7b472e1

Set TLS ports only in tests that need to enable TLS

24be1dc

massakam force-pushed the ssl-context branch from e0015b5 to 24be1dc Compare February 9, 2019 14:13

merlimat merged commit f22bbea into apache:master Feb 9, 2019

massakam deleted the ssl-context branch February 10, 2019 02:04

jiazhai mentioned this pull request May 25, 2019

Authentication provider getAuthData called before initialization #4365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create SSL context in constructor of ChannelInitializer #3550

Create SSL context in constructor of ChannelInitializer #3550

massakam commented Feb 8, 2019 •

edited

Loading

massakam commented Feb 8, 2019

merlimat left a comment

merlimat commented Feb 8, 2019

merlimat commented Feb 8, 2019

massakam commented Feb 9, 2019

rdhabalia commented Feb 9, 2019

massakam commented Feb 10, 2019

merlimat commented Feb 10, 2019

rdhabalia commented Feb 11, 2019

jai1 commented Feb 11, 2019 via email

	protected void initChannel(SocketChannel ch) throws Exception {
	ServiceConfiguration serviceConfig = pulsar.getConfiguration();
	if (enableTLS) {
	SslContext sslCtx = SecurityUtility.createNettySslContextForServer(
	serviceConfig.isTlsAllowInsecureConnection(), serviceConfig.getTlsTrustCertsFilePath(),
	serviceConfig.getTlsCertificateFilePath(), serviceConfig.getTlsKeyFilePath(),
	serviceConfig.getTlsCiphers(), serviceConfig.getTlsProtocols(),
	serviceConfig.isTlsRequireTrustedClientCertOnConnect());
	ch.pipeline().addLast(TLS_HANDLER, sslCtx.newHandler(ch.alloc()));
	ch.pipeline().addLast("ByteBufPairEncoder", ByteBufPair.COPYING_ENCODER);
	} else {
	ch.pipeline().addLast("ByteBufPairEncoder", ByteBufPair.ENCODER);
	}

	ch.pipeline().addLast("frameDecoder", new LengthFieldBasedFrameDecoder(PulsarDecoder.MaxFrameSize, 0, 4, 0, 4));
	ch.pipeline().addLast("handler", new ServerCnx(pulsar));
	}

Create SSL context in constructor of ChannelInitializer #3550

Create SSL context in constructor of ChannelInitializer #3550

Conversation

massakam commented Feb 8, 2019 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

massakam commented Feb 8, 2019

merlimat left a comment

Choose a reason for hiding this comment

merlimat commented Feb 8, 2019

merlimat commented Feb 8, 2019

massakam commented Feb 9, 2019

rdhabalia commented Feb 9, 2019

massakam commented Feb 10, 2019

merlimat commented Feb 10, 2019

rdhabalia commented Feb 11, 2019

jai1 commented Feb 11, 2019 via email

massakam commented Feb 8, 2019 •

edited

Loading