-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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<>(); | ||
|
||
|
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it OK to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
} | ||
|
@@ -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)) | ||
|
This file was deleted.
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> |
There was a problem hiding this comment.
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.