You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/11/16 13:57:08 UTC

[GitHub] [pulsar] lhotari opened a new pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

lhotari opened a new pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581


   Fixes #8580
   
   ### Motivation
   
   See #8580 , some tests fail with newer Java 8 versions that include TLS1.3 protocol. This might also impacts the runtime behavior of Pulsar.
   
   ### Modifications
   
   Set the configured TLS protocols to the SSLEngine instance.


----------------------------------------------------------------
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] [pulsar] lhotari commented on pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

lhotari commented on pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581#issuecomment-728136822


   It seems that the cerficates are invalid for TLS 1.3 . signature algorithm for the *-cert.pem files is sha1WithRSAEncryption . SHA-1 is invalid for TLS 1.3 .
   
   For example
   
   ```
   $ openssl x509 -in pulsar-proxy/src/test/resources/authentication/tls/server-cert.pem -text
   Certificate:
       Data:
           Version: 3 (0x2)
           Serial Number:
               88:08:98:b3:13:d8:00:97
           Signature Algorithm: sha1WithRSAEncryption
           Issuer: C = US, ST = CA, O = Apache, OU = Pulsar Incubator, CN = localhost
           Validity
               Not Before: Feb 17 02:06:21 2018 GMT
               Not After : Nov 16 00:00:00 2030 GMT
           Subject: C = US, ST = CA, O = Apache, OU = Apache Pulsar, CN = localhost
           Subject Public Key Info:
               Public Key Algorithm: rsaEncryption
                   RSA Public-Key: (2048 bit)
                   Modulus:
                       00:af:bf:b7:2d:98:ad:9d:f6:da:a3:13:d4:62:0f:
                       98:be:1c:a2:89:22:ba:6f:d5:fd:1f:67:e3:91:03:
                       98:80:81:0e:ed:d8:f6:70:7f:2c:36:68:3d:53:ea:
                       58:3a:a6:d5:89:66:4b:bd:1e:57:71:13:6d:4b:11:
                       e5:40:a5:76:84:24:92:40:58:80:96:c9:1f:2c:c4:
                       55:eb:a3:79:73:70:5c:37:9a:89:ed:2f:ba:6b:e3:
                       82:7c:69:4a:02:54:8b:81:5e:3c:bf:4c:8a:cb:ea:
                       2c:5e:83:e7:b7:10:08:5f:82:58:a3:89:d1:da:92:
                       ba:2a:28:ee:30:28:3f:5b:ae:10:71:96:c7:e1:12:
                       c5:b0:1a:ad:44:6f:44:3a:11:4a:9a:3c:0f:8d:06:
                       80:7b:34:ef:3f:6c:f4:5e:c5:44:54:1e:c8:dd:c7:
                       80:85:80:d9:68:e6:c6:53:03:77:e1:fe:18:61:07:
                       77:05:4c:ed:59:bc:5d:41:38:6a:ef:5d:a1:b2:60:
                       98:d4:48:28:95:02:8a:0e:fd:cf:7b:1b:d2:11:cc:
                       10:0c:50:73:d7:cc:38:6c:83:dd:79:26:aa:90:c8:
                       9b:84:86:bc:59:e9:62:69:f4:98:1b:c4:80:78:7e:
                       a0:1a:81:9d:d2:e1:66:dd:c4:cc:fc:63:04:ac:ec:
                       a7:35
                   Exponent: 65537 (0x10001)
           X509v3 extensions:
               X509v3 Basic Constraints: 
                   CA:FALSE
               Netscape Comment: 
                   OpenSSL Generated Certificate
               X509v3 Subject Key Identifier: 
                   D3:F3:19:AE:74:B1:AF:E7:AF:08:7B:16:72:78:29:87:79:ED:30:8C
               X509v3 Authority Key Identifier: 
                   keyid:D4:7A:CD:0F:44:1B:16:29:25:14:ED:A2:EF:13:0F:A7:46:09:78:F6
   
       Signature Algorithm: sha1WithRSAEncryption
            0f:04:f3:91:f2:87:19:fe:9d:f8:34:5a:24:4a:00:d1:58:bf:
            1e:b2:77:67:07:bc:78:b5:4b:9a:4b:fd:a1:e5:dc:0e:09:84:
            9e:59:c4:dd:cf:f7:2e:bf:da:f3:31:36:6b:81:6e:a2:88:76:
            e4:2e:0b:36:44:82:36:8f:80:93:f4:9e:fc:ed:85:d0:97:da:
            0f:fb:c9:b9:8b:da:ae:07:3d:4f:82:b7:0c:25:22:63:12:6b:
            0a:e9:c4:12:a4:5c:ed:11:12:cc:fe:b0:2e:d4:c1:ec:79:01:
            60:ea:cc:cc:e5:66:cc:57:f6:55:a9:09:4c:63:01:e9:b4:2e:
            73:a5
   -----BEGIN CERTIFICATE-----
   MIIDLjCCApegAwIBAgIJAIgImLMT2ACXMA0GCSqGSIb3DQEBBQUAMFoxCzAJBgNV
   BAYTAlVTMQswCQYDVQQIEwJDQTEPMA0GA1UEChMGQXBhY2hlMRkwFwYDVQQLExBQ
   dWxzYXIgSW5jdWJhdG9yMRIwEAYDVQQDEwlsb2NhbGhvc3QwHhcNMTgwMjE3MDIw
   NjIxWhcNMzAxMTE2MDAwMDAwWjBXMQswCQYDVQQGEwJVUzELMAkGA1UECBMCQ0Ex
   DzANBgNVBAoTBkFwYWNoZTEWMBQGA1UECxMNQXBhY2hlIFB1bHNhcjESMBAGA1UE
   AxMJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAr7+3
   LZitnfbaoxPUYg+YvhyiiSK6b9X9H2fjkQOYgIEO7dj2cH8sNmg9U+pYOqbViWZL
   vR5XcRNtSxHlQKV2hCSSQFiAlskfLMRV66N5c3BcN5qJ7S+6a+OCfGlKAlSLgV48
   v0yKy+osXoPntxAIX4JYo4nR2pK6KijuMCg/W64QcZbH4RLFsBqtRG9EOhFKmjwP
   jQaAezTvP2z0XsVEVB7I3ceAhYDZaObGUwN34f4YYQd3BUztWbxdQThq712hsmCY
   1EgolQKKDv3PexvSEcwQDFBz18w4bIPdeSaqkMibhIa8WeliafSYG8SAeH6gGoGd
   0uFm3cTM/GMErOynNQIDAQABo3sweTAJBgNVHRMEAjAAMCwGCWCGSAGG+EIBDQQf
   Fh1PcGVuU1NMIEdlbmVyYXRlZCBDZXJ0aWZpY2F0ZTAdBgNVHQ4EFgQU0/MZrnSx
   r+evCHsWcngph3ntMIwwHwYDVR0jBBgwFoAU1HrND0QbFiklFO2i7xMPp0YJePYw
   DQYJKoZIhvcNAQEFBQADgYEADwTzkfKHGf6d+DRaJEoA0Vi/HrJ3Zwe8eLVLmkv9
   oeXcDgmEnlnE3c/3Lr/a8zE2a4Fuooh25C4LNkSCNo+Ak/Se/O2F0JfaD/vJuYva
   rgc9T4K3DCUiYxJrCunEEqRc7RESzP6wLtTB7HkBYOrMzOVmzFf2VakJTGMB6bQu
   c6U=
   -----END CERTIFICATE-----
   ```


----------------------------------------------------------------
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] [pulsar] lhotari edited a comment on pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

lhotari edited a comment on pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581#issuecomment-728679620

> What is the value returns before?

@sijie
For Java 8u272, Arrays.asList(sslEngine.getSupportedProtocols()).toString() returns `[TLSv1.3, TLSv1.2, TLSv1.1, TLSv1, SSLv3, SSLv2Hello]`.
For Java 8u232, it's `[SSLv2Hello, SSLv3, TLSv1, TLSv1.1, TLSv1.2]`.

[The `protocols` field in `KeyStoreSSLContext` was unused before](https://github.com/apache/pulsar/blob/5bbd44784a9e4bc58ee5025025d748b52b21825a/pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java#L79) and it's an old bug that the field was never used to configure the enabled TLS protocols for the SSLEngine.

TLS `protocols` are properly passed in `NettyServerSslContextBuilder` class, so this has been a bug in the code that uses `KeyStoreSSLContext`.

btw. The KeyStoreSSLContext class is also used for the web server. In those cases, [the protocols aren't configured and there's also a comment about it in the code](https://github.com/apache/pulsar/blob/5bbd44784a9e4bc58ee5025025d748b52b21825a/pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java#L247).
However there's a default value in `KeyStoreSSLContext` which will get used if null is passed to protocols.
It's defined at https://github.com/apache/pulsar/blob/5bbd44784a9e4bc58ee5025025d748b52b21825a/pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java#L52 .
Previously this default value has been ignored since it was never set to the SSLEngine. With this PR, it will get set.

----------------------------------------------------------------
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] [pulsar] codelipenghui merged pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

codelipenghui merged pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581


   


----------------------------------------------------------------
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] [pulsar] lhotari commented on pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

lhotari commented on pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581#issuecomment-728681953


   > Is there any way to add a test case?
   > This is security related code
   > We should prevent regressions
   
   Yes. It would be possible to create a negative test case where the client and server are configured to use different TLS protocols. In that case, the TLS handshake shouldn't succeed and the connection shouldn't get established. That would catch the bug that was in the code. In addition, there could be a few tests that check that connections can be established when the server contains at least one of the protocols that is enabled on the client.
   
   


----------------------------------------------------------------
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] [pulsar] lhotari commented on pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

lhotari commented on pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581#issuecomment-728679620

> What is the value returns before?

TLS `protocols` are properly passed in `NettyServerSslContextBuilder` class, so this has been a bug in the code that uses `KeyStoreSSLContext`.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #8581: [Issue 8580][pulsar-common] Set configured TLS protocols to SSLEngine instance

Posted by GitBox <gi...@apache.org>.

lhotari commented on pull request #8581:
URL: https://github.com/apache/pulsar/pull/8581#issuecomment-728112501


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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