gemmellr commented on code in PR #5956:
URL: https://github.com/apache/activemq-artemis/pull/5956#discussion_r2426633478
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java:
##########
@@ -481,4 +490,48 @@ private void closeChannel(final Channel channel, boolean
inEventLoop) {
}
}
+ public X509Certificate[] getCertificates() {
+ return Objects.requireNonNullElse(certificates, getCertsFromChannel());
Review Comment:
This seems wrong as it will a) Always have to call getCertsFromChannel
first, even if it already has 'certificates', which is both inefficient and
leads to other problems (see later) and b) Throws if the latter returns null,
which it wouldnt in the existing bits and shouldnt since there is absolutely no
guarantee the connection has peer certificates. Disposing of the
requireNonNullElse would allow avoiding both of those things.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java:
##########
@@ -481,4 +490,48 @@ private void closeChannel(final Channel channel, boolean
inEventLoop) {
}
}
+ public X509Certificate[] getCertificates() {
+ return Objects.requireNonNullElse(certificates, getCertsFromChannel());
+ }
+
+ private X509Certificate[] getCertsFromChannel() {
+ Certificate[] plainCerts = null;
+ ChannelHandler channelHandler = channel.pipeline().get("ssl");
+ if (channelHandler instanceof SslHandler sslHandler) {
+ try {
+ plainCerts =
sslHandler.engine().getSession().getPeerCertificates();
+ } catch (SSLPeerUnverifiedException e) {
+ // ignore
+ }
+ }
+
+ /*
+ * When using the OpenSSL provider on the broker the
getPeerCertificates() method does *not* return a
+ * X509Certificate[] so we need to convert the Certificate[] that is
returned. This code is inspired by Tomcat's
+ * org.apache.tomcat.util.net.jsse.JSSESupport class.
+ */
+ certificates = null;
Review Comment:
I think having a local value as the existing code did (x509Certs) that is
assigned to the field only at the end would be nicer and safer than this. It is
setting the field null here (even if already non-null), then setting it
non-null below, and again is doing this every time the calling
getCertificates() method is called currently as it must always call
this...making it totally unsafe if ever called concurrently as the field would
flip flop to/from null as they operated. At least with the local value, worst
case is they both safely assign the same thing separately at the end.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
