You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "jbonofre (via GitHub)" <gi...@apache.org> on 2023/06/21 09:17:12 UTC
[GitHub] [activemq] jbonofre opened a new pull request, #1035: [AMQ-9244] Add JWT authentication plugin
jbonofre opened a new pull request, #1035:
URL: https://github.com/apache/activemq/pull/1035
(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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #1035:
URL: https://github.com/apache/activemq/pull/1035#issuecomment-1602614929
@jbonofre, understood. That wasn't clear from [your comment](https://issues.apache.org/jira/browse/AMQ-9244?focusedCommentId=17723067&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17723067) on the Jira where you said,
> It doesn't break anything, just add a new plugin, so I don't see problem to merge.
Thanks for the clarification.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237577663
##########
activemq-broker/src/main/java/org/apache/activemq/security/jwt/JwtAuthenticationPlugin.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * 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.security.jwt;
+
+import com.nimbusds.jose.JWSAlgorithm;
+import org.apache.activemq.broker.Broker;
+import org.apache.activemq.broker.BrokerPlugin;
+
+/**
+ * A simple JWT plugin giving ActiveMQ the ability to support JWT tokens to authenticate and authorize users
+ *
+ * @org.apache.xbean.XBean element="jwtAuthenticationPlugin"
+ * description="Provides a JWT authentication plugin"
+ *
+ *
+ * This plugin is rather simple and is only meant to be used as a first step. In real world applications, we would need
+ * to being able to specify different key format (RSA, DSA, EC, etc), the issuer, the claim to use for groups and maybe
+ * some mapping functionalities.
+ * The header name is also hard coded for simplicity.
+ */
+public class JwtAuthenticationPlugin implements BrokerPlugin {
Review Comment:
Perhaps I'm wrong, but this doesn't appear usable as is. How can users configure these values? Is the idea here that they will need implement and deploy their own version of this class in order to configure auth as needed? If so, are there plans to make this configurable in the future?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1035:
URL: https://github.com/apache/activemq/pull/1035#issuecomment-1603401207
I went ahead and converted this to a draft PR. Generally speaking we should be using draft PRs if something is not ready to merge. This way it makes it clear it's still a work in progress. Marking things like this draft PRs in the future should hopefully help with the confusion about the state.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237582673
##########
activemq-broker/src/main/java/org/apache/activemq/security/jwt/TokenUtils.java:
##########
@@ -0,0 +1,157 @@
+/**
+ * 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.security.jwt;
+
+import com.nimbusds.jose.JOSEObjectType;
+import com.nimbusds.jose.JWSHeader;
+import com.nimbusds.jose.crypto.RSASSASigner;
+import com.nimbusds.jose.shaded.json.JSONObject;
+import com.nimbusds.jwt.JWTClaimsSet;
+import com.nimbusds.jwt.SignedJWT;
+
+import java.io.InputStream;
+import java.security.KeyFactory;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.Base64;
+import java.util.List;
+
+/**
+ * Utilities for generating a JWT for testing
+ */
+public class TokenUtils {
Review Comment:
Nothing from this class is used anywhere. The JavaDoc says it's used for testing, but there are no tests. This class seems unnecessary at this point.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237586536
##########
activemq-broker/src/main/java/org/apache/activemq/security/jwt/JwtAuthenticationBroker.java:
##########
@@ -0,0 +1,179 @@
+/**
+ * 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.security.jwt;
+
+import org.apache.activemq.broker.Broker;
+import org.apache.activemq.broker.ConnectionContext;
+import org.apache.activemq.command.ConnectionInfo;
+import org.apache.activemq.jaas.GroupPrincipal;
+import org.apache.activemq.security.AbstractAuthenticationBroker;
+import org.apache.activemq.security.SecurityContext;
+import org.jose4j.jwa.AlgorithmConstraints;
+import org.jose4j.jws.AlgorithmIdentifiers;
+import org.jose4j.jwt.JwtClaims;
+import org.jose4j.jwt.MalformedClaimException;
+import org.jose4j.jwt.consumer.InvalidJwtException;
+import org.jose4j.jwt.consumer.JwtConsumer;
+import org.jose4j.jwt.consumer.JwtConsumerBuilder;
+import org.jose4j.jwt.consumer.JwtContext;
+
+import java.security.Key;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.Principal;
+import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.X509EncodedKeySpec;
+import java.util.Base64;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Handle authentication an user based on a JWT token
+ */
+public class JwtAuthenticationBroker extends AbstractAuthenticationBroker {
+
+ private final String jwtIssuer;
+ private final Claims jwtGroupsClaim;
+ private final String jwtValidatingPublicKey;
+
+ public JwtAuthenticationBroker(
+ final Broker next,
+ final String jwtIssuer,
+ final Claims jwtGroupsClaim,
+ final String jwtValidatingPublicKey) {
+ super(next);
+ this.jwtIssuer = jwtIssuer;
+ this.jwtGroupsClaim = jwtGroupsClaim;
+ this.jwtValidatingPublicKey = jwtValidatingPublicKey;
+ }
+
+ @Override
+ public void addConnection(final ConnectionContext context, final ConnectionInfo info) throws Exception {
+ SecurityContext securityContext = context.getSecurityContext();
+ if (securityContext == null) {
+ securityContext = authenticate(info.getUserName(), info.getPassword(), null);
+ context.setSecurityContext(securityContext);
+ securityContexts.add(securityContext);
+ }
+
+ try {
+ super.addConnection(context, info);
+ } catch (Exception e) {
+ securityContexts.remove(securityContext);
+ context.setSecurityContext(null);
+ throw e;
+ }
+ }
+
+ @Override
+ public SecurityContext authenticate(final String username, final String password, final X509Certificate[] certificates) throws SecurityException {
+ SecurityContext securityContext = null;
+ if (!username.isEmpty()) {
+
+ // parse the JWT token and check signature, validity, nbf
+ try {
+ final JwtConsumerBuilder builder = new JwtConsumerBuilder()
+ .setRelaxVerificationKeyValidation()
+ .setRequireSubject()
+ .setSkipDefaultAudienceValidation()
+ .setRequireExpirationTime()
+ .setExpectedIssuer(jwtIssuer)
+ .setAllowedClockSkewInSeconds(5)
+ .setVerificationKey(parsePCKS8(jwtValidatingPublicKey))
+ .setJwsAlgorithmConstraints(
+ new AlgorithmConstraints(AlgorithmConstraints.ConstraintType.WHITELIST,
+ AlgorithmIdentifiers.RSA_USING_SHA256,
+ AlgorithmIdentifiers.RSA_USING_SHA384,
+ AlgorithmIdentifiers.RSA_USING_SHA512)
+ );
+
+ final JwtConsumer jwtConsumer = builder.build();
+ final JwtContext jwtContext = jwtConsumer.process(username);
Review Comment:
On [the Jira](https://issues.apache.org/jira/browse/AMQ-9244?focusedCommentId=17723012&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17723012) it was indicated that the _password_ field should contain the token. If `username` is used instead will this fit the reported use-case?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237566439
##########
activemq-broker/pom.xml:
##########
@@ -66,6 +66,18 @@
<artifactId>activemq-jaas</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>com.nimbusds</groupId>
+ <artifactId>nimbus-jose-jwt</artifactId>
+ <version>9.22</version>
+ <optional>true</optional>
+ </dependency>
+ <dependency>
+ <groupId>org.bitbucket.b_c</groupId>
+ <artifactId>jose4j</artifactId>
+ <version>0.7.9</version>
Review Comment:
The latest version is `0.9.3`. How come an older version is being used instead of the latest?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #1035:
URL: https://github.com/apache/activemq/pull/1035#issuecomment-1602439407
That's the starting point. The intention is not to merge right now. I wanted to share here as few users want to try. I will work on this PR after the releases plan.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237570092
##########
activemq-broker/src/main/java/org/apache/activemq/security/jwt/Claims.java:
##########
@@ -0,0 +1,104 @@
+/**
+ * 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.security.jwt;
+
+import com.fasterxml.jackson.annotation.JsonValue;
+
+import java.util.Set;
+
+public enum Claims {
Review Comment:
What exactly are all the values, and why are so many unused values needed?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on a diff in pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #1035:
URL: https://github.com/apache/activemq/pull/1035#discussion_r1237565253
##########
activemq-broker/pom.xml:
##########
@@ -66,6 +66,18 @@
<artifactId>activemq-jaas</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>com.nimbusds</groupId>
+ <artifactId>nimbus-jose-jwt</artifactId>
+ <version>9.22</version>
Review Comment:
The latest version is `9.31`. How come an older version is being used instead of the latest?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbertram commented on pull request #1035: [AMQ-9244] Add JWT authentication plugin
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #1035:
URL: https://github.com/apache/activemq/pull/1035#issuecomment-1601569286
Generally speaking this looks more like a proof-of-concept (i.e. based on [this blog post](https://www.tomitribe.com/blog/jwt-authentication-and-authorization-with-apache-activemq/)) rather than a feature which is ready to use. I'm not sure it makes sense to merge it at this point especially with no tests to validate the functionality and to mitigate future regressions.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org