fix: fedramp issues #10092

aliehsaeedii · 2023-10-16T11:03:18Z

KSE-1768
This PR intends to fix the following issues:

enable ksql to accept BCFKS keystore type
making ssl.cipher.suites as optional config

Sample fips compliant config:

security.providers=io.confluent.kafka.security.fips.provider.BcFipsProviderCreator,io.confluent.kafka.security.fips.provider.BcFipsJsseProviderCreator
ssl.enabled.protocols=TLSv1.2
ssl.key.password=${file:/mnt/sslcerts/jksPassword.txt:jksPassword}
ssl.keymanager.algorithm=PKIX
ssl.keystore.location=/mnt/sslcerts/keystore.bcfks
ssl.keystore.password=${file:/mnt/sslcerts/jksPassword.txt:jksPassword}
ssl.keystore.type=BCFKS
ssl.trustmanager.algorithm=PKIX
ssl.truststore.location=/mnt/sslcerts/truststore.bcfks
ssl.truststore.password=${file:/mnt/sslcerts/jksPassword.txt:jksPassword}
ssl.truststore.type=BCFKS

Description

What behavior do you want to change, why, how does your patch achieve the changes?

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")
Do these changes have compatibility implications for rollback? If so, ensure that the ksql command version is bumped.

cla-assistant · 2023-10-16T11:03:33Z

All committers have signed the CLA.

cadonna

@aliehsaeedii Thanks for the PR!

Here my comments!

cadonna · 2023-10-16T12:06:53Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlServerMain.java

-    if (cipherSuites.isEmpty()) {
-      final String errorMsg = "No cipher suites "
-          + "('"
-          + KsqlRestConfig.SSL_CIPHER_SUITES_CONFIG
-          + "') is specified.";
-      log.error(errorMsg);
-      throw new SecurityException(errorMsg);


Why did you remove this?

Why did you remove this?

Based on info form CP team, ssl.cipher.suites must be an optional parameter.

cadonna · 2023-10-16T12:07:26Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestConfig.java

  public static final ConfigDef.ValidString SSL_STORE_TYPE_VALIDATOR =
      ConfigDef.ValidString.in(
          SSL_STORE_TYPE_JKS,
-          SSL_STORE_TYPE_PKCS12
+          SSL_STORE_TYPE_PKCS12,
+          SSL_STORE_TYPE_BCFKS


Isn't there a unit test you need to adapt?

Isn't there a unit test you need to adapt?

For the other two we don't have any.

OK

I think you need to adapt SSL_KEYSTORE_TYPE_DOC and SSL_TRUSTSTORE_TYPE_DOC, though.

cadonna · 2023-10-16T12:08:21Z

...ackDump-io.confluent.ksql.cli.BasicAuthFunctionalTest-shouldBeAbleToUseCliWithValidCreds.log

These logs are not needed, right?

These logs are not needed, right?

Right. Strange that they have been added!

spena · 2023-10-16T19:21:16Z

ksqldb-common/src/main/java/io/confluent/ksql/util/VertxSslOptionsFactory.java

+  private static KeyStoreOptions buildBcfksOptions(final String password) {
+    return new KeyStoreOptions()
+        .setType("BCFKS")
+        .setProvider("/opt/confluent/confluent-7.4.0/share/java/kafka/bc-fips-1.0.2.3.jar")


What if this JAR appears in a different location and/or different version? Can this location be configurable? or why is it hard-coded?

spena · 2023-10-16T19:22:36Z

ksqldb-common/src/main/java/io/confluent/ksql/util/VertxSslOptionsFactory.java

+
+    final String password = getTrustStorePassword(props);
+
+    if (!Strings.isNullOrEmpty(password)) {


Don't you need the location obtained like getPfxTrustStoreOptions? I see the location is hard-coded in buildBcvfksOptions.

spena · 2023-10-17T15:44:30Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestConfig.java

@@ -115,21 +115,24 @@ public class KsqlRestConfig extends AbstractConfig {

  public static final String SSL_KEYSTORE_TYPE_CONFIG = "ssl.keystore.type";
  protected static final String SSL_KEYSTORE_TYPE_DOC =
-      "The type of keystore file. Must be either 'JKS' or 'PKCS12'.";
+      "The type of keystore file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";


Suggested change

"The type of keystore file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";

"The type of keystore file. Must be either 'JKS', 'PKCS12' or 'BCFKS'.";

spena · 2023-10-17T15:44:39Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestConfig.java


  protected static final String SSL_TRUSTSTORE_LOCATION_DEFAULT = "";
  protected static final String SSL_TRUSTSTORE_PASSWORD_DEFAULT = "";

  public static final String SSL_TRUSTSTORE_TYPE_CONFIG = "ssl.truststore.type";
  protected static final String SSL_TRUSTSTORE_TYPE_DOC =
-      "The type of trust store file. Must be either 'JKS' or 'PKCS12'.";
+      "The type of trust store file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";


Suggested change

"The type of trust store file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";

"The type of trust store file. Must be either 'JKS', 'PKCS12' or 'BCFKS'.";

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/util/ApiServerUtils.java

spena · 2023-10-17T15:55:02Z

ksqldb-common/src/main/java/io/confluent/ksql/util/VertxSslOptionsFactory.java

@@ -59,6 +60,13 @@ private static JksOptions buildJksOptions(final Buffer buffer, final String pass
    return new JksOptions().setValue(buffer).setPassword(Strings.nullToEmpty(password));
  }

+  private static KeyStoreOptions buildBcfksOptions(final String password) {
+    return new KeyStoreOptions()
+        .setType("BCFKS")


Just to follow the code in this file, could you add a constant, like the SSL_STORE_TYPE_JKS for this too?

spena · 2023-11-01T15:03:14Z

...ackDump-io.confluent.ksql.cli.BasicAuthFunctionalTest-shouldBeAbleToUseCliWithValidCreds.log

@@ -0,0 +1,1355 @@
+Thread: 496 - ksql-workers-0


This and the other *.log files were accidentally committed, right?

spena · 2023-11-01T15:08:25Z

ksqldb-common/src/main/java/io/confluent/ksql/util/VertxSslOptionsFactory.java

+  public static Optional<KeyStoreOptions> getBcfksTrustStoreOptions(
+      final String location,
+      final String password,
+      final String keyPassword) {


is the keyPassword needed for truststores? My understanding is that truststores only stores certificates while keystores stores keys. So, truststores don't require a key password, right?

spena · 2023-11-01T15:10:09Z

ksqldb-common/src/main/java/io/confluent/ksql/util/VertxSslOptionsFactory.java

+    final String providers = getSecurityProviders(props);
+    final String location = getTrustStoreLocation(props);
+    final String password = getTrustStorePassword(props);
+    final String keyPassword = getKeyPassword(props);


Truststores don't require a key password, do they?

fix: fix fedramp issues

ff4a861

aliehsaeedii requested a review from a team as a code owner October 16, 2023 11:03

cadonna reviewed Oct 16, 2023

View reviewed changes

aliehsaeedii added 3 commits October 16, 2023 15:14

fix: remove log files

ef47865

fix: update docs

cbc8700

fix: add bcfks supprt

3cd316d

aliehsaeedii requested a review from spena October 16, 2023 18:07

spena reviewed Oct 16, 2023

View reviewed changes

spena reviewed Oct 17, 2023

View reviewed changes

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/util/ApiServerUtils.java Show resolved Hide resolved

spena reviewed Oct 17, 2023

View reviewed changes

fix: add configs to both server and client side

40207a0

spena reviewed Nov 1, 2023

View reviewed changes

aliehsaeedii added 3 commits November 2, 2023 14:30

fix: add security providers

ba9d0e8

fix: remove log files

4fc7c7d

fix:add providers in appropriate place

a34d2ba

spena previously approved these changes Nov 6, 2023

View reviewed changes

fix: remove key pass for trust store

7307fe3

aliehsaeedii dismissed spena’s stale review via 7307fe3 November 7, 2023 14:40

spena approved these changes Nov 7, 2023

View reviewed changes

aliehsaeedii merged commit 127f223 into master Nov 7, 2023
4 checks passed

aliehsaeedii deleted the fix-fedramp-issues branch November 7, 2023 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fedramp issues #10092

fix: fedramp issues #10092

aliehsaeedii commented Oct 16, 2023 •

edited

cla-assistant bot commented Oct 16, 2023 •

edited

cadonna left a comment

cadonna Oct 16, 2023

aliehsaeedii Oct 16, 2023

cadonna Oct 16, 2023

aliehsaeedii Oct 16, 2023

cadonna Oct 16, 2023

cadonna Oct 16, 2023

aliehsaeedii Oct 16, 2023

spena Oct 16, 2023

spena Oct 16, 2023

spena Oct 17, 2023

spena Oct 17, 2023

spena Oct 17, 2023

spena Nov 1, 2023

spena Nov 1, 2023

spena Nov 1, 2023


		final String password = getTrustStorePassword(props);

		if (!Strings.isNullOrEmpty(password)) {

	"The type of keystore file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";
	"The type of keystore file. Must be either 'JKS', 'PKCS12' or 'BCFKS'.";

	"The type of trust store file. Must be either 'JKS' or 'PKCS12' or 'BCFKS'.";
	"The type of trust store file. Must be either 'JKS', 'PKCS12' or 'BCFKS'.";

fix: fedramp issues #10092

fix: fedramp issues #10092

Conversation

aliehsaeedii commented Oct 16, 2023 • edited

Description

Testing done

Reviewer checklist

cla-assistant bot commented Oct 16, 2023 • edited

cadonna 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

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

aliehsaeedii commented Oct 16, 2023 •

edited

cla-assistant bot commented Oct 16, 2023 •

edited