Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARTEMIS-5163 Broker fails to send MQTT LWT using certificate-based mutual TLS #5460

Closed
wants to merge 11 commits into from
Closed

ARTEMIS-5163 Broker fails to send MQTT LWT using certificate-based mutual TLS #5460

wants to merge 11 commits into from

Conversation

JeanLucGraphalo
Copy link

Implements a fix of bug ARTEMIS-5163.

The changes follow the proposals as described in the ticket comments.
A lot of code refactoring was necessary to fix the problem, which could make the review challenging.

@JeanLucGraphalo
ARTEMIS-5163: LWT identity added
2bf3298 
A Subject attribute has been added to MQTTSessionState
in order to enable the broker to send LWT.
The provided client's identity is necessary when using
mutual TLS for authentication.
@JeanLucGraphalo
ARTEMIS-5163: Prepare MQTTPublishManager to use will identity
5c3cea6 
The newly created willIdentity attribute is considered to
be used when sending the LWT.
Some refactoring to the class MQTTPublishManager has been
applied.
@JeanLucGraphalo
ARTEMIS-5163: Prepare ServerSessionImpl to consider will identity
e69acf8 
Some refactoring to the class ServerSessionImpl has been
applied.
@JeanLucGraphalo
ARTEMIS-5163: Prepare ServerSessionImpl to call new security check
22aec2a 
Some refactoring to the class ServerSessionImpl has been
applied in order to be able to call a new SecurityStore::check
that accepts a Subject attribute.
@JeanLucGraphalo
ARTEMIS-5163: Implement check without re-authentication
47e993c 
A checkWithoutReAuthentication method has been added to
SecurityStore and implemented in SecurityStoreImpl.
This method will apply an authorization check based on
the given Subject attribute.
@JeanLucGraphalo
ARTEMIS-5163: Update stored Subject on change
915346d 
Each time the HierarchicalRepositoryChangeListener.onChange()
method is called, the willIdentity is updated. This is
implemented by using the security repository in the class
SecurityStore.
@JeanLucGraphalo
ARTEMIS-5163: Unregister listener when session is stopped
61c3c7b 
The method unregisterWillIdentityUpdateFromSecurityRepository()
is additionally called when the MQTTSession is stopped
@JeanLucGraphalo
ARTEMIS-5163: Consider Subject when creating LWT address
7fb3588 
The will identity Subject is considered when creating a new
address, that is used to send the LWT.
@JeanLucGraphalo
ARTEMIS-5163: Refactor MQTTPublishManager::sendToQueue
b926257 
Some reduction to the method's cognitive complexity.
@JeanLucGraphalo
ARTEMIS-5163: Test setting and unsetting willIdentity
833a141 
Tests were added to check set/unset of the willIdentity attribute.
@JeanLucGraphalo
ARTEMIS-5163: Test send LWT using mutual TLS
a485243 
Tests were added to check successful and failing sending of the LWT
when certificate-based mutual TLS is used. This illustrates the
need to store the client's user and role in Subject format as an
MQTTSessionState attribute.
@jbertram
Copy link
Contributor

First off, thanks for sending a PR! There's some good work here, but I have a few concerns.

  1. There are a number of changes that aren't strictly required to fix the issue. For example, just in the first commit there's a block of code that is refactored into its own method as well as changes to a JavaDoc comment. Other commits change the names of variables, finalize variables, change the format of boolean expressions, etc. These unnecessary changes make it difficult to see where the actual semantic changes are.
  2. At the very least, the first commit doesn't build properly because it contains a Checkstyle problem.
  3. Code is added in one commit and then removed in another commit. For example,
  4. Conceptually speaking this change violates the auth caching model used by the broker. Instead of having all the Subject values centralized in a single place (e.g. the cache) they also exist in the MQTT session state object. This increases the size of the heap makes the code more complex. It also subverts the expectation that auth entries will expire and be renewed.
  5. Typically a PR is single commit which includes all necessary code changes, tests, and doc updates. This simplifies the process of tracking changes for a specific bug or feature. You have 11 commits here which are all very directly related and could probably be squashed down to just 1. If the commits contains multiple notable changes then simply enumerate them in the commit message (e.g. like this). Once you remove all the unnecessary changes that will simplify the commit substantially.

@jbertram
Copy link
Contributor

jbertram commented Jan 21, 2025 •
edited
Loading

I think a really simple solution here would be to cache the X509Certificate[] on the RemotingConnection when o.a.a.a.c.r.CertificateUtil#getCertsFromConnection is called so that when the underlying Netty channel is unavailable (e.g. closed) auth will still work.

@jbertram
Copy link
Contributor

@JeanLucGraphalo, do you plan on addressing my concerns or should I just close this?

@JeanLucGraphalo
Copy link
Author

@jbertram, your comment has been added to the jira. I close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JeanLucGraphalo @jbertram