Adding TLS support on the deployed Kafka cluster #487

ppatierno · 2018-06-11T14:42:02Z

Type of change

Enhancement / new feature

Description

This is a WIP PR for adding TLS support.
It provides a library for generating certificates and secrets and the CO mechanics for using it in order to add TLS support to the cluster.
As WIP it has some TODO and missing point. The PR is open in order to discuss about the way for doing this.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging

ppatierno · 2018-06-11T14:44:29Z

@tombentley @scholzj pay attention that part of the PR is related to the library you already reviewed in the previous PR. It was closed automatically when I changed the branch name this morning. So please concentrate on the CO stuff :-)

tombentley

Looks good so far. The devil's in those TODOs though :-)

tombentley · 2018-06-11T15:04:58Z

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java

@@ -34,7 +35,7 @@
    private final String metricsCmJson = "{\"animal\":\"wombat\"}";
    private final String configurationJson = "{\"foo\":\"bar\"}";
    private final ConfigMap cm = ResourceUtils.createKafkaClusterConfigMap(namespace, cluster, replicas, image, healthDelay, healthTimeout, metricsCmJson, configurationJson);
-    private final KafkaCluster kc = KafkaCluster.fromConfigMap(cm);
+    private final KafkaCluster kc = KafkaCluster.fromDescription(cm, Collections.EMPTY_LIST);


Minor point of style: I usually use Collection.emptyList(), which lets the compiler infer the type arguments for the resulting list, and avoids raw types leaking into the code.

ppatierno · 2018-06-11T15:22:05Z

@tombentley I know that the devil is elsewhere but I needed to have a starting point for opening the PR ... I'm walking around the devil for attacking it at right moment :P

ppatierno · 2018-06-12T16:36:37Z

@scholzj @tombentley added logic for certificates and secrets generation until exposing them to the brokers through related volumes. Maybe it's better having another step in the review process for the overall solution. I'll fix conflicts with master later.

tombentley · 2018-06-13T02:35:39Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/AbstractModel.java

+    }
+
+    protected void setEncryptionEnabled(boolean isEncryptionEnabled) {
+        this.isEncryptionEnabled = isEncryptionEnabled;


Just wonding is there's any use case for just enabling encryption on interbroker or clients side and not the other. But I can't think of one.

tombentley · 2018-06-13T02:44:01Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

@@ -317,6 +402,52 @@ public ConfigMap generateMetricsConfigMap() {
        }
    }

+    public Secret generateClientsCASecret() {


Can we javadoc these generate*() methods to the extent that just from looking at the code we know how the CAs and certs are related?

Yes it was on the plan ;)

tombentley · 2018-06-13T02:46:47Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+        return createSecret(name + "-clients-ca", data);
+    }
+
+    public Secret generateClientsPublicSecret() {


'Public secret' is an oxymoron. I think something like generateClientsPublicKeySecret() would be less jarring.

tombentley · 2018-06-13T02:54:05Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+
+                            Labels labels = Labels.forCluster(assemblyName);
+                            List<Secret> secrets = secretOperations.list(namespace, labels);
+                            secrets.add(secretOperations.get(namespace, "internal-ca"));


I don't understand what's going on here, maybe a comment is needed: I assume the "internal-ca" isn't labelled with the assemblyName (and so has to be added to the list explicitly), but I don't understand why.

The "internal-ca" CA certificate (key + cert) is related to the all clusters deployed by the operator. I mean that if you deploy a kafka and a kafka-connect cluster we are going to use the same CA for signing certificates related to internal communication (i.e. a kafka-connect connector connecting to the kafka cluster).
One doubt that I have now is that the same CA cert could be used even by different Kafka clusters deployed by the CO which should not be the right thing to do. @scholzj wdyt ?
I was thinking about a specific label for that but I didn't come up with a good proposal.

tombentley · 2018-06-13T02:55:45Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

                            } else {
-                                handler.handle(createResult);
+                                log.error(certResult.cause().toString());


No need to release the lock in this branch?

tombentley · 2018-06-13T03:05:08Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

+    private Secret clientsCASecret;
+    private Secret clientsPublicSecret;
+    private Secret brokersClientsSecret;
+    private Secret brokersInternalSecret;


I don't think you can to this. The KafkaAssemblyOperator is stateless, and while the lock prevents it running concurrently on the same cluster, I don't think there's anything preventing it running simultaneously across different clusters.

I think you need to make the chained compose() pass this data via the (mostly currently ignored) lambda arguments. You could just simple java bean (hell, I'm not sure even getters are really necessary) to do that. It could be immutable in the above fields, but would need to be mutable in diffs and scale. That way you could just pass the one thing though the chain of compose()s. Wdyt?

If I have got your idea ....

First of all, all the above fields plus service, headlessService and so on seems to be not needed because they come from kafka.generateXXXXX which can be used in the compose() chain.
It seems to me that just the kafka instance is needed.
The getKafkaCluster could return a Future<KafkaCluster> in order to have the kafka instance available to the first compose for calling kafka.getName() and kafka.getReplicas().
The first compose could return a Future<MyBean> where MyBean should bring the kafka instance and the result of the reconcile in the first compose; in this way the kafka instance can be used in the next compose and so on.
Did I get your point correctly ?

ppatierno · 2018-06-14T11:11:48Z

@scholzj @tombentley I have added the secrets deletion on cluster deletion and the certificates handling when a reconciliation starts (no need to re-generate certs), a scale-up (generate the missing certs and update secrets), a scale-up (leaving just the needed certs and update secrets). A new review is really welcome. I'll fix conflicts with master later.

tombentley · 2018-06-14T13:10:14Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java


+                int replicasInSecret = 0;
+                Optional<Secret> internalSecret = secrets.stream().filter(s -> s.getMetadata().getName().equals(name + "-brokers-internal")).findFirst();


Long line (breaks after filter() and findFirst())

tombentley · 2018-06-14T13:11:00Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+
+                if (internalSecret.isPresent() ^ clientsSecret.isPresent()) {
+                    log.error("Inconsistency!!!");
+                    // TODO: what to do ? raising exception ?


Just regenerate the missing one?

Yeah I think that it's a good way to fix it. Instead, the other case where sizes are different should never happen; I'll raise an exception for that one.

I decided to generate certificates separately factoring out a method for that and fixing all scenario with missing certs just generating them.

tombentley · 2018-06-14T13:12:45Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+                    // TODO: what to do ? raising exception ?
+                } else {
+
+                    if (internalSecret.isPresent() && clientsSecret.isPresent()) {


this can be an else if of the enclosing if

tombentley · 2018-06-14T13:14:18Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+                } else {
+
+                    if (internalSecret.isPresent() && clientsSecret.isPresent()) {
+                        if ((internalSecret.get().getData().size() - 1) != (clientsSecret.get().getData().size() - 2)) {


What's going on with the - 1 and - 2? It probably deserves a comment.

tombentley · 2018-06-14T13:18:43Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+                    internalCerts.put(
+                            KafkaCluster.kafkaPodName(cluster, i),
+                            new Cert(
+                                    decoder.decode(internalSecret.get().getData().get(KafkaCluster.kafkaPodName(cluster, i) + ".key")),


I think you should refactor the decoder.decode(internalSecret.get().getData().get(KafkaCluster.kafkaPodName(cluster, i) + ".key")) stuff into separate methods (one for .key, andother for .crt). You seem to be repeating this code a lot.

tombentley · 2018-06-14T13:19:57Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

@@ -302,15 +357,12 @@ public void generateCertificates(List<Secret> secrets) {
                            new Cert(Files.readAllBytes(brokerKeyFile.toPath()), Files.readAllBytes(brokerCertFile.toPath())));

                    certManager.generateCsr(brokerKeyFile, brokerCsrFile, sbj);
-                    certManager.generateCert(brokerCsrFile, clientsCAkeyFile, clientsCAcertFile, brokerCertFile, 365);


365 should at least be a constant. Should it be configurable?

tombentley · 2018-06-14T13:24:13Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

-    private final Future<Void> getKafkaCluster(ConfigMap assemblyCm, List<Secret> assemblySecrets) {
-        Future<Void> fut = Future.future();
+    /**
+     * Brings the description of a Kafka cluster entity


Probably worth a sentence or two about how this is used in the compose() chains, because just reading those compose chains it's not completely obvious what's going on.

scholzj

I added bunch of comments, most of them should be fairly minor.

But there is one big issue I have with this. We might have split the tasks in a wrong way. But without adding TLS support to Kafka Connect / Connect S2I and Topic Controller, this will actually break them. So I think we need to add that within this PR.

scholzj · 2018-06-18T17:57:04Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+        if (isEncryptionEnabled()) {
+            Map<String, String> data = new HashMap<>();
+            data.put("clients-ca.crt", Base64.getEncoder().encodeToString(clientsCA.cert()));
+            return createSecret(name + "-clients-ca-cert", data);


This is a resource which will be used by the users. Should we think of a shorter (and thus more user friendly name)?

May you suggest a better name please? It was the best I had in mind :-)

Why not just name it -cert? Right now it is the only cert needed for talking with Kafka for users.

scholzj · 2018-06-18T18:01:09Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+        vertx.createSharedWorkerExecutor("kubernetes-ops-pool").executeBlocking(
+                future -> {
+
+                    if (secretOperations.get(namespace, "internal-ca") == null) {


If we want to enable / disable SSL ... how do we get this information to this place?

As agreed on Strimzi/Kafka channel, we decided to create this cert in any case even if SSL was disabled because we don't have the enable/disable information here.

Ahh, right ... my bad ... I forgot it already.

If it was me, I would pass the cm into this method, and have the handler be a Handler<AsyncResult<List<Secret>>>. In the case the feature was enabled (which it could glean from the CM) it would be a noop and would just return an empty list.

It's a solution but we are giving the logic to get information from the cm to the secretOperations (so SecretOperator class) which in my understanding should not have this kind of knowledge but just handling Secret resources and nothing more. Or maybe I was wrong to think that.

If we want to avoid to generate the cert, the AbstractOperatorAssembly has the ConfigMap in the reconcileAssembly so it should be able to get the TLS enable status as well from there; even in this case it sounded to me as breaking some rules to have the operator assembly looking into something it shouldn't be interested in.

Ah, yes you're right. Then I guess we should use a compose at the call site then.

@tombentley wdym ?

scholzj · 2018-06-18T18:02:45Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+                future -> {
+
+                    if (secretOperations.get(namespace, "internal-ca") == null) {
+                        log.info("Generating internal CA certificate ...");


Although I personally tend to use ... quite often, it looks bit strange in logs. Especially the second one starting with ....

scholzj · 2018-06-18T18:03:57Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+                        }
+                        log.info("... end generating certificate");
+                    } else {
+                        log.info("The internal CA certificate already exists");


Assuming this runs on every reconciliation (since the certificates will exist most of the time) ... is the INFO level really needed? I think we should put it to lower log level.

Agreed. I left this level by mistake. I needed it for testing.

scholzj · 2018-06-18T18:04:29Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+                            Labels labels = Labels.forCluster(assemblyName);
+                            List<Secret> secrets = secretOperations.list(namespace, labels);
+                            secrets.add(secretOperations.get(namespace, "internal-ca"));
+                            //Map<String, Secret> map = secrets.stream().collect(Collectors.toMap(s -> s.getMetadata().getName(), s -> s));


Can this be deleted?

scholzj · 2018-06-18T18:14:36Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

@@ -136,6 +251,10 @@ public void createOrUpdate(Reconciliation reconciliation, ConfigMap assemblyCm,
        result.add(serviceOperations.reconcile(namespace, KafkaCluster.kafkaClusterName(name), null));
        result.add(serviceOperations.reconcile(namespace, KafkaCluster.headlessName(name), null));
        result.add(kafkaSetOperations.reconcile(namespace, KafkaCluster.kafkaClusterName(name), null));
+        result.add(secretOperations.reconcile(namespace, KafkaCluster.kafkaClusterName(name) + "-clients-ca", null));


Same as above ... the secret suffixes should be defined centrally.

scholzj · 2018-06-18T18:16:09Z

...rc/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaConnectAssemblyOperator.java

        this.serviceOperations = serviceOperations;
        this.deploymentOperations = deploymentOperations;
    }

    @Override
-    protected void createOrUpdate(Reconciliation reconciliation, ConfigMap assemblyCm, Handler<AsyncResult<Void>> handler) {
+    protected void createOrUpdate(Reconciliation reconciliation, ConfigMap assemblyCm, List<Secret> assemblySecrets, Handler<AsyncResult<Void>> handler) {


This seems a bit unfortunate design that this signature is shared across all AssemblyOperators. But probably nothing to change within this PR.

TBH, from the previous discussions I understood that this will be the place where the Description class will be used to make it possible to wrap the different requirements for each *Cluster class.

scholzj · 2018-06-18T18:21:07Z

docker-images/kafka/scripts/kafka_run.sh

+    # Generate the SSL healthcheck config file
+    ./kafka_healthcheck_ssl_config.sh | tee /tmp/healthcheck.properties
+
+    export KAFKA_SECURITY="listener.name.replication.ssl.keystore.location=/var/lib/kafka/replication.keystore.jks


/var/lib/kafka is the PV (or emptyDir). We definitely donm't want to store the certificates / keys here. This should be probably somewhere in /tmp. This applies to all the certs.

scholzj · 2018-06-18T18:23:12Z

docker-images/kafka/scripts/kafka_run.sh

+    ./kafka_healthcheck_ssl_config.sh | tee /tmp/healthcheck.properties
+
+    export KAFKA_SECURITY="listener.name.replication.ssl.keystore.location=/var/lib/kafka/replication.keystore.jks
+listener.name.replication.ssl.keystore.password=${ENC_PASSWORD}


The password is the same. Can't we just use it once on the upper configuration level? (ssl.keystore.password)

Yeah you are right it always fallback to this one if the specific listener one is not defined.

scholzj · 2018-06-18T18:24:37Z

docker-images/kafka/scripts/kafka_run.sh

+    keytool -keystore /var/lib/kafka/replication.truststore.jks -storepass $ENC_PASSWORD -noprompt -alias internal-ca -import -file /var/lib/kafka/internal-certs/internal-ca.crt
+    openssl pkcs12 -export -in /var/lib/kafka/internal-certs/$HOSTNAME.crt -inkey /var/lib/kafka/internal-certs/$HOSTNAME.key -chain -CAfile /var/lib/kafka/internal-certs/internal-ca.crt -name $HOSTNAME -password pass:$ENC_PASSWORD -out /var/lib/kafka/$HOSTNAME-internal.p12
+    keytool -importkeystore -deststorepass $ENC_PASSWORD -destkeystore /var/lib/kafka/replication.keystore.jks -srcstorepass $ENC_PASSWORD -srckeystore /var/lib/kafka/$HOSTNAME-internal.p12 -srcstoretype PKCS12
+    echo "... end importing certificates"


Again, I don't think this style of log messages looks so great. Should we change it?

scholzj · 2018-06-19T13:11:39Z

Just realised during the current conference call - you are missing RBAC rules update for secrets, or?

ppatierno · 2018-06-19T15:43:09Z

@scholzj right. I have just fixed that.

ppatierno · 2018-06-22T10:56:32Z

@scholzj @tombentley this PR lacks the documentation only.

scholzj

Starts taking a good shape. Just few minor comments. Looking forward to the docu.

scholzj · 2018-06-22T20:59:53Z

.checkstyle/checkstyle.xml

@@ -113,7 +113,7 @@
        </module>
        <module name="NPathComplexity">
            <!-- default is 200 -->
-            <property name="max" value="500"/>
+            <property name="max" value="900"/>


Not really a question for this PR but more for the community. Why the hell do we have these rules if every second PR increases them to pass? Should we either follow them or remove them?

That is a good question and maybe we should discuss about it with the entire team. Sometimes I don't see any reason to spend more time to be compliant with the current rules (so I just increase them).

I'm wiling to bet this was increased because of AbstractModel. I know that because I've also had to deal with AbstractModel getting increasingly cumbersome, and having to increase these parameters. So in that sense checkstyle has fulfilled its purpose of highlighting to us that there's a problem with AbstractModel. I'm hoping we'll be able to refactor AbstractModel soon and revert these parameters.

Really it's incumbent on whoever increases these to open (or ensure there is already open) a PR to address the complexity in a future sprint.

scholzj · 2018-06-22T21:16:53Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/AbstractModel.java

@@ -58,6 +62,8 @@

    protected static final Logger log = LogManager.getLogger(AbstractModel.class.getName());

+    protected static final int DEFAULT_CERTS_EXPIRATION_DAYS = 365;


We currently do not support any changes to this. So it should not be called DEFAULT_CERTS_EXPIRATION_DAYS but just CERTS_EXPIRATION_DAYS? When I see DEFAULT_ I normally expect that the actual used value would be somewhere else.

Yeah agree on that.

scholzj · 2018-06-22T21:18:23Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

@@ -40,22 +47,30 @@
    private static final Logger log = LogManager.getLogger(AbstractAssemblyOperator.class.getName());

    protected static final int LOCK_TIMEOUT = 60000;
+    protected static final int DEFAULT_CERTS_EXPIRATION_DAYS = 365;


We should not define this here - we should reuse the value from AbstractModel, or? To have the value defined in single place only.

I don't have any specific use case in mind but maybe could be useful to have the "general" internal CA certificates having a different expiration date from the other "cluster specific" certificates?

scholzj · 2018-06-22T21:20:27Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

@@ -46,9 +57,16 @@
    protected static final int REPLICATION_PORT = 9091;
    protected static final String REPLICATION_PORT_NAME = "replication";

+    protected static final int CLIENT_ENC_PORT = 9093;
+    protected static final String CLIENT_ENC_PORT_NAME = "clientsenc";


It took me a while to figure out what it means. Is it just me? Wouldn't something like clientstls be better? Ot maybe just name them plaintext and ssl to follow Kafka terminology.

Yeah ENC stays for "ENCrypted". I thought a lot about it if using SEC for "SECurity" was better or using TLS as you are suggesting. I have no strong opinion on that so TLS works fine. I'll change everywhere ENC is used and using TLS instead.

…elated secrets

Fixed some minor changes after feedback Dealing with "boring" checkstyle rules

Run getting Kafka cluster description in a worker thread

Added generation of CA certificate for clients communication Added generation of the needed secrets for hosting certs/keys related to the Kafka cluster Added volumes and mount points for exposing certs/keys to each Kafka broker Added/Changed few overloads on ssl library + tests Fixed changes about Collections emptylist

Added some Javadoc to generate secret methods

Added certificates and secrets handling on reconcilian, scale up/down checking certificates exist or not

Added some Javadoc Defined a constant for certificates expiration days Addressed review feedback about secret suffixes, certs location and logging style Reverted back to fromConfigMap method New ResourceTester ctor for Secrets list Dealing with "boring" findbugs warnings

Updated the REPLICATION listener to be encrypted by default Removed enable/disable encryption

Added utility methods for generating Secret collection for the Kafka cluster Adapted KafkaAssemblyOperator tests in order to use Kafka cluster Secrets Updated and fixed Kafka and Connect assembly operator tests using certificates Secrets Added build Secrets to the MockKube Added assertions for Secrets

Fixed clients encrypted listener parameters for keystore and truststore Fixed path for the findbugs-exclude.xml (for Travis CI)

ppatierno · 2018-06-27T13:48:07Z

@tombentley @scholzj I wrote the documentation in a new Security chapter. Please check if we want to dig into the feature more than that or we have to add something that I forgot.
PS : the PR is not WIP anymore.

tombentley

I'll try to review the docs later.

tombentley · 2018-06-27T14:30:59Z

.checkstyle/checkstyle.xml

@@ -113,7 +113,7 @@
        </module>
        <module name="NPathComplexity">
            <!-- default is 200 -->
-            <property name="max" value="500"/>
+            <property name="max" value="900"/>


I'm wiling to bet this was increased because of AbstractModel. I know that because I've also had to deal with AbstractModel getting increasingly cumbersome, and having to increase these parameters. So in that sense checkstyle has fulfilled its purpose of highlighting to us that there's a problem with AbstractModel. I'm hoping we'll be able to refactor AbstractModel soon and revert these parameters.

Really it's incumbent on whoever increases these to open (or ensure there is already open) a PR to address the complexity in a future sprint.

tombentley · 2018-06-27T14:32:53Z

.findbugs/findbugs-exclude.xml

+    <Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2"/>
+  </Match>
+  <Match>
+    <Package name="io.strimzi.certs"/>


I think (but would be wrong) that this is excluding those bugs (from all packages) and all bugs in that package (i.e. the union), which isn't what you meant. I think you need both the Bug and the Package inside a single Match to get their intersection.

Good point. It should work as you said. I moved Package into the first Match and it works.

tombentley · 2018-06-27T14:37:49Z

certificate-manager/src/main/java/io/strimzi/certs/Cert.java

+ */
+package io.strimzi.certs;
+
+public class Cert {


I have a feeling I've asked this already, but if this class has both a key and a cert is Cert really the right name for it? Should it be CertAndKey?

The only doubt is if it could have something more than just key and cert in the future. Maybe in that case even Cert can be wrong ... something with `Bundle' in the name?

We have to give it the best name for the code as it is today, I think. It would be easy enough to change the class name at the time we added the extra thing. Or maybe if there was a need for an extra thing in the future that would be a different class (maybe a subclass).

tombentley · 2018-06-27T14:55:19Z

certificate-manager/src/main/java/io/strimzi/certs/OpenSslCertManager.java

+            if (sbj.subjectAltNames() != null && sbj.subjectAltNames().size() > 0) {
+
+                // subject alt names need to be in an openssl configuration file
+                File file = new File(getClass().getClassLoader().getResource("openssl.conf").getFile());


I assume this works because you'll've tested it, but really getFile() on a URL does something different that what you're assuming here -- it returns the URL's path as a File. I don't see how that would work for a resource in a jar on a file system, because File doesn't understand about the !/ (or is ir /!) separator in the resource path used to delimit the end of the file on the filesystem, the remaining path being the path within the zip archive. It might not work on all platforms though. Really you should be getting the resource as a stream, rather than assuming you can access it as a File.

It works at least in the unit tests because we don't have a real use case for now but in any case I changed to load data as resource stream. In any case I put them into a File used by the methods exposed in the cert manager class.

OK, I guess maven runs the tests with the .class and resource files on the filesystem and the -classpath pointing to the directory containing them, so the resource URL would be a file:// URL and so it works. I suspect if you could jar up the test .class and resource files and run the tests from that jar it would fail.

tombentley · 2018-06-27T15:07:12Z

certificate-manager/src/main/java/io/strimzi/certs/OpenSslCertManager.java

+        PrintWriter out = null;
+        try {
+            out = new PrintWriter(new BufferedWriter(new OutputStreamWriter(new FileOutputStream(sna, true), "UTF8")));
+            out.append(sb.toString());


Why are you building the sb only to write it to out? Why not just skip sb and write it to out directly? Also PrintWriter will swallow exceptions and you need to invoke checkError() to determine if it did so. Personally I'd just stick with calling write() on the BufferedWriter, since you're having to catch the IOException anyway.

tombentley · 2018-06-27T15:31:36Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+                // CA private key + self-signed certificate for clients communications
+                Optional<Secret> clientsCAsecret = secrets.stream().filter(s -> s.getMetadata().getName().equals(KafkaCluster.clientsCASecretName(cluster))).findFirst();
+                if (!clientsCAsecret.isPresent()) {
+                    log.info("Clients CA to generate");


tombentley · 2018-06-27T15:33:45Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+        // scale up -> it will copy all certificates
+        // scale down -> it will copy just the requested number of replicas
+        for (int i = 0; i < Math.min(replicasInSecret, replicas); i++) {
+            log.info("{} already exists", KafkaCluster.kafkaPodName(cluster, i));


This seems like a weird and not-very-useful log message.

Well I think that maybe just the log level is wrong and debug should be ok. With log on the next for, it provides information on what certs are going to be "reused" and what certs are going to be created.

tombentley · 2018-06-27T15:35:41Z

...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/AbstractAssemblyOperator.java

+                        secretOperations.reconcile(namespace, INTERNAL_CA_NAME, secret)
+                                .compose(future::complete, future);
+
+                    } catch (IOException e) {


catch Throwable, I would have thought.

tombentley · 2018-06-27T16:02:42Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

+                                    kafka.generateBrokersClientsSecret(), kafka.generateBrokersInternalSecret());
+
+                    future.complete(desc);
+                } catch (Exception e) {


catch Throwable

tombentley · 2018-06-27T16:03:08Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

+                if (res.succeeded()) {
+                    fut.complete((KafkaClusterDescription) res.result());
+                } else {
+                    fut.fail("");


fut.fail(res.cause())

tombentley · 2018-06-28T10:05:17Z

In "real code" I would tend to agree, but this is test code where the contract for anything annotated @test is that it can throw anything anyway.

…

On Thu, Jun 28, 2018 at 10:38 AM, Paolo Patierno ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In certificate-manager/src/test/java/io/strimzi/certs/ OpenSslCertManagerTest.java <#487 (comment)> : > + + testGenerateSelfSignedCert(key, cert, sbj); + + key.delete(); + cert.delete(); + } + + private void testGenerateSelfSignedCert(File key, File cert, Subject sbj) throws IOException, CertificateException { + ssl.generateSelfSignedCert(key, cert, sbj, 365); + + Certificate c = certFactory.generateCertificate(new FileInputStream(cert)); + + try { + c.verify(c.getPublicKey()); + } catch (Exception e) { + fail(); In general I prefer to have specific exceptions in that list but I'm fine with your proposal if you want. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#487 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1rf0dlP5hNSlaG3-ums1JQjN2p1uDUks5uBKQxgaJpZM4Ui08O> .

tombentley · 2018-06-28T16:49:31Z

You can add a specific one, I guess.

ppatierno · 2018-06-29T08:05:33Z

@tombentley @scholzj this is the final PR. I have handled all your feedback.

scholzj · 2018-07-02T07:06:44Z

documentation/book/security.adoc

+
+The Apache Kafka project supports data encryption by means of the SSL/TLS protocol.
+This makes it possible to encrypt data transferred between brokers (interbroker communication) and between clients and brokers.
+Leveraging the SSL/TLS support, it is also possible to have mutual authentication, where the Kafka broker authenticates the client by verifying its certificate, and the client does likewise with the server's certificate.


Do we actually support this in this PR?

No, it's out of the scope for this PR. Do you think we should remove this statement because it sounds that we are going to support client auth right now?

That is how I would interpret it ... that we support TLC client auth. So I would probably remove it.

scholzj · 2018-07-02T07:07:50Z

documentation/book/security.adoc

+The public key has to be signed by a certificate authority (CA) in order to have a related X.509 certificate for providing server authentication and encrypting the communication channel with the client (which could be another broker as well).
+More precisely, each broker has two different certificates (and related private keys), one for interbroker communication  and one for communication with clients.
+These broker certificates are signed using different CAs: The "internal-ca" signs the certificates used for interbroker communication, and the "clients-ca" signs the certificates used for client communication.
+The CAs themselves use self-signed certificates.


CA is always self-signed. So I'm, not sure we need to mention this here.

Well in a PKI you can have different CA certificates at different levels. The first one is self-signed but the other ones down the chain could be signed by the previous one (so not be self-signed), or ?

Yeah, but if you say CA, I expect the root certificate ... and that is always self-signed if its a root, right? Anyway ... I don't mind this, it just seemed strange.

scholzj · 2018-07-02T07:08:44Z

documentation/book/security.adoc

+More precisely, each broker has two different certificates (and related private keys), one for interbroker communication  and one for communication with clients.
+These broker certificates are signed using different CAs: The "internal-ca" signs the certificates used for interbroker communication, and the "clients-ca" signs the certificates used for client communication.
+The CAs themselves use self-signed certificates.
+The "internal-ca" is used across multiple clusters, that is, there is a single "internal-ca" for each instance of a cluster operator. 


Really? I thought the original installation had it per cluster. Having it per CO is IMHO a risk. We should have at least an issue for fixing it.

Also, how does the internal CA work when it is in a different namespace than the Kafka cluster?

I thought we agreed on having this way otherwise there were no reason for having the AbstractAssemblyOperator creating the internal-ca (which was something we agreed as well in the previous reviews); it's done in this way because it was not related to the specific cluster. Moving to be cluster specific means changing the current logic and having KafkaCluster handling this creation as all the other certificates.
I think that at this point an issue is better than changing this PR.

Well, it cannot be part of KafkaCluster, because the first one using it will be ZookeeperCluster. So IMHO it needs to be created in KafkaAssemblyOperator or even in AbstractAssemblyOperator, but based on a cluster. But I don't think it should be CO wide. That would mean that the clusters are not secured against each other (if I have access to cluster A I can use the keys to authenticate to cluster B).

I also didn't saw anywhere any handling of the namespace issue - that would be much easier with per cluster internal CA.

Good point. I'm going to open an issue about that as soon as we'll merge this one. I'll work on this issue but maybe it's better for now merging a few stuff with CRD and Zk/Kafka log levels instead of adding code which generates more conflicts. It'll avoid to add pains on ... pains :-)

scholzj · 2018-07-02T07:09:50Z

documentation/book/security.adoc

+* `<cluster-name>-kafka-brokers-internal`: contains all the brokers private and public keys (certificates signed with "internal-ca") used for interbroker communication.
+* `<cluster-name>-kafka-brokers-clients`: contains all the brokers private and public keys (certificates signed with specific cluster "clients-ca") used for communicating with clients.
+
+All the keys are 2048 bits in size and are valid for 365 days from initial generation.


We should probably mention that certificate rotation is on the backlog.

scholzj · 2018-07-02T07:12:30Z

documentation/book/security.adoc

+[source,shell]
+keytool -keystore client.truststore.jks -alias CARoot -import -file clients-ca.crt
+
+Finally, the minimal content for the properties file used by the Kafka client is the following (the password can be omitted if not needed).


Are people actually using properties files to configure the clients?

Do you mean providing an example on passing this parameters to the console consumer on command line?

I would probably just leave out the file ... and say something like To configure the Kafka Consumer or Producer APIs, following properties should be specified.

scholzj · 2018-07-02T07:12:51Z

documentation/book/security.adoc

+[source]
+security.protocol=SSL
+ssl.truststore.location=/var/private/ssl/client.truststore.jks
+ssl.truststore.password=test1234


We should probably use some placeholder instead of test1234.

scholzj · 2018-07-02T07:18:28Z

docker-images/kafka/scripts/kafka_run.sh

+
+mkdir -p /tmp/kafka
+
+# Import certificates into keystore and truststore


What about moving these 3 sections into a separate file?

scholzj · 2018-07-02T07:24:54Z

certificate-manager/src/test/java/io/strimzi/certs/OpenSslCertManagerTest.java

+
+            assertEquals(String.format("CN=%s, O=%s", sbj.commonName(), sbj.organizationName()), p.getName());
+
+            // subject alternative names are not transferred automatically from a CSR to the final certificate


I think I asked it already somewhere, but I'm not sure I got an answer.What does this mean for us? Does it mean that we need to disable hostname verification?

For the current iteration yes. I guess it's the same problem EnMasse team has today, right? I opened an issue for investigating on how having SAN working support when using openssl.

Ok, if the clients need to disable hostname verification, it probably should be mentioned in the docu (ideally with a record in the properties example). There was a KIP which enabled it by default which I guess ended in Kafka 2.0 - so users will need to disable it. We should also hjave an issue for it - because this is quite an important problem.

tombentley · 2018-06-28T09:37:06Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+            }
+
+        } catch (IOException e) {
+            e.printStackTrace();


Yes, that's a good idea. If nothing else it will be much more informative in the logs.

scholzj

LGTM

Removed TODO about copy_extensions Added raising exception if internal CA certificate Secret is missing Changed log level to DEBUG for the openssl command output Doc improvments? (#523) * Doc improvments? * Update security.adoc * Update security.adoc Splitted in different scripts certs import into keystore/truststore Addresses feedback about security doc Last doc fix

ppatierno · 2018-07-02T09:56:14Z

So I'm going to merge. I have already squashed "minor" commits in the the main ones steps representing "mailstones" in the PR.

ppatierno requested review from tombentley and scholzj June 11, 2018 14:42

tombentley reviewed Jun 11, 2018

View reviewed changes

tombentley reviewed Jun 13, 2018

View reviewed changes

tombentley reviewed Jun 14, 2018

View reviewed changes

scholzj reviewed Jun 18, 2018

View reviewed changes

ppatierno force-pushed the tls-support branch from e139cc0 to 26eb6a2 Compare June 19, 2018 10:55

ppatierno force-pushed the tls-support branch 3 times, most recently from 131f1cc to 65d7ab1 Compare June 19, 2018 15:42

ppatierno force-pushed the tls-support branch from bb76ea6 to 2b30403 Compare June 20, 2018 09:21

scholzj mentioned this pull request Jun 21, 2018

Providing a schema registry implementation #29

Closed

ppatierno force-pushed the tls-support branch 2 times, most recently from 4e3a8c2 to 7534bee Compare June 22, 2018 10:49

scholzj reviewed Jun 22, 2018

View reviewed changes

ppatierno added 7 commits June 27, 2018 10:22

Added first draft of common component for handling certificates and r…

59d1729

…elated secrets

Added support for subject alternative names

e9c2d6a

Fixed some minor changes after feedback Dealing with "boring" checkstyle rules

Added cluster secrets for getting Kafka cluster description

1ba07aa

Run getting Kafka cluster description in a worker thread

Applied changes to KafkaAssemblyOperator to avoid state

0fbac35

Added some Javadoc to generate secret methods

Ignored defaultMode on secret volumes for diff

9d34feb

Added secrets deletion

04cece7

Added certificates and secrets handling on reconcilian, scale up/down checking certificates exist or not

ppatierno added 5 commits June 27, 2018 10:22

Added Kafka brokers configuration with TLS/SSL support

4a8c42b

Refactored way to generate certificates for handling edge cases better

4835927

Added a new CLIENT_ENC port 9093 for encrypted traffic (from clients)

0d4a2e7

Updated the REPLICATION listener to be encrypted by default Removed enable/disable encryption

ppatierno force-pushed the tls-support branch 2 times, most recently from a65e87a to 2e35d88 Compare June 27, 2018 10:26

Some renaming on TLS ports, password and expiration date

d0ae530

Fixed clients encrypted listener parameters for keystore and truststore Fixed path for the findbugs-exclude.xml (for Travis CI)

ppatierno force-pushed the tls-support branch from 5ac754b to d0ae530 Compare June 27, 2018 13:44

Added documentation about TLS support

7288fa7

ppatierno changed the title ~~WIP : adding TLS support~~ Adding TLS support on the deployed Kafka cluster Jun 27, 2018

ppatierno added this to the 0.5.0 milestone Jun 27, 2018

tombentley reviewed Jun 27, 2018

View reviewed changes

ppatierno force-pushed the tls-support branch from 2343f10 to 133392e Compare June 28, 2018 16:19

ppatierno force-pushed the tls-support branch from 1df60f7 to aea9e6c Compare June 29, 2018 08:04

scholzj reviewed Jul 2, 2018

View reviewed changes

tombentley approved these changes Jul 2, 2018

View reviewed changes

ppatierno force-pushed the tls-support branch from b8357a0 to 58a2e3c Compare July 2, 2018 09:07

scholzj approved these changes Jul 2, 2018

View reviewed changes

ppatierno force-pushed the tls-support branch from cfba02b to 97236fe Compare July 2, 2018 09:52

ppatierno merged commit 06ac3b0 into master Jul 2, 2018

tombentley mentioned this pull request Jul 3, 2018

Stub CertManager in unit tests #535

Closed


		int replicasInSecret = 0;
		Optional<Secret> internalSecret = secrets.stream().filter(s -> s.getMetadata().getName().equals(name + "-brokers-internal")).findFirst();

		@@ -58,6 +62,8 @@

		protected static final Logger log = LogManager.getLogger(AbstractModel.class.getName());

		protected static final int DEFAULT_CERTS_EXPIRATION_DAYS = 365;


		mkdir -p /tmp/kafka

		# Import certificates into keystore and truststore


		assertEquals(String.format("CN=%s, O=%s", sbj.commonName(), sbj.organizationName()), p.getName());

		// subject alternative names are not transferred automatically from a CSR to the final certificate

Adding TLS support on the deployed Kafka cluster #487

Adding TLS support on the deployed Kafka cluster #487

Conversation

ppatierno commented Jun 11, 2018 • edited Loading

Type of change

Description

Checklist

ppatierno commented Jun 11, 2018

tombentley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno commented Jun 11, 2018 • edited Loading

ppatierno commented Jun 12, 2018

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

Choose a reason for hiding this comment

ppatierno commented Jun 14, 2018

tombentley Jun 14, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj 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

tombentley Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno Jun 19, 2018 • 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

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

scholzj commented Jun 19, 2018

ppatierno commented Jun 19, 2018

ppatierno commented Jun 22, 2018

scholzj 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno commented Jun 27, 2018

tombentley 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

ppatierno commented Jun 11, 2018 •

edited

Loading

ppatierno commented Jun 11, 2018 •

edited

Loading

tombentley Jun 14, 2018 •

edited

Loading

tombentley Jun 19, 2018 •

edited

Loading

ppatierno Jun 19, 2018 •

edited

Loading