Skip to content

ARTEMIS-3302 swap deprecated X509Certificate#3671

Closed
jbertram wants to merge 1 commit intoapache:mainfrom
jbertram:ARTEMIS-3302
Closed

ARTEMIS-3302 swap deprecated X509Certificate#3671
jbertram wants to merge 1 commit intoapache:mainfrom
jbertram:ARTEMIS-3302

Conversation

@jbertram
Copy link
Contributor

Casting the result of getPeerCertificates() to X509Certificate[] mirrors
what is done in the ActiveMQ "Classic" code-base.

@jbertram jbertram force-pushed the ARTEMIS-3302 branch 3 times, most recently from ab3935e to 4d09ee4 Compare July 26, 2021 15:49
@gemmellr
Copy link
Member

I think you may have missed some related instances, in the test modules I think, as the PR only touches 13 files but the related warnings are emitted for 16 files. I added a list on https://issues.apache.org/jira/browse/ARTEMIS-3302.

(Should be more visible in logs of a fresh CI run, now that I fixed a similar problem with my own earlier changes on ARTEMIS 3304 via d9a4400)

@jbertram
Copy link
Contributor Author

@gemmellr, I updated everything except org.apache.activemq.transport.tcp.StubSSLSession because it is actually implementing javax.net.ssl.SSLSession so it must use javax.security.cert.X509Certificate[]. This, of course, will cause a problem when it is eventually removed as that class will still work in older JDKs but will fail in the latest. It looks like the only place where StubSSLSession is used is org.apache.activemq.transport.tcp.SslTransportTest. I'm not sure if we should wait and remove that test now or later. What do you think?

@gemmellr
Copy link
Member

@jbertram Ah. I didnt actually look at every use so hadnt spotted that hehe.

Looking at what StubSSLSession is doing I would probably just try to replace it, e.g with an mock, or faking one with a Proxy. Its not really implementing any but 1 of the methods, and not the one of concern as it is ironically using the intended replacement. That one test usage isnt even using the full functionality implemented in the stub, as its only used when verification is required, and so the stub is arguably even over-engineered. A replacement could just be doing a trivial 'return ' action for the interesting method.

The test could even alternatively be implemented with a real SSLSession.

@gemmellr
Copy link
Member

So, actually looking at the wider usage and considering it a bit more....the particular use is in testing for a SslTransport class coming from the activemq-client 5.14.0 dependency used in that module. Seems those tests arent even using the same version that the artemis-openwire-protocol actually does, since that is on 5.16.0.

Its not clear to me that the class under test is actually used anywhere in the broker. If not it would seem like the stub and related test etc should just be straight up deleted because they would be extraneous already, not testing anything the artemis codebase could influence.

Casting the result of getPeerCertificates() to X509Certificate[] mirrors
what is done in the ActiveMQ "Classic" code-base.

A few tests which were imported from ActiveMQ "Classic" to verify our
OpenWire implementation were removed as they relied on a "stub"
implementation of javax.net.ssl.SSLSession that never would have worked
across multiple JDKs once javax.security.cert.X509Certificate[] was
removed. Furthermore, the tests appeared to be related to the OpenWire
*client* and not relevant to our broker-side implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants