You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/09/28 19:01:16 UTC
[GitHub] [kafka] rajinisivaram opened a new pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
rajinisivaram opened a new pull request #9345:
URL: https://github.com/apache/kafka/pull/9345
Adds support for SSL key and trust stores to be specified in PEM format either as files or directly as configuration values.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-704456290
@omkreddy Thanks for the review, merging to trunk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] teabot commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
teabot commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-916994728
How is PEM certificate renewal possible on the producer/consumer client? Is this documented anywhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram merged pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #9345:
URL: https://github.com/apache/kafka/pull/9345
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-700664402
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] amccague commented on a change in pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
amccague commented on a change in pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#discussion_r758306900
##########
File path: clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java
##########
@@ -246,56 +269,104 @@ private SSLContext createSSLContext() {
}
}
- private static SecurityStore createKeystore(String type, String path, Password password, Password keyPassword) {
- if (path == null && password != null) {
- throw new KafkaException("SSL key store is not specified, but key store password is specified.");
+ // Visibility to override for testing
+ protected SecurityStore createKeystore(String type, String path, Password password, Password keyPassword, Password privateKey, Password certificateChain) {
+ if (privateKey != null) {
+ if (!PEM_TYPE.equals(type))
+ throw new InvalidConfigurationException("SSL private key can be specified only for PEM, but key store type is " + type + ".");
+ else if (certificateChain == null)
+ throw new InvalidConfigurationException("SSL private key is specified, but certificate chain is not specified.");
+ else if (path != null)
+ throw new InvalidConfigurationException("Both SSL key store location and separate private key are specified.");
+ else if (password != null)
+ throw new InvalidConfigurationException("SSL key store password cannot be specified with PEM format, only key password may be specified.");
+ else
+ return new PemStore(certificateChain, privateKey, keyPassword);
+ } else if (certificateChain != null) {
+ throw new InvalidConfigurationException("SSL certificate chain is specified, but private key is not specified");
+ } else if (PEM_TYPE.equals(type) && path != null) {
+ if (password != null)
+ throw new InvalidConfigurationException("SSL key store password cannot be specified with PEM format, only key password may be specified");
+ else if (keyPassword == null)
+ throw new InvalidConfigurationException("SSL PEM key store is specified, but key password is not specified.");
Review comment:
Is there an intention here to not allow a `FileBasedPemStore` containing an unencrypted private key when a key could be provided to `PemStore` with a `null` `keyPassword`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#discussion_r500360036
##########
File path: clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##########
@@ -167,17 +181,12 @@ public void testValidEndpointIdentificationSanIp() throws Exception {
@Test
public void testValidEndpointIdentificationCN() throws Exception {
String node = "0";
Review comment:
fixed these and few other warnings
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#discussion_r500359056
##########
File path: clients/src/test/java/org/apache/kafka/common/network/CertStores.java
##########
@@ -54,13 +60,30 @@ public CertStores(boolean server, String commonName, InetAddress hostAddress) th
}
private CertStores(boolean server, String commonName, TestSslUtils.CertificateBuilder certBuilder) throws Exception {
+ this(server, commonName, "RSA", certBuilder, false);
+ }
+
+ private CertStores(boolean server, String commonName, String keyAlgorithm, TestSslUtils.CertificateBuilder certBuilder, boolean usePem) throws Exception {
String name = server ? "server" : "client";
Mode mode = server ? Mode.SERVER : Mode.CLIENT;
- File truststoreFile = File.createTempFile(name + "TS", ".jks");
- sslConfig = TestSslUtils.createSslConfig(!server, true, mode, truststoreFile, name, commonName, certBuilder);
+ File truststoreFile = usePem ? null : File.createTempFile(name + "TS", ".jks");
+ sslConfig = new SslConfigsBuilder(mode)
+ .useClientCert(!server)
+ .certAlias(name)
+ .cn(commonName)
+ .createNewTrustStore(truststoreFile)
+ .certBuilder(certBuilder)
+ .algorithm(keyAlgorithm)
+ .usePem(usePem)
+ .build();
}
+
public Map<String, Object> getTrustingConfig(CertStores truststoreConfig) {
+ return getTrustingConfig(truststoreConfig, false);
+ }
+
+ public Map<String, Object> getTrustingConfig(CertStores truststoreConfig, boolean usePemCerts) {
Review comment:
removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-704327619
@omkreddy Thanks for the review, have addressed the comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] teabot commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
teabot commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-1067106590
See: https://github.com/confluentinc/schema-registry/pull/2062
On Mon, 14 Mar 2022 at 14:44, Peter Vendamere ***@***.***>
wrote:
> Was this change meant to work for the schema registry client as well? I've
> tried connecting to the schema registry using these settings, but it fails
> with a message indicating it's looking for a file still.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/kafka/pull/9345#issuecomment-1066888018>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABX4VWRLMTKQFQABRUB553U75GETANCNFSM4R43VXXA>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vendamere commented on pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
vendamere commented on pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#issuecomment-1066888018
Was this change meant to work for the schema registry client as well? I've tried connecting to the schema registry using these settings, but it fails with a message indicating it's looking for a file still.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on a change in pull request #9345: KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651)
Posted by GitBox <gi...@apache.org>.
omkreddy commented on a change in pull request #9345:
URL: https://github.com/apache/kafka/pull/9345#discussion_r500186433
##########
File path: clients/src/test/java/org/apache/kafka/test/TestSslUtils.java
##########
@@ -199,6 +206,156 @@ public static void createKeyStore(String filename,
return builder.build();
}
+ public static void convertToPem(Map<String, Object> sslProps, boolean writeToFile, boolean encryptPrivateKey) throws Exception {
+ String tsPath = (String) sslProps.get(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG);
+ String tsType = (String) sslProps.get(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG);
+ Password tsPassword = (Password) sslProps.remove(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG);
+ Password trustCerts = (Password) sslProps.remove(SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG);
+ if (trustCerts == null && tsPath != null) {
+ trustCerts = exportCertificates(tsPath, tsPassword, tsType);
+ }
+ if (trustCerts != null) {
+ if (tsPath == null) {
+ tsPath = File.createTempFile("truststore", ".pem").getPath();
+ sslProps.put(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG, tsPath);
+ }
+ sslProps.put(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG, PEM_TYPE);
+ if (writeToFile)
+ writeToFile(tsPath, trustCerts);
+ else {
+ sslProps.put(SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG, trustCerts);
+ sslProps.remove(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG);
+ }
+ }
+
+ String ksPath = (String) sslProps.get(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG);
+ Password certChain = (Password) sslProps.remove(SslConfigs.SSL_KEYSTORE_CERTIFICATE_CHAIN_CONFIG);
+ Password key = (Password) sslProps.remove(SslConfigs.SSL_KEYSTORE_KEY_CONFIG);
+ if (certChain == null && ksPath != null) {
+ String ksType = (String) sslProps.get(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG);
+ Password ksPassword = (Password) sslProps.remove(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG);
+ Password keyPassword = (Password) sslProps.get(SslConfigs.SSL_KEY_PASSWORD_CONFIG);
+ certChain = exportCertificates(ksPath, ksPassword, ksType);
+ Password pemKeyPassword = encryptPrivateKey ? keyPassword : null;
+ key = exportPrivateKey(ksPath, ksPassword, keyPassword, ksType, pemKeyPassword);
+ if (!encryptPrivateKey)
+ sslProps.remove(SslConfigs.SSL_KEY_PASSWORD_CONFIG);
+ } else if (!encryptPrivateKey) {
Review comment:
Can we remove empty else block?
##########
File path: clients/src/test/java/org/apache/kafka/common/network/CertStores.java
##########
@@ -54,13 +60,30 @@ public CertStores(boolean server, String commonName, InetAddress hostAddress) th
}
private CertStores(boolean server, String commonName, TestSslUtils.CertificateBuilder certBuilder) throws Exception {
+ this(server, commonName, "RSA", certBuilder, false);
+ }
+
+ private CertStores(boolean server, String commonName, String keyAlgorithm, TestSslUtils.CertificateBuilder certBuilder, boolean usePem) throws Exception {
String name = server ? "server" : "client";
Mode mode = server ? Mode.SERVER : Mode.CLIENT;
- File truststoreFile = File.createTempFile(name + "TS", ".jks");
- sslConfig = TestSslUtils.createSslConfig(!server, true, mode, truststoreFile, name, commonName, certBuilder);
+ File truststoreFile = usePem ? null : File.createTempFile(name + "TS", ".jks");
+ sslConfig = new SslConfigsBuilder(mode)
+ .useClientCert(!server)
+ .certAlias(name)
+ .cn(commonName)
+ .createNewTrustStore(truststoreFile)
+ .certBuilder(certBuilder)
+ .algorithm(keyAlgorithm)
+ .usePem(usePem)
+ .build();
}
+
public Map<String, Object> getTrustingConfig(CertStores truststoreConfig) {
+ return getTrustingConfig(truststoreConfig, false);
+ }
+
+ public Map<String, Object> getTrustingConfig(CertStores truststoreConfig, boolean usePemCerts) {
Review comment:
unused usePemCerts?
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -923,9 +926,12 @@ object KafkaConfig {
val SslKeystoreLocationDoc = SslConfigs.SSL_KEYSTORE_LOCATION_DOC
val SslKeystorePasswordDoc = SslConfigs.SSL_KEYSTORE_PASSWORD_DOC
val SslKeyPasswordDoc = SslConfigs.SSL_KEY_PASSWORD_DOC
+ val SslKeystoreKeyDoc = SslConfigs.SSL_KEYSTORE_KEY_DOC
+ val SslKeystoreCertificateChainDoc = SslConfigs.SSL_KEYSTORE_CERTIFICATE_CHAIN_CONFIG
Review comment:
SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG => SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_DOC
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -923,9 +926,12 @@ object KafkaConfig {
val SslKeystoreLocationDoc = SslConfigs.SSL_KEYSTORE_LOCATION_DOC
val SslKeystorePasswordDoc = SslConfigs.SSL_KEYSTORE_PASSWORD_DOC
val SslKeyPasswordDoc = SslConfigs.SSL_KEY_PASSWORD_DOC
+ val SslKeystoreKeyDoc = SslConfigs.SSL_KEYSTORE_KEY_DOC
+ val SslKeystoreCertificateChainDoc = SslConfigs.SSL_KEYSTORE_CERTIFICATE_CHAIN_CONFIG
val SslTruststoreTypeDoc = SslConfigs.SSL_TRUSTSTORE_TYPE_DOC
val SslTruststorePasswordDoc = SslConfigs.SSL_TRUSTSTORE_PASSWORD_DOC
val SslTruststoreLocationDoc = SslConfigs.SSL_TRUSTSTORE_LOCATION_DOC
+ val SslTruststoreCertificatesDoc = SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG
Review comment:
SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG => SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_DOC
##########
File path: clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##########
@@ -167,17 +181,12 @@ public void testValidEndpointIdentificationSanIp() throws Exception {
@Test
public void testValidEndpointIdentificationCN() throws Exception {
String node = "0";
Review comment:
unused `node` variable here and other methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org