lmccay commented on code in PR #639:
URL: https://github.com/apache/knox/pull/639#discussion_r985259046
##########
gateway-release/home/conf/gateway-site.xml:
##########
@@ -88,6 +88,35 @@ limitations under the License.
<value>60</value>
<description>The interval (in seconds) for polling Ambari for cluster
configuration changes.</description>
</property>
+ <!-- @since 2.0.0 WebShell configs -->
+ <!-- must have websocket enabled to use webshell -->
+ <property>
+ <name>gateway.webshell.feature.enabled</name>
+ <value>false</value>
+ <description>Enable/Disable webshell feature.</description>
+ </property>
+ <property>
+ <name>webshell.max.concurrent.sessions</name>
+ <value>3</value>
+ <description>maximum number of concurrent webshell
sessions</description>
+ </property>
+ <property>
+ <name>gateway.webshell.audit.logging.enabled</name>
+ <value>false</value>
+ <description>Enable/Disable webshell audit logging.</description>
+ </property>
+ <property>
+ <name>gateway.webshell.read.buffer.size</name>
+ <value>1024</value>
+ <description>Web Shell buffer size for reading</description>
+ </property>
+
+ <!-- @since 2.0.0 websocket JWT validation configs -->
+ <property>
+ <name>gateway.websocket.JWT.validation.feature.enabled</name>
+ <value>false</value>
+ <description>Enable/Disable websocket JWT validation
feature.</description>
Review Comment:
Is this not always needed when the feature is enabled? If so, there is not
sense in requiring too things to be enabled for the feature.
##########
gateway-server/src/main/java/org/apache/knox/gateway/websockets/JWTValidatorFactory.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.knox.gateway.websockets;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.provider.federation.jwt.JWTMessages;
+import
org.apache.knox.gateway.provider.federation.jwt.filter.SignatureVerificationCache;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.security.token.TokenStateService;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+import org.apache.knox.gateway.services.topology.TopologyService;
+import org.apache.knox.gateway.topology.Service;
+import org.apache.knox.gateway.topology.Topology;
+import org.apache.knox.gateway.util.CertificateUtils;
+import org.eclipse.jetty.websocket.servlet.ServletUpgradeRequest;
+import javax.servlet.ServletException;
+import java.net.HttpCookie;
+import java.security.interfaces.RSAPublicKey;
+import java.text.ParseException;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+public class JWTValidatorFactory {
Review Comment:
Again, there is very little websocket specific in here and I think that the
ServletUpgradeRequest can likely be made more generic depending on its
hierarchy. It seems like this Factory should be more generic and common JWT
code rather than websocket specific.
##########
pom.xml:
##########
@@ -280,7 +284,13 @@
<xml-matchers.version>0.10</xml-matchers.version>
<zookeeper.version>3.5.7</zookeeper.version>
</properties>
-
+ <repositories>
+ <repository>
+ <id>jcenter</id>
Review Comment:
what is this for?
##########
gateway-server/src/main/java/org/apache/knox/gateway/websockets/GatewayWebsocketHandler.java:
##########
@@ -109,18 +115,48 @@ public void configure(final WebSocketServletFactory
factory) {
}
+ private Boolean isWebshellRequest(URI requestURI){
+ return requestURI.toString().matches(REGEX_WEBSHELL_REQUEST_PATH);
+ }
+
+ private WebshellWebSocketAdapter handleWebshellRequest(ServletUpgradeRequest
req){
+ if (config.isWebShellEnabled()){
+ if (concurrentWebshells.get() >=
config.getMaximumConcurrentWebshells()){
+ throw new RuntimeException("Number of allowed concurrent Web Shell
sessions exceeded");
+ }
+ JWTValidator jwtValidator =
JWTValidatorFactory.create(req,services,config);
Review Comment:
spaces following param names
##########
gateway-release/home/conf/gateway-site.xml:
##########
@@ -88,6 +88,35 @@ limitations under the License.
<value>60</value>
<description>The interval (in seconds) for polling Ambari for cluster
configuration changes.</description>
</property>
+ <!-- @since 2.0.0 WebShell configs -->
+ <!-- must have websocket enabled to use webshell -->
+ <property>
+ <name>gateway.webshell.feature.enabled</name>
+ <value>false</value>
+ <description>Enable/Disable webshell feature.</description>
+ </property>
+ <property>
+ <name>webshell.max.concurrent.sessions</name>
+ <value>3</value>
+ <description>maximum number of concurrent webshell
sessions</description>
Review Comment:
Is this per user or in total? 3 may be too low for in total.
##########
gateway-server/src/main/java/org/apache/knox/gateway/websockets/JWTValidator.java:
##########
@@ -0,0 +1,193 @@
+/*
+ * 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.knox.gateway.websockets;
+
+import com.nimbusds.jose.JWSHeader;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.provider.federation.jwt.JWTMessages;
+import
org.apache.knox.gateway.provider.federation.jwt.filter.SignatureVerificationCache;
+import org.apache.knox.gateway.services.security.token.JWTokenAuthority;
+import org.apache.knox.gateway.services.security.token.TokenMetadata;
+import org.apache.knox.gateway.services.security.token.TokenServiceException;
+import org.apache.knox.gateway.services.security.token.TokenStateService;
+import org.apache.knox.gateway.services.security.token.TokenUtils;
+import org.apache.knox.gateway.services.security.token.UnknownTokenException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.util.Tokens;
+
+import java.security.interfaces.RSAPublicKey;
+import java.text.ParseException;
+import java.util.Date;
+
+public class JWTValidator {
Review Comment:
Not seeing anything websocket specific in this class and there seems to
likely be redundant implementations of verification. Is there some reason that
an existing class couldn't be used or that this isn't made more common?
##########
gateway-server/src/main/java/org/apache/knox/gateway/websockets/JWTValidatorFactory.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.knox.gateway.websockets;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.provider.federation.jwt.JWTMessages;
+import
org.apache.knox.gateway.provider.federation.jwt.filter.SignatureVerificationCache;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.security.token.TokenStateService;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+import org.apache.knox.gateway.services.topology.TopologyService;
+import org.apache.knox.gateway.topology.Service;
+import org.apache.knox.gateway.topology.Topology;
+import org.apache.knox.gateway.util.CertificateUtils;
+import org.eclipse.jetty.websocket.servlet.ServletUpgradeRequest;
+import javax.servlet.ServletException;
+import java.net.HttpCookie;
+import java.security.interfaces.RSAPublicKey;
+import java.text.ParseException;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+public class JWTValidatorFactory {
Review Comment:
if ServletUpgradeRequest can't be cast to or used to retrieve the
HttpServletRequest to be more generic then maybe there should be an abstract
based class that can override extractToken here.
##########
pom.xml:
##########
@@ -248,13 +249,16 @@
<mina.version>2.0.22</mina.version>
<netty.version>4.1.77.Final</netty.version>
<nimbus-jose-jwt.version>8.14.1</nimbus-jose-jwt.version>
- <nodejs.version>v12.18.2</nodejs.version>
+ <nodejs.version>v12.20.2</nodejs.version>
<okhttp.version>2.7.5</okhttp.version>
<opensaml.version>3.4.5</opensaml.version>
<pac4j.version>4.5.6</pac4j.version>
<postgresql.version>42.3.3</postgresql.version>
<mysql.version>8.0.28</mysql.version>
<protobuf.version>3.16.1</protobuf.version>
+ <powermock.version>2.0.9</powermock.version>
+ <purejavacomm.version>0.0.11.1</purejavacomm.version>
+ <pty4j.version>0.11.4</pty4j.version>
Review Comment:
Note that this is EPLv1 licensed. This is a weak copy-left licence under
what ASLv2 considers Category B. We will need to meet the conditions in
https://www.apache.org/legal/resolved.html#category-b
##########
gateway-release/home/conf/gateway-site.xml:
##########
@@ -88,6 +88,35 @@ limitations under the License.
<value>60</value>
<description>The interval (in seconds) for polling Ambari for cluster
configuration changes.</description>
</property>
+ <!-- @since 2.0.0 WebShell configs -->
+ <!-- must have websocket enabled to use webshell -->
+ <property>
+ <name>gateway.webshell.feature.enabled</name>
+ <value>false</value>
+ <description>Enable/Disable webshell feature.</description>
+ </property>
+ <property>
+ <name>webshell.max.concurrent.sessions</name>
+ <value>3</value>
+ <description>maximum number of concurrent webshell
sessions</description>
+ </property>
+ <property>
+ <name>gateway.webshell.audit.logging.enabled</name>
+ <value>false</value>
Review Comment:
If this is disabled by default due to security implications of logging
commands and maybe secrets than we may want to make this clear in the
description and maybe call it something other than audit. Audit seems like
something that you would definitely want to enable for this feature.
##########
pom.xml:
##########
@@ -248,13 +249,16 @@
<mina.version>2.0.22</mina.version>
<netty.version>4.1.77.Final</netty.version>
<nimbus-jose-jwt.version>8.14.1</nimbus-jose-jwt.version>
- <nodejs.version>v12.18.2</nodejs.version>
+ <nodejs.version>v12.20.2</nodejs.version>
<okhttp.version>2.7.5</okhttp.version>
<opensaml.version>3.4.5</opensaml.version>
<pac4j.version>4.5.6</pac4j.version>
<postgresql.version>42.3.3</postgresql.version>
<mysql.version>8.0.28</mysql.version>
<protobuf.version>3.16.1</protobuf.version>
+ <powermock.version>2.0.9</powermock.version>
+ <purejavacomm.version>0.0.11.1</purejavacomm.version>
+ <pty4j.version>0.11.4</pty4j.version>
Review Comment:
Adding pty4j and anything else with this license to the entry in LICENSE
file where we are calling out Jetty and Jerico is likely enough.
##########
gateway-server/src/main/java/org/apache/knox/gateway/deploy/DeploymentFactory.java:
##########
@@ -260,7 +260,13 @@ private static void addMissingDefaultProviders(Topology
topology) {
// for some tests the number of providers are limited to the classpath of
the module
// and exceptions will be thrown as topologies are deployed even though
they will
// work fine at actual server runtime.
- if (PROVIDER_CONTRIBUTOR_MAP.get("identity-assertion") != null) {
+
+ // SRM: Only add "Default" provider iff it is found in the provider
contributor map
+ // There could be cases where service loader might find other
+ // identity-assertion providers e.g. JWTAuthCodeAsserter
+ if (PROVIDER_CONTRIBUTOR_MAP.get("identity-assertion") != null &&
+ PROVIDER_CONTRIBUTOR_MAP.get("identity-assertion").keySet() !=
null &&
+
PROVIDER_CONTRIBUTOR_MAP.get("identity-assertion").keySet().contains("Default"))
{
Review Comment:
This doesn't seem to be specific to WebShell - is this a generic issue that
should be pulled out separately?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
