Configurable authentication, authorization, and SSL for Cruise Control API #5073

kyguy · 2021-06-02T15:20:52Z

Type of change

Enhancement / new feature

Description

Locks down the Cruise Control API by enforcing API authentication, authorization, and SSL by default. But gives users the flexibility to disable these security configurations [1] if needed through the spec.cruiseControl.config section the the Kafka resource. e.g.

apiVersion: kafka.strimzi.io/v1beta2
kind: Kafka
metadata:
  ...
spec:
  cruiseControl:
    config:
      webserver.security.enable: false  # Disables CC HTTP Basic authentication
      webserver.ssl.enable: false       # Disables CC API SSL

Addresses #3770

[1] https://github.com/linkedin/cruise-control/wiki/Security

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
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

scholzj

The file api/src/test/resources/io/strimzi/api/kafka/model/040-Crd-kafka.yaml should not exist anymore. You might need to rebase the PR.

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

scholzj · 2021-06-30T06:52:54Z

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

@@ -129,8 +129,10 @@ static EnvVar tlsSidecarLogEnvVar(TlsSidecar tlsSidecar) {
    }

    public static Secret buildSecret(Reconciliation reconciliation, ClusterCa clusterCa, Secret secret, String namespace, String secretName,
-                                     String commonName, String keyCertName, Labels labels, OwnerReference ownerReference, boolean isMaintenanceTimeWindowsSatisfied) {
-        Map<String, String> data = new HashMap<>(4);
+                                     String commonName, String keyCertName, Labels labels, Map<String, String> data, OwnerReference ownerReference, boolean isMaintenanceTimeWindowsSatisfied) {


This method main focus is to deal with the certificate secrets. I do not think you should mix any other data into it. You should add it in some other way or maybe even better, create additional secret instead of using the certificate secrets.

Following your suggestion, I have just separated the Cruise Control API credentials its own secret in the latest commit

scholzj · 2021-06-30T06:57:17Z

operator-common/src/test/java/io/strimzi/operator/common/operator/MockCertManager.java

+    public static String clientsCaCertStorePassword() {
+        return Base64.getEncoder().encodeToString(CLIENTS_CERT_STORE_PASSWORD.getBytes(Charset.defaultCharset()));
+    }
+


How is the secret mocked in other tests without this method? Maybe you can use the same instead of adding this?

It is hard-coded in the other tests e.g. [1] [2] I figured we could refactor the other occurances in a separate PR since there are a lot of other references [3]

[1]

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

Line 74 in 2015404

ssl.addCertToTrustStore(cert, "ca", store, "123456");

[2]

strimzi-kafka-operator/cluster-operator/src/test/java/io/strimzi/operator/cluster/ResourceUtils.java

Line 211 in 76cd806

AbstractModel.clusterCaCertSecretName(name), MockCertManager.clusterCaCert(), MockCertManager.clusterCaCertStore(), "123456"));

[3] https://github.com/strimzi/strimzi-kafka-operator/search?q=123456

scholzj · 2021-06-30T06:59:03Z

docker-images/kafka/cruise-control-scripts/cruise_control_config_generator.sh

@@ -43,12 +43,16 @@ cluster.configs.file=$CC_CLUSTER_CONFIG_FILE
 webserver.accesslog.path=$CC_ACCESS_LOG
 webserver.http.address=0.0.0.0
 webserver.http.cors.allowmethods=OPTIONS,GET
+webserver.ssl.keystore.location=/tmp/cruise-control/client-server.keystore.p12


How does this deal with the TLS hostname verification when you use the same secret for both connecting to Kafka and as a server certificate for the webserver?

Thanks for pointing this out. I have now added Cruise Control's public certificate to the truststore of the KafkaRebalance client so that TLS hostname verification is preformed

Maybe some better name than client-server would be better? Is it a client certificate? Server certificate? What client and what server? Maybe it should be called cruise-control.keystore.p12?

kyguy · 2021-08-06T17:22:42Z

Currently working on the Cruise Control mock-server tests to handle TLS requests, I will ask for another round of reviews once that is fixed!

kyguy · 2021-08-20T19:27:38Z

This PR is now ready for another round of reviews @scholzj @tomncooper @ppatierno !

scholzj

I think the api/src/test/resources/io/strimzi/api/kafka/model/040-Crd-kafka.yaml file should not be there anymore.

scholzj · 2021-08-21T16:20:04Z

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

+        return API_AUTH_CONFIG_VOLUME_MOUNT + CC_AUTH_CREDENTIALS_FILENAME;
+    }
+
+    private static boolean isEnabled(CruiseControlConfiguration config, String s1, String s2) {


Maybe something like isEnabledInConfiguration would be better name. isEnabled might be confusing as it suggests to tell whether CC is enabled or disabled.

scholzj · 2021-08-21T17:30:27Z

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

+            CruiseControlConfiguration c = (CruiseControlConfiguration) cruiseControl.getConfiguration();
+            Boolean sslEnabled = isApiAuthenticationEnabled(c);
+            if (sslEnabled) {
+                cruiseControl.uriScheme = HTTPS_SCHEME;
+            } else {
+                cruiseControl.uriScheme = HTTP_SCHEME;
+            }


I'm a bit confused. From some parts it seems like the authentication is based on some username / password. But here you seem to use it only to enable the TLS.

Yes, this part only to sets the scheme used by the probes without setting up the certs/keys since the kubelet skips certificate verification when the scheme is set to HTTPS [1]

The username/password bits in the other parts is setting up HTTP Basic authentication primarily used here for role based(authorization) access to the API

[1] https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#http-probes

scholzj · 2021-08-21T17:33:54Z

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

+                .withLivenessProbe(ProbeGenerator.httpProbe(livenessProbeOptions, livenessPath, REST_API_PORT_NAME, uriScheme, getHttpHeaders()))
+                .withReadinessProbe(ProbeGenerator.httpProbe(readinessProbeOptions, readinessPath, REST_API_PORT_NAME, uriScheme, getHttpHeaders()))


This will expose the username and password in the Pod YAML. I do not think this is secure. Doesn't Cruise Control have some health check endpoint outside of authentication?

Unfortunately Cruise Control doesn't have a health check outside of the authentication but the credentials of the role exposed is meant to be public anyway. The role exposed has USER privileges which only has access to the GET endpoints except bootstrap and train. This role is exposed so users can get connect third-party tools for querying Cruise Control for information about their cluster

I don't think this makes sense. Why do we build the authentication if we say that the secret credentials are not secret?

There are two roles:

ADMIN role which has access to all of the endpoints, these credential for this role are kept secret and used exclusively by Strimzi for Cruise Control admin operations.

USER role which has access to the GET endpoints, these credentials are exposed for Strimzi users to use as they please
The probes only need/expose the USER role to check the health of Cruise Control

Well, I don't think this is right:

You cannot say you implement security and share the credentials publicly like this. You need to find some other way. This would if nothing else trigger a lot of questions and issues about exposed credentials etc.

You say this is intended to be used by the users. But this is not the impression I got from the docs changes or PR description ... it does not explain how to create the credentials for the users or how to get them etc. Or did I missed it somewhere?

You are right. To address these concerns I have done the following:

Updated the probes to use healthcheck script to protect the username and password from being exposed in the Pod Yaml

Removed the static password for the user role. If people want direct access to the REST API whether it be for reading or writing, they will have to disable the security configurations. We can always make the API roles configurable in a later PR.

scholzj · 2021-08-21T17:38:11Z

docker-images/kafka/cruise-control-scripts/cruise_control_config_generator.sh

@@ -43,12 +43,16 @@ cluster.configs.file=$CC_CLUSTER_CONFIG_FILE
 webserver.accesslog.path=$CC_ACCESS_LOG
 webserver.http.address=0.0.0.0
 webserver.http.cors.allowmethods=OPTIONS,GET
+webserver.ssl.keystore.location=/tmp/cruise-control/client-server.keystore.p12


Maybe some better name than client-server would be better? Is it a client certificate? Server certificate? What client and what server? Maybe it should be called cruise-control.keystore.p12?

scholzj · 2021-08-21T17:38:50Z

docker-images/kafka/cruise-control-scripts/cruise_control_config_generator.sh

 ssl.keystore.password=$CERTS_STORE_PASSWORD
 ssl.truststore.type=PKCS12
-ssl.truststore.location=/tmp/cruise-control/replication.truststore.p12
+ssl.truststore.location=/tmp/cruise-control/client.truststore.p12


Again, the naming seems to be a bit unintuitice ... should this be called something like kafka.truststore or something? Or keep the old name whicih was much more clear than this.

I like your suggestion for the keystore: cruise-control.keystore.p12 and I was thinking we could use the same pattern for the truststore: cruise-control.trustore.p12 since it is cruise control's truststore. Although I do understand the reasoning behind the original naming scheme that the truststore contains the public Kafka certs. What do you think?

cruise-control.trustore.p12 reads as This is the truststore used for mTLS authentication of users connecting to the Cruise Control APIs.. But it is not that, or? It is the Truststore used to trust the Kafka brokers when connecting to them. So if I got it right, I think something what relates it to the Kafka cluster instead of Cruise Control makes more sense.

I was reading cruise-control.truststore.p12 as This is the truststore holding the certs used for identifying apps which Cruise Control trusts. From what I understood a truststore can hold multiple certificate entries used for identifying different apps ( although in this specific case, Cruise Control only needs the certs for identifying/trusting the Kafka brokers).

Well, I guess this This is the truststore holding the certs used for identifying apps which Cruise Control trusts applies for both cases ... but at least me personally, I would by the name link it to the Cruise Control API which for me is the main CC interface. Not to the Kafka client which is inside.

I understand, in that case I will switch it back to what is was originally, replication.truststore.p12, I want it to be as clear as possible and there is no sense in me changing the name if it is going to mislead people!

scholzj · 2021-08-21T17:40:35Z

docker-images/kafka/cruise-control-scripts/cruise_control_run.sh

@@ -14,10 +14,10 @@ mkdir -p /tmp/cruise-control
 # Import certificates into keystore and truststore
 "$CRUISE_CONTROL_HOME"/cruise_control_tls_prepare_certificates.sh

-export STRIMZI_TRUSTSTORE_LOCATION=/tmp/cruise-control/replication.truststore.p12
+export STRIMZI_TRUSTSTORE_LOCATION=/tmp/cruise-control/client-server.truststore.p12


Is the file name correct? In other places you use client.truststore.p12 (which as I commented above might not be the best choice, but here I wonder if the name is correct in the first place).

Thanks for the catch, this was a typo

scholzj · 2021-08-21T17:42:11Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+.An example of disabling Cruise Control API authorization.
+[source,yaml,subs="attributes+"]
+----
+apiVersion: {KafkaApiVersion}
+kind: Kafka
+metadata:
+  name: my-cluster
+spec:
+  # ...
+  cruiseControl:
+    config:
+      webserver.security.enable: false
+# ...
+----
+
+.An example of disabling Cruise Control API authentication.
+[source,yaml,subs="attributes+"]
+----
+apiVersion: {KafkaApiVersion}
+kind: Kafka
+metadata:
+  name: my-cluster
+spec:
+  # ...
+  cruiseControl:
+    config:
+       webserver.ssl.enable: false
+# ...
+----


This is weird. Does Cruise Control really enable HTTP Basic authentication with .ssl.enable option? And Authorization with security.enable? If it is like than then I guess we have no choice, but this seems highly misleading.

Sorry I mixed these up in the original description of the PR, it should be:

webserver.security.enable -> HTTP Basic authentication (which I am using for authorization)

webserver.ssl.enable -> HTTPS authentication

d-laing

Looks good. I made a few more comments.

documentation/modules/appendix_crds.adoc

d-laing · 2021-08-23T09:36:56Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+
+[[api-security-configuration]]
+[discrete]
+== API security


Suggested change

== API security

== Disabling API security

d-laing · 2021-08-23T09:37:59Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+[discrete]
+== API security
+
+The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.


Suggested change

The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.

The Cruise Control REST API is secured by default to protect the cluster against potentially destructive Cruise Control operations.

Please give an example of such an operation.

"The Cruise Control REST API is secured by default to protect the cluster against potentially destructive Cruise Control operations, such as decommissioning Kafka brokers."

d-laing · 2021-08-23T09:41:28Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+
+The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.
+
+However, these security settings can be disabled.


Suggested change

However, these security settings can be disabled.

However, you can disable these security settings if needed.

d-laing · 2021-08-23T09:45:09Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+
+However, these security settings can be disabled.
+
+.An example of disabling Cruise Control API authorization.


Suggested change

.An example of disabling Cruise Control API authorization.

.Example Cruise Control configuration to disable API authorization.

d-laing · 2021-08-23T09:45:30Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+# ...
+----
+
+.An example of disabling Cruise Control API authentication.


Suggested change

.An example of disabling Cruise Control API authentication.

.Example Cruise Control configuration to disable API authentication.

d-laing · 2021-08-23T09:57:37Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.
+
+However, these security settings can be disabled.
+


Add a bulleted list:

To disable the built-in authorization, set webserver.security.enable to false.

To disable the built-in authentication, set webserver.ssl.enable false.

d-laing

A few nits

d-laing · 2021-08-24T12:45:36Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+
+However, you can disable these security settings if needed.
+
+* To disable the built-in authorization, set webserver.security.enable to false.


Suggested change

* To disable the built-in authorization, set webserver.security.enable to false.

* To disable the built-in authorization, set `webserver.security.enable` to `false`.

d-laing · 2021-08-24T12:45:55Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+However, you can disable these security settings if needed.
+
+* To disable the built-in authorization, set webserver.security.enable to false.
+* To disable the built-in authentication, set webserver.ssl.enable false.


Suggested change

* To disable the built-in authentication, set webserver.ssl.enable false.

* To disable the built-in authentication, set `webserver.ssl.enable` to `false`.

d-laing · 2021-08-24T12:46:10Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+* To disable the built-in authorization, set webserver.security.enable to false.
+* To disable the built-in authentication, set webserver.ssl.enable false.
+
+.Example Cruise Control configuration to disable API authorization.


Suggested change

.Example Cruise Control configuration to disable API authorization.

.Example Cruise Control configuration to disable API authorization

d-laing · 2021-08-24T12:46:15Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+# ...
+----
+
+.Example Cruise Control configuration to disable API authentication.


Suggested change

.Example Cruise Control configuration to disable API authentication.

.Example Cruise Control configuration to disable API authentication

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

d-laing

LGTM

d-laing · 2021-09-21T11:32:06Z

Should we change the title of the procedure to "Cruise Control REST API security"?

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy · 2021-09-21T18:14:16Z

@scholzj I updated the Cruise Control systemtest client logic as well as added some tests to confirm the Cruise Control REST API is reachable when the security is enabled and disabled. Let me know what you think!

This is ready for another automated system tests run!

scholzj · 2021-09-21T18:41:53Z

/azp run regression

azure-pipelines · 2021-09-21T18:42:04Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno · 2021-09-30T14:45:08Z

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

@@ -105,6 +128,8 @@
    private TlsSidecar tlsSidecar;
    private String tlsSidecarImage;
    private String minInsyncReplicas = "1";
+    private boolean authenticationEnabled;
+    private boolean authorizationEnabled;


I am not sure about the naming of these ... I mean they should just reflect what they are in Cruise Control so "security" and "ssl". AFAIU from CC doc, the webserver.security.enable enables the HTTP basic authentication so why using the isApiAuthorizationEnabled method to fill the authorizationEnabled field? it's about enabling the HTTP basic authentication; for webserver.ssl.enable, it's enabling the SSL/TLS layer on HTTP, so HTTPS but it is not related to authentication, so why using isApiAuthenticationEnabled to fill the corresponding authenticationEnabled? I would say that fields should just be securityEnabled and sslEnabled and so the corresponding methods getting the config from CC. Am I misunderstading anything here?

Or maybe you could use authenticationEnabled for the webserver.security.enable and something like encryptionEnabled for webserver.ssl.enable

Yes you are right! Let's go with your suggestion below:

authEnabled for the webserver.security.enable

sslEnabled for webserver.ssl.enable

This will be more clear and reduce the characters in the variable names!

ppatierno · 2021-09-30T15:38:02Z

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

@@ -105,6 +128,8 @@
    private TlsSidecar tlsSidecar;
    private String tlsSidecarImage;
    private String minInsyncReplicas = "1";
+    private boolean authenticationEnabled;
+    private boolean authorizationEnabled;


Or maybe you could use authenticationEnabled for the webserver.security.enable and something like encryptionEnabled for webserver.ssl.enable

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

ppatierno · 2021-09-30T19:43:32Z

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Enable Cruise Control anomaly.detection configurations
 * Add support for building connector images from the Maven coordinates
 * Allow Kafka Connect Build artifacts to be downloaded from insecure servers (#5542)
+* Configurable auth and SSL for Cruise Control API


I think for users we should be more explicit, there is no hurt to say "authentication/authorization" here.

ppatierno · 2021-09-30T19:45:05Z

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

@@ -216,6 +261,9 @@ public static CruiseControl fromCrd(Reconciliation reconciliation, Kafka kafkaAs
            cruiseControl.setTlsSidecar(tlsSidecar);

            cruiseControl = cruiseControl.updateConfiguration(spec);
+            CruiseControlConfiguration c = (CruiseControlConfiguration) cruiseControl.getConfiguration();


ccConfiguration variable maybe better than just c ?

ppatierno · 2021-09-30T19:51:15Z

docker-images/kafka/cruise-control-scripts/cruise_control_config_generator.sh

+webserver.ssl.keystore.location=/tmp/cruise-control/cruise-control.keystore.p12
+webserver.ssl.keystore.password=$CERTS_STORE_PASSWORD
+webserver.ssl.keystore.type=PKCS12
+webserver.ssl.key.password=$CERTS_STORE_PASSWORD


this should be the passphrase to "decrypt" the key stored in the key store I guess. Is it mandatory in cruise control SSL feature? We don't have this kind of configuration for Kafka related certificates in the corresponding keystores, right? I mean, we already have the password for the keystore to get certificate and key, do we really need the "passphrase" for the key as well? You are setting to the same as the certs store password so kind of not adding more security. Just wondering if it's not mandatory and we can avoid setting it.

I have the same understanding and although we shouldn't need it because we don't encrypt keystore keys(from what I understand), Cruise Control with throw a NPE if we have don't include the webserver.ssl.key.password when webserver.ssl.enable is enabled [1]

[1] https://github.com/linkedin/cruise-control/blob/0ba9ddd44aecf8f278d9fca3d38df6d54b1ac55b/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/KafkaCruiseControlApp.java#L107 (edited to fix link)

From the CC code I see that it just sets the password on the Jetty SslContextFactory, and without specifying it we are passing null. From the Jetty code [1] I see that passing null should not hurt, it should mean just "no password on the key". Have you tried to avoid using the password? Maybe it's worth giving it a try?

[1] https://github.com/eclipse/jetty.project/blob/b56edf511ab4399122ea2c6162a4a5988870f479/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L816

So when I tested this yesterday, I removed the whole line webserver.ssl.key.password=$CERTS_STORE_PASSWORD, that's probably why CC threw the NPE from [1], Cruise Control goes to look for the webserver.ssl.key.password key and throws a NPE when it's not found. Let me try testing with just webserver.ssl.key.password= or webserver.ssl.key.password="" in the config to see if we get a different result!

[1] https://github.com/linkedin/cruise-control/blob/0ba9ddd44aecf8f278d9fca3d38df6d54b1ac55b/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/KafkaCruiseControlApp.java#L107

So in the CC config, I just tested with:

webserver.ssl.key.password=

webserver.ssl.key.password=""

webserver.ssl.key.password=null

All of which cause CC to throw the exception below. From what I understand from SO, this can happen when you use the wrong password leading me to think that the $CERT_STORE_PASSWORD is used/needed afterall

java.security.UnrecoverableKeyException: Get Key failed: Given final block not properly padded. Such issues can arise if a bad key is used during decryption. at sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:446) ~[?:?] at sun.security.util.KeyStoreDelegator.engineGetKey(KeyStoreDelegator.java:90) ~[?:?] at java.security.KeyStore.getKey(KeyStore.java:1057) ~[?:?] at sun.security.ssl.SunX509KeyManagerImpl.<init>(SunX509KeyManagerImpl.java:145) ~[?:?] at sun.security.ssl.KeyManagerFactoryImpl$SunX509.engineInit(KeyManagerFactoryImpl.java:70) ~[?:?] at javax.net.ssl.KeyManagerFactory.init(KeyManagerFactory.java:271) ~[?:?] at org.eclipse.jetty.util.ssl.SslContextFactory.getKeyManagers(SslContextFactory.java:1249) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.ssl.SslContextFactory$Server.getKeyManagers(SslContextFactory.java:2363) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.ssl.SslContextFactory.load(SslContextFactory.java:373) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.ssl.SslContextFactory.doStart(SslContextFactory.java:244) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:117) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.server.SslConnectionFactory.doStart(SslConnectionFactory.java:97) ~[jetty-server-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:117) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.server.AbstractConnector.doStart(AbstractConnector.java:321) ~[jetty-server-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.server.AbstractNetworkConnector.doStart(AbstractNetworkConnector.java:81) ~[jetty-server-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.server.ServerConnector.doStart(ServerConnector.java:234) ~[jetty-server-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.server.Server.doStart(Server.java:401) ~[jetty-server-9.4.42.v20210604.jar:9.4.42.v20210604] at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[jetty-util-9.4.42.v20210604.jar:9.4.42.v20210604] at com.linkedin.kafka.cruisecontrol.KafkaCruiseControlApp.start(KafkaCruiseControlApp.java:66) ~[cruise-control-2.5.57.jar:?] at com.linkedin.kafka.cruisecontrol.KafkaCruiseControlMain.main(KafkaCruiseControlMain.java:40) ~[cruise-control-2.5.57.jar:?] Caused by: javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption. at com.sun.crypto.provider.CipherCore.unpad(CipherCore.java:975) ~[?:?] at com.sun.crypto.provider.CipherCore.fillOutputBuffer(CipherCore.java:1056) ~[?:?] at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:853) ~[?:?] at com.sun.crypto.provider.PKCS12PBECipherCore.implDoFinal(PKCS12PBECipherCore.java:408) ~[?:?] at com.sun.crypto.provider.PKCS12PBECipherCore$PBEWithSHA1AndDESede.engineDoFinal(PKCS12PBECipherCore.java:440) ~[?:?] at javax.crypto.Cipher.doFinal(Cipher.java:2202) ~[?:?] at sun.security.pkcs12.PKCS12KeyStore.lambda$engineGetKey$0(PKCS12KeyStore.java:387) ~[?:?] at sun.security.pkcs12.PKCS12KeyStore$RetryWithZero.run(PKCS12KeyStore.java:283) ~[?:?] at sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:381) ~[?:?] ... 24 more

So it seems to be Jetty that sucks! We have to keep the password for now but I would open an issue and PR on CC repo to avoiding running setKeyManagerPassword if it's not provided in the configuration.

Opened an issue here [1], I'll follow up from there

[1] linkedin/cruise-control#1707

ppatierno · 2021-09-30T20:01:21Z

docker-images/kafka/cruise-control-scripts/cruise_control_run.sh

@@ -17,7 +17,7 @@ mkdir -p /tmp/cruise-control
 export STRIMZI_TRUSTSTORE_LOCATION=/tmp/cruise-control/replication.truststore.p12
 export STRIMZI_TRUSTSTORE_PASSWORD="$CERTS_STORE_PASSWORD"

-export STRIMZI_KEYSTORE_LOCATION=/tmp/cruise-control/replication.keystore.p12
+export STRIMZI_KEYSTORE_LOCATION=/tmp/cruise-control/cruise-control.keystore.p12


why this change in naming? Let me see if I got it correctly ... before this PR, this keystore was used for containing CC certificate and private key in order to have "mutual" authentication (so TLS client authentication) of CC against Kafka cluster (when it connects to the cluster for doing its stuff).
With this PR you are adding in the same keystore, the cert and key that are used by CC for TLS encryption versus clients connecting to its API (so the cluster operator).
Would it make sense to change the name of replication.truststore.p12 in the same way? In the end it's not about replication here. Wdyt @kyguy @tomncooper @scholzj ?

Exactly right Paolo, this was the reasoning behind the name change for the keystore. As for the truststore, Jakub and I had the same discussion here [1] and decided that it was best to leave the truststore as replication. Take a look over the discussion and let us know what you think!

[1] #5073 (comment)

ppatierno · 2021-09-30T20:02:44Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+* To disable the built-in HTTP Basic authentication, set `webserver.security.enable` to `false`.
+* To disable the built-in SSL, set `webserver.ssl.enable` to `false`.
+
+.Example Cruise Control configuration to disable API authorization and authentication


"authorization and authentication" now it's more authentication/authorization and SSL

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

tomncooper

Really great work Kyle. My only outstanding question is why you have included the USER role as well as the ADMIN one? You don't seem (but I may have missed something) to be using it?

kyguy · 2021-10-01T14:09:17Z

@tomncooper Nice catch! The USER role is only used by the probes, but the probes could have easily been set up to use the ADMIN role instead. I originally included the USER role for Strimzi users to have read access to the Cruise Control API but stripped out the public password to simplify the PR(with the intention of adding it and documentation in another PR!). Since the probes only need read access to the CC API I left them using the USER role

kyguy · 2021-10-01T14:18:35Z

@scholzj when you get a chance, could you run the automated system tests again?

scholzj · 2021-10-01T14:44:45Z

/azp run regression

azure-pipelines · 2021-10-01T14:44:57Z

Azure Pipelines successfully started running 1 pipeline(s).

kyguy marked this pull request as draft June 2, 2021 15:21

kyguy force-pushed the lock-down-cc branch from 4a29de1 to 115d6dc Compare June 2, 2021 15:23

kyguy changed the title ~~Prototype for configurable authentication and authorization for Cruis…~~ Configurable authentication and authorization for Cruis… Jun 2, 2021

kyguy force-pushed the lock-down-cc branch 3 times, most recently from f2a5b7f to f3905d2 Compare June 15, 2021 20:48

kyguy force-pushed the lock-down-cc branch 3 times, most recently from d1527a3 to 32a60ff Compare June 29, 2021 19:39

kyguy marked this pull request as ready for review June 29, 2021 19:40

kyguy added this to the 0.25.0 milestone Jun 29, 2021

kyguy requested review from tomncooper, ppatierno and scholzj June 29, 2021 19:40

scholzj reviewed Jun 30, 2021

View reviewed changes

scholzj modified the milestones: 0.25.0, 0.26.0 Aug 2, 2021

kyguy force-pushed the lock-down-cc branch 3 times, most recently from 5f057a3 to 2b8bf5b Compare August 20, 2021 16:30

kyguy requested a review from d-laing August 20, 2021 19:26

kyguy force-pushed the lock-down-cc branch from f492a98 to da19441 Compare August 20, 2021 19:33

scholzj reviewed Aug 21, 2021

View reviewed changes

d-laing suggested changes Aug 23, 2021

View reviewed changes

d-laing suggested changes Aug 24, 2021

View reviewed changes

kyguy force-pushed the lock-down-cc branch 3 times, most recently from ea0c603 to e9b8a4b Compare September 2, 2021 18:27

kyguy added 2 commits September 20, 2021 17:22

Add creds to CC systemtest client + test for unauthenticated client

a27e951

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Add documentation revisions

153c859

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy force-pushed the lock-down-cc branch from 2154a65 to 153c859 Compare September 20, 2021 21:31

Adding more system tests

0e787e9

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

d-laing approved these changes Sep 21, 2021

View reviewed changes

kyguy added 2 commits September 21, 2021 12:47

Fix systemtest

33c487a

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Another documentation revision

c007693

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

ppatierno requested changes Sep 30, 2021

View reviewed changes

kyguy added 2 commits September 30, 2021 13:32

Refactor auth and ssl variable names

c70c239

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Update CHANGELOG.md

affed57

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy changed the title ~~Configurable authentication and authorization for Cruis…~~ Configurable auth and SSL for Cruise Control API Sep 30, 2021

kyguy requested a review from ppatierno September 30, 2021 18:40

ppatierno reviewed Sep 30, 2021

View reviewed changes

Addressing more comments

5e35883

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy changed the title ~~Configurable auth and SSL for Cruise Control API~~ Configurable authentication, authorization, and SSL for Cruise Control API Sep 30, 2021

kyguy requested a review from ppatierno September 30, 2021 21:36

ppatierno approved these changes Oct 1, 2021

View reviewed changes

tomncooper approved these changes Oct 1, 2021

View reviewed changes

scholzj merged commit 45eecae into strimzi:main Oct 4, 2021

scholzj mentioned this pull request Oct 6, 2021

Fix CruiseControlApiImpl and truststore creation #5669

Merged

kyguy deleted the lock-down-cc branch November 16, 2021 15:34

		.withLivenessProbe(ProbeGenerator.httpProbe(livenessProbeOptions, livenessPath, REST_API_PORT_NAME, uriScheme, getHttpHeaders()))
		.withReadinessProbe(ProbeGenerator.httpProbe(readinessProbeOptions, readinessPath, REST_API_PORT_NAME, uriScheme, getHttpHeaders()))

	The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.
	The Cruise Control REST API is secured by default to protect the cluster against potentially destructive Cruise Control operations.


		The Cruise Control API is secured by default to protect the cluster against any potentially destructive Cruise Control operations.

		However, these security settings can be disabled.

	However, these security settings can be disabled.
	However, you can disable these security settings if needed.


		However, these security settings can be disabled.

		.An example of disabling Cruise Control API authorization.

	.An example of disabling Cruise Control API authorization.
	.Example Cruise Control configuration to disable API authorization.


		However, you can disable these security settings if needed.

		* To disable the built-in authorization, set webserver.security.enable to false.

	* To disable the built-in authentication, set webserver.ssl.enable false.
	* To disable the built-in authentication, set `webserver.ssl.enable` to `false`.

Configurable authentication, authorization, and SSL for Cruise Control API #5073

Configurable authentication, authorization, and SSL for Cruise Control API #5073

Conversation

kyguy commented Jun 2, 2021 • edited

Type of change

Description

Checklist

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

kyguy Aug 6, 2021 • edited

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

kyguy commented Aug 6, 2021

kyguy commented Aug 20, 2021

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

kyguy Aug 23, 2021 • edited

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

kyguy Aug 23, 2021 • edited

Choose a reason for hiding this comment

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

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

d-laing left a comment

Choose a reason for hiding this comment

d-laing commented Sep 21, 2021

kyguy commented Sep 21, 2021

scholzj commented Sep 21, 2021

azure-pipelines bot commented Sep 21, 2021

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

kyguy Sep 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyguy Oct 1, 2021 • edited

Choose a reason for hiding this comment

kyguy Oct 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyguy commented Jun 2, 2021 •

edited

kyguy Aug 6, 2021 •

edited

kyguy Aug 23, 2021 •

edited

kyguy Aug 23, 2021 •

edited

kyguy Sep 30, 2021 •

edited

kyguy Oct 1, 2021 •

edited

kyguy Oct 1, 2021 •

edited

kyguy Sep 30, 2021 •

edited