Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZEPPELIN-6142] Apply synchronizer token pattern to terminal interpreter for CSWSH protection #4892

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,12 @@ public void createTerminalDashboard(String noteId, String paragraphId, String ho
Jinjava jinjava = new Jinjava();
HashMap<String, Object> jinjaParams = new HashMap();
Date now = new Date();
String terminalServerUrl = generateOrigin(hostIp, port) +
"?noteId=" + noteId + "&paragraphId=" + paragraphId + "&t=" + now.getTime();
String terminalServerUrl = generateOrigin(hostIp, port) + "/terminal-ui" +
"?noteId=" + noteId + "&paragraphId=" + paragraphId + "&t=" + now.getTime();
jinjaParams.put("HOST_NAME", hostName);
jinjaParams.put("HOST_IP", hostIp);
jinjaParams.put("TERMINAL_SERVER_URL", terminalServerUrl);

String terminalDashboardTemplate = jinjava.render(template, jinjaParams);

LOGGER.info(terminalDashboardTemplate);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.apache.zeppelin.shell.terminal;

import java.util.Map;
import java.util.Objects;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import org.apache.zeppelin.shell.terminal.websocket.TerminalSocket;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class TerminalCsrfTokenManager {
private static TerminalCsrfTokenManager instance;

private static final Logger LOGGER = LoggerFactory.getLogger(TerminalSocket.class);

private final Map<String, String> csrfTokens = new ConcurrentHashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSRF tokens are managed per note and paragraph.



public static synchronized TerminalCsrfTokenManager getInstance(){
if (instance == null) {
instance = new TerminalCsrfTokenManager();
}
return instance;
}

public String generateToken(String noteId, String paragraphId) {
String key = formatId(noteId, paragraphId);
return csrfTokens.computeIfAbsent(key, k -> UUID.randomUUID().toString());
}

public boolean validateToken(String noteId, String paragraphId, String token) {
if (token == null) {
LOGGER.warn("Received null CSRF token for validation");
return false;
}

String storedToken = csrfTokens.get(formatId(noteId, paragraphId));
return Objects.equals(storedToken, token);
}

private String formatId(String noteId, String paragraphId) {
return noteId + "@" + paragraphId;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.apache.zeppelin.shell.terminal;

import com.google.common.io.Resources;
import com.hubspot.jinjava.Jinjava;
import java.io.IOException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class TerminalServlet extends HttpServlet {

private Jinjava jinjava = new Jinjava();

@Override
protected void doGet(
HttpServletRequest request,
HttpServletResponse response
)
throws ServletException, IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to remove ServletException here? My IDE shows that it is never thrown in this method.

Copy link
Contributor Author

@seung-00 seung-00 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. That said, other classes that extend from HttpServlet, like PrometheusServlet and IndexHtmlServlet, don’t use these exceptions but keep them explicitly.
IMO, for consistency, it’s better to keep them in this PR too. If you plan to remove them, it’d be better to handle that in a separate PR.

URL urlTemplate = Resources.getResource("ui_templates/terminal-ui.jinja");
String template = Resources.toString(urlTemplate, StandardCharsets.UTF_8);

String noteId = request.getParameter("noteId");
String paragraphId = request.getParameter("paragraphId");

String csrfToken = TerminalCsrfTokenManager.getInstance().generateToken(noteId, paragraphId);

Map<String, Object> context = new HashMap<>();
context.put("CSRF_TOKEN", csrfToken);
Copy link
Contributor

@tbonelee tbonelee Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, this handler is publicly accesible. When testing in my local environment, I noticed that, since it includes the CSRF token, anyone who knows the mapped address for terminal-ui could potentially access the terminal.
It might be worth considering receiving the CSRF token exclusively through the channel between NotebookServer and the WebSocket (e.g., within HTML elements rendered by terminal-dashboard.jinja).
This way, even if someone discovers the terminal UI address, they wouldn't have direct access to the CSRF token.

Copy link
Contributor Author

@seung-00 seung-00 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It lacks authentication, so it’s still vulnerable to token theft. But other HTML elements, like terminal-dashboard, aren’t accessible within the iframe for terminal-ui.


String renderedTemplate = jinjava.render(template, context);

response.setContentType("text/html; charset=UTF-8");
response.setStatus(HttpServletResponse.SC_OK);
response.getWriter().write(renderedTemplate);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ public void run() {
connector.setPort(port);
jettyServer.addConnector(connector);

ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
context.setContextPath("/terminal/");
ServletContextHandler terminalSocketContext = new ServletContextHandler(ServletContextHandler.SESSIONS);
terminalSocketContext.setContextPath("/terminal/");

// We look for a file, as ClassLoader.getResource() is not
// designed to look for directories (we resolve the directory later)
ClassLoader clazz = TerminalThread.class.getClassLoader();
URL url = clazz.getResource("html");
URL url = clazz.getResource("static");
Copy link
Contributor Author

@seung-00 seung-00 Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed html directory to static, as index.html is now rendered by Jinja and has been moved to an ui_templates directory.

if (url == null) {
throw new RuntimeException("Unable to find resource directory");
}
Expand All @@ -68,14 +68,17 @@ public void run() {
String webRootUri = url.toExternalForm();
LOGGER.info("WebRoot is " + webRootUri);
// debug
// webRootUri = "/home/hadoop/zeppelin-current/interpreter/sh";
resourceHandler.setResourceBase(webRootUri);

HandlerCollection handlers = new HandlerCollection(context, resourceHandler);
ServletContextHandler terminalWebContext = new ServletContextHandler(ServletContextHandler.SESSIONS);
terminalWebContext.setContextPath("/terminal-ui/");
terminalWebContext.addServlet(TerminalServlet.class, "/");

HandlerCollection handlers = new HandlerCollection(terminalSocketContext, terminalWebContext, resourceHandler);
jettyServer.setHandler(handlers);

try {
ServerContainer container = WebSocketServerContainerInitializer.configureContext(context);
ServerContainer container = WebSocketServerContainerInitializer.configureContext(terminalSocketContext);
container.addEndpoint(
ServerEndpointConfig.Builder.create(TerminalSocket.class, "/")
.configurator(new TerminalSessionConfigurator(allwedOrigin))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
import java.io.IOException;
import org.apache.zeppelin.shell.terminal.TerminalCsrfTokenManager;
import org.apache.zeppelin.shell.terminal.TerminalManager;
import org.apache.zeppelin.shell.terminal.service.TerminalService;
import org.slf4j.Logger;
Expand Down Expand Up @@ -54,7 +56,7 @@ public void onWebSocketConnect(Session sess) {
}

@OnMessage
public void onWebSocketText(String message) {
public void onWebSocketText(String message, Session sess) throws IOException {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Received TEXT message: " + message);
}
Expand All @@ -74,6 +76,12 @@ public void onWebSocketText(String message) {
terminalService.onTerminalReady();
this.noteId = messageMap.get("noteId");
this.paragraphId = messageMap.get("paragraphId");
String csrfToken = messageMap.get("csrfToken");
if (!isValidCsrfToken(csrfToken)) {
LOGGER.error("Invalid CSRF token: " + csrfToken);
sess.close(new CloseReason(CloseReason.CloseCodes.CANNOT_ACCEPT, "Invalid CSRF Token"));
return;
}
terminalManager.onWebSocketConnect(noteId, paragraphId);
break;
case "TERMINAL_COMMAND":
Expand Down Expand Up @@ -108,4 +116,8 @@ private Map<String, String> getMessageMap(String message) {
new TypeToken<Map<String, String>>(){}.getType());
return map;
}

private boolean isValidCsrfToken(String csrfToken) {
return TerminalCsrfTokenManager.getInstance().validateToken(noteId, paragraphId, csrfToken);
}
}
32 changes: 0 additions & 32 deletions shell/src/main/resources/html/index.html

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,10 @@ let app = {
},
onTerminalReady() {
// alert("TERMINAL_READY");
const csrfToken = document.querySelector('meta[name="csrf-token"]').content;

ws.send(action("TERMINAL_READY", {
noteId, paragraphId
noteId, paragraphId, csrfToken
}));
}
};
Expand Down
35 changes: 35 additions & 0 deletions shell/src/main/resources/ui_templates/terminal-ui.jinja
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{#
# 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.
-#}
<html>
<header>
<meta name="csrf-token" content="{{ CSRF_TOKEN }}">
<style>
* {
margin: 0;
padding: 0;
}
html, body {
height: 100%;
}
</style>
<script src="/hterm_all.js"></script>
<script src="/index.js"></script>
</header>
<body>
<div id="terminal" style="position:relative; width:100%; height:100%"></div>
</body>
</html>