Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 349: Documentation for security feature in 4.5.0 #350

Closed
wants to merge 5 commits into from

Conversation

sijie
Copy link
Member

@sijie sijie commented Aug 2, 2017

Descriptions of the changes in this PR:

  • add a section for security
  • overview to introduce security feature
  • encryption and authentication using tls
  • sasl authentication
  • zookeeper authentication

@sijie sijie added this to the 4.5.0 milestone Aug 2, 2017
@sijie sijie self-assigned this Aug 2, 2017
@sijie
Copy link
Member Author

sijie commented Aug 2, 2017

You can review the staging site here: https://sijie.github.io/bookkeeper-staging-site/docs/security/

@sijie
Copy link
Member Author

sijie commented Aug 2, 2017

@kishorekasi : can you review the details for tls support?
@eolivelli : can you review the details for SASL and zookeeper authentication?

/cc @lucperkins @merlimat for reviews general reviews.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice explanation of TLS and Kerberos. Just added a couple of minor suggestions.

If you have installed your own Kerberos, you will need to create these principals yourself using the following commands:


sudo /usr/sbin/kadmin.local -q 'addprinc -randkey bookkeeper/{hostname}@{REALM}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the ``` syntax followed by shell to have the syntax highlighting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to use ```shell


Then sign it with the CA:

openssl x509 -req -CA ca-cert -CAkey ca-key -in cert-file -out cert-signed -days {validity} -CAcreateserial -passin pass:{ca-password}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use some highlighting to mark the parameters to fill up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


## Overview

The bookies and clients need their own key and certificate in order to use TLS. The key is used for encryption, while the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clients need their own key and certificate

If you only want to do encryption, in general there's no need for the client to configure a certificate. A self-signed certificate should be created on the spot. (I have not checked if it's actually true in the current implementation)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, current implementation does not require clients to have a local certificate
The test TestTLS#testConnectToTLSClusterNonTLSClient is about this setup.

So it is better to change the line with 'clients need'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. the subsequent sentence explains key is for encryption and certificate for authentication. is that enough? can you guys suggest a better structure for explaining that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bookies and clients need their own key and certificate in order to use TLS

can be reworded as:

The bookies need their own key and certificate in order to use TLS. Clients can optionally provide a key and a certificate for mutual authentication.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestTLS#testConnectToTLSClusterNonTLSClient - Tests whether a client, not configured to use TLS, can talk to bookie cluster with TLS enabled. I do not have a test to only encrypt but not authenticate. Not sure if it is even possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good docs @sijie !

I have left some important comments about ZK Acls and SASL DIGEST-MD5


It’s worth noting that security is optional - non-secured clusters are supported, as well as a mix of authenticated, unauthenticated, encrypted and non-encrypted clients.

NOTE: currenlty `authorization` is not yet available in `4.5.0`. The Apache BookKeeper community is looking for adding this feature in subsequent releases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: currenlty


## Overview

The bookies and clients need their own key and certificate in order to use TLS. The key is used for encryption, while the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, current implementation does not require clients to have a local certificate
The test TestTLS#testConnectToTLSClusterNonTLSClient is about this setup.

So it is better to change the line with 'clients need'

## Configuring Bookies

Bookies support TLS for connections on the same service port. In order to enable TLS, you need to configure `tlsProvider` to be either
`JDK` or `OpenSSL`. If `OpenSSL` is configured but the open ssl library is not found, it will automatically fall back to use default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using netty-tcnative-boringssl-static which is in the classpath by default, it will be loaded on supported archs/SOs.

We MUST state that BookKeeper is not going to use the SO-provided OpenSSL library.
This is important, because sys admins will upgrade OpenSSL in case of security flaws on it (like the recent ones) but this will not affect BookKeeper

we can add a
TDB: describe how to replace the JARs on the classpath with Netty bindings to leverage OpenSSL installed on the system see http://netty.io/wiki/forked-tomcat-native.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is important. We will not be using OpenSSL .so library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


## SASL configuration for Bookies

1. Select the mechanisms to enable in the bookies. `GSSAPI` is the only mechanism currently supported by BookKeeper.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are supporting GSSAPI (kerberos) for production.
There is support for username/password usually good for tests and staging environments.
Actually this is known as DIGEST-MD5, so passwords are not sent on the wire but only digests.
As @revans2 states we should state clearly that DIGEST-MD5 is not suitable for production.

see MD5DigestBookKeeperTest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically we don't treat DIGEST-MD5 as a security feature. there were discussions before to remove passwd completely, because it doesn't really make any sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be good to cite this opportunity for tests/staging ?
I am fine with not writing about it as it is not "secure"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not mentioning it at all, because it is not secure at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sijie ok, do not mention


#### Kerberos

If your organization is already using a Kerberos server (for example, by using `Active Directory`), there is no need to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I never used Active Directory for Kerberos, I don't know if there is some adapter/proxy to install in order to make it work with Java GSSAPI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

installing kerberos is somehow out of the scope of this documentation, no? this is just giving an example. what are you suggesting here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, @sijie forget this comment,
I found in other docs that ActiveDirectory is well supported.

install a new server just for BookKeeper. Otherwise you will need to install one, your Linux vendor likely has packages
for `Kerberos` and a short guide on how to install and configure it ([Ubuntu](https://help.ubuntu.com/community/Kerberos),
[Redhat](https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Managing_Smart_Cards/installing-kerberos.html)).
Note that if you are using Oracle Java, you will need to download JCE policy files for your Java version and copy them to `$JAVA_HOME/jre/lib/security`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run with basic JDK JCE files (no unlimited strenght....) but there is some setup on the KDC to be done, I don't know it is worth to say here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop my comment, any KDC administrator will be able to do the better setup, it is out of the scope of BK docs to provide suggestions about KDC setup

If you have installed your own Kerberos, you will need to create these principals yourself using the following commands:


sudo /usr/sbin/kadmin.local -q 'addprinc -randkey bookkeeper/{hostname}@{REALM}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


## Enabling Logging for SASL

To enable SASL debug output, you can set `sun.security.krb5.debug` system property to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java GSSAPI errors are not clear, most of the times I need to enable debug to understand the cause.
in my cases most of the times the problems are related to reverse-lookup of IP addresses to hostname.
for Kerberos it is very important that DNS is setup coherently both for names and for PTRs

there is also a 'debug=true' option in Krb5LoginModule

Server {
  com.sun.security.auth.module.Krb5LoginModule required debug=true
  useKeyTab=true
  keyTab="xxxxx.keytab"
  storeKey=true
  useTicketCache=false
  principal="xxx/xxx@xxx";
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have real debug experience. I would defer this to a subsequent request if you are willing to contribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if users will have trouble they will ask on the user@ mailing list.
It is better to leave the doc as simple as possible, expecially for third-party deps.

So I am OK with this part of the doc as you have written

1. Create a `JAAS` login file and set the appropriate system property to point to it as described in [GSSAPI (Kerberos)](../sasl#notes).
2. Set the configuration property `zkEnableSecurity` in each bookie to `true`.

The metadata stored in `ZooKeeper` is such that only certain clients will be able to modify the corresponding znodes, but znodes are world readable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry but we are using ZooDefs.Ids.CREATOR_ALL_ACL, this way only the authenticated and authorized clients will be able to read the contents of znode.

      /**
         * This is a completely open ACL .
         */
        public final ArrayList<ACL> OPEN_ACL_UNSAFE = new ArrayList<ACL>(
                Collections.singletonList(new ACL(Perms.ALL, ANYONE_ID_UNSAFE)));

        /**
         * This ACL gives the creators authentication id's all permissions.
         */
        public final ArrayList<ACL> CREATOR_ALL_ACL = new ArrayList<ACL>(
                Collections.singletonList(new ACL(Perms.ALL, AUTH_IDS)));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sijie sijie mentioned this pull request Aug 2, 2017
Copy link
Contributor

@lucperkins lucperkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some formatting changes that need to be made to make the security docs more closely resemble the rest of the documentation, but I can spend a few minutes and do that on my own after the PR is submitted. LGTM for now.


Optional settings that are worth considering:

1. tlsClientAuthentication=false: Enable/Disable using TLS for authentication.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config when enabled will authenticate the other end of the communication channel. It should be enabled on both bookie and client for MutualTLS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Optional settings that are worth considering:

1. tlsClientAuthentication=false: Enable/Disable using TLS for authentication.
2. tlsEnabledCipherSuites= A cipher suite is a named combination of authentication, encryption, MAC and key exchange
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a link to all the openssl and JDK cipher suites

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


In the output of this command you should see the server's certificate:

(TBD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more details here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

clientTrustStore=/var/private/tls/client.truststore.jks
clientTrustStorePasswordPath=/var/private/tls/client.truststore.passwd

If client authentication is required, then a keystore must be created for each client, and the bookies' truststores must
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide the config option 'clientAuthentication' example here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sijie
Copy link
Member Author

sijie commented Aug 7, 2017

addressed all comments. @merlimat @kishorekasi @eolivelli

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 looks very good!

@sijie sijie closed this in 49b90a6 Aug 7, 2017
sijie added a commit that referenced this pull request Aug 7, 2017
Descriptions of the changes in this PR:

- add a section for `security`
- overview to introduce security feature
- encryption and authentication using tls
- sasl authentication
- zookeeper authentication

Author: Sijie Guo <sijie@apache.org>

Reviewers: Luc Perkins <lucperkins@gmail.com>, Matteo Merli <mmerli@apache.org>

This closes #350 from sijie/security_docs, closes #349

(cherry picked from commit 49b90a6)
Signed-off-by: Sijie Guo <sijie@apache.org>
caliuf pushed a commit to caliuf/bookkeeper that referenced this pull request Aug 11, 2017
Descriptions of the changes in this PR:

- add a section for `security`
- overview to introduce security feature
- encryption and authentication using tls
- sasl authentication
- zookeeper authentication

Author: Sijie Guo <sijie@apache.org>

Reviewers: Luc Perkins <lucperkins@gmail.com>, Matteo Merli <mmerli@apache.org>

This closes apache#350 from sijie/security_docs, closes apache#349
@sijie sijie deleted the security_docs branch July 16, 2018 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants