Added basic and API key and secret credentials for Kafka and SR cluster configs in direct connections

[rhauch]

Summary of Changes

Resolves #124

Adds basic and API key+secret credentials to direct connections, including validating the credentials in the Connections API and using credentials when connecting to the Kafka cluster and SR defined in the direct connection spec.

New Credentials types

The Credentials interface and BasicCredentials and ApiKeyAndSecret record types have methods that build the auth-related configuration properties for Kafka clients and SR clients. Each concrete Credentials type customizes the logic, though parameters are used to supply information not in the Credentials objects.

The Credentials interface defines three methods that will likely be overridden by each concrete subtype:

• kafkaClientProperties(...) -- Construct the auth-related Kafka client configuration properties. The method parameter defines connectivity options that might affect these properties.
• schemaRegistryClientProperties(...) -- Construct the auth-related SR client configuration properties. The method parameter defines connectivity options that might affect these properties.
• httpClientHeaders(...) -- Construct the auth-related HTTP headers.

New Redactable types for write-only objects

The BasicCredentials has a password field, and the ApiKeyAndSecret record type has a api_secret field. Because these fields will contain secrets, they must ensure that these fields are always masked (e.g., ********) when written to the log or in API responses.

To do this, this PR defines a new Password class and ApiSecret class that extend a new Redactable abstract class representing any literal String value that must be redacted in all API responses and never logged in messages (or output by the sidecar). These are essentially write-only values that prevent external reads. The Redactable class includes a custom serializer that always writes a masked representation consisting of exactly eight asterisk (*) characters regardless of the actual literal value. The toString() method also outputs the same masked representation, primarily to help prevent sensitive literal values from being included in logs or exception messages. There are also a few methods that can be used in validating, such as checking whether the value is empty or longer than some size. The hashCode() and equals() methods never use the value. All of these methods are marked as final to ensure subclasses do not alter the behavior.)

Building Kafka and SR client configurations

The logic to build the complete configurations for the Kafka admin, consumer and producer clients and the Schema Registry clients are moved into a new ClientConfigurator bean that is @ApplicationScoped. These methods rely upon the Credentials methods for the auth-related config properties and the KafkaCluster or SchemaRegistry cluster for the remaining configuration properties.

The ClientConfigurator bean’s methods have a boolean parameter as to whether the resulting configuration should redact secrets, so that the configuration can be expose the connection properties to the user, say to allow them to copy the connection properties and use them in their application, or if we use the generated (but redacted) connection configs in the template service. But the AdminClients, KafkaProducerClients, KafkaConsumerFactory and SchemaRegistryClients beans use the configurator and do not redact the configuration.

New methods have been added to the ConnectionState class to make it easy to get the Credentials for a Kafka cluster with a given ID or a Schema Registry cluster with a given ID. The DirectConnectionState subclass always returns the credentials for the one Kafka cluster or one SR cluster. In the future, other ConnectionState subclasses (e.g., for CP MDS) might need to maintain a map of credentials by cluster ID for any clusters do not have the same MDS credentials (e.g., the Kafka or SR cluster does not delegate authN functionality to MDS).

Adding other types of credentials in the future

In the future, the only thing we need to do to support other types of authN credentials, such as OAuth 2.0, mTLS, Kerberos (SASL/GSSAPI), etc., is to define new Credentials subtypes and implement the methods to construct the auth-related client properties using the subtype-specific credential information.

Limitations

There are a few shortcuts taken for direct connections that will be addressed in subsequent PR as part of #123:

• the status for direct connections is not accurate or useful, and will have to use an AdminClient and SR client to verify the credentials and update the status.
• the kafka_cluster.id and schema_registry.id are currently optional in the OpenAPI spec but are required until we can obtain the cluster ID of the remote system and verify it matches. The RealDirectFetcher will need to perform a describe-cluster using the admin client, and set the cluster ID. (We might consider remove the id fields from the connection spec, if we always get a good ID from the describe-cluster.)

Testing

I've done some manual testing with quarkus:dev and native executable by using the REST API to create a direct connection that uses a CCloud cluster with API key and secret, and have verified the admin client and consumer clients are built correctly and will successfully work with the remote cluster.

Pull request checklist

Please check if your PR fulfills the following (if applicable):

• Tests:
  • Added new
  • Updated existing
  • Deleted existing
• Have you validated this change locally against a running instance of the Quarkus dev server?

  make quarkus-dev

• Have you validated this change against a locally running native executable?

  make mvn-package-native && ./target/ide-sidecar-*-runner

[rohitsanj]

This is excellent. 💯 Thank you!

I have some minor suggestions regarding the OpenAPI spec for the Credentials object.

[flippingbits]

Excellent work, @rhauch! I have only a few minor questions and suggestions. Otherwise, your PR looks good to me.

[flippingbits]

I have one more suggestion that I'd like to discuss with you.

[rhauch]

Several builds failed with OOM errors when trying to start the Apicurio container during our tests. Disabling the Apicurio dev services that starts an Apicurio container during testing worked, and will be fixed separately with #156.

Also, we should consider upgrading the CI machine types (#155), since we're clearly getting close to the limit of our CI machine types. Disabling the Apicurio container will help in the short term, but we'll soon be adding other containers for CP.

[flippingbits]

Thanks for the update! I have one suggestion. Otherwise, your PR looks good to me.

[rhauch]

I've done some manual testing with quarkus:dev and native executable by using the REST API to create a direct connection that uses a CCloud cluster with API key and secret, and have verified the admin client and consumer clients are built correctly and will successfully work with the remote cluster.

There are a few shortcuts taken for direct connections that will be addressed in subsequent PR as part of #123:

• the status for direct connections is not accurate or useful, and will have to use an AdminClient and SR client to verify the credentials and update the status.
• the kafka_cluster.id and schema_registry.id are currently optional in the OpenAPI spec but are required until we can obtain the cluster ID of the remote system and verify it matches. The RealDirectFetcher will need to perform a describe-cluster using the admin client, and set the cluster ID. (We might consider remove the id fields from the connection spec, if we always get a good ID from the describe-cluster.)

[rhauch]

I've updated the PR description, and I think this PR is in state that's ready to be merged, pending approval.

[rohitsanj]

TLDR: Including allOf here generates the API client code with credentials as a generic object rather than the more strongly typed union object. Let's keep it this way while the incessant bugs in the typescript-fetch template are fixed, at the cost of a bit of developer unfriendliness in trying to determine whether the opaque object is a BasicCredentials or ApiKeyAndSecret.

Upon running npx gulp apigen format in the confluentinc/vscode repo after copying over the OpenAPI spec from this branch, I see the following diff for src/clients/sidecar/models/KafkaClusterConfig.ts (out of a few other changed files too). You'll see that the type for credentials in the generated TypeScript interface is object | null.

Git diff for `src/clients/sidecar/models/KafkaClusterConfig.ts`

diff --git a/src/clients/sidecar/models/KafkaClusterConfig.ts b/src/clients/sidecar/models/KafkaClusterConfig.ts
index d9fb41b..d1f0695 100644
--- a/src/clients/sidecar/models/KafkaClusterConfig.ts
+++ b/src/clients/sidecar/models/KafkaClusterConfig.ts
@@ -31,6 +31,24 @@ export interface KafkaClusterConfig {
    * @memberof KafkaClusterConfig
    */
   bootstrap_servers: string;
+  /**
+   * The credentials for the Kafka cluster, or null if no authentication is required
+   * @type {object}
+   * @memberof KafkaClusterConfig
+   */
+  credentials?: object | null;
+  /**
+   * Whether to communicate with the Kafka cluster over TLS/SSL. Defaults to 'true', but set to 'false' when the Kafka cluster does not support TLS/SSL.
+   * @type {boolean}
+   * @memberof KafkaClusterConfig
+   */
+  ssl?: boolean | null;
+  /**
+   * Whether to verify the Kafka cluster certificates. Defaults to 'true', but set to 'false' when the Kafka cluster has self-signed certificates.
+   * @type {boolean}
+   * @memberof KafkaClusterConfig
+   */
+  verify_ssl_certificates?: boolean | null;
 }
 
 /**
@@ -55,6 +73,10 @@ export function KafkaClusterConfigFromJSONTyped(
   return {
     id: json["id"] == null ? undefined : json["id"],
     bootstrap_servers: json["bootstrap_servers"],
+    credentials: json["credentials"] == null ? undefined : json["credentials"],
+    ssl: json["ssl"] == null ? undefined : json["ssl"],
+    verify_ssl_certificates:
+      json["verify_ssl_certificates"] == null ? undefined : json["verify_ssl_certificates"],
   };
 }
 
@@ -65,5 +87,8 @@ export function KafkaClusterConfigToJSON(value?: KafkaClusterConfig | null): any
   return {
     id: value["id"],
     bootstrap_servers: value["bootstrap_servers"],
+    credentials: value["credentials"],
+    ssl: value["ssl"],
+    verify_ssl_certificates: value["verify_ssl_certificates"],
   };
 }

To actually declare the type of credentials as a union of BasicCredentials and ApiKeyAndSecret, we'll need to remove the allOf from the spec above (using some bespoke parameter in the @Schema annotation somewhere).

So, I did exactly that, removed the allOf field from both KafkaClusterConfig and SchemaRegistryClusterConfig, then generated the clients again using npx gulp apigen format. Well, this opened a can of worms. Read on..

confluentinc/vscode uses the typescript-fetch template to generate the API client code, and we've currently pinned its version to 7.7.0. The generated type for credentials was how I'd expected it to be (see below), but unfortunately, there was a bug in one of the functions in the generated code: "Function lacks ending return statement and return type does not include 'undefined'".

// In src/clients/sidecar/models/KafkaClusterConfig.ts
...
  /**
   *
   * @type {KafkaClusterConfigCredentials}
   * @memberof KafkaClusterConfig
   */
  credentials?: KafkaClusterConfigCredentials | null;
...

// In src/clients/sidecar/models/KafkaClusterConfigCredentials.ts
...
/**
 * @type KafkaClusterConfigCredentials
 * The credentials for the Kafka cluster, or null if no authentication is required
 * @export
 */
export type KafkaClusterConfigCredentials = ApiKeyAndSecret | BasicCredentials;
...

So, I upgraded the openapi-generator-version to the latest stable version 7.9.0 in the hopes that this bug will disappear and regenerated the API clients (370 file diff) with the sans-allOf OpenAPI spec. Unfortunately, there was yet another bug in the generated code. It was fixed just last week and will likely be a while before it makes it into a stable release.

All this to say, let's keep this spec as is and wait to see if the next release of openapi-generator poses issues.

[rohitsanj]

LGTM. Thank you! I'll let @flippingbits do a pass as well.

[flippingbits]

Excellent work, @rhauch! 💯