Issue 933: Add support for PEM Key file formats #965

kishorekasi · 2018-01-10T19:49:04Z

(@bug W-4203319@) TLS authentication to support PEM format X.509
certificates along with JKS and PKCS12 formats.

Added another KeyStore Type to the list of formats (JKS, PKCS12)
currently supported during SSLContext creation.

@Rev ayegorov@

Signed-off-by: Kishore Kasi Udayashankar kudayashankar@salesforce.com

Descriptions of the changes in this PR:

(PR description content here)...

Master Issue: #933

@Rev

certificates along with JKS and PKCS12 formats. Added another KeyStore Type to the list of formats (JKS, PKCS12) currently supported during SSLContext creation. @Rev ayegorov@ Signed-off-by: Kishore Kasi Udayashankar <kudayashankar@salesforce.com>

eolivelli

Awesome.
Left some comments

eolivelli · 2018-01-10T22:08:56Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java

     *
     * @return
     */
-    public String getTLSKeyStoreType() {
-        return getString(TLS_KEYSTORE_TYPE, "JKS");
+    public String getTLSKeyFileType() {


I think we cannot change all of these names it is public API.
I would prefer to keep old names

will revert to old names

eolivelli · 2018-01-10T22:10:07Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java

-        try (FileInputStream ksin = new FileInputStream(keyStoreLocation)) {
-            ks.load(ksin, keyStorePassword.trim().toCharArray());
+        KeyStore ks = KeyStore.getInstance(keyFileType);
+        FileInputStream ksin = new FileInputStream(keyFileLocation);


Nit: use try-with-resources

eolivelli · 2018-01-10T22:12:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java

@@ -282,10 +389,12 @@ public SslHandler newTLSHandler() {
        if (protocols != null && protocols.length != 0) {
            sslHandler.engine().setEnabledProtocols(protocols);
        }
+        LOG.info("Enabled cipher protocols: {} ", Arrays.toString(sslHandler.engine().getEnabledProtocols()));



Do you need this at info level?

Not necessarily; this can move to debug

eolivelli · 2018-01-10T22:14:56Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java

+        switch (serverKeyFileFormat) {
+        case PEM:
+            baseConf.setTLSKeyFileType("PEM");
+            baseConf.setTLSKeyFilePath(this.getClass().getClassLoader().getResource("server-key.pem").getPath());


Do you think it is possible to generate these files at build time instead of using static files?
Certificates will expire one day.
If not so simple I am fine with keeping files, but having a readme files which explains how to rebuild the files in each format

sijie

also I think:

apache-rat:check is failing on travis ci
TestTLS tests seem to be failing with NullPointerException on both java 8 and java 9

sijie · 2018-01-12T07:40:52Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java

@@ -145,14 +145,6 @@
    // Client auth provider factory class name. It must be configured on Bookies to for the Auditor
    protected static final String CLIENT_AUTH_PROVIDER_FACTORY_CLASS = "clientAuthProviderFactoryClass";

-    // Client TLS
-    protected static final String TLS_KEYSTORE_TYPE = "clientKeyStoreType";


this breaks BC.

for each field you are change here, I think you have to do:

in this file: change "TLS_XYZ" to "CLIENT_XYZ", but not change the value.

change getTLSxyz() to : getProperty(TLS_XYZ, getProperty(CLIENT_XYZ, ...))

I am not what AutoRecovery is using, is it using client settings or server settings? if it is using client settings, you need also to update the configuration settings in the bookie conf.

I am moving TLS_XYZ from ServerConfiguration and ClientConfiguration to the common class AbstractConfiguration and just use one set of config params (no clientXYZ params anymore). So, effectively this is a dead code. What I am doing here is to keep just one set of config params so both bookie and client config params names are same.

AutoRecovery can still use the same set of params as bookies to configure the bookkeeper client.

Only config param names are changing, but the variable name still remains the same as TLS_XYZ. Do you still think its a BC issue?

yeah param name changing is considered as breaking BC. because if a client upgrades from 4.6.0 to 4.7.0, without changing the configuration, it won't work.

sijie · 2018-01-19T22:05:09Z

fyi - had a conversation with @kishorekasi offline. we agreed on the approach for addressing the BC concerns here. @kishorekasi will update this PR.

The example PR on showing how to deal with the BC concerns is here: kishorekasi#1

…start a bookie sever. Client and server are not used in the same process. So, no need for two set of TLS parameters.

eolivelli

Overall is good but I would like to change the way certificates are generated

eolivelli · 2018-01-21T09:38:35Z

bookkeeper-server/pom.xml

+                </goals>
+                <configuration>
+                    <workingDirectory>${basedir}/src/test/resources</workingDirectory>
+                    <executable>${basedir}/src/test/resources/generateKeysAndCerts.sh</executable>


This is going to create new certificates at every build, two problems:

this is slow

it requires some jind of setup of openssl

I suggest to add a profile to build the certificates and commit a working version of certificates.
When they will expire we will do another commit

How slow is this?

With this patch at every "mvn test" we are going to re-create new certificates.
I think we have two options:

commit certificates and rebuild them when expired

make the script rebuild the certificate only if it does not exist

I prefer 1)as it makes tests reproducible at 100% and it does not impact build and dev time

@eolivelli If it makes sense, I can commit certificates with a very long expiry so that we may never need to generate the certificates again. Also move certification generation to a new profile, just in case we need it.

It would be fine for me, thank you

+1 on @kishorekasi 's proposed approach.

kishorekasi · 2018-01-21T18:48:15Z

2-3 seconds on my Ubuntu server

- Add a profile to generate self signed certificates on demand

eolivelli

Now it looks good for me
Thank you +1

eolivelli requested changes Jan 10, 2018

View reviewed changes

Kishore Udayashankar added 3 commits January 11, 2018 17:57

revert KeyFile->KeyStore and TrustFile->TrustStore

920f3ac

address review comments

efafda3

Generate Keys and Self-Signed Certs during build

da0a715

sijie reviewed Jan 12, 2018

View reviewed changes

sijie changed the title ~~BOOKKEEPER-933: Add support for PEM Key file formats~~ Issue 933: Add support for PEM Key file formats Jan 19, 2018

Kishore Udayashankar added 2 commits January 19, 2018 16:34

fix test failure

c2dbbec

Deprecate TLS client specific parameters. AR uses client and bookies …

cb59a66

…start a bookie sever. Client and server are not used in the same process. So, no need for two set of TLS parameters.

eolivelli requested changes Jan 21, 2018

View reviewed changes

Kishore Udayashankar added 2 commits January 22, 2018 12:33

fix typo in filename

ec1ca91

- Add self signed long expiry certs to the repo

003995b

- Add a profile to generate self signed certificates on demand

eolivelli approved these changes Jan 25, 2018

View reviewed changes

sijie approved these changes Jan 27, 2018

View reviewed changes

sijie added this to the 4.7.0 milestone Jan 28, 2018

sijie added area/bookie area/client area/security release/4.7.0 type/improvement labels Jan 28, 2018

sijie assigned kishorekasi Jan 28, 2018

sijie closed this in 76c1fd0 Jan 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 933: Add support for PEM Key file formats #965

Issue 933: Add support for PEM Key file formats #965

kishorekasi commented Jan 10, 2018 •

edited

Loading

eolivelli left a comment

eolivelli Jan 10, 2018

kishorekasi Jan 12, 2018

eolivelli Jan 10, 2018

kishorekasi Jan 12, 2018

eolivelli Jan 10, 2018

kishorekasi Jan 12, 2018

eolivelli Jan 10, 2018

sijie left a comment

sijie Jan 12, 2018

kishorekasi Jan 17, 2018

sijie Jan 17, 2018

sijie commented Jan 19, 2018

eolivelli left a comment

eolivelli Jan 21, 2018

sijie Jan 21, 2018

eolivelli Jan 22, 2018

kishorekasi Jan 23, 2018

eolivelli Jan 23, 2018

sijie Jan 24, 2018

kishorekasi commented Jan 21, 2018

eolivelli left a comment

Issue 933: Add support for PEM Key file formats #965

Issue 933: Add support for PEM Key file formats #965

Conversation

kishorekasi commented Jan 10, 2018 • edited Loading

eolivelli 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

sijie 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

sijie commented Jan 19, 2018

eolivelli 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

kishorekasi commented Jan 21, 2018

eolivelli left a comment

Choose a reason for hiding this comment

kishorekasi commented Jan 10, 2018 •

edited

Loading