[
https://issues.apache.org/jira/browse/ARTEMIS-5163?focusedWorklogId=953464&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-953464
]
ASF GitHub Bot logged work on ARTEMIS-5163:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 21/Jan/25 22:22
Start Date: 21/Jan/25 22:22
Worklog Time Spent: 10m
Work Description: jbertram commented on PR #5460:
URL:
https://github.com/apache/activemq-artemis/pull/5460#issuecomment-2605860614
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](https://github.com/apache/activemq-artemis/commit/10adca5479c7d0aa518c0ffce44008e95a268287)).
Once you remove all the unnecessary changes that will simplify the commit
substantially.
Issue Time Tracking
-------------------
Worklog Id: (was: 953464)
Remaining Estimate: 0h
Time Spent: 10m
> Artemis fails to send mqtt will message using mutual TLS
> --------------------------------------------------------
>
> Key: ARTEMIS-5163
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5163
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Components: MQTT
> Affects Versions: 2.31.2, 2.33.0, 2.38.0, 2.39.0
> Reporter: Olaf Gustav
> Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> As discussed in the user mailing list, the MQTT broker fails to sent the
> provided will message when using mutual TLS.
> +set-up for testing:+
> * ActiveMQ Artemis 2.33 as MQTT broker
> * Artemis runs on jdk-21
> * clients are authenticated using mutual TLS
> * certificate DN is used to map to a user and eventually to the configured
> roles
> +issue:+
> During testing we discovered, that the provided will message is not sent as
> expected. We got the following error messages:
> {code:none}
> WARN [org.apache.activemq.artemis.core.server] AMQ222216: Security problem
> while authenticating: AMQ229031: Unable to validate user from /
> 127.0.0.1:51770. Username: null; SSL certificate subject DN: unavailable
> ERROR [org.apache.activemq.artemis.core.protocol.mqtt] AMQ834007:
> Authorization failure sending will message: AMQ229031: Unable to validate
> user from / 127.0.0.1:51770. Username: null; SSL certificate subject DN:
> unavailable
> {code}
> I did some research in the code base. The class
> *org.apache.activemq.artemis.core.remoting.CertificateUtil* retrieves the
> certificate subject DN based on the actual client certificate provided by an
> existing connection. When trying to send a mqtt will message, there is no
> connection to the client anymore. Consequently, the broker fails to get the
> DN. Since the subject DN serves as the key in the authentication cache
> ({*}org.apache.activemq.artemis.core.security.impl. SecurityStoreImpl{*}),
> the will message fails to be checked against access permissions.
> As a workaround, I used the RemotingConnection.clientID as authentication
> cache key instead of the DN. That works as long as the parameter
> *security-invalidation-interval* is properly defined, that means
> {{{}security-invalidation-interval >> sessionExpiryInterval{}}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
