Improve the certificate handling inside the Strimzi operators code

[sknot-rh]

Currently we've failed to really encapsulate and abstract our certificate handling. What I mean by that is everywhere we pass around a Secret which we happen to know contains a key store (broker certificate and key), or truststore (ca certificate). But that requires that the consumer of that Secret knows what type of secret is it, and how to decode the values within it into something it can use.

I think there is value is trying to better encapsulate these concepts, so rather than passing around Secrets we pass around something higher level. For example a AdminClientSupplier might accept some Strimzi Truststore class and a Strimzi Keystore class. Providing Resource Operators for those things (Truststore and Keystore) would be a logical thing to do. This would provide a path towards being able to abstract where certain credentials are stored (e.g. option to store keys in Vault), and should make the KAO slightly simpler to understand, because there are fewer generic Secrets floating around.

At least that was my motivation for suggesting this, and I'd be interested to know whether people think that's a desirable goal or not (and if they have specific reasons to think it wouldn't work out).

Originally posted by @tombentley in https://github.com/strimzi/strimzi-kafka-operator/pull/5549#discussion_r714675992

[SDBrett]

Am I correct that you're proposing a singular CR which contains relevant credentials for a Kafka cluster?

I mocked up two manifests to help illustrate my understanding, though I may be way off.

apiVersion: v1
kind: KafkaCredential
metadata:
  name: cluster-01-creds
spec:
  truststore:
    dataSource: literal
    truststore: encoded cert
  clientCredentials:
    type: certificate
    certificate:
      dataSource: vault
      secretName: secret/clientCert

---
apiVersion: v1
kind: KafkaCredential
metadata:
  name: cluster-02-creds
spec:
  truststore:
    dataSource: literal
    truststore: encoded cert
  clientCredentials:
    type: plain
    plain:
      dataSource: literal
      userName: user
      password: test123

[scholzj]

Triaged on 28. 4. 2022: This still makes sense. It is about internal Java APIs in the Strimzi code only and how the certificates are handled. It is not about any new resources or outside representation.

@tombentley will try to provide a better description / design what should be the intended change. Then it should be easier to implement by someone.

[tombentley]

I think since I originally made this comment we've change the DefaultAdminClientProvider to using PEM certificates. But the basic idea still holds. Here are the kind of abstractions I would try to use:

public interface PemTrustStoreSupplier {
    String pemTrustedCertificates();
}
public interface PemKeyStoreSupplier {
    String pemPrivateKey();
    String pemCertificateChain();
}

Then I would change AdminClientProvider (and its subclasses) to use those interfaces rather than Secret.

Implement those interfaces with classes, for example like this:

class ClusterCaTrustStoreSupplier implements PemTrustStoreSupplier {
    public ClusterCaTrustStoreSupplier(KubernetesClient client, String namespace, String clusterName) {
        // ...
    }
    @Overrride
    public String pemTrustedCertificates() {
        var secret = client.secrets().inNamespace(namesapce).withName(name).get()
        String pem = //... TODO base64 decode
        return pem;
    }
}

That's the first step. It should result in the code path where we consume those certificates (e.g. the KafkaRoller) much more type safe, and make it much easier (via IDE find callers) to see where in the code we're consuming those certificates.

It would provide a route to having other implementations of those interfaces in the future (e.g. we decided we wanted to pull these things from Vault or similar, rather than assuming they're in Secrets). Though if we're seriously considering this we should check exactly what info is needed.

Another step would be to clean up the write path similarly, by abstracting out interfaces which are in terms of the Java classes X509Certificate/KeyStore (there is no TrustStore sadly, but we could easily roll our own, encapsulating a KeyStore, but being a distinct type for additional type safety, if we felt that was worthwhile). The default implementation would be to update the existing Secrets, but again it would provide the option of having other implementations later on.

Hopefully this is enough detail, at least for you to be able to point out if/how it might not work @scholzj.

[scholzj]

Hopefully this is enough detail, at least for you to be able to point out if/how it might not work @scholzj.

I think this is good. It should give everyone better idea what it means. Let's get back to it next triage call and decide how to label it etc. Thanks for updating this.

[scholzj]

Triaged on 10.5.2022: The better description from Tom should make it more clear what to do here.

[katheris]

I'm happy to pick this up as I think it will be a good first issue around certificates to tackle.

[scholzj]

@katheris Not a hard requirement ... but something to consider while working on this ... lately we have more various things that connect to Kafka / ZooKeeper. But we currently gather the certificates in every step independently. Having that done only once might be useful improvement.