You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/19 17:21:54 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
michaeljmarshall opened a new pull request, #18121:
URL: https://github.com/apache/pulsar/pull/18121
### Motivation
Give client applications the configure TLS Certificates, Keys, and TrustStores as in memory objects.
The naming for the new configurations had two options from a consistency perspective: the `ClientConfiguration` class and the `AuthenticationDataTls` class. Both classes have relevant field names. Since `AuthenticationDataTls` already supports in memory TLS configuration, I used the names from that class. The one downside to this implementation is that the names slightly diverge from the configurations in the same `ClientConfiguration` class.
Note also that the configuration works just like `AuthenticationDataTls`: when `tlsTrustStoreStream` is set, the other in memory certs/key are used.
### Modifications
* Add `tlsPrivateKey` to `ClientConfiguration`
* Add `tlsCertificates` to `ClientConfiguration`
* Add `tlsTrustStoreStream` to `ClientConfiguration`
* Update configuration of the `NettySslContext` to optionally use these in memory configs.
### Verifying this change
This is a trivial change, but it likely requires test coverage. @nodece is adding new test coverage here https://github.com/apache/pulsar/pull/17141. It might be worth adding tests that leverage that work so that there are not unnecessary conflicts.
### Does this pull request potentially affect one of the following parts:
- [x] The public API
This feature adds new configuration options. The fundamental
### Documentation
- [x] `doc-not-needed`
This PR includes annotated documentation in relevant classes that will generate documentation.
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/6
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r1004590825
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
+
+ @ApiModelProperty(
+ name = "tlsCertificates",
+ value = "Client certificate chain. Only used when tlsTrustStoreStream is non-null."
+ )
+ private Certificate[] tlsCertificates = null;
+
+ @ApiModelProperty(
+ name = "tlsTrustStoreStream",
+ value = "Input-stream of the trust store. When configured, the tlsPrivateKey and tlsCertificates are used."
+ )
+ private InputStream tlsTrustStoreStream = null;
Review Comment:
That explains the design for the `AuthenticationDataTls` constructor. I was wondering about that.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r999857890
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
Review Comment:
would it be possible to renew the key?
what about creating a interface like TLSClientMaterialProvider ?
interface TLSClientMaterialProvider {
PrivateKey getPrivateKey();
InputStream openTrustStoreStream();
Certificate[] getCertificateChain();
}
we can also provide default implementations that read from a local file (with automatic reload) or from a classpath resource
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
+
+ @ApiModelProperty(
+ name = "tlsCertificates",
+ value = "Client certificate chain. Only used when tlsTrustStoreStream is non-null."
+ )
+ private Certificate[] tlsCertificates = null;
+
+ @ApiModelProperty(
+ name = "tlsTrustStoreStream",
+ value = "Input-stream of the trust store. When configured, the tlsPrivateKey and tlsCertificates are used."
+ )
+ private InputStream tlsTrustStoreStream = null;
Review Comment:
please explain the lifecycle of this Stream.
is the Pulsar client intended to always read it fully and then close it ?
what happens in case of failure of the creation of the PulsarClient ?
also, who is responsible for closing it ?
as this Stream is bound to some external resource (a file, a network connection) the lifecycle of the object must be well-know.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#issuecomment-1284400326
There are some Find Bugs issues. Making a draft for now.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r999861935
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
Review Comment:
Great question. I was just noticing that the regular configuration does not allow for renewing the key, which is likely a missing feature. I was about to change my implementation to match this class: https://github.com/apache/pulsar/blob/330fcb9787d9f822667f0617b22bcae66b4644e5/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataTls.java#L69-L85
However, that might not work either.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r999862523
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
+
+ @ApiModelProperty(
+ name = "tlsCertificates",
+ value = "Client certificate chain. Only used when tlsTrustStoreStream is non-null."
+ )
+ private Certificate[] tlsCertificates = null;
+
+ @ApiModelProperty(
+ name = "tlsTrustStoreStream",
+ value = "Input-stream of the trust store. When configured, the tlsPrivateKey and tlsCertificates are used."
+ )
+ private InputStream tlsTrustStoreStream = null;
Review Comment:
I think it should have been `ByteArrayInputStream`, though I'm not sure that is the right direction.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r1004637290
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
Review Comment:
I agreed with Enrico, creating a provider is a good idea, because we have too many TLS configs on the builder.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#issuecomment-1295397036
I am going to take a different direction here. The PR will be similar to https://github.com/datastax/pulsar-jms/pull/91 and will require a PIP.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall closed pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
michaeljmarshall closed pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
URL: https://github.com/apache/pulsar/pull/18121
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18121: [feat][client] Add ClientConfiguration options for in memory TLS certs
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #18121:
URL: https://github.com/apache/pulsar/pull/18121#discussion_r1004583572
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java:
##########
@@ -162,6 +165,24 @@ public class ClientConfigurationData implements Serializable, Cloneable {
)
private String tlsTrustCertsFilePath = null;
+ @ApiModelProperty(
+ name = "tlsPrivateKey",
+ value = "The private key for the client certificate. Only used when tlsTrustStoreStream is non-null."
+ )
+ private PrivateKey tlsPrivateKey = null;
+
+ @ApiModelProperty(
+ name = "tlsCertificates",
+ value = "Client certificate chain. Only used when tlsTrustStoreStream is non-null."
+ )
+ private Certificate[] tlsCertificates = null;
+
+ @ApiModelProperty(
+ name = "tlsTrustStoreStream",
+ value = "Input-stream of the trust store. When configured, the tlsPrivateKey and tlsCertificates are used."
+ )
+ private InputStream tlsTrustStoreStream = null;
Review Comment:
It would be better to make the type `Supplier<InputStream>` instead of `InputStream`. The caller would take responsibility for closing the InputStream instance.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org