You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/02/05 06:13:59 UTC
[GitHub] [activemq-artemis] laeubi opened a new pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
laeubi opened a new pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432
This adds the opportunity to register new SASL schemes via the default
java service-loader mechanism.
Implementors have to provide an implementation of the ServerSASLFactory
that is responsible for providing instances of the actual scheme.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774001534
@gtully what do you think about transforming the both defaults (PLAIN, ANONYMOUS) to the new way of providing a SASL-Mechanism? Would this suffice?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774702476
Thanks for all the reviews and of course the merge :+1:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r570989952
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,31 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.stream.Stream;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
+
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.put(factory.getMechanism(), factory);
+ }
+ }
public static String[] getKnownMechanisms() {
- return KNOWN_MECHANISMS;
+ return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
Review comment:
With the precedence feature, having the existing mechanisms use this loader makes sense. The actual published mechanisms on the amqp acceptor can be explicitly configured on amqp protocol manager via the saslMechanisms property. That separates the loading from the use, but the defaults should have a sensible order as @gemmellr points out. I think there are existing tests that verify that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r570995173
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,31 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.stream.Stream;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
+
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.put(factory.getMechanism(), factory);
+ }
+ }
public static String[] getKnownMechanisms() {
- return KNOWN_MECHANISMS;
+ return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
Review comment:
maybe use "position", follow java.security.Security#insertProviderAt - a provider at an existing position cannot be overridden. In this way it can be clear what the default order is. duplicates need to figure out where they fit in the ordered list to be effective.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-773988304
It can be any dummy SASL thing, just that it completes the round trip from being requested by a client to being found and loaded by the server.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r570898820
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,31 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.stream.Stream;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
+
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.put(factory.getMechanism(), factory);
+ }
+ }
public static String[] getKnownMechanisms() {
- return KNOWN_MECHANISMS;
+ return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
Review comment:
The server is meant to announce them in descending order of preference, and little should ever come after ANONYMOUS, so this seems questionable. So would just dumping them at the front, though possibly less so.
It should perhaps also be guarded that its not a duplicate, avoiding dup mech announcements and perhaps also alert that a loaded mechanism wont be used should it already be known/handled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774085809
I have updated the PR but can't see why the build fails any idea?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774094039
>
>
> I have updated the PR but can't see why the build fails any idea?
It seems to be unrelated, I see similar sporadically failures in a lot in other recent prior builds. Perhaps the test failure to look at before release @clebertsuconic (perhaps its even the thing you already noted looking at).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-773956194
I have prepared [an example ](https://github.com/laeubi/scram-sasl/commit/973a0a2b1c495a30bc4d6720ae9b735f5467525f)that builds up this new feature to support SASL-SCRAM.
Would be really helpful if this could go into the release next week.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r571110875
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,41 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> Integer.compare(f1.getPrecedence(), f2.getPrecedence());
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.merge(factory.getMechanism(), factory, (f1, f2) -> {
+ if (f2.getPrecedence() > f1.getPrecedence()) {
+ return f2;
+ } else {
+ return f1;
+ }
+ });
+ }
+ }
+
+ public static String[] getDefaultMechanisms() {
+ return FACTORY_MAP.values()
+ .stream()
+ .filter(ServerSASLFactory::isDefaultPermitted)
+ .sorted(PRECEDENCE_COMPARATOR.reversed())
+ .map(ServerSASLFactory::getMechanism)
+ .toArray(String[]::new);
Review comment:
The value is just read once on connector creation if I see right, nerveless I'll extract this so it is only computed once.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r571025540
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,31 +17,48 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
-import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.stream.Stream;
public class MechanismFinder {
- private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
-
- private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Map<String, List<ServerSASLFactory>> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> f1.getPrecedence() - f2.getPrecedence();
static {
ServiceLoader<ServerSASLFactory> serviceLoader =
ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
for (ServerSASLFactory factory : serviceLoader) {
- FACTORY_MAP.put(factory.getMechanism(), factory);
+ List<ServerSASLFactory> list = FACTORY_MAP.computeIfAbsent(factory.getMechanism(), k -> new ArrayList<>());
+ list.add(factory);
+ }
+ for (List<ServerSASLFactory> factories : FACTORY_MAP.values()) {
+ Collections.sort(factories, PRECEDENCE_COMPARATOR);
Review comment:
Maintaining a map of lists seems unecessarily complicated. Cant you jsut merge the seen factory objects and keep the one with the higher precedence? This would simplify everything done afterwards too.
Note I am not seeing use of the 'isDefaultPerimitted' bits from the factory, which presumably means this might currently be advertising GSSAPI and EXTERNAL by default when it definitely shouldnt.
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,31 +17,48 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
-import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.stream.Stream;
public class MechanismFinder {
- private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
-
- private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Map<String, List<ServerSASLFactory>> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> f1.getPrecedence() - f2.getPrecedence();
static {
ServiceLoader<ServerSASLFactory> serviceLoader =
ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
for (ServerSASLFactory factory : serviceLoader) {
- FACTORY_MAP.put(factory.getMechanism(), factory);
+ List<ServerSASLFactory> list = FACTORY_MAP.computeIfAbsent(factory.getMechanism(), k -> new ArrayList<>());
+ list.add(factory);
+ }
+ for (List<ServerSASLFactory> factories : FACTORY_MAP.values()) {
+ Collections.sort(factories, PRECEDENCE_COMPARATOR);
}
}
- public static String[] getKnownMechanisms() {
- return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
+ public static String[] getDefaultMechanisms() {
+ return FACTORY_MAP.values()
+ .stream()
+ .flatMap(List<ServerSASLFactory>::stream)
+ .sorted(PRECEDENCE_COMPARATOR)
+ .map(ServerSASLFactory::getMechanism)
+ .distinct()
+ .toArray(String[]::new);
}
public static ServerSASLFactory getFactory(String mechanism) {
- return FACTORY_MAP.get(mechanism);
+ List<ServerSASLFactory> list = FACTORY_MAP.get(mechanism);
+ if (list == null || list.isEmpty()) {
+ return null;
+ }
+ // return mechanism with highest precedence if multiple are registered
+ return list.get(0);
Review comment:
This could be reduced to a simple map lookup after prior comments.
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,31 +17,48 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
-import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.stream.Stream;
public class MechanismFinder {
- private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
-
- private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Map<String, List<ServerSASLFactory>> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> f1.getPrecedence() - f2.getPrecedence();
static {
ServiceLoader<ServerSASLFactory> serviceLoader =
ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
for (ServerSASLFactory factory : serviceLoader) {
- FACTORY_MAP.put(factory.getMechanism(), factory);
+ List<ServerSASLFactory> list = FACTORY_MAP.computeIfAbsent(factory.getMechanism(), k -> new ArrayList<>());
+ list.add(factory);
+ }
+ for (List<ServerSASLFactory> factories : FACTORY_MAP.values()) {
+ Collections.sort(factories, PRECEDENCE_COMPARATOR);
}
}
- public static String[] getKnownMechanisms() {
- return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
+ public static String[] getDefaultMechanisms() {
+ return FACTORY_MAP.values()
+ .stream()
+ .flatMap(List<ServerSASLFactory>::stream)
+ .sorted(PRECEDENCE_COMPARATOR)
+ .map(ServerSASLFactory::getMechanism)
+ .distinct()
+ .toArray(String[]::new);
Review comment:
Reworking the bit above and then computing the default mechanisms there at the start as loaded, rather than here, would simplify this and avoid repeating the same evaluation over and over for every connection created.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774701778
nice PR... ran the whole test suite with this commit pulled and it worked fine
I squashed all 4 commits into one before I merged 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774800720
yes.. thanks a lot @laeubi
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r570986144
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,31 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.stream.Stream;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
+
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.put(factory.getMechanism(), factory);
+ }
+ }
public static String[] getKnownMechanisms() {
- return KNOWN_MECHANISMS;
+ return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
Review comment:
Thanks for the advice, I have added now a "precedence" that is used to maintains it ordering. In case of duplicate mechanism registered the one with the highest precedence wins.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r571102054
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,11 +17,41 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.ServiceLoader;
+
public class MechanismFinder {
- public static String[] KNOWN_MECHANISMS = new String[]{PlainSASL.NAME, AnonymousServerSASL.NAME};
+ private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> Integer.compare(f1.getPrecedence(), f2.getPrecedence());
+
+ static {
+ ServiceLoader<ServerSASLFactory> serviceLoader =
+ ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
+ for (ServerSASLFactory factory : serviceLoader) {
+ FACTORY_MAP.merge(factory.getMechanism(), factory, (f1, f2) -> {
+ if (f2.getPrecedence() > f1.getPrecedence()) {
+ return f2;
+ } else {
+ return f1;
+ }
+ });
+ }
+ }
+
+ public static String[] getDefaultMechanisms() {
+ return FACTORY_MAP.values()
+ .stream()
+ .filter(ServerSASLFactory::isDefaultPermitted)
+ .sorted(PRECEDENCE_COMPARATOR.reversed())
+ .map(ServerSASLFactory::getMechanism)
+ .toArray(String[]::new);
Review comment:
This is still being evaluated on every get, computing the same result over and over for every connection. Determine the result while creating the map and this can then simply return 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on a change in pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on a change in pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#discussion_r571041960
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/MechanismFinder.java
##########
@@ -17,31 +17,48 @@
package org.apache.activemq.artemis.protocol.amqp.sasl;
-import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.stream.Stream;
public class MechanismFinder {
- private static final String[] DEFAULT_MECHANISMS = new String[] {PlainSASL.NAME, AnonymousServerSASL.NAME};
-
- private static final Map<String, ServerSASLFactory> FACTORY_MAP = new HashMap<>();
+ private static final Map<String, List<ServerSASLFactory>> FACTORY_MAP = new HashMap<>();
+ private static final Comparator<? super ServerSASLFactory> PRECEDENCE_COMPARATOR =
+ (f1, f2) -> f1.getPrecedence() - f2.getPrecedence();
static {
ServiceLoader<ServerSASLFactory> serviceLoader =
ServiceLoader.load(ServerSASLFactory.class, MechanismFinder.class.getClassLoader());
for (ServerSASLFactory factory : serviceLoader) {
- FACTORY_MAP.put(factory.getMechanism(), factory);
+ List<ServerSASLFactory> list = FACTORY_MAP.computeIfAbsent(factory.getMechanism(), k -> new ArrayList<>());
+ list.add(factory);
+ }
+ for (List<ServerSASLFactory> factories : FACTORY_MAP.values()) {
+ Collections.sort(factories, PRECEDENCE_COMPARATOR);
}
}
- public static String[] getKnownMechanisms() {
- return Stream.concat(Arrays.stream(DEFAULT_MECHANISMS), FACTORY_MAP.keySet().stream()).toArray(String[]::new);
+ public static String[] getDefaultMechanisms() {
+ return FACTORY_MAP.values()
+ .stream()
+ .flatMap(List<ServerSASLFactory>::stream)
+ .sorted(PRECEDENCE_COMPARATOR)
+ .map(ServerSASLFactory::getMechanism)
+ .distinct()
+ .toArray(String[]::new);
Review comment:
you are right, I'll adjust 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774702977
Thanks much for the contribution, @laeubi.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] laeubi commented on pull request #3432: ARTEMIS-33 Generic integration with SASL Frameworks
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #3432:
URL: https://github.com/apache/activemq-artemis/pull/3432#issuecomment-774408418
@gemmellr @gtully I think this can be used as a first step, just one thing I noticed (but that could be changed later also):
Even though I removed the storage of multiple providers for the same scheme it might still be valuable to restore this. I noticed that e.g. the java sasl API do it the way that they (in case of multiple ones) ask them in order and return the first one returning a non-null client/server object.
We could do a similar thing here, MechanismFinder#getFactory would then simply return a list of factories.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org