You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by Christopher Shannon <ch...@gmail.com> on 2024/01/16 14:38:46 UTC
[PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
As background, last week it was discovered (thanks Justin) that there was
an oversight with the Exception handling over OpenWire with the upgrade to
Jakarta apis in the 6.0.x broker.
https://issues.apache.org/jira/browse/AMQ-9418
The problem is that OpenWire has a response type to return an exception to
the client and this response type includes the actual throwable type
written as a string with the fully qualified package. This means that if an
older 5.x client that is using the javax JMS apis receives back something
like InvalidClientIDException from a 6.x broker the exception type received
is actually going to be "jakarta.jms.InvalidIDException" and then the
client can't unmarshall that since it's likely not on the classpath. The
result is that if it converts it will get a ClassNotFoundException and just
convert to a generic exception type so this could be a breaking change for
clients that are trying to handle specific exceptions. Note that the
reverse is actually also true, if a new 6.x client connected to a 5.x
broker the same problem happens.
I've been looking at ways to fix this broker side and it's not great. We
would need to detect if the client wants jakarta or javax types (we can
kind of do this for newer clients because
https://issues.apache.org/jira/browse/AMQ-6379 has new clients set their
actual client version from the manifest file in the jar) and then we have
to pass that to the marshallers and signal in some way to convert. The
main problem is having to pass it to the marshaller means probably having
to update the Openwire format to include the version discovered and having
complicated detection logic depending on if it is set or not as not
everything sets it. If we didn't want to do it at the marshaller level,
we'd have to include or repackage both versions of the Exceptions which is
also not ideal as having to include both javax and jakarta exception types
could cause mistakes elsewhere and maintaining isn't fun.
I think a much simpler approach is to just let the receiver (usually
client) handle it. I think instead of trying to have the broker detect what
to send, it should be handled on the unmarshal side of things by the
receiver of the error command. The receiver (client or broker) knows if it
will need to convert so it makes things much simpler. Obviously this would
only work if we update the marshaller in the client code jar so my proposal
would be to just release a small update in the next version of 5.18.x
client and also 6.0.x clients to handle this. We can just document saying
that if you need to have specific exception type translation and are using
a broker and client that are not both javax or jakarta then you need to
upgrade to a new client version for the translation. Otherwise they can
just get the generic Throwable back and will work. The main use case I
think is if clients are stuck on older versions of Java and can't upgrade
yet when talking to a new broker or an older broker can't be upgraded yet
but the clients can.
Here is an example of fixing the 5.18.x client:
https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
It's pretty simple, if we get back jakarta then convert to javax and done.
We would do the reverse in 6.x branch.
Going forward as a permanent fix, we should just fix this entirely in
OpenWire v13. Including the Java exception types already caused a major CVE
issue and now it's causing issues here. I think for the next OpenWire
version we should create a new Exception type that uses an Error code just
like Artemis and other projects do and then the client can just take the
code and convert it to whatever exception it wants. We could either include
the error handling change by itself in OpenWire v13 so we could even
upgrade older 5.x brokers with it or just include the new openwire
exception handling only for 6.x along with shared sub OpenWire changes.
Thoughts?
Re: [PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
Posted by Arthur Naseef <ar...@apache.org>.
Thanks Chris. I looked at the PR - looks good.
I hope nobody relies on our specific sub-classes of JMSException...
Art
On Tue, Jan 16, 2024 at 5:21 PM Christopher Shannon <
christopher.l.shannon@gmail.com> wrote:
> Art,
>
> For your first point, existing clients will run into this but they already
> properly handle the ClassNotFoundException fine so the client never sees
> the error. The marshaller just converts to a generic Throwable exception
> when it can't find the type given on the classpath and then the ActiveMQ
> client sees that and converts that to a JMSException. So it doesn't cause a
> client error but the exception type isn't quite right. (And no the broker
> doesn't create the type). My update fixes things so the client can
> translate easily back to the correct type so that the more specific JMS
> exception type and not a generic JMSException is thrown by the client.
>
> There is no breaking change so there is no need to opt in. This solution
> is fixing a bug when trying to convert exception types so there is no
> reason to allow turning it off. Without the change if a javax client talks
> to a jakarta broker (or vice versa) the exception type won't be mapped
> correctly and just be generic as I said. When we make things better in the
> future and use a new openwire version to handle things, this should be
> transparent and the client will still correctly use the right exception
> types by converting the newly returned error codes to the right exceptions
> so from the users stand point and client behavior there's no real change.
>
> I made all the changes and pushed up to the branches and tested things, you
> can see the commits linked to the Jira:
> https://issues.apache.org/jira/browse/AMQ-9418
>
> Chris
>
> On Tue, Jan 16, 2024 at 5:40 PM Arthur Naseef <ar...@amlinv.com> wrote:
>
> > Looks like the key points here are:
> >
> > 1. Existing clients will run into ClassNotFoundException when
> > unmarshalling an exception from a non-compatible broker. (Do broker's
> > ever
> > create an instance of the class specified by the client?)
> > 2. The proposed change will update clients to convert between
> javax.jms
> > and jakarta.jms package names
> > 3. In the future we can look to replace the existing approach with
> > something safer/simpler
> >
> > If so, I like that approach. One thing I can see that could be a concern
> > for users - going to the intermediate solution means that they will hit
> > another breaking change in the not-too-distant future when we get to (3).
> > I wonder if we could perhaps make it opt-in so users are pushed to
> > understand what they are getting into.
> >
> > Art
> >
> >
> >
> > On Tue, Jan 16, 2024 at 1:04 PM Christopher Shannon <
> > christopher.l.shannon@gmail.com> wrote:
> >
> > > I talked with Tim and Matt offline and will go with that approach, I
> have
> > > PRs open. If needed we can backport to some older clients too.
> > >
> > > On Tue, Jan 16, 2024 at 9:38 AM Christopher Shannon <
> > > christopher.l.shannon@gmail.com> wrote:
> > >
> > > > As background, last week it was discovered (thanks Justin) that there
> > was
> > > > an oversight with the Exception handling over OpenWire with the
> upgrade
> > > to
> > > > Jakarta apis in the 6.0.x broker.
> > > > https://issues.apache.org/jira/browse/AMQ-9418
> > > >
> > > > The problem is that OpenWire has a response type to return an
> exception
> > > to
> > > > the client and this response type includes the actual throwable type
> > > > written as a string with the fully qualified package. This means that
> > if
> > > an
> > > > older 5.x client that is using the javax JMS apis receives back
> > something
> > > > like InvalidClientIDException from a 6.x broker the exception type
> > > received
> > > > is actually going to be "jakarta.jms.InvalidIDException" and then the
> > > > client can't unmarshall that since it's likely not on the classpath.
> > The
> > > > result is that if it converts it will get a ClassNotFoundException
> and
> > > just
> > > > convert to a generic exception type so this could be a breaking
> change
> > > for
> > > > clients that are trying to handle specific exceptions. Note that the
> > > > reverse is actually also true, if a new 6.x client connected to a 5.x
> > > > broker the same problem happens.
> > > >
> > > > I've been looking at ways to fix this broker side and it's not great.
> > We
> > > > would need to detect if the client wants jakarta or javax types (we
> can
> > > > kind of do this for newer clients because
> > > > https://issues.apache.org/jira/browse/AMQ-6379 has new clients set
> > their
> > > > actual client version from the manifest file in the jar) and then we
> > have
> > > > to pass that to the marshallers and signal in some way to convert.
> The
> > > > main problem is having to pass it to the marshaller means probably
> > having
> > > > to update the Openwire format to include the version discovered and
> > > having
> > > > complicated detection logic depending on if it is set or not as not
> > > > everything sets it. If we didn't want to do it at the marshaller
> level,
> > > > we'd have to include or repackage both versions of the Exceptions
> which
> > > is
> > > > also not ideal as having to include both javax and jakarta exception
> > > types
> > > > could cause mistakes elsewhere and maintaining isn't fun.
> > > >
> > > > I think a much simpler approach is to just let the receiver (usually
> > > > client) handle it. I think instead of trying to have the broker
> detect
> > > what
> > > > to send, it should be handled on the unmarshal side of things by the
> > > > receiver of the error command. The receiver (client or broker) knows
> if
> > > it
> > > > will need to convert so it makes things much simpler. Obviously this
> > > would
> > > > only work if we update the marshaller in the client code jar so my
> > > proposal
> > > > would be to just release a small update in the next version of 5.18.x
> > > > client and also 6.0.x clients to handle this. We can just document
> > saying
> > > > that if you need to have specific exception type translation and are
> > > using
> > > > a broker and client that are not both javax or jakarta then you need
> to
> > > > upgrade to a new client version for the translation. Otherwise they
> can
> > > > just get the generic Throwable back and will work. The main use case
> I
> > > > think is if clients are stuck on older versions of Java and can't
> > upgrade
> > > > yet when talking to a new broker or an older broker can't be upgraded
> > yet
> > > > but the clients can.
> > > >
> > > > Here is an example of fixing the 5.18.x client:
> > > >
> > >
> >
> https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
> > > > It's pretty simple, if we get back jakarta then convert to javax and
> > > done.
> > > > We would do the reverse in 6.x branch.
> > > >
> > > > Going forward as a permanent fix, we should just fix this entirely in
> > > > OpenWire v13. Including the Java exception types already caused a
> major
> > > CVE
> > > > issue and now it's causing issues here. I think for the next OpenWire
> > > > version we should create a new Exception type that uses an Error code
> > > just
> > > > like Artemis and other projects do and then the client can just take
> > the
> > > > code and convert it to whatever exception it wants. We could either
> > > include
> > > > the error handling change by itself in OpenWire v13 so we could even
> > > > upgrade older 5.x brokers with it or just include the new openwire
> > > > exception handling only for 6.x along with shared sub OpenWire
> changes.
> > > >
> > > > Thoughts?
> > > >
> > > >
> > > >
> > >
> >
>
Re: [PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
Posted by Christopher Shannon <ch...@gmail.com>.
Art,
For your first point, existing clients will run into this but they already
properly handle the ClassNotFoundException fine so the client never sees
the error. The marshaller just converts to a generic Throwable exception
when it can't find the type given on the classpath and then the ActiveMQ
client sees that and converts that to a JMSException. So it doesn't cause a
client error but the exception type isn't quite right. (And no the broker
doesn't create the type). My update fixes things so the client can
translate easily back to the correct type so that the more specific JMS
exception type and not a generic JMSException is thrown by the client.
There is no breaking change so there is no need to opt in. This solution
is fixing a bug when trying to convert exception types so there is no
reason to allow turning it off. Without the change if a javax client talks
to a jakarta broker (or vice versa) the exception type won't be mapped
correctly and just be generic as I said. When we make things better in the
future and use a new openwire version to handle things, this should be
transparent and the client will still correctly use the right exception
types by converting the newly returned error codes to the right exceptions
so from the users stand point and client behavior there's no real change.
I made all the changes and pushed up to the branches and tested things, you
can see the commits linked to the Jira:
https://issues.apache.org/jira/browse/AMQ-9418
Chris
On Tue, Jan 16, 2024 at 5:40 PM Arthur Naseef <ar...@amlinv.com> wrote:
> Looks like the key points here are:
>
> 1. Existing clients will run into ClassNotFoundException when
> unmarshalling an exception from a non-compatible broker. (Do broker's
> ever
> create an instance of the class specified by the client?)
> 2. The proposed change will update clients to convert between javax.jms
> and jakarta.jms package names
> 3. In the future we can look to replace the existing approach with
> something safer/simpler
>
> If so, I like that approach. One thing I can see that could be a concern
> for users - going to the intermediate solution means that they will hit
> another breaking change in the not-too-distant future when we get to (3).
> I wonder if we could perhaps make it opt-in so users are pushed to
> understand what they are getting into.
>
> Art
>
>
>
> On Tue, Jan 16, 2024 at 1:04 PM Christopher Shannon <
> christopher.l.shannon@gmail.com> wrote:
>
> > I talked with Tim and Matt offline and will go with that approach, I have
> > PRs open. If needed we can backport to some older clients too.
> >
> > On Tue, Jan 16, 2024 at 9:38 AM Christopher Shannon <
> > christopher.l.shannon@gmail.com> wrote:
> >
> > > As background, last week it was discovered (thanks Justin) that there
> was
> > > an oversight with the Exception handling over OpenWire with the upgrade
> > to
> > > Jakarta apis in the 6.0.x broker.
> > > https://issues.apache.org/jira/browse/AMQ-9418
> > >
> > > The problem is that OpenWire has a response type to return an exception
> > to
> > > the client and this response type includes the actual throwable type
> > > written as a string with the fully qualified package. This means that
> if
> > an
> > > older 5.x client that is using the javax JMS apis receives back
> something
> > > like InvalidClientIDException from a 6.x broker the exception type
> > received
> > > is actually going to be "jakarta.jms.InvalidIDException" and then the
> > > client can't unmarshall that since it's likely not on the classpath.
> The
> > > result is that if it converts it will get a ClassNotFoundException and
> > just
> > > convert to a generic exception type so this could be a breaking change
> > for
> > > clients that are trying to handle specific exceptions. Note that the
> > > reverse is actually also true, if a new 6.x client connected to a 5.x
> > > broker the same problem happens.
> > >
> > > I've been looking at ways to fix this broker side and it's not great.
> We
> > > would need to detect if the client wants jakarta or javax types (we can
> > > kind of do this for newer clients because
> > > https://issues.apache.org/jira/browse/AMQ-6379 has new clients set
> their
> > > actual client version from the manifest file in the jar) and then we
> have
> > > to pass that to the marshallers and signal in some way to convert. The
> > > main problem is having to pass it to the marshaller means probably
> having
> > > to update the Openwire format to include the version discovered and
> > having
> > > complicated detection logic depending on if it is set or not as not
> > > everything sets it. If we didn't want to do it at the marshaller level,
> > > we'd have to include or repackage both versions of the Exceptions which
> > is
> > > also not ideal as having to include both javax and jakarta exception
> > types
> > > could cause mistakes elsewhere and maintaining isn't fun.
> > >
> > > I think a much simpler approach is to just let the receiver (usually
> > > client) handle it. I think instead of trying to have the broker detect
> > what
> > > to send, it should be handled on the unmarshal side of things by the
> > > receiver of the error command. The receiver (client or broker) knows if
> > it
> > > will need to convert so it makes things much simpler. Obviously this
> > would
> > > only work if we update the marshaller in the client code jar so my
> > proposal
> > > would be to just release a small update in the next version of 5.18.x
> > > client and also 6.0.x clients to handle this. We can just document
> saying
> > > that if you need to have specific exception type translation and are
> > using
> > > a broker and client that are not both javax or jakarta then you need to
> > > upgrade to a new client version for the translation. Otherwise they can
> > > just get the generic Throwable back and will work. The main use case I
> > > think is if clients are stuck on older versions of Java and can't
> upgrade
> > > yet when talking to a new broker or an older broker can't be upgraded
> yet
> > > but the clients can.
> > >
> > > Here is an example of fixing the 5.18.x client:
> > >
> >
> https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
> > > It's pretty simple, if we get back jakarta then convert to javax and
> > done.
> > > We would do the reverse in 6.x branch.
> > >
> > > Going forward as a permanent fix, we should just fix this entirely in
> > > OpenWire v13. Including the Java exception types already caused a major
> > CVE
> > > issue and now it's causing issues here. I think for the next OpenWire
> > > version we should create a new Exception type that uses an Error code
> > just
> > > like Artemis and other projects do and then the client can just take
> the
> > > code and convert it to whatever exception it wants. We could either
> > include
> > > the error handling change by itself in OpenWire v13 so we could even
> > > upgrade older 5.x brokers with it or just include the new openwire
> > > exception handling only for 6.x along with shared sub OpenWire changes.
> > >
> > > Thoughts?
> > >
> > >
> > >
> >
>
Re: [PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
Posted by Arthur Naseef <ar...@amlinv.com>.
Looks like the key points here are:
1. Existing clients will run into ClassNotFoundException when
unmarshalling an exception from a non-compatible broker. (Do broker's ever
create an instance of the class specified by the client?)
2. The proposed change will update clients to convert between javax.jms
and jakarta.jms package names
3. In the future we can look to replace the existing approach with
something safer/simpler
If so, I like that approach. One thing I can see that could be a concern
for users - going to the intermediate solution means that they will hit
another breaking change in the not-too-distant future when we get to (3).
I wonder if we could perhaps make it opt-in so users are pushed to
understand what they are getting into.
Art
On Tue, Jan 16, 2024 at 1:04 PM Christopher Shannon <
christopher.l.shannon@gmail.com> wrote:
> I talked with Tim and Matt offline and will go with that approach, I have
> PRs open. If needed we can backport to some older clients too.
>
> On Tue, Jan 16, 2024 at 9:38 AM Christopher Shannon <
> christopher.l.shannon@gmail.com> wrote:
>
> > As background, last week it was discovered (thanks Justin) that there was
> > an oversight with the Exception handling over OpenWire with the upgrade
> to
> > Jakarta apis in the 6.0.x broker.
> > https://issues.apache.org/jira/browse/AMQ-9418
> >
> > The problem is that OpenWire has a response type to return an exception
> to
> > the client and this response type includes the actual throwable type
> > written as a string with the fully qualified package. This means that if
> an
> > older 5.x client that is using the javax JMS apis receives back something
> > like InvalidClientIDException from a 6.x broker the exception type
> received
> > is actually going to be "jakarta.jms.InvalidIDException" and then the
> > client can't unmarshall that since it's likely not on the classpath. The
> > result is that if it converts it will get a ClassNotFoundException and
> just
> > convert to a generic exception type so this could be a breaking change
> for
> > clients that are trying to handle specific exceptions. Note that the
> > reverse is actually also true, if a new 6.x client connected to a 5.x
> > broker the same problem happens.
> >
> > I've been looking at ways to fix this broker side and it's not great. We
> > would need to detect if the client wants jakarta or javax types (we can
> > kind of do this for newer clients because
> > https://issues.apache.org/jira/browse/AMQ-6379 has new clients set their
> > actual client version from the manifest file in the jar) and then we have
> > to pass that to the marshallers and signal in some way to convert. The
> > main problem is having to pass it to the marshaller means probably having
> > to update the Openwire format to include the version discovered and
> having
> > complicated detection logic depending on if it is set or not as not
> > everything sets it. If we didn't want to do it at the marshaller level,
> > we'd have to include or repackage both versions of the Exceptions which
> is
> > also not ideal as having to include both javax and jakarta exception
> types
> > could cause mistakes elsewhere and maintaining isn't fun.
> >
> > I think a much simpler approach is to just let the receiver (usually
> > client) handle it. I think instead of trying to have the broker detect
> what
> > to send, it should be handled on the unmarshal side of things by the
> > receiver of the error command. The receiver (client or broker) knows if
> it
> > will need to convert so it makes things much simpler. Obviously this
> would
> > only work if we update the marshaller in the client code jar so my
> proposal
> > would be to just release a small update in the next version of 5.18.x
> > client and also 6.0.x clients to handle this. We can just document saying
> > that if you need to have specific exception type translation and are
> using
> > a broker and client that are not both javax or jakarta then you need to
> > upgrade to a new client version for the translation. Otherwise they can
> > just get the generic Throwable back and will work. The main use case I
> > think is if clients are stuck on older versions of Java and can't upgrade
> > yet when talking to a new broker or an older broker can't be upgraded yet
> > but the clients can.
> >
> > Here is an example of fixing the 5.18.x client:
> >
> https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
> > It's pretty simple, if we get back jakarta then convert to javax and
> done.
> > We would do the reverse in 6.x branch.
> >
> > Going forward as a permanent fix, we should just fix this entirely in
> > OpenWire v13. Including the Java exception types already caused a major
> CVE
> > issue and now it's causing issues here. I think for the next OpenWire
> > version we should create a new Exception type that uses an Error code
> just
> > like Artemis and other projects do and then the client can just take the
> > code and convert it to whatever exception it wants. We could either
> include
> > the error handling change by itself in OpenWire v13 so we could even
> > upgrade older 5.x brokers with it or just include the new openwire
> > exception handling only for 6.x along with shared sub OpenWire changes.
> >
> > Thoughts?
> >
> >
> >
>
Re: [PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
Posted by Christopher Shannon <ch...@gmail.com>.
I talked with Tim and Matt offline and will go with that approach, I have
PRs open. If needed we can backport to some older clients too.
On Tue, Jan 16, 2024 at 9:38 AM Christopher Shannon <
christopher.l.shannon@gmail.com> wrote:
> As background, last week it was discovered (thanks Justin) that there was
> an oversight with the Exception handling over OpenWire with the upgrade to
> Jakarta apis in the 6.0.x broker.
> https://issues.apache.org/jira/browse/AMQ-9418
>
> The problem is that OpenWire has a response type to return an exception to
> the client and this response type includes the actual throwable type
> written as a string with the fully qualified package. This means that if an
> older 5.x client that is using the javax JMS apis receives back something
> like InvalidClientIDException from a 6.x broker the exception type received
> is actually going to be "jakarta.jms.InvalidIDException" and then the
> client can't unmarshall that since it's likely not on the classpath. The
> result is that if it converts it will get a ClassNotFoundException and just
> convert to a generic exception type so this could be a breaking change for
> clients that are trying to handle specific exceptions. Note that the
> reverse is actually also true, if a new 6.x client connected to a 5.x
> broker the same problem happens.
>
> I've been looking at ways to fix this broker side and it's not great. We
> would need to detect if the client wants jakarta or javax types (we can
> kind of do this for newer clients because
> https://issues.apache.org/jira/browse/AMQ-6379 has new clients set their
> actual client version from the manifest file in the jar) and then we have
> to pass that to the marshallers and signal in some way to convert. The
> main problem is having to pass it to the marshaller means probably having
> to update the Openwire format to include the version discovered and having
> complicated detection logic depending on if it is set or not as not
> everything sets it. If we didn't want to do it at the marshaller level,
> we'd have to include or repackage both versions of the Exceptions which is
> also not ideal as having to include both javax and jakarta exception types
> could cause mistakes elsewhere and maintaining isn't fun.
>
> I think a much simpler approach is to just let the receiver (usually
> client) handle it. I think instead of trying to have the broker detect what
> to send, it should be handled on the unmarshal side of things by the
> receiver of the error command. The receiver (client or broker) knows if it
> will need to convert so it makes things much simpler. Obviously this would
> only work if we update the marshaller in the client code jar so my proposal
> would be to just release a small update in the next version of 5.18.x
> client and also 6.0.x clients to handle this. We can just document saying
> that if you need to have specific exception type translation and are using
> a broker and client that are not both javax or jakarta then you need to
> upgrade to a new client version for the translation. Otherwise they can
> just get the generic Throwable back and will work. The main use case I
> think is if clients are stuck on older versions of Java and can't upgrade
> yet when talking to a new broker or an older broker can't be upgraded yet
> but the clients can.
>
> Here is an example of fixing the 5.18.x client:
> https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
> It's pretty simple, if we get back jakarta then convert to javax and done.
> We would do the reverse in 6.x branch.
>
> Going forward as a permanent fix, we should just fix this entirely in
> OpenWire v13. Including the Java exception types already caused a major CVE
> issue and now it's causing issues here. I think for the next OpenWire
> version we should create a new Exception type that uses an Error code just
> like Artemis and other projects do and then the client can just take the
> code and convert it to whatever exception it wants. We could either include
> the error handling change by itself in OpenWire v13 so we could even
> upgrade older 5.x brokers with it or just include the new openwire
> exception handling only for 6.x along with shared sub OpenWire changes.
>
> Thoughts?
>
>
>
Re: [PROPOSAL] OpenWire exception handling mismatch between Javax and Jakarta APIs
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
It looks good to me.
Thanks,
Regards
JB
On Tue, Jan 16, 2024 at 3:38 PM Christopher Shannon
<ch...@gmail.com> wrote:
>
> As background, last week it was discovered (thanks Justin) that there was
> an oversight with the Exception handling over OpenWire with the upgrade to
> Jakarta apis in the 6.0.x broker.
> https://issues.apache.org/jira/browse/AMQ-9418
>
> The problem is that OpenWire has a response type to return an exception to
> the client and this response type includes the actual throwable type
> written as a string with the fully qualified package. This means that if an
> older 5.x client that is using the javax JMS apis receives back something
> like InvalidClientIDException from a 6.x broker the exception type received
> is actually going to be "jakarta.jms.InvalidIDException" and then the
> client can't unmarshall that since it's likely not on the classpath. The
> result is that if it converts it will get a ClassNotFoundException and just
> convert to a generic exception type so this could be a breaking change for
> clients that are trying to handle specific exceptions. Note that the
> reverse is actually also true, if a new 6.x client connected to a 5.x
> broker the same problem happens.
>
> I've been looking at ways to fix this broker side and it's not great. We
> would need to detect if the client wants jakarta or javax types (we can
> kind of do this for newer clients because
> https://issues.apache.org/jira/browse/AMQ-6379 has new clients set their
> actual client version from the manifest file in the jar) and then we have
> to pass that to the marshallers and signal in some way to convert. The
> main problem is having to pass it to the marshaller means probably having
> to update the Openwire format to include the version discovered and having
> complicated detection logic depending on if it is set or not as not
> everything sets it. If we didn't want to do it at the marshaller level,
> we'd have to include or repackage both versions of the Exceptions which is
> also not ideal as having to include both javax and jakarta exception types
> could cause mistakes elsewhere and maintaining isn't fun.
>
> I think a much simpler approach is to just let the receiver (usually
> client) handle it. I think instead of trying to have the broker detect what
> to send, it should be handled on the unmarshal side of things by the
> receiver of the error command. The receiver (client or broker) knows if it
> will need to convert so it makes things much simpler. Obviously this would
> only work if we update the marshaller in the client code jar so my proposal
> would be to just release a small update in the next version of 5.18.x
> client and also 6.0.x clients to handle this. We can just document saying
> that if you need to have specific exception type translation and are using
> a broker and client that are not both javax or jakarta then you need to
> upgrade to a new client version for the translation. Otherwise they can
> just get the generic Throwable back and will work. The main use case I
> think is if clients are stuck on older versions of Java and can't upgrade
> yet when talking to a new broker or an older broker can't be upgraded yet
> but the clients can.
>
> Here is an example of fixing the 5.18.x client:
> https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4
> It's pretty simple, if we get back jakarta then convert to javax and done.
> We would do the reverse in 6.x branch.
>
> Going forward as a permanent fix, we should just fix this entirely in
> OpenWire v13. Including the Java exception types already caused a major CVE
> issue and now it's causing issues here. I think for the next OpenWire
> version we should create a new Exception type that uses an Error code just
> like Artemis and other projects do and then the client can just take the
> code and convert it to whatever exception it wants. We could either include
> the error handling change by itself in OpenWire v13 so we could even
> upgrade older 5.x brokers with it or just include the new openwire
> exception handling only for 6.x along with shared sub OpenWire changes.
>
> Thoughts?