[
https://issues.apache.org/jira/browse/KNOX-2631?focusedWorklogId=815627&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-815627
]
ASF GitHub Bot logged work on KNOX-2631:
----------------------------------------
Author: ASF GitHub Bot
Created on: 11/Oct/22 11:33
Start Date: 11/Oct/22 11:33
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #639:
URL: https://github.com/apache/knox/pull/639#discussion_r992149578
##########
gateway-server/src/test/java/org/apache/knox/gateway/websockets/GatewayWebsocketHandlerTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.audit.api.AuditService;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.i18n.GatewaySpiMessages;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.provider.federation.jwt.JWTMessages;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.webshell.WebshellWebSocketAdapter;
+import org.easymock.EasyMock;
+import org.eclipse.jetty.websocket.servlet.ServletUpgradeRequest;
+import org.eclipse.jetty.websocket.servlet.ServletUpgradeResponse;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.net.URI;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.easymock.EasyMock.isA;
Review Comment:
nit: static imports should go first
##########
gateway-server/src/test/java/org/apache/knox/gateway/webshell/WebshellWebsocketAdapterTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.webshell;
+
+import org.apache.knox.gateway.audit.api.AuditService;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+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.websockets.JWTValidator;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.eclipse.jetty.websocket.api.RemoteEndpoint;
+import org.eclipse.jetty.websocket.api.Session;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.easymock.EasyMock.isA;
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({WebshellWebSocketAdapter.class, MessagesFactory.class,
AuditServiceFactory.class})
+public class WebshellWebsocketAdapterTest extends EasyMockSupport {
+
+ private GatewayConfig setupGatewayConfig(){
+ GatewayConfig gatewayConfig =
EasyMock.createNiceMock(GatewayConfig.class);
+
EasyMock.expect(gatewayConfig.getGatewayPIDDir()).andReturn("testPIDDir").anyTimes();
+
EasyMock.expect(gatewayConfig.getWebShellReadBufferSize()).andReturn(1024).anyTimes();
+
EasyMock.expect(gatewayConfig.isWebShellAuditLoggingEnabled()).andReturn(false).anyTimes();
+ EasyMock.replay(gatewayConfig);
+ return gatewayConfig;
+ }
+ private void setupMessagesFactory(){
+ PowerMock.mockStatic(MessagesFactory.class);
+
EasyMock.expect(MessagesFactory.get(JWTMessages.class)).andReturn(EasyMock.createNiceMock(JWTMessages.class)).anyTimes();
+
EasyMock.expect(MessagesFactory.get(WebsocketLogMessages.class)).andReturn(EasyMock.createNiceMock(WebsocketLogMessages.class)).anyTimes();
+ PowerMock.replay(MessagesFactory.class);
+ }
+ private void setupAuditor(){
+ PowerMock.mockStatic(AuditServiceFactory.class);
+ AuditService auditService =
EasyMock.createNiceMock(AuditService.class);
+
EasyMock.expect(AuditServiceFactory.getAuditService()).andReturn(auditService).anyTimes();
+ Auditor auditor = EasyMock.createNiceMock(Auditor.class);
+
EasyMock.expect(auditService.getAuditor(isA(String.class),isA(String.class),isA(String.class))).andReturn(auditor).anyTimes();
+ PowerMock.replay(AuditServiceFactory.class);
+ EasyMock.replay(auditService,auditor);
+ }
+
+ @Test
+ public void testOnWebSocketConnect() throws Exception{
+
+ GatewayConfig gatewayConfig = setupGatewayConfig();
+ setupMessagesFactory();
+ setupAuditor();
+
+ JWTValidator jwtValidator =
EasyMock.createNiceMock(JWTValidator.class);
+
EasyMock.expect(jwtValidator.getUsername()).andReturn("Alice").anyTimes();
+ EasyMock.replay(jwtValidator);
+
+ ConnectionInfo connectionInfo =
PowerMock.createMock(ConnectionInfo.class);
+ PowerMock.expectNew(ConnectionInfo.class, isA(String.class) ,
isA(String.class), isA(AtomicInteger.class)).andReturn(connectionInfo);
+ connectionInfo.connect();
+
+ String bashOuput = "fake bash output";
+ InputStream inputStream = new
ByteArrayInputStream(bashOuput.getBytes(StandardCharsets.UTF_8));
+
EasyMock.expect(connectionInfo.getInputStream()).andReturn(inputStream).times(1);
+ InputStream inputStreamEmpty = new
ByteArrayInputStream("".getBytes(StandardCharsets.UTF_8));
+
EasyMock.expect(connectionInfo.getInputStream()).andReturn(inputStreamEmpty).times(1);
+
+ Session session = EasyMock.createNiceMock(Session.class);
+ RemoteEndpoint remote = EasyMock.createNiceMock(RemoteEndpoint.class);
+ // send to client
+ EasyMock.expect(session.getRemote()).andReturn(remote).anyTimes();
+ remote.sendString(bashOuput);
+ // clean up
+ EasyMock.expect(session.isOpen()).andReturn(true).anyTimes();
+ session.close();
+ EasyMock.replay(session, remote);
+
+ connectionInfo.disconnect();
+ PowerMock.replay(connectionInfo, ConnectionInfo.class);
+
+ ExecutorService pool = Executors.newFixedThreadPool(10);
+ AtomicInteger concurrentWebshells = new AtomicInteger(0);
+ WebshellWebSocketAdapter webshellWebSocketAdapter = new
WebshellWebSocketAdapter(pool, gatewayConfig, jwtValidator,
concurrentWebshells);
+ webshellWebSocketAdapter.onWebSocketConnect(session);
+ verifyAll();
+ }
+ @Rule
+ public ExpectedException thrown = ExpectedException.none();
+
+ @Test
+ public void testOnWebSocketConnectWithoutUsername() throws Exception {
+ thrown.expect(RuntimeException.class);
+ thrown.expectMessage("Needs user name in JWT to use WebShell");
+ GatewayConfig gatewayConfig = setupGatewayConfig();
+ setupMessagesFactory();
+ setupAuditor();
+
+ JWTValidator jwtValidator =
EasyMock.createNiceMock(JWTValidator.class);
+ EasyMock.expect(jwtValidator.getUsername()).andReturn(null).anyTimes();
+ EasyMock.replay(jwtValidator);
+
+ ExecutorService pool = Executors.newFixedThreadPool(10);
+ AtomicInteger concurrentWebshells = new AtomicInteger(0);
+ new WebshellWebSocketAdapter(pool, gatewayConfig, jwtValidator,
concurrentWebshells);
+ verifyAll();
+ }
+
+
+ @Test
+ public void testOnWebSocketText() throws Exception{
+
+ GatewayConfig gatewayConfig = setupGatewayConfig();
+ setupMessagesFactory();
+ setupAuditor();
+
+ JWTValidator jwtValidator =
EasyMock.createNiceMock(JWTValidator.class);
+
EasyMock.expect(jwtValidator.getUsername()).andReturn("Alice").anyTimes();
+
EasyMock.expect(jwtValidator.tokenIsStillValid()).andReturn(true).anyTimes();
+ EasyMock.replay(jwtValidator);
+
+ ConnectionInfo connectionInfo =
PowerMock.createMock(ConnectionInfo.class);
+ PowerMock.expectNew(ConnectionInfo.class, isA(String.class) ,
isA(String.class), isA(AtomicInteger.class)).andReturn(connectionInfo);
+ connectionInfo.connect();
+
+ OutputStream outputStream =
EasyMock.createNiceMock(OutputStream.class);
+
EasyMock.expect(connectionInfo.getOutputStream()).andReturn(outputStream).times(2);
+ outputStream.write("fake user input".getBytes(StandardCharsets.UTF_8));
+ outputStream.flush();
+ PowerMock.replay(connectionInfo, ConnectionInfo.class);
+ EasyMock.replay(outputStream);
+
+ ExecutorService pool = Executors.newFixedThreadPool(10);
+ AtomicInteger concurrentWebshells = new AtomicInteger(0);
+ WebshellWebSocketAdapter webshellWebSocketAdapter = new
WebshellWebSocketAdapter(pool, gatewayConfig, jwtValidator,
concurrentWebshells);
+ webshellWebSocketAdapter.onWebSocketText("{ \"userInput\" : \"fake
user input\"}");
+ verifyAll();
+ }
+
+ @Test
+ public void testOnWebSocketTextWithExpiredToken() throws Exception {
+ GatewayConfig gatewayConfig = setupGatewayConfig();
+ setupMessagesFactory();
+ setupAuditor();
+
+ JWTValidator jwtValidator =
EasyMock.createNiceMock(JWTValidator.class);
+
EasyMock.expect(jwtValidator.getUsername()).andReturn("Alice").anyTimes();
+
EasyMock.expect(jwtValidator.tokenIsStillValid()).andReturn(false).anyTimes();
+ EasyMock.replay(jwtValidator);
+
+ ConnectionInfo connectionInfo =
PowerMock.createMock(ConnectionInfo.class);
+ PowerMock.expectNew(ConnectionInfo.class, isA(String.class) ,
isA(String.class), isA(AtomicInteger.class)).andReturn(connectionInfo);
+ connectionInfo.disconnect();
+ PowerMock.replay(connectionInfo, ConnectionInfo.class);
+
+ ExecutorService pool = Executors.newFixedThreadPool(10);
+ AtomicInteger concurrentWebshells = new AtomicInteger(0);
+ WebshellWebSocketAdapter webshellWebSocketAdapter = new
WebshellWebSocketAdapter(pool, gatewayConfig, jwtValidator,
concurrentWebshells);
+ webshellWebSocketAdapter.onWebSocketText("{ \"userInput\" : \"fake
user input\"}");
+ verifyAll();
+ }
+
Review Comment:
nit: remove 2 empty lines
##########
knox-homepage-ui/home/app/generalProxyInformation/general.proxy.information.component.ts:
##########
@@ -108,6 +116,7 @@ export class GeneralProxyInformationComponent implements
OnInit {
this['showAdminAPI'] = (profile['gpi_admin_api'] === 'true');
this['showMetadataAPI'] = (profile['gpi_md_api'] === 'true');
this['showTokens'] = (profile['gpi_tokens'] === 'true');
+ this['showWebShell'] = (profile['gpi_webshell'] === 'true');
Review Comment:
It will not work since you are missing the relevant profile declaration from
`org.apache.knox.gateway.dto.HomePageProfile`
##########
pom.xml:
##########
@@ -2473,11 +2524,17 @@
<dependency>
<groupId>org.apache.velocity</groupId>
- <artifactId>velocity-engine-core</artifactId>
+ <artifactId>velocity</artifactId>
Review Comment:
We need to remove `velocity`; see KNOX-2741 for more information.
##########
gateway-server/src/main/java/org/apache/knox/gateway/webshell/ConnectionInfo.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.knox.gateway.webshell;
+
+import com.pty4j.PtyProcess;
+import com.pty4j.PtyProcessBuilder;
+import de.thetaphi.forbiddenapis.SuppressForbidden;
+import org.apache.commons.io.FileUtils;
+import org.apache.knox.gateway.audit.api.Action;
+import org.apache.knox.gateway.audit.api.ActionOutcome;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+import org.apache.knox.gateway.audit.api.ResourceType;
+import org.apache.knox.gateway.audit.log4j.audit.AuditConstants;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.eclipse.jetty.io.RuntimeIOException;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+
+/**
+* data structure to store a connection session
+*/
+public class ConnectionInfo {
+ private static final Auditor auditor =
AuditServiceFactory.getAuditService().getAuditor(
+ AuditConstants.DEFAULT_AUDITOR_NAME,
AuditConstants.KNOX_SERVICE_NAME,
+ AuditConstants.KNOX_COMPONENT_NAME );
+ private static final WebsocketLogMessages LOG =
MessagesFactory.get(WebsocketLogMessages.class);
+
+ private InputStream inputStream;
+ private OutputStream outputStream;
+ private PtyProcess ptyProcess;
+ private final String username;
+
+ private final String gatewayPIDDir;
+ @SuppressWarnings("PMD.DoNotUseThreads") //we need to define a Thread to
clean up resources using shutdown hook
+ private final Thread shutdownHook;
+ private final AtomicInteger concurrentWebshells;
+ private long pid;
+ /* set up webshell environment variables */
+ private final Map<String, String> env = new HashMap();
Review Comment:
Instead of adding a comment what this class member is called we may have a
better name. E.g. `webshellEnvVariables`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -910,6 +930,28 @@ public boolean isWebsocketEnabled() {
return Boolean.parseBoolean(result);
}
+ @Override
+ public boolean isWebShellEnabled() {
+ final String result = get( WEBSHELL_FEATURE_ENABLED,
Boolean.toString(DEFAULT_WEBSHELL_FEATURE_ENABLED));
+ return Boolean.parseBoolean(result);
+ }
+
+ @Override
+ public boolean isWebShellAuditLoggingEnabled(){
+ final String result = get( WEBSHELL_AUDIT_LOGGING_ENABLED,
Boolean.toString(DEFAULT_WEBSHELL_AUDIT_LOGGING_ENABLED));
Review Comment:
There is a `getBoolean(String name, boolean defaultValue)` that would make
the code less complex.
##########
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:
That NPE could be the result of a missing service loader declaration from
one of the resources. I checked that
`org.apache.knox.gateway.provider.federation.jwt.deploy.JWTAuthCodeAssertionContributor`
is added into the appropriate resource file.
I'd handle it in a separate JIRA to not to mix this with WebShell.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -910,6 +930,28 @@ public boolean isWebsocketEnabled() {
return Boolean.parseBoolean(result);
}
+ @Override
+ public boolean isWebShellEnabled() {
+ final String result = get( WEBSHELL_FEATURE_ENABLED,
Boolean.toString(DEFAULT_WEBSHELL_FEATURE_ENABLED));
Review Comment:
There is a `getBoolean(String name, boolean defaultValue)` that would make
the code less complex.
##########
gateway-server/src/test/java/org/apache/knox/gateway/webshell/ConnectionInfoTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.webshell;
+
+import com.pty4j.PtyProcess;
+import com.pty4j.PtyProcessBuilder;
+import org.apache.commons.io.FileUtils;
+import org.apache.knox.gateway.audit.api.AuditService;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.eclipse.jetty.io.RuntimeIOException;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.easymock.EasyMock.anyInt;
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.anyString;
+import static org.easymock.EasyMock.isA;
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Runtime.class, MessagesFactory.class,
AuditServiceFactory.class, ConnectionInfo.class})
+public class ConnectionInfoTest extends EasyMockSupport {
+ private static String testPIDdir = "webshell-test";
+ private static String testUserName = "Alice";
+ private static long testPID = 12345;
+
+ private static void setupMessagesFactory(){
+ PowerMock.mockStatic(MessagesFactory.class);
+
EasyMock.expect(MessagesFactory.get(WebsocketLogMessages.class)).andReturn(EasyMock.createNiceMock(WebsocketLogMessages.class)).anyTimes();
+ PowerMock.replay(MessagesFactory.class);
+ }
+ private static void setupAuditService(){
+ PowerMock.mockStatic(AuditServiceFactory.class);
+ AuditService auditService =
EasyMock.createNiceMock(AuditService.class);
+
EasyMock.expect(AuditServiceFactory.getAuditService()).andReturn(auditService).anyTimes();
+ Auditor auditor = EasyMock.createNiceMock(Auditor.class);
+
EasyMock.expect(auditService.getAuditor(isA(String.class),isA(String.class),isA(String.class))).andReturn(auditor).anyTimes();
+ PowerMock.replay(AuditServiceFactory.class);
+ EasyMock.replay(auditService, auditor);
+ }
+
+
+ @SuppressWarnings("PMD.DoNotUseThreads")
+ private static void setupRuntime(){
+ PowerMock.mockStatic(Runtime.class);
+ Runtime runtime = EasyMock.createMock(Runtime.class);
+ runtime.addShutdownHook(isA(Thread.class));
+
EasyMock.expect(runtime.removeShutdownHook(isA(Thread.class))).andReturn(true).anyTimes();
+ EasyMock.replay(runtime);
+ EasyMock.expect(Runtime.getRuntime()).andReturn(runtime).anyTimes();
+ PowerMock.replay(Runtime.class);
+ }
+
+ @BeforeClass
+ public static void setupBeforeClass(){
+ setupMessagesFactory();
+ setupAuditService();
+ setupRuntime();
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception{
+ //remove testPIDdir directory
+ FileUtils.deleteDirectory(new File(testPIDdir));
+
+ }
+
+ @Test
+ public void testInstantiation() {
+ new ConnectionInfo(testUserName, testPIDdir, new AtomicInteger(0));
+ verifyAll();
+ }
+
+ @Test
+ public void testConnectSuccess() throws Exception {
+ AtomicInteger concurrentWebshell =
EasyMock.createNiceMock(AtomicInteger.class);
+ concurrentWebshell.incrementAndGet();
+ concurrentWebshell.decrementAndGet();
+ EasyMock.replay(concurrentWebshell);
+
+ PtyProcess ptyProcess = EasyMock.createNiceMock(PtyProcess.class);
+ EasyMock.expect(ptyProcess.pid()).andReturn(testPID);
+ EasyMock.replay(ptyProcess);
+
+ PtyProcessBuilder ptyProcessBuilder =
PowerMock.createNiceMock(PtyProcessBuilder.class);
+ String[] cmd = { "sudo","--user", testUserName ,"bash","-i"};
+
EasyMock.expect(ptyProcessBuilder.setCommand(cmd)).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setRedirectErrorStream(true)).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setWindowsAnsiColorEnabled(true)).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setInitialColumns(anyInt())).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setInitialRows(anyInt())).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setDirectory(anyString())).andReturn(ptyProcessBuilder);
+
EasyMock.expect(ptyProcessBuilder.setEnvironment(anyObject())).andReturn(ptyProcessBuilder);
+ EasyMock.expect(ptyProcessBuilder.start()).andReturn(ptyProcess);
+
PowerMock.expectNew(PtyProcessBuilder.class).andReturn(ptyProcessBuilder);
+ PowerMock.replay(ptyProcessBuilder,PtyProcessBuilder.class);
+
+ ConnectionInfo connectionInfo = new ConnectionInfo(testUserName,
testPIDdir, concurrentWebshell);
+ connectionInfo.connect();
+ connectionInfo.disconnect();
+ verifyAll();
+ }
+
+ @Rule
Review Comment:
Rules should be defined above in the class (i.e. not between test methods).
##########
gateway-server/src/main/java/org/apache/knox/gateway/webshell/ConnectionInfo.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.knox.gateway.webshell;
+
+import com.pty4j.PtyProcess;
+import com.pty4j.PtyProcessBuilder;
+import de.thetaphi.forbiddenapis.SuppressForbidden;
+import org.apache.commons.io.FileUtils;
+import org.apache.knox.gateway.audit.api.Action;
+import org.apache.knox.gateway.audit.api.ActionOutcome;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+import org.apache.knox.gateway.audit.api.ResourceType;
+import org.apache.knox.gateway.audit.log4j.audit.AuditConstants;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.eclipse.jetty.io.RuntimeIOException;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+
+/**
+* data structure to store a connection session
+*/
+public class ConnectionInfo {
+ private static final Auditor auditor =
AuditServiceFactory.getAuditService().getAuditor(
+ AuditConstants.DEFAULT_AUDITOR_NAME,
AuditConstants.KNOX_SERVICE_NAME,
+ AuditConstants.KNOX_COMPONENT_NAME );
+ private static final WebsocketLogMessages LOG =
MessagesFactory.get(WebsocketLogMessages.class);
+
+ private InputStream inputStream;
+ private OutputStream outputStream;
+ private PtyProcess ptyProcess;
+ private final String username;
+
+ private final String gatewayPIDDir;
+ @SuppressWarnings("PMD.DoNotUseThreads") //we need to define a Thread to
clean up resources using shutdown hook
+ private final Thread shutdownHook;
+ private final AtomicInteger concurrentWebshells;
+ private long pid;
+ /* set up webshell environment variables */
+ private final Map<String, String> env = new HashMap();
+
+ @SuppressWarnings("PMD.DoNotUseThreads") //we need to define a Thread to
clean up resources using shutdown hook
+ public ConnectionInfo(String username, String gatewayPIDDir, AtomicInteger
concurrentWebshells) {
+ this.username = username;
+ this.gatewayPIDDir = gatewayPIDDir;
+ this.concurrentWebshells = concurrentWebshells;
+ shutdownHook = new Thread(this::disconnect);
+ Runtime.getRuntime().addShutdownHook(shutdownHook);
+ env.put("TERM", "xterm");
Review Comment:
`TERM` and `xterm` texts could be constants.
##########
gateway-server/src/test/java/org/apache/knox/gateway/webshell/WebshellWebsocketAdapterTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.webshell;
+
+import org.apache.knox.gateway.audit.api.AuditService;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+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.websockets.JWTValidator;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.eclipse.jetty.websocket.api.RemoteEndpoint;
+import org.eclipse.jetty.websocket.api.Session;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.easymock.EasyMock.isA;
Review Comment:
nit: static imports should go first
##########
gateway-server/src/test/java/org/apache/knox/gateway/webshell/WebshellWebsocketAdapterTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.webshell;
+
+import org.apache.knox.gateway.audit.api.AuditService;
+import org.apache.knox.gateway.audit.api.AuditServiceFactory;
+import org.apache.knox.gateway.audit.api.Auditor;
+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.websockets.JWTValidator;
+import org.apache.knox.gateway.websockets.WebsocketLogMessages;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.eclipse.jetty.websocket.api.RemoteEndpoint;
+import org.eclipse.jetty.websocket.api.Session;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.easymock.EasyMock.isA;
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({WebshellWebSocketAdapter.class, MessagesFactory.class,
AuditServiceFactory.class})
+public class WebshellWebsocketAdapterTest extends EasyMockSupport {
+
+ private GatewayConfig setupGatewayConfig(){
+ GatewayConfig gatewayConfig =
EasyMock.createNiceMock(GatewayConfig.class);
+
EasyMock.expect(gatewayConfig.getGatewayPIDDir()).andReturn("testPIDDir").anyTimes();
+
EasyMock.expect(gatewayConfig.getWebShellReadBufferSize()).andReturn(1024).anyTimes();
+
EasyMock.expect(gatewayConfig.isWebShellAuditLoggingEnabled()).andReturn(false).anyTimes();
+ EasyMock.replay(gatewayConfig);
+ return gatewayConfig;
+ }
+ private void setupMessagesFactory(){
+ PowerMock.mockStatic(MessagesFactory.class);
+
EasyMock.expect(MessagesFactory.get(JWTMessages.class)).andReturn(EasyMock.createNiceMock(JWTMessages.class)).anyTimes();
+
EasyMock.expect(MessagesFactory.get(WebsocketLogMessages.class)).andReturn(EasyMock.createNiceMock(WebsocketLogMessages.class)).anyTimes();
+ PowerMock.replay(MessagesFactory.class);
+ }
+ private void setupAuditor(){
+ PowerMock.mockStatic(AuditServiceFactory.class);
+ AuditService auditService =
EasyMock.createNiceMock(AuditService.class);
+
EasyMock.expect(AuditServiceFactory.getAuditService()).andReturn(auditService).anyTimes();
+ Auditor auditor = EasyMock.createNiceMock(Auditor.class);
+
EasyMock.expect(auditService.getAuditor(isA(String.class),isA(String.class),isA(String.class))).andReturn(auditor).anyTimes();
+ PowerMock.replay(AuditServiceFactory.class);
+ EasyMock.replay(auditService,auditor);
+ }
+
+ @Test
+ public void testOnWebSocketConnect() throws Exception{
+
+ GatewayConfig gatewayConfig = setupGatewayConfig();
+ setupMessagesFactory();
+ setupAuditor();
+
+ JWTValidator jwtValidator =
EasyMock.createNiceMock(JWTValidator.class);
+
EasyMock.expect(jwtValidator.getUsername()).andReturn("Alice").anyTimes();
+ EasyMock.replay(jwtValidator);
+
+ ConnectionInfo connectionInfo =
PowerMock.createMock(ConnectionInfo.class);
+ PowerMock.expectNew(ConnectionInfo.class, isA(String.class) ,
isA(String.class), isA(AtomicInteger.class)).andReturn(connectionInfo);
+ connectionInfo.connect();
+
+ String bashOuput = "fake bash output";
+ InputStream inputStream = new
ByteArrayInputStream(bashOuput.getBytes(StandardCharsets.UTF_8));
+
EasyMock.expect(connectionInfo.getInputStream()).andReturn(inputStream).times(1);
+ InputStream inputStreamEmpty = new
ByteArrayInputStream("".getBytes(StandardCharsets.UTF_8));
+
EasyMock.expect(connectionInfo.getInputStream()).andReturn(inputStreamEmpty).times(1);
+
+ Session session = EasyMock.createNiceMock(Session.class);
+ RemoteEndpoint remote = EasyMock.createNiceMock(RemoteEndpoint.class);
+ // send to client
+ EasyMock.expect(session.getRemote()).andReturn(remote).anyTimes();
+ remote.sendString(bashOuput);
+ // clean up
+ EasyMock.expect(session.isOpen()).andReturn(true).anyTimes();
+ session.close();
+ EasyMock.replay(session, remote);
+
+ connectionInfo.disconnect();
+ PowerMock.replay(connectionInfo, ConnectionInfo.class);
+
+ ExecutorService pool = Executors.newFixedThreadPool(10);
+ AtomicInteger concurrentWebshells = new AtomicInteger(0);
+ WebshellWebSocketAdapter webshellWebSocketAdapter = new
WebshellWebSocketAdapter(pool, gatewayConfig, jwtValidator,
concurrentWebshells);
+ webshellWebSocketAdapter.onWebSocketConnect(session);
+ verifyAll();
+ }
+ @Rule
+ public ExpectedException thrown = ExpectedException.none();
Review Comment:
Rules should be defined above in the class (i.e. not between test methods).
##########
knox-webshell-ui/webshell-ui/assets/.gitkeep:
##########
@@ -0,0 +1,17 @@
+##########################################################################
Review Comment:
What's the purpose of this file/folder? If there are no assets now, we
should not add the folder and remove all references.
##########
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:
See my comment above on this topic. I'm with Larry here, we should not
duplicate code at this scale.
##########
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:
I believe there has to be a more elegant way that copy-pasting validation
code from an already existing class. I got that you tried to avoid refactoring,
but - IMO - this refactoring should not be saved here.
Issue Time Tracking
-------------------
Worklog Id: (was: 815627)
Time Spent: 7h 10m (was: 7h)
> KnoxSSO for Secure Shell Access
> -------------------------------
>
> Key: KNOX-2631
> URL: https://issues.apache.org/jira/browse/KNOX-2631
> Project: Apache Knox
> Issue Type: New Feature
> Components: Homepage, KnoxSSO
> Affects Versions: 1.5.0
> Reporter: Lu Liu
> Assignee: Lu Liu
> Priority: Minor
> Fix For: 2.0.0
>
> Time Spent: 7h 10m
> Remaining Estimate: 0h
>
> While Apache Knox aids in helping to lessen the need to gain physical access
> to deployment machines, there are still numerous compelling reasons for users
> to require such access:
> 1. Debugging, log access, etc
> 2. CLI use and automation
> 3. beeline and other clients that are deployed to gateway machines
> Gaining access to a shell for these purposes currently requires the user to
> have
> an ssh client installed and valid credentials that can be used for ssh, such
> as:
> username and password or SSH keys. Separate management of credentials for this
> access introduces some additional complexities which may even violate
> enterprise
> infosec policies and require the secure distribution and management of keys.
> The intent of this proposed improvement is to add a browser based terminal
> application
> that will provide secure access to a shell on the Knox machine. Just as any
> resource
> exposed by or hosted by Knox, you would be able to protect access to this
> terminal
> with any of the available security providers. We would also like to make this
> available out of the box as available from the Knox Homepage. This would make
> the
> terminal/shell available via KnoxSSO thus providing shell access with your
> existing
> enterprise credential authenticated SSO session.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
