You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@artemis.apache.org by "GChuf (via GitHub)" <gi...@apache.org> on 2026/04/30 10:21:38 UTC
[PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
GChuf opened a new pull request, #6403:
URL: https://github.com/apache/artemis/pull/6403
(no comment)
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-4825410969
@jbertram I tried to clean things up a bit. You'll notice a couple of changes - I switched to caffeine to still be able to have an upper limit for caches, while keeping the performance of a concurrentHashMap compared to a synchronizedMap, and I've switched roles type from List to Set - which didn't seem to have a dramatic effect.
I've appended the results from the latest commit into the tables above. The performances are more-or-less the same as before the force push.
I've also changed the JMH test a bit as well - you should confirm if the scenarios look OK to you ... For the most part (most tests) i did not see a big difference (on this branch) between the "guards" (the default one and the "generated one" which contains the default + 1000 roles). Similarly, there were no huge differences when changing the number of objects/queues from 10 to 1000.
That said, since the test scenarios are a bit different ... some comparisons are not valid (particularly: some numbers from GC allocation changed a bit).
As a sanity check, I logged into the console with different users (using the [etc](https://github.com/alexander-milovidov/artemis-slow-console-reproducer/tree/main/etc_example) example provided here) and the results are reproducible. The jolokia calls are now consistently between **1.9s** and **2.2s** on my machine (compared to ~**60s** on main branch). Some of the remaining bottlenecks are now on jolokia side (maybe I can get another 5-10% there), but I don't think it gets _much_ better than this.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6072996246
The `"*"` bucket fallback fixes the "never matched" bug, but introduces a **priority-ordering regression**. The loop now always exhausts `bucketedMap.get(propKey)` before checking `bucketedMap.get("*")`, but on `main` all patterns for a domain are tried in one priority order (longest/most-specific first), regardless of which bucket they'd land in now. So a longer, more specific `"*"`-bucket pattern can lose to a shorter `propKey`-bucket pattern that should rank below it, e.g.:
```java
addToRoleAccess("org.myDomain", "type=*", "listSomething", "roleA");
addToRoleAccess("org.myDomain", "*=verylongvalue", "listSomething", "roleB");
```
For org.myDomain:type=verylongvalue:
- `main` → `roleB` (longer pattern wins)
- this branch → `roleA` (type bucket checked first)
Neither new test covers two competing patterns across buckets like this.
One potential solution would be to merge both buckets and rank by the same priority rule before matching, rather than always preferring the `propKey` bucket.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-4373249689
I had Claude create a JMH test for this and the results look promising, especially when `operationName` is `null`.
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.activemq.artemis.tests.performance.jmh;
import org.apache.activemq.artemis.core.server.management.ArtemisMBeanServerGuard;
import org.apache.activemq.artemis.core.server.management.JMXAccessControlList;
import org.apache.activemq.artemis.spi.core.security.jaas.RolePrincipal;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import javax.security.auth.Subject;
import java.security.PrivilegedAction;
import java.util.concurrent.ThreadLocalRandom;
@State(Scope.Benchmark)
@Fork(2)
@Warmup(iterations = 5, time = 1)
@Measurement(iterations = 8, time = 1)
@BenchmarkMode(Mode.Throughput)
public class MBeanServerGuardCanInvokePerfTest {
private ArtemisMBeanServerGuard guard;
private String[] objectNames;
private String[] operationNames;
private String[] operationNamesWithParams;
private String allowListedObjectName;
private Subject subjectWithViewRole;
private Subject subjectWithAmqRole;
private Subject subjectWithNoRole;
@Param({"10", "50", "100"})
int objectNameCount;
@Setup
public void init() {
guard = new ArtemisMBeanServerGuard();
JMXAccessControlList acl = JMXAccessControlList.createDefaultList();
guard.setJMXAccessControlList(acl);
// Create test object names from different domains
objectNames = new String[objectNameCount];
for (int i = 0; i < objectNameCount; i++) {
if (i % 3 == 0) {
objectNames[i] = "org.apache.activemq.artemis:broker=\"0.0.0.0\",component=addresses,address=\"address." + i + "\"";
} else if (i % 3 == 1) {
objectNames[i] = "org.apache.activemq.artemis:broker=\"0.0.0.0\",component=addresses,subcomponent=queues,routing-type=ANYCAST,name=\"queue-" + i + "\"";
} else {
objectNames[i] = "java.lang:type=Memory,name=HeapMemoryUsage-" + i;
}
}
// Allowlisted object name (hawtio domain is in the default allowlist)
allowListedObjectName = "hawtio:type=security,name=RBACRegistry";
// Common operation names
operationNames = new String[]{
"getMessageCount",
"listMessages",
"sendMessage",
"removeMessage",
"getConsumerCount",
"listConsumers",
"getAttribute",
"setAttribute",
"invoke"
};
// Operation names with parameter lists
operationNamesWithParams = new String[]{
"sendMessage(java.lang.String)",
"sendMessage(java.lang.String,java.lang.String)",
"removeMessage(long)",
"listMessages(java.lang.String)",
"setAttribute(java.lang.String,java.lang.Object)"
};
// Set up test subjects with different roles
subjectWithViewRole = new Subject();
subjectWithViewRole.getPrincipals().add(new RolePrincipal("view"));
subjectWithAmqRole = new Subject();
subjectWithAmqRole.getPrincipals().add(new RolePrincipal("amq"));
subjectWithNoRole = new Subject();
}
@Benchmark
public boolean testCanInvokeAllowListed() {
return guard.canInvoke(allowListedObjectName, "anyOperation");
}
@Benchmark
public boolean testCanInvokeWithViewRole() {
return Subject.doAs(subjectWithViewRole, (PrivilegedAction<Boolean>) () -> {
int idx = ThreadLocalRandom.current().nextInt(objectNames.length);
int opIdx = ThreadLocalRandom.current().nextInt(operationNames.length);
return guard.canInvoke(objectNames[idx], operationNames[opIdx]);
});
}
@Benchmark
public boolean testCanInvokeWithAmqRole() {
return Subject.doAs(subjectWithAmqRole, (PrivilegedAction<Boolean>) () -> {
int idx = ThreadLocalRandom.current().nextInt(objectNames.length);
int opIdx = ThreadLocalRandom.current().nextInt(operationNames.length);
return guard.canInvoke(objectNames[idx], operationNames[opIdx]);
});
}
@Benchmark
public boolean testCanInvokeWithNoRole() {
return Subject.doAs(subjectWithNoRole, (PrivilegedAction<Boolean>) () -> {
int idx = ThreadLocalRandom.current().nextInt(objectNames.length);
int opIdx = ThreadLocalRandom.current().nextInt(operationNames.length);
return guard.canInvoke(objectNames[idx], operationNames[opIdx]);
});
}
@Benchmark
public boolean testCanInvokeWithParameterStripping() {
return Subject.doAs(subjectWithViewRole, (PrivilegedAction<Boolean>) () -> {
int idx = ThreadLocalRandom.current().nextInt(objectNames.length);
int opIdx = ThreadLocalRandom.current().nextInt(operationNamesWithParams.length);
return guard.canInvoke(objectNames[idx], operationNamesWithParams[opIdx]);
});
}
@Benchmark
public boolean testCanInvokeNullOperation() {
return guard.canInvoke(objectNames[0], null);
}
@Benchmark
public boolean testCanInvokeInvalidObjectName() {
return guard.canInvoke("invalid:object:name:with:too:many:colons", "operation");
}
@Benchmark
public boolean testCanInvokeMixedScenarios() {
return Subject.doAs(subjectWithViewRole, (PrivilegedAction<Boolean>) () -> {
int scenario = ThreadLocalRandom.current().nextInt(4);
switch (scenario) {
case 0:
return guard.canInvoke(allowListedObjectName, "operation");
case 1:
int idx = ThreadLocalRandom.current().nextInt(objectNames.length);
int opIdx = ThreadLocalRandom.current().nextInt(operationNames.length);
return guard.canInvoke(objectNames[idx], operationNames[opIdx]);
case 2:
idx = ThreadLocalRandom.current().nextInt(objectNames.length);
opIdx = ThreadLocalRandom.current().nextInt(operationNamesWithParams.length);
return guard.canInvoke(objectNames[idx], operationNamesWithParams[opIdx]);
default:
return guard.canInvoke(objectNames[0], null);
}
});
}
}
```
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-4678860526
@jbertram thanks for the test you provided - I finally ran it yesterday (i didn't realise more than a month has passed) and the results seemed a bit strange to me - I expected to see bigger improvements. So I explained a couple of things to claude and after a couple of tries I got another [test](https://github.com/GChuf/activemq-artemis/blob/c685bce6d777b2bc8c4419a75b7c3d8eb23b6953/tests/performance-jmh/src/main/java/org/apache/activemq/artemis/tests/performance/jmh/MBeanServerGuardCanInvokePerfTest2.java) out of it, which should be closer to the "console scenario". In my case, though, some improvements can be seen in the tests you provided as well.
Results:
JBertram's tests
```
main
Benchmark (objectNameCount) Mode Cnt Score Error Units
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 10 thrpt 16 1452471.937 ± 22805.579 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 50 thrpt 16 1472493.395 ± 28510.019 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 100 thrpt 16 1447981.249 ± 34360.371 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 10 thrpt 16 604590.702 ± 16191.796 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 50 thrpt 16 610694.673 ± 7113.871 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 100 thrpt 16 628876.398 ± 8770.463 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 10 thrpt 16 1914158.654 ± 15324.318 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 50 thrpt 16 1900909.821 ± 26807.880 ops/s
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 100 thrpt 16 1902533.223 ± 48632.228 ops/s
ARTEMIS-6042 (except the last commit)
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 10 thrpt 16 2750674.221 ± 37961.436 ops/s (+89%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 50 thrpt 16 2734767.447 ± 26081.250 ops/s (+85%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeAllowListed 100 thrpt 16 2690331.313 ± 169788.706 ops/s (+85%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 10 thrpt 16 711172.067 ± 12913.097 ops/s (+17%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 50 thrpt 16 733427.656 ± 14333.428 ops/s (+20%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeInvalidObjectName 100 thrpt 16 738054.220 ± 7062.869 ops/s (+17%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 10 thrpt 16 889325992.447 ± 13148015.740 ops/s (+46360%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 50 thrpt 16 895038928.613 ± 9677822.348 ops/s (+47084%)
MBeanServerGuardCanInvokePerfTest.testCanInvokeNullOperation 100 thrpt 16 891405790.721 ± 30036267.452 ops/s (+46853%)
```
"Console scenario" tests:
```
main:
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadAmqRole thrpt 16 40.597 ± 0.610 ops/s
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadNoRole thrpt 16 0.094 ± 0.001 ops/s
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadViewRole thrpt 16 0.097 ± 0.002 ops/s
ARTEMIS-6042
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadAmqRole thrpt 16 50.465 ± 0.695 ops/s (+24%)
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadNoRole thrpt 16 14.338 ± 0.309 ops/s (+15253%)
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadViewRole thrpt 16 13.834 ± 0.212 ops/s (+14261%)
```
I also tested gc with `java -jar target/benchmark.jar MBeanServerGuardCanInvokePerfTest -prof gc`, since claude thought it can decrease memory pressure (take a look at the last commit)
Putting just the gc.alloc.rate here:
```
main:
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadAmqRole:gc.alloc.rate thrpt 16 979.006 ± 10.923 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadNoRole:gc.alloc.rate thrpt 16 204.844 ± 2.719 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadViewRole:gc.alloc.rate thrpt 16 211.390 ± 1.135 MB/sec
ARTEMIS-6042 (except the last commit)
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadAmqRole:gc.alloc.rate thrpt 16 678.382 ± 13.282 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadNoRole:gc.alloc.rate thrpt 16 183.129 ± 4.022 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadViewRole:gc.alloc.rate thrpt 16 176.503 ± 12.003 MB/sec
ARTEMIS-6042
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadAmqRole:gc.alloc.rate thrpt 16 591.859 ± 25.651 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadNoRole:gc.alloc.rate thrpt 16 183.471 ± 2.184 MB/sec
MBeanServerGuardCanInvokePerfTest.testConsolePageLoadViewRole:gc.alloc.rate thrpt 16 185.519 ± 2.447 MB/sec
```
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6189813018
The logic for adding `role` as well as `fqcn:role` to cache was added to `ArtemisMBeanServerGuard#getCurrentUserRoles`. 2 additional tests were added as well.
For audit logging - I just duplicated the code ...
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #6403:
URL: https://github.com/apache/artemis/pull/6403#discussion_r3167808823
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/management/ArtemisMBeanServerGuard.java:
##########
@@ -162,6 +166,7 @@ public boolean canInvoke(String object, String operationName) {
}
Set<String> currentUserRoles = getCurrentUserRoles();
+
Review Comment:
This extra line can be removed.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5522922216
The current tests passed because the existing tests only test the `getRolesForObject` method directly.
I duplicated the tests, so that the `authorizeUserForMethod` is now tested as well.
I also added 2 tests to test wildcards in keys.
The tests can be deduplicated after changing the `handleInvoke` method to use authorizeUserForMethod as well.
Waiting on your input here.
The scenarios you mentioned are now solved by setting a prefix to `*` and looping through `bucketedMap.get("*")`'s regexp patterns if no exact match or regexp match was found earlier.
I'm not sure if the logic for setting the prefix to * is the best way to handle it, but it should cover all the scenarios with wildcards.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6132337717
The removed `currentUserHasRole` method matched on both the principal _class_ and the _name_ while the new `getCurrentUserRoles()` just collects the _name_ of every principal regardless of type. Every login module puts a `UserPrincipal` carrying the authenticated username into the same `Subject` so the username is now usable as a JMX role. This also broke the roles="fqcn:role" config form, which is how Karaf/OSGi deployments grant JMX access.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6362602254
Cleaned everything up and squashed some commits.
Since a lot has changed both on main and on this branch, i re-run the tests. The tests mentioned above use `guard`, not `defaultGuard`.
```
# JMH version: 1.37
# VM version: JDK 21.0.2, OpenJDK 64-Bit Server VM, 21.0.2+13-58
# VM invoker: /root/.sdkman/candidates/java/21.0.2-open/bin/java
```
### Throughput
| Benchmark | main (598c76a356a30710cb7ccb7000091a86f0511032) | ARTEMIS-6042 (badbeacce81a999183b396ad5d6c6c2178d83b6d) | % diff |
|---|---:|---:|---:|
| testCanInvokeAllowListed (defaultGuard) | 1,027,473 | 59,329,902 | +5,674% |
| testCanInvokeAllowListed (guard) | 1,035,778 | 59,853,041 | +5,679% |
| testCanInvokeInvalidObjectName (defaultGuard) | 590,403 | 491,373 | -16.8% |
| testCanInvokeInvalidObjectName (guard) | 585,796 | 482,533 | -17.6% |
| testCanInvokeMixedScenarios (defaultGuard) | 197,558 | 309,019 | +56.4% |
| testCanInvokeMixedScenarios (guard) | 3,162 | 310,279 | +9,713% |
| testCanInvokeNullOperation (defaultGuard) | 1,343,985 | 1,082,206,367 | +80,422% |
| testCanInvokeNullOperation (guard) | 1,327,302 | 1,085,907,832 | +81,713% |
| testCanInvokeWithAmqRole (defaultGuard) | 129,600 | 211,297 | +63.0% |
| testCanInvokeWithAmqRole (guard) | 121,778 | 215,503 | +77.0% |
| testCanInvokeWithNoRole (defaultGuard) | 128,423 | 207,471 | +61.6% |
| testCanInvokeWithNoRole (guard) | 2,131 | 208,973 | +9,606% |
| testCanInvokeWithParameterStripping (defaultGuard) | 152,570 | 202,157 | +32.5% |
| testCanInvokeWithParameterStripping (guard) | 1,256 | 213,725 | +16,916% |
| testCanInvokeWithViewRole (defaultGuard) | 130,501 | 210,049 | +61.0% |
| testCanInvokeWithViewRole (guard) | 2,120 | 209,903 | +9,801% |
| testConsolePageLoadAmqRole (defaultGuard) | 34.6 | 57.6 | +66.4% |
| testConsolePageLoadAmqRole (guard) | 28.4 | 59.9 | +110.7% |
| testConsolePageLoadNoRole (defaultGuard) | 33.5 | 58.0 | +73.4% |
| testConsolePageLoadNoRole (guard) | 0.166 | 57.2 | +34,330% |
| testConsolePageLoadViewRole (defaultGuard) | 34.1 | 59.4 | +74.1% |
| testConsolePageLoadViewRole (guard) | 0.167 | 58.7 | +35,064% |
### GC allocation per op (B/op)
| Benchmark | main (598c76a356a30710cb7ccb7000091a86f0511032) | ARTEMIS-6042 (badbeacce81a999183b396ad5d6c6c2178d83b6d) | % diff |
|---|---:|---:|---:|
| testCanInvokeAllowListed (defaultGuard) | 2,032 | ≈0 | -100% |
| testCanInvokeAllowListed (guard) | 2,032 | ≈0 | -100% |
| testCanInvokeInvalidObjectName (defaultGuard) | 1,200 | 1,328 | +10.7% |
| testCanInvokeInvalidObjectName (guard) | 1,200 | 1,328 | +10.7% |
| testCanInvokeMixedScenarios (defaultGuard) | 3,412 | 1,164 | -65.9% |
| testCanInvokeMixedScenarios (guard) | 72,184 | 1,162 | -98.4% |
| testCanInvokeNullOperation (defaultGuard) | 1,184 | ≈0 | -100% |
| testCanInvokeNullOperation (guard) | 1,184 | ≈0 | -100% |
| testCanInvokeWithAmqRole (defaultGuard) | 4,822 | 1,910 | -60.4% |
| testCanInvokeWithAmqRole (guard) | 4,922 | 1,892 | -61.6% |
| testCanInvokeWithNoRole (defaultGuard) | 4,823 | 1,913 | -60.3% |
| testCanInvokeWithNoRole (guard) | 106,057 | 1,910 | -98.2% |
| testCanInvokeWithParameterStripping (defaultGuard) | 4,625 | 1,926 | -58.4% |
| testCanInvokeWithParameterStripping (guard) | 191,100 | 1,948 | -99.0% |
| testCanInvokeWithViewRole (defaultGuard) | 4,821 | 1,914 | -60.3% |
| testCanInvokeWithViewRole (guard) | 107,158 | 1,895 | -98.2% |
| testConsolePageLoadAmqRole (defaultGuard) | 26,655,925 | 6,895,379 | -74.1% |
| testConsolePageLoadAmqRole (guard) | 28,127,953 | 6,799,348 | -75.8% |
| testConsolePageLoadNoRole (defaultGuard) | 26,655,934 | 6,711,369 | -74.8% |
| testConsolePageLoadNoRole (guard) | 1,418,919,144 | 6,711,453 | -99.5% |
| testConsolePageLoadViewRole (defaultGuard) | 26,655,930 | 6,711,372 | -74.8% |
| testConsolePageLoadViewRole (guard) | 1,418,919,163 | 6,689,200 | -99.5% |
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5483562727
I'm aware about the `getRolesForObject` - I've noticed it (again) today and made a separate branch where `handleInvoke` uses `authorizeUserForMethod `as well.
I'd like to remove `getRolesForObject` and `jmxAccessControlList.getRolesForObject` completely. I still have to rewrite the tests that use `getRolesForObject` though. I thought all of this should go under a separate issue/PR, what do you think?
I'll take a look at the bucket implementation issue as soon as I can, hopefully tomorrow.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6291595669
Reverted the accidental .project file changes.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-4716341456
This is still a WIP, but I've put all tests into 1 file and added cache for object names in f0afa2c9, and then changed from synchronized map to concurrent hash map in 3ceb8d1e.
All of the tests/checks are green.
@jbertram can you guys check and confirm if this is something you'd be willing to review and merge eventually?
---
Results on my machine running java 21.0.2:
### Throughput
| Benchmark | main | f0afa2c9 | 3ceb8d1e |
|---|---:|---:|---:|
| testCanInvokeAllowListed | 1,761,465 | 550,356 | 44,208,762 |
| testCanInvokeInvalidObjectName | 544,846 | 493,172 | 498,332 |
| testCanInvokeMixedScenarios | 1,404 | 132,199 | 329,699 |
| testCanInvokeNullOperation | 1,237,538 | 1,045,438,183 | 1,048,246,229 |
| testCanInvokeWithAmqRole | 205,195 | 235,535 | 234,300 |
| testCanInvokeWithNoRole | 696 | 95,535 | 238,326 |
| testCanInvokeWithParameterStripping | 667 | 88,486 | 240,743 |
| testCanInvokeWithViewRole | 706 | 84,518 | 238,184 |
| testConsolePageLoadAmqRole | 39 | 71 | 64 |
| testConsolePageLoadNoRole | 0.098 | 14 | 63 |
| testConsolePageLoadViewRole | 0.090 | 14 | 64 |
---
### GC allocation per op
| Benchmark | main | f0afa2c9 | 3ceb8d1e |
|---|---:|---:|---:|
| testCanInvokeAllowListed | 920 | 80 | 16 |
| testCanInvokeInvalidObjectName | 1,200 | 1,200 | 1,232 |
| testCanInvokeMixedScenarios | 155,908 | 1,452 | 1,111 |
| testCanInvokeNullOperation | 1,184 | ≈0 | ≈0 |
| testCanInvokeWithAmqRole | 3,953 | 1,797 | 1,800 |
| testCanInvokeWithNoRole | 313,304 | 1,797 | 1,800 |
| testCanInvokeWithParameterStripping | 312,481 | 1,865 | 1,871 |
| testCanInvokeWithViewRole | 312,268 | 1,797 | 1,800 |
| testConsolePageLoadAmqRole | 25,599,870 | 4,961,027 | 6,720,688 |
| testConsolePageLoadNoRole | 2,343,208,040 | 5,761,230 | 6,640,705 |
| testConsolePageLoadViewRole | 2,343,208,001 | 5,761,243 | 6,640,704 |
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5481099582
I noticed something else with the bucket implementation.
The existing matcher treats a rule's key as an opaque regex matched against the whole `key=value` string. However, the bucketed rewrite assumes every rule's key has the shape `<literalPropertyName>=<valuePattern>`. It splits the pattern on the first `=`, takes the part before it as a literal bucket name, and then only ever evaluates a rule if that literal equals one of the `ObjectName`'s property keys.
For a normal rule like `type=Queue` or `type=*` this is fine. Here the prefix is "type", and the object's type property finds the bucket. The model breaks for any rule whose prefix isn't a literal property name, e.g.:
- Bare wildcard key (e.g., `key="*"`)
- Wildcard in the key name (e.g., `key="*=foo"`)
Normal `key="type=Queue"` configs are unaffected. That's why the existing tests pass.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5480559221
The "Pass currentUserRoles to jmxAccessControlList" commit creates 2 paths for authorization:
1. `canInvoke` now uses the new bucketed `authorizeUserForMethod`
2. the actual enforcement path (`handleInvoke`, reached by `getAttribute`/`setAttribute`/`invoke`) still uses the old `getRolesForObject`
Therefore, the advisory check and the real security gate run different code that can produce different answers, and the precedence logic is now duplicated.
Even assuming the two paths are functionally equivalent today, there's still a real danger that they will diverge over time.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6517392901
Done, all tests are green now.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5413479071
There's some confusion between several of these commits.
There's one commit which says, "Add bucketedmap cache." However, the `bucketedDomainCache` variable is actually added in the previous commit (which says, "Add domain cache"), and that commit actually adds 3 caches - `keyPropertyCache`, `domainCache`, and the aforementioned `bucketedDomainCache`. Then later there's another commit which changes those caches to use Caffeine. I would rather have each line modified once in the PR. If it's too much work to separate the individual cache updates then just do it all in one commit.
That said, you replaced the authoritative ACL config maps with eviction caches, i.e.:
```
private final Cache<String, TreeMap<String, Access>> domainAccess = Caffeine.newBuilder().maximumSize(32).build();
private final Cache<String, TreeMap<String, Access>> allowList = Caffeine.newBuilder().maximumSize(32).build();
```
`domainAccess` & `allowList` hold the real authorization rules (populated by `addToRoleAccess` & `addToAllowList` respectively), and there is no way to recover an evicted entry. Storing authoritative config in a size-bounded, approximately-evicting cache means rules can silently disappear, and lookups fall through to defaultAccess. In practice Artemis uses essentially one JMX domain, so `maximumSize(32)` isn't a problem today, but it's the wrong data structure for authoritative state.
Furthermore, you call `domainMap.get("")` with no null check which relies on `bucketedDomainCache` and `domainAccess` (two separate size-32 caches) staying consistent. If `domainAccess` evicts an entry
that `bucketedDomainCache` still holds, this throws an NPE.
In my opinion both of these should remain as `ConcurrentHashMap`.
Lastly, the "Add cached roles" commit mutates the `Subject` which I'm not crazy about. A cleaner implementation would be to follow the same pattern as `objectNameCache` and `bypassRBACCache`. That said, I'm not sure there's a clear benefit to caching here.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5890183105
I went ahead and added 3 commits.
The "old" unused methods and the old tests are now removed.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6513257686
You need to rebase again to pick up some fixes for test failures on `main`.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6465307610
Rebased - please double check.
I also cleaned the history a bit more (adding/deleting tests in JMXAccessControlListTest).
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-4734227459
@GChuf I definitely think this is something we'd merge assuming all the commits were referencing a valid Jira.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-5475416340
@jbertram thank you for taking the time and for the explanations!
I've cleaned the commits up properly now - let me know if I missed something.
For the `domainAccess` & `allowList` - I reverted them to `ConcurrentHashMap`.
For the cached roles: I used the same pattern as with `objectNameCache` and `bypassRBACCache` and performed one more test.
There's no obvious speedups, but the mem allocation per operation (for testConsolePage JMH tests) drops from ~6MB to ~3.3MB, so I decided to keep it - unless you want to drop it.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6073090645
BTW, `org.apache.activemq.artemis.core.server.management.ArtemisMBeanServerGuard#currentUserHasRole` is also dead code now so it should be removed as well.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "GChuf (via GitHub)" <gi...@apache.org>.
GChuf commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6077609083
All done - buckets were merged like you suggested, a test was added and the dead code was removed.
There was 1 more thing that claude noticed when it helped me - so I've added cache invalidation with a corresponding test as well.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6042 Improve ArtemisMBeanServerGuard.canInvoke implementation [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6403:
URL: https://github.com/apache/artemis/pull/6403#issuecomment-6306643506
The history would be much cleaner if instead of having separate commits to re-add code that was removed the original commits were modified so they didn't actually remove the code in question.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org