From ea54b55bfadf6a1ab777866c2e1d03979dc049d6 Mon Sep 17 00:00:00 2001 From: joelz Date: Wed, 12 Aug 2015 12:16:29 -0700 Subject: [PATCH 01/19] Fixing issue with ZEPPELIN-173: Zeppelin websocket server is vulnerable to Cross-Site WebSocket Hijacking --- .../zeppelin/socket/NotebookServer.java | 151 +++---- .../zeppelin/socket/NotebookServerTests.java | 53 +++ .../socket/TestHttpServletRequest.java | 372 ++++++++++++++++++ .../socket/TestNotebookSocketListener.java | 41 ++ 4 files changed, 516 insertions(+), 101 deletions(-) create mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java create mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java create mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java index fe0d3912bb8..2f11e8a5e53 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java @@ -14,19 +14,17 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.zeppelin.socket; - import java.io.IOException; -import java.net.InetSocketAddress; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.UnknownHostException; import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; - import javax.servlet.http.HttpServletRequest; - import org.apache.zeppelin.display.AngularObject; import org.apache.zeppelin.display.AngularObjectRegistry; import org.apache.zeppelin.display.AngularObjectRegistryListener; @@ -46,34 +44,51 @@ import org.quartz.SchedulerException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - import com.google.common.base.Strings; import com.google.gson.Gson; - /** * Zeppelin websocket service. * * @author anthonycorbacho */ public class NotebookServer extends WebSocketServlet implements - NotebookSocketListener, JobListenerFactory, AngularObjectRegistryListener { - + NotebookSocketListener, JobListenerFactory, AngularObjectRegistryListener { private static final Logger LOG = LoggerFactory - .getLogger(NotebookServer.class); - + .getLogger(NotebookServer.class); Gson gson = new Gson(); - Map> noteSocketMap = new HashMap>(); - List connectedSockets = new LinkedList(); + final Map> noteSocketMap = new HashMap<>(); + final List connectedSockets = new LinkedList<>(); private Notebook notebook() { return ZeppelinServer.notebook; } + @Override + public boolean checkOrigin(HttpServletRequest request, String origin) { + URI sourceUri = null; + String currentHost = null; + + try { + sourceUri = new URI(origin); + currentHost = java.net.InetAddress.getLocalHost().getHostName(); + } catch (UnknownHostException e) { + e.printStackTrace(); + } + catch (URISyntaxException e) { + e.printStackTrace(); + } + + String sourceHost = sourceUri.getHost(); + if (currentHost.equals(sourceHost) || "localhost".equals(sourceHost)) { + return true; + } + + return false; + } @Override public WebSocket doWebSocketConnect(HttpServletRequest req, String protocol) { return new NotebookSocket(req, protocol, this); } - @Override public void onOpen(NotebookSocket conn) { LOG.info("New connection from {} : {}", conn.getRequest().getRemoteAddr(), @@ -82,7 +97,6 @@ public void onOpen(NotebookSocket conn) { connectedSockets.add(conn); } } - @Override public void onMessage(NotebookSocket conn, String msg) { Notebook notebook = notebook(); @@ -98,7 +112,7 @@ public void onMessage(NotebookSocket conn, String msg) { sendNote(conn, notebook, messagereceived); break; case NEW_NOTE: - createNote(conn, notebook, messagereceived); + createNote(conn, notebook); break; case DEL_NOTE: removeNote(conn, notebook, messagereceived); @@ -141,7 +155,6 @@ public void onMessage(NotebookSocket conn, String msg) { LOG.error("Can't handle message", e); } } - @Override public void onClose(NotebookSocket conn, int code, String reason) { LOG.info("Closed connection to {} : {}. ({}) {}", conn.getRequest() @@ -151,32 +164,26 @@ public void onClose(NotebookSocket conn, int code, String reason) { connectedSockets.remove(conn); } } - private Message deserializeMessage(String msg) { - Message m = gson.fromJson(msg, Message.class); - return m; + return gson.fromJson(msg, Message.class); } - private String serializeMessage(Message m) { return gson.toJson(m); } - private void addConnectionToNote(String noteId, NotebookSocket socket) { synchronized (noteSocketMap) { removeConnectionFromAllNote(socket); // make sure a socket relates only a - // single note. + // single note. List socketList = noteSocketMap.get(noteId); if (socketList == null) { - socketList = new LinkedList(); + socketList = new LinkedList<>(); noteSocketMap.put(noteId, socketList); } - - if (socketList.contains(socket) == false) { + if (!socketList.contains(socket)) { socketList.add(socket); } } } - private void removeConnectionFromNote(String noteId, NotebookSocket socket) { synchronized (noteSocketMap) { List socketList = noteSocketMap.get(noteId); @@ -185,13 +192,11 @@ private void removeConnectionFromNote(String noteId, NotebookSocket socket) { } } } - private void removeNote(String noteId) { synchronized (noteSocketMap) { List socketList = noteSocketMap.remove(noteId); } } - private void removeConnectionFromAllNote(NotebookSocket socket) { synchronized (noteSocketMap) { Set keys = noteSocketMap.keySet(); @@ -200,7 +205,6 @@ private void removeConnectionFromAllNote(NotebookSocket socket) { } } } - private String getOpenNoteId(NotebookSocket socket) { String id = null; synchronized (noteSocketMap) { @@ -214,7 +218,6 @@ private String getOpenNoteId(NotebookSocket socket) { } return id; } - private void broadcastToNoteBindedInterpreter(String interpreterGroupId, Message m) { Notebook notebook = notebook(); @@ -228,16 +231,13 @@ private void broadcastToNoteBindedInterpreter(String interpreterGroupId, } } } - private void broadcast(String noteId, Message m) { synchronized (noteSocketMap) { List socketLists = noteSocketMap.get(noteId); if (socketLists == null || socketLists.size() == 0) { return; } - LOG.info("SEND >> " + m.op); - for (NotebookSocket conn : socketLists) { try { conn.send(serializeMessage(m)); @@ -247,7 +247,6 @@ private void broadcast(String noteId, Message m) { } } } - private void broadcastAll(Message m) { synchronized (connectedSockets) { for (NotebookSocket conn : connectedSockets) { @@ -259,24 +258,21 @@ private void broadcastAll(Message m) { } } } - private void broadcastNote(Note note) { broadcast(note.id(), new Message(OP.NOTE).put("note", note)); } - private void broadcastNoteList() { Notebook notebook = notebook(); List notes = notebook.getAllNotes(); - List> notesInfo = new LinkedList>(); + List> notesInfo = new LinkedList<>(); for (Note note : notes) { - Map info = new HashMap(); + Map info = new HashMap<>(); info.put("id", note.id()); info.put("name", note.getName()); notesInfo.add(info); } broadcastAll(new Message(OP.NOTES_INFO).put("notes", notesInfo)); } - private void sendNote(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String noteId = (String) fromMessage.get("id"); @@ -284,14 +280,12 @@ private void sendNote(NotebookSocket conn, Notebook notebook, return; } Note note = notebook.getNote(noteId); - if (note != null) { addConnectionToNote(note.id(), conn); conn.send(serializeMessage(new Message(OP.NOTE).put("note", note))); sendAllAngularObjects(note, conn); } } - private void updateNote(WebSocket conn, Notebook notebook, Message fromMessage) throws SchedulerException, IOException { String noteId = (String) fromMessage.get("id"); @@ -309,17 +303,14 @@ private void updateNote(WebSocket conn, Notebook notebook, Message fromMessage) boolean cronUpdated = isCronUpdated(config, note.getConfig()); note.setName(name); note.setConfig(config); - if (cronUpdated) { notebook.refreshCron(note.id()); } note.persist(); - broadcastNote(note); broadcastNoteList(); } } - private boolean isCronUpdated(Map configA, Map configB) { boolean cronUpdated = false; @@ -333,20 +324,13 @@ private boolean isCronUpdated(Map configA, } return cronUpdated; } - - private void createNote(WebSocket conn, Notebook notebook, Message message) throws IOException { + private void createNote(WebSocket conn, Notebook notebook) throws IOException { Note note = notebook.createNote(); note.addParagraph(); // it's an empty note. so add one paragraph - if (message != null) { - String noteName = (String) message.get("name"); - if (noteName != null && !noteName.isEmpty()) - note.setName(noteName); - } note.persist(); broadcastNote(note); broadcastNoteList(); } - private void removeNote(WebSocket conn, Notebook notebook, Message fromMessage) throws IOException { String noteId = (String) fromMessage.get("id"); @@ -358,7 +342,6 @@ private void removeNote(WebSocket conn, Notebook notebook, Message fromMessage) removeNote(noteId); broadcastNoteList(); } - private void updateParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String paragraphId = (String) fromMessage.get("id"); @@ -378,7 +361,6 @@ private void updateParagraph(NotebookSocket conn, Notebook notebook, note.persist(); broadcast(note.id(), new Message(OP.PARAGRAPH).put("paragraph", p)); } - private void removeParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); @@ -393,31 +375,27 @@ private void removeParagraph(NotebookSocket conn, Notebook notebook, broadcastNote(note); } } - private void completion(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String paragraphId = (String) fromMessage.get("id"); String buffer = (String) fromMessage.get("buf"); int cursor = (int) Double.parseDouble(fromMessage.get("cursor").toString()); Message resp = new Message(OP.COMPLETION_LIST).put("id", paragraphId); - if (paragraphId == null) { conn.send(serializeMessage(resp)); return; } - final Note note = notebook.getNote(getOpenNoteId(conn)); List candidates = note.completion(paragraphId, buffer, cursor); resp.put("completions", candidates); conn.send(serializeMessage(resp)); } - /** * When angular object updated from client - * - * @param conn - * @param notebook - * @param fromMessage + * + * @param conn the web socket. + * @param notebook the notebook. + * @param fromMessage the message. */ private void angularObjectUpdated(WebSocket conn, Notebook notebook, Message fromMessage) { @@ -425,10 +403,8 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, String interpreterGroupId = (String) fromMessage.get("interpreterGroupId"); String varName = (String) fromMessage.get("name"); Object varValue = fromMessage.get("value"); - AngularObject ao = null; boolean global = false; - // propagate change to (Remote) AngularObjectRegistry Note note = notebook.getNote(noteId); if (note != null) { @@ -438,11 +414,9 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, if (setting.getInterpreterGroup() == null) { continue; } - if (interpreterGroupId.equals(setting.getInterpreterGroup().getId())) { AngularObjectRegistry angularObjectRegistry = setting .getInterpreterGroup().getAngularObjectRegistry(); - // first trying to get local registry ao = angularObjectRegistry.get(varName, noteId); if (ao == null) { @@ -460,14 +434,12 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, ao.set(varValue, false); global = false; } - break; } } } - if (global) { // broadcast change to all web session that uses related - // interpreter. + // interpreter. for (Note n : notebook.getAllNotes()) { List settings = note.getNoteReplLoader() .getInterpreterSettings(); @@ -475,7 +447,6 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, if (setting.getInterpreterGroup() == null) { continue; } - if (interpreterGroupId.equals(setting.getInterpreterGroup().getId())) { AngularObjectRegistry angularObjectRegistry = setting .getInterpreterGroup().getAngularObjectRegistry(); @@ -495,14 +466,12 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, .put("noteId", note.id())); } } - private void moveParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } - final int newIndex = (int) Double.parseDouble(fromMessage.get("index") .toString()); final Note note = notebook.getNote(getOpenNoteId(conn)); @@ -510,30 +479,25 @@ private void moveParagraph(NotebookSocket conn, Notebook notebook, note.persist(); broadcastNote(note); } - private void insertParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final int index = (int) Double.parseDouble(fromMessage.get("index") - .toString()); - + .toString()); final Note note = notebook.getNote(getOpenNoteId(conn)); note.insertParagraph(index); note.persist(); broadcastNote(note); } - private void cancelParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } - final Note note = notebook.getNote(getOpenNoteId(conn)); Paragraph p = note.getParagraph(paragraphId); p.abort(); } - private void runParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); @@ -546,12 +510,11 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, p.setText(text); p.setTitle((String) fromMessage.get("title")); Map params = (Map) fromMessage - .get("params"); + .get("params"); p.settings.setParams(params); Map config = (Map) fromMessage - .get("config"); + .get("config"); p.setConfig(config); - // if it's the last paragraph, let's add a new one boolean isTheLastParagraph = note.getLastParagraph().getId() .equals(p.getId()); @@ -560,7 +523,6 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, } note.persist(); broadcastNote(note); - try { note.run(paragraphId); } catch (Exception ex) { @@ -573,7 +535,6 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, } } } - /** * Need description here. * @@ -581,12 +542,10 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, public static class ParagraphJobListener implements JobListener { private NotebookServer notebookServer; private Note note; - public ParagraphJobListener(NotebookServer notebookServer, Note note) { this.notebookServer = notebookServer; this.note = note; } - @Override public void onProgressUpdate(Job job, int progress) { notebookServer.broadcast( @@ -594,11 +553,9 @@ public void onProgressUpdate(Job job, int progress) { new Message(OP.PROGRESS).put("id", job.getId()).put("progress", job.progress())); } - @Override public void beforeStatusChange(Job job, Status before, Status after) { } - @Override public void afterStatusChange(Job job, Status before, Status after) { if (after == Status.ERROR) { @@ -617,22 +574,18 @@ public void afterStatusChange(Job job, Status before, Status after) { notebookServer.broadcastNote(note); } } - @Override public JobListener getParagraphJobListener(Note note) { return new ParagraphJobListener(this, note); } - private void pong() { } - private void sendAllAngularObjects(Note note, NotebookSocket conn) throws IOException { List settings = note.getNoteReplLoader() .getInterpreterSettings(); if (settings == null || settings.size() == 0) { return; } - for (InterpreterSetting intpSetting : settings) { AngularObjectRegistry registry = intpSetting.getInterpreterGroup() .getAngularObjectRegistry(); @@ -646,31 +599,25 @@ private void sendAllAngularObjects(Note note, NotebookSocket conn) throws IOExce } } } - @Override public void onAdd(String interpreterGroupId, AngularObject object) { onUpdate(interpreterGroupId, object); } - @Override public void onUpdate(String interpreterGroupId, AngularObject object) { Notebook notebook = notebook(); if (notebook == null) { return; } - List notes = notebook.getAllNotes(); for (Note note : notes) { if (object.getNoteId() != null && !note.id().equals(object.getNoteId())) { continue; } - List intpSettings = note.getNoteReplLoader() .getInterpreterSettings(); - if (intpSettings.isEmpty()) continue; - for (InterpreterSetting setting : intpSettings) { if (setting.getInterpreterGroup().getId().equals(interpreterGroupId)) { broadcast( @@ -683,7 +630,6 @@ public void onUpdate(String interpreterGroupId, AngularObject object) { } } } - @Override public void onRemove(String interpreterGroupId, String name, String noteId) { Notebook notebook = notebook(); @@ -692,16 +638,19 @@ public void onRemove(String interpreterGroupId, String name, String noteId) { if (noteId != null && !note.id().equals(noteId)) { continue; } - List ids = note.getNoteReplLoader().getInterpreters(); for (String id : ids) { if (id.equals(interpreterGroupId)) { broadcast( note.id(), new Message(OP.ANGULAR_OBJECT_REMOVE).put("name", name).put( - "noteId", noteId)); + "noteId", noteId)); } } } } + private String getOrigin(NotebookSocket conn) { + return conn.getRequest().getHeader("Origin"); + } } + diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java new file mode 100644 index 00000000000..3ab06f0e2c5 --- /dev/null +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java @@ -0,0 +1,53 @@ +/** + * Created by joelz on 8/6/15. + * + * + * 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.zeppelin.socket; + +import org.apache.zeppelin.notebook.Note; +import org.apache.zeppelin.server.ZeppelinServer; +import org.junit.Assert; +import org.junit.FixMethodOrder; +import org.junit.Test; +import org.junit.runners.MethodSorters; + +import java.io.IOException; +import java.net.UnknownHostException; + +/** + * BASIC Zeppelin rest api tests + * + * + * @author joelz + * + */ + public class NotebookServerTests { + + @Test + public void CheckOrigin() throws UnknownHostException { + NotebookServer server = new NotebookServer(); + Assert.assertTrue(server.checkOrigin(new TestHttpServletRequest(), + "http://" + java.net.InetAddress.getLocalHost().getHostName() + ":8080")); + } + + @Test + public void CheckInvalidOrigin(){ + NotebookServer server = new NotebookServer(); + Assert.assertFalse(server.checkOrigin(new TestHttpServletRequest(), "http://evillocalhost:8080")); + } +} diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java new file mode 100644 index 00000000000..9ec54baa95a --- /dev/null +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java @@ -0,0 +1,372 @@ +/** + * Created by joelz on 8/6/15. + * + * + * 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.zeppelin.socket; + +import javax.servlet.*; +import javax.servlet.http.*; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.security.Principal; +import java.util.Collection; +import java.util.Enumeration; +import java.util.Locale; +import java.util.Map; + +/** + * Created by joelz on 8/6/15. + * Helps mocking a http servlet request + */ +public class TestHttpServletRequest implements HttpServletRequest { + @Override + public boolean authenticate(HttpServletResponse httpServletResponse) throws IOException, ServletException { + return false; + } + + @Override + public String getAuthType() { + return null; + } + + @Override + public String getContextPath() { + return null; + } + + @Override + public Cookie[] getCookies() { + return new Cookie[0]; + } + + @Override + public long getDateHeader(String s) { + return 0; + } + + @Override + public String getHeader(String s) { + switch (s) { + case "Origin": + return "http://localhost:8080"; + } + + return null; + } + + @Override + public Enumeration getHeaderNames() { + return null; + } + + @Override + public Enumeration getHeaders(String s) { + return null; + } + + @Override + public int getIntHeader(String s) { + return 0; + } + + @Override + public String getMethod() { + return null; + } + + @Override + public Part getPart(String s) throws IOException, ServletException { + return null; + } + + @Override + public Collection getParts() throws IOException, ServletException { + return null; + } + + @Override + public String getPathInfo() { + return null; + } + + @Override + public String getPathTranslated() { + return null; + } + + @Override + public String getQueryString() { + return null; + } + + @Override + public String getRemoteUser() { + return null; + } + + @Override + public String getRequestedSessionId() { + return null; + } + + @Override + public String getRequestURI() { + return null; + } + + @Override + public StringBuffer getRequestURL() { + return null; + } + + @Override + public String getServletPath() { + return null; + } + + @Override + public HttpSession getSession() { + return null; + } + + @Override + public HttpSession getSession(boolean b) { + return null; + } + + @Override + public Principal getUserPrincipal() { + return null; + } + + @Override + public boolean isRequestedSessionIdFromCookie() { + return false; + } + + @Override + public boolean isRequestedSessionIdFromUrl() { + return false; + } + + @Override + public boolean isRequestedSessionIdFromURL() { + return false; + } + + @Override + public boolean isRequestedSessionIdValid() { + return false; + } + + @Override + public boolean isUserInRole(String s) { + return false; + } + + @Override + public void login(String s, String s1) throws ServletException { + + } + + @Override + public void logout() throws ServletException { + + } + + @Override + public AsyncContext getAsyncContext() { + return null; + } + + @Override + public Object getAttribute(String s) { + return null; + } + + @Override + public Enumeration getAttributeNames() { + return null; + } + + @Override + public String getCharacterEncoding() { + return null; + } + + @Override + public int getContentLength() { + return 0; + } + + @Override + public String getContentType() { + return null; + } + + @Override + public DispatcherType getDispatcherType() { + return null; + } + + @Override + public ServletInputStream getInputStream() throws IOException { + return null; + } + + @Override + public String getLocalAddr() { + return null; + } + + @Override + public Locale getLocale() { + return null; + } + + @Override + public Enumeration getLocales() { + return null; + } + + @Override + public String getLocalName() { + return null; + } + + @Override + public int getLocalPort() { + return 0; + } + + @Override + public String getParameter(String s) { + return null; + } + + @Override + public Map getParameterMap() { + return null; + } + + @Override + public Enumeration getParameterNames() { + return null; + } + + @Override + public String[] getParameterValues(String s) { + return new String[0]; + } + + @Override + public String getProtocol() { + return null; + } + + @Override + public BufferedReader getReader() throws IOException { + return null; + } + + @Override + public String getRealPath(String s) { + return null; + } + + @Override + public String getRemoteAddr() { + return null; + } + + @Override + public String getRemoteHost() { + return null; + } + + @Override + public int getRemotePort() { + return 0; + } + + @Override + public RequestDispatcher getRequestDispatcher(String s) { + return null; + } + + @Override + public String getScheme() { + return null; + } + + @Override + public String getServerName() { + return null; + } + + @Override + public int getServerPort() { + return 0; + } + + @Override + public ServletContext getServletContext() { + return null; + } + + @Override + public boolean isAsyncStarted() { + return false; + } + + @Override + public boolean isAsyncSupported() { + return false; + } + + @Override + public boolean isSecure() { + return false; + } + + @Override + public void removeAttribute(String s) { + + } + + @Override + public void setAttribute(String s, Object o) { + + } + + @Override + public void setCharacterEncoding(String s) throws UnsupportedEncodingException { + + } + + @Override + public AsyncContext startAsync() { + return null; + } + + @Override + public AsyncContext startAsync(ServletRequest servletRequest, ServletResponse servletResponse) { + return null; + } +} diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java new file mode 100644 index 00000000000..8d0637cdbce --- /dev/null +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java @@ -0,0 +1,41 @@ +/** + * Created by joelz on 8/6/15. + * + * + * 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.zeppelin.socket; + +/** + * Created by joelz on 8/6/15. + * This enables mocking a socket listener. + */ +public class TestNotebookSocketListener implements NotebookSocketListener { + @Override + public void onClose(NotebookSocket socket, int code, String message) { + + } + + @Override + public void onOpen(NotebookSocket socket) { + + } + + @Override + public void onMessage(NotebookSocket socket, String message) { + + } +} From 013f22da77e44ee9285a6997d8c4976ca178be98 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 13 Aug 2015 11:21:31 -0700 Subject: [PATCH 02/19] Fixing issue with ZEPPELIN-173: Zeppelin websocket server is vulnerable to Cross-Site WebSocket Hijacking --- .../zeppelin/socket/NotebookServer.java | 71 +++++++++++++++++-- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java index 2f11e8a5e53..8c8b60089a2 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java @@ -89,6 +89,7 @@ public boolean checkOrigin(HttpServletRequest request, String origin) { public WebSocket doWebSocketConnect(HttpServletRequest req, String protocol) { return new NotebookSocket(req, protocol, this); } + @Override public void onOpen(NotebookSocket conn) { LOG.info("New connection from {} : {}", conn.getRequest().getRemoteAddr(), @@ -97,6 +98,7 @@ public void onOpen(NotebookSocket conn) { connectedSockets.add(conn); } } + @Override public void onMessage(NotebookSocket conn, String msg) { Notebook notebook = notebook(); @@ -112,7 +114,7 @@ public void onMessage(NotebookSocket conn, String msg) { sendNote(conn, notebook, messagereceived); break; case NEW_NOTE: - createNote(conn, notebook); + createNote(conn, notebook, messagereceived); break; case DEL_NOTE: removeNote(conn, notebook, messagereceived); @@ -155,6 +157,7 @@ public void onMessage(NotebookSocket conn, String msg) { LOG.error("Can't handle message", e); } } + @Override public void onClose(NotebookSocket conn, int code, String reason) { LOG.info("Closed connection to {} : {}. ({}) {}", conn.getRequest() @@ -164,12 +167,15 @@ public void onClose(NotebookSocket conn, int code, String reason) { connectedSockets.remove(conn); } } + private Message deserializeMessage(String msg) { return gson.fromJson(msg, Message.class); } + private String serializeMessage(Message m) { return gson.toJson(m); } + private void addConnectionToNote(String noteId, NotebookSocket socket) { synchronized (noteSocketMap) { removeConnectionFromAllNote(socket); // make sure a socket relates only a @@ -184,6 +190,7 @@ private void addConnectionToNote(String noteId, NotebookSocket socket) { } } } + private void removeConnectionFromNote(String noteId, NotebookSocket socket) { synchronized (noteSocketMap) { List socketList = noteSocketMap.get(noteId); @@ -192,11 +199,13 @@ private void removeConnectionFromNote(String noteId, NotebookSocket socket) { } } } + private void removeNote(String noteId) { synchronized (noteSocketMap) { List socketList = noteSocketMap.remove(noteId); } } + private void removeConnectionFromAllNote(NotebookSocket socket) { synchronized (noteSocketMap) { Set keys = noteSocketMap.keySet(); @@ -205,6 +214,7 @@ private void removeConnectionFromAllNote(NotebookSocket socket) { } } } + private String getOpenNoteId(NotebookSocket socket) { String id = null; synchronized (noteSocketMap) { @@ -216,8 +226,10 @@ private String getOpenNoteId(NotebookSocket socket) { } } } + return id; } + private void broadcastToNoteBindedInterpreter(String interpreterGroupId, Message m) { Notebook notebook = notebook(); @@ -231,6 +243,7 @@ private void broadcastToNoteBindedInterpreter(String interpreterGroupId, } } } + private void broadcast(String noteId, Message m) { synchronized (noteSocketMap) { List socketLists = noteSocketMap.get(noteId); @@ -247,6 +260,7 @@ private void broadcast(String noteId, Message m) { } } } + private void broadcastAll(Message m) { synchronized (connectedSockets) { for (NotebookSocket conn : connectedSockets) { @@ -258,9 +272,11 @@ private void broadcastAll(Message m) { } } } + private void broadcastNote(Note note) { broadcast(note.id(), new Message(OP.NOTE).put("note", note)); } + private void broadcastNoteList() { Notebook notebook = notebook(); List notes = notebook.getAllNotes(); @@ -271,14 +287,17 @@ private void broadcastNoteList() { info.put("name", note.getName()); notesInfo.add(info); } + broadcastAll(new Message(OP.NOTES_INFO).put("notes", notesInfo)); } + private void sendNote(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String noteId = (String) fromMessage.get("id"); if (noteId == null) { return; } + Note note = notebook.getNote(noteId); if (note != null) { addConnectionToNote(note.id(), conn); @@ -286,6 +305,7 @@ private void sendNote(NotebookSocket conn, Notebook notebook, sendAllAngularObjects(note, conn); } } + private void updateNote(WebSocket conn, Notebook notebook, Message fromMessage) throws SchedulerException, IOException { String noteId = (String) fromMessage.get("id"); @@ -298,6 +318,7 @@ private void updateNote(WebSocket conn, Notebook notebook, Message fromMessage) if (config == null) { return; } + Note note = notebook.getNote(noteId); if (note != null) { boolean cronUpdated = isCronUpdated(config, note.getConfig()); @@ -306,11 +327,13 @@ private void updateNote(WebSocket conn, Notebook notebook, Message fromMessage) if (cronUpdated) { notebook.refreshCron(note.id()); } + note.persist(); broadcastNote(note); broadcastNoteList(); } } + private boolean isCronUpdated(Map configA, Map configB) { boolean cronUpdated = false; @@ -322,32 +345,44 @@ private boolean isCronUpdated(Map configA, } else if (configA.get("cron") != null || configB.get("cron") != null) { cronUpdated = true; } + return cronUpdated; } - private void createNote(WebSocket conn, Notebook notebook) throws IOException { + private void createNote(WebSocket conn, Notebook notebook, Message message) throws IOException { Note note = notebook.createNote(); note.addParagraph(); // it's an empty note. so add one paragraph + if (message != null) { + String noteName = (String) message.get("name"); + if (noteName != null && !noteName.isEmpty()){ + note.setName(noteName); + } + } + note.persist(); broadcastNote(note); broadcastNoteList(); } + private void removeNote(WebSocket conn, Notebook notebook, Message fromMessage) throws IOException { String noteId = (String) fromMessage.get("id"); if (noteId == null) { return; } + Note note = notebook.getNote(noteId); notebook.removeNote(noteId); removeNote(noteId); broadcastNoteList(); } + private void updateParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } + Map params = (Map) fromMessage .get("params"); Map config = (Map) fromMessage @@ -361,12 +396,14 @@ private void updateParagraph(NotebookSocket conn, Notebook notebook, note.persist(); broadcast(note.id(), new Message(OP.PARAGRAPH).put("paragraph", p)); } + private void removeParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } + final Note note = notebook.getNote(getOpenNoteId(conn)); /** We dont want to remove the last paragraph */ if (!note.isLastParagraph(paragraphId)) { @@ -375,6 +412,7 @@ private void removeParagraph(NotebookSocket conn, Notebook notebook, broadcastNote(note); } } + private void completion(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { String paragraphId = (String) fromMessage.get("id"); @@ -385,11 +423,13 @@ private void completion(NotebookSocket conn, Notebook notebook, conn.send(serializeMessage(resp)); return; } + final Note note = notebook.getNote(getOpenNoteId(conn)); List candidates = note.completion(paragraphId, buffer, cursor); resp.put("completions", candidates); conn.send(serializeMessage(resp)); } + /** * When angular object updated from client * @@ -438,6 +478,7 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, } } } + if (global) { // broadcast change to all web session that uses related // interpreter. for (Note n : notebook.getAllNotes()) { @@ -466,12 +507,14 @@ private void angularObjectUpdated(WebSocket conn, Notebook notebook, .put("noteId", note.id())); } } + private void moveParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } + final int newIndex = (int) Double.parseDouble(fromMessage.get("index") .toString()); final Note note = notebook.getNote(getOpenNoteId(conn)); @@ -479,6 +522,7 @@ private void moveParagraph(NotebookSocket conn, Notebook notebook, note.persist(); broadcastNote(note); } + private void insertParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final int index = (int) Double.parseDouble(fromMessage.get("index") @@ -488,22 +532,26 @@ private void insertParagraph(NotebookSocket conn, Notebook notebook, note.persist(); broadcastNote(note); } + private void cancelParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } + final Note note = notebook.getNote(getOpenNoteId(conn)); Paragraph p = note.getParagraph(paragraphId); p.abort(); } + private void runParagraph(NotebookSocket conn, Notebook notebook, Message fromMessage) throws IOException { final String paragraphId = (String) fromMessage.get("id"); if (paragraphId == null) { return; } + final Note note = notebook.getNote(getOpenNoteId(conn)); Paragraph p = note.getParagraph(paragraphId); String text = (String) fromMessage.get("paragraph"); @@ -521,6 +569,7 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, if (!Strings.isNullOrEmpty(text) && isTheLastParagraph) { note.addParagraph(); } + note.persist(); broadcastNote(note); try { @@ -535,6 +584,7 @@ private void runParagraph(NotebookSocket conn, Notebook notebook, } } } + /** * Need description here. * @@ -546,6 +596,7 @@ public ParagraphJobListener(NotebookServer notebookServer, Note note) { this.notebookServer = notebookServer; this.note = note; } + @Override public void onProgressUpdate(Job job, int progress) { notebookServer.broadcast( @@ -553,9 +604,11 @@ public void onProgressUpdate(Job job, int progress) { new Message(OP.PROGRESS).put("id", job.getId()).put("progress", job.progress())); } + @Override public void beforeStatusChange(Job job, Status before, Status after) { } + @Override public void afterStatusChange(Job job, Status before, Status after) { if (after == Status.ERROR) { @@ -563,6 +616,7 @@ public void afterStatusChange(Job job, Status before, Status after) { LOG.error("Error", job.getException()); } } + if (job.isTerminated()) { LOG.info("Job {} is finished", job.getId()); try { @@ -571,21 +625,25 @@ public void afterStatusChange(Job job, Status before, Status after) { e.printStackTrace(); } } + notebookServer.broadcastNote(note); } } + @Override public JobListener getParagraphJobListener(Note note) { return new ParagraphJobListener(this, note); } private void pong() { } + private void sendAllAngularObjects(Note note, NotebookSocket conn) throws IOException { List settings = note.getNoteReplLoader() .getInterpreterSettings(); if (settings == null || settings.size() == 0) { return; } + for (InterpreterSetting intpSetting : settings) { AngularObjectRegistry registry = intpSetting.getInterpreterGroup() .getAngularObjectRegistry(); @@ -599,21 +657,25 @@ private void sendAllAngularObjects(Note note, NotebookSocket conn) throws IOExce } } } + @Override public void onAdd(String interpreterGroupId, AngularObject object) { onUpdate(interpreterGroupId, object); } + @Override public void onUpdate(String interpreterGroupId, AngularObject object) { Notebook notebook = notebook(); if (notebook == null) { return; } + List notes = notebook.getAllNotes(); for (Note note : notes) { if (object.getNoteId() != null && !note.id().equals(object.getNoteId())) { continue; } + List intpSettings = note.getNoteReplLoader() .getInterpreterSettings(); if (intpSettings.isEmpty()) @@ -630,6 +692,7 @@ public void onUpdate(String interpreterGroupId, AngularObject object) { } } } + @Override public void onRemove(String interpreterGroupId, String name, String noteId) { Notebook notebook = notebook(); @@ -638,6 +701,7 @@ public void onRemove(String interpreterGroupId, String name, String noteId) { if (noteId != null && !note.id().equals(noteId)) { continue; } + List ids = note.getNoteReplLoader().getInterpreters(); for (String id : ids) { if (id.equals(interpreterGroupId)) { @@ -649,8 +713,5 @@ public void onRemove(String interpreterGroupId, String name, String noteId) { } } } - private String getOrigin(NotebookSocket conn) { - return conn.getRequest().getHeader("Origin"); - } } From 08ff36956f9c641949bfe2ee9c982a536863555e Mon Sep 17 00:00:00 2001 From: djoelz Date: Thu, 13 Aug 2015 11:31:15 -0700 Subject: [PATCH 03/19] unecessary file --- .../socket/TestNotebookSocketListener.java | 41 ------------------- 1 file changed, 41 deletions(-) delete mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java deleted file mode 100644 index 8d0637cdbce..00000000000 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java +++ /dev/null @@ -1,41 +0,0 @@ -/** - * Created by joelz on 8/6/15. - * - * - * 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.zeppelin.socket; - -/** - * Created by joelz on 8/6/15. - * This enables mocking a socket listener. - */ -public class TestNotebookSocketListener implements NotebookSocketListener { - @Override - public void onClose(NotebookSocket socket, int code, String message) { - - } - - @Override - public void onOpen(NotebookSocket socket) { - - } - - @Override - public void onMessage(NotebookSocket socket, String message) { - - } -} From 61e857d078043b80bbbd7c8ecbc42728c0e737e0 Mon Sep 17 00:00:00 2001 From: joelz Date: Fri, 14 Aug 2015 17:00:26 -0700 Subject: [PATCH 04/19] Fixing Rest request lack of Origin validation bug, Added tests that use Mockito (unit test framework) and forces the servlet to use version 3.0 instead of 2.5 --- .../org/apache/zeppelin/server/CorsFilter.java | 16 ++++++++++++---- .../zeppelin/socket/NotebookServerTests.java | 7 ++----- .../zeppelin/socket/TestHttpServletRequest.java | 2 +- zeppelin-web/src/WEB-INF/web.xml | 12 +++++++++--- 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java index 1524d5b8987..c2a137f169d 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java @@ -18,6 +18,7 @@ package org.apache.zeppelin.server; import java.io.IOException; +import java.net.URI; import java.text.DateFormat; import java.util.Date; import java.util.Locale; @@ -40,21 +41,28 @@ public class CorsFilter implements Filter { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws IOException, ServletException { + String sourceHost = request.getServerName(); + String currentHost = java.net.InetAddress.getLocalHost().getHostName(); + String origin = ""; + if (currentHost.equals(sourceHost) || "localhost".equals(sourceHost)) { + origin = ((HttpServletRequest) request).getHeader("Origin"); + } + if (((HttpServletRequest) request).getMethod().equals("OPTIONS")) { HttpServletResponse resp = ((HttpServletResponse) response); - addCorsHeaders(resp); + addCorsHeaders(resp, origin); return; } if (response instanceof HttpServletResponse) { HttpServletResponse alteredResponse = ((HttpServletResponse) response); - addCorsHeaders(alteredResponse); + addCorsHeaders(alteredResponse, origin); } filterChain.doFilter(request, response); } - private void addCorsHeaders(HttpServletResponse response) { - response.addHeader("Access-Control-Allow-Origin", "*"); + private void addCorsHeaders(HttpServletResponse response, String origin) { + response.addHeader("Access-Control-Allow-Origin", origin); response.addHeader("Access-Control-Allow-Credentials", "true"); response.addHeader("Access-Control-Allow-Headers", "authorization,Content-Type"); response.addHeader("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, HEAD, DELETE"); diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java index 3ab06f0e2c5..c262593fdaf 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java @@ -19,14 +19,9 @@ */ package org.apache.zeppelin.socket; -import org.apache.zeppelin.notebook.Note; -import org.apache.zeppelin.server.ZeppelinServer; import org.junit.Assert; -import org.junit.FixMethodOrder; import org.junit.Test; -import org.junit.runners.MethodSorters; -import java.io.IOException; import java.net.UnknownHostException; /** @@ -50,4 +45,6 @@ public void CheckInvalidOrigin(){ NotebookServer server = new NotebookServer(); Assert.assertFalse(server.checkOrigin(new TestHttpServletRequest(), "http://evillocalhost:8080")); } + + } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java index 9ec54baa95a..0a176baa4e3 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java @@ -317,7 +317,7 @@ public String getScheme() { @Override public String getServerName() { - return null; + return "localhost"; } @Override diff --git a/zeppelin-web/src/WEB-INF/web.xml b/zeppelin-web/src/WEB-INF/web.xml index f34da18eb5a..08c263966eb 100644 --- a/zeppelin-web/src/WEB-INF/web.xml +++ b/zeppelin-web/src/WEB-INF/web.xml @@ -17,8 +17,8 @@ --> + xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_3_0.xsd" + version="3.0"> zeppelin-web @@ -30,7 +30,6 @@ 1 - default @@ -41,4 +40,11 @@ configuration deployment + + + + true + true + + From cecbab8fbb82fde0138d8bff11e16c2b220b0c16 Mon Sep 17 00:00:00 2001 From: joelz Date: Sun, 16 Aug 2015 03:08:38 -0700 Subject: [PATCH 05/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- zeppelin-server/pom.xml | 5 +++++ .../org/apache/zeppelin/server/CorsFilter.java | 16 ++++++++++++---- .../zeppelin/socket/TestHttpServletRequest.java | 2 +- zeppelin-web/src/WEB-INF/web.xml | 12 +++++++++--- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/zeppelin-server/pom.xml b/zeppelin-server/pom.xml index cc6df976357..9f908fe213e 100644 --- a/zeppelin-server/pom.xml +++ b/zeppelin-server/pom.xml @@ -315,6 +315,11 @@ test + + org.mockito + mockito-all + 1.9.0 + diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java index 1524d5b8987..c2a137f169d 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java @@ -18,6 +18,7 @@ package org.apache.zeppelin.server; import java.io.IOException; +import java.net.URI; import java.text.DateFormat; import java.util.Date; import java.util.Locale; @@ -40,21 +41,28 @@ public class CorsFilter implements Filter { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws IOException, ServletException { + String sourceHost = request.getServerName(); + String currentHost = java.net.InetAddress.getLocalHost().getHostName(); + String origin = ""; + if (currentHost.equals(sourceHost) || "localhost".equals(sourceHost)) { + origin = ((HttpServletRequest) request).getHeader("Origin"); + } + if (((HttpServletRequest) request).getMethod().equals("OPTIONS")) { HttpServletResponse resp = ((HttpServletResponse) response); - addCorsHeaders(resp); + addCorsHeaders(resp, origin); return; } if (response instanceof HttpServletResponse) { HttpServletResponse alteredResponse = ((HttpServletResponse) response); - addCorsHeaders(alteredResponse); + addCorsHeaders(alteredResponse, origin); } filterChain.doFilter(request, response); } - private void addCorsHeaders(HttpServletResponse response) { - response.addHeader("Access-Control-Allow-Origin", "*"); + private void addCorsHeaders(HttpServletResponse response, String origin) { + response.addHeader("Access-Control-Allow-Origin", origin); response.addHeader("Access-Control-Allow-Credentials", "true"); response.addHeader("Access-Control-Allow-Headers", "authorization,Content-Type"); response.addHeader("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, HEAD, DELETE"); diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java index 9ec54baa95a..0a176baa4e3 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestHttpServletRequest.java @@ -317,7 +317,7 @@ public String getScheme() { @Override public String getServerName() { - return null; + return "localhost"; } @Override diff --git a/zeppelin-web/src/WEB-INF/web.xml b/zeppelin-web/src/WEB-INF/web.xml index f34da18eb5a..08c263966eb 100644 --- a/zeppelin-web/src/WEB-INF/web.xml +++ b/zeppelin-web/src/WEB-INF/web.xml @@ -17,8 +17,8 @@ --> + xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_3_0.xsd" + version="3.0"> zeppelin-web @@ -30,7 +30,6 @@ 1 - default @@ -41,4 +40,11 @@ configuration deployment + + + + true + true + + From df324dea2ff81017c2b9a306b082cdcea5b80ecb Mon Sep 17 00:00:00 2001 From: joelz Date: Sun, 16 Aug 2015 03:10:28 -0700 Subject: [PATCH 06/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../zeppelin/server/CorsFilterTests.java | 95 +++++++++++++++++++ .../zeppelin/socket/NotebookServerTests.java | 7 +- 2 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java new file mode 100644 index 00000000000..3c9152d35cc --- /dev/null +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java @@ -0,0 +1,95 @@ +/** + * Created by joelz on 8/6/15. + * + * + * 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.zeppelin.server; + +import org.apache.zeppelin.socket.TestHttpServletRequest; +import org.junit.Assert; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; + +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.*; + +/** + * BASIC Cors rest api tests + * + * + * @author joelz + * + */ +public class CorsFilterTests { + + public static String[] headers = new String[8]; + public static Integer count = 0; + + @Test + public void ValidCorsFilterTest() throws IOException, ServletException { + CorsFilter filter = new CorsFilter(); + HttpServletResponse mockResponse = mock(HttpServletResponse.class); + FilterChain mockedFilterChain = mock(FilterChain.class); + TestHttpServletRequest mockRequest = mock(TestHttpServletRequest.class); + when(mockRequest.getHeader("Origin")).thenReturn("http://localhost:8080"); + when(mockRequest.getMethod()).thenReturn("Empty"); + when(mockRequest.getServerName()).thenReturn("localhost"); + + + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocationOnMock) throws Throwable { + headers[count] = invocationOnMock.getArguments()[1].toString(); + count++; + return null; + } + }).when(mockResponse).addHeader(anyString(), anyString()); + + filter.doFilter(mockRequest, mockResponse, mockedFilterChain); + Assert.assertTrue(headers[0].equals("http://localhost:8080")); + } + + @Test + public void InvalidCorsFilterTest() throws IOException, ServletException { + CorsFilter filter = new CorsFilter(); + HttpServletResponse mockResponse = mock(HttpServletResponse.class); + FilterChain mockedFilterChain = mock(FilterChain.class); + TestHttpServletRequest mockRequest = mock(TestHttpServletRequest.class); + when(mockRequest.getHeader("Origin")).thenReturn("http://evillocalhost:8080"); + when(mockRequest.getMethod()).thenReturn("Empty"); + when(mockRequest.getServerName()).thenReturn("evillocalhost"); + + + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocationOnMock) throws Throwable { + headers[count] = invocationOnMock.getArguments()[1].toString(); + count++; + return null; + } + }).when(mockResponse).addHeader(anyString(), anyString()); + + filter.doFilter(mockRequest, mockResponse, mockedFilterChain); + Assert.assertTrue(headers[0].equals("")); + } +} diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java index 3ab06f0e2c5..c262593fdaf 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java @@ -19,14 +19,9 @@ */ package org.apache.zeppelin.socket; -import org.apache.zeppelin.notebook.Note; -import org.apache.zeppelin.server.ZeppelinServer; import org.junit.Assert; -import org.junit.FixMethodOrder; import org.junit.Test; -import org.junit.runners.MethodSorters; -import java.io.IOException; import java.net.UnknownHostException; /** @@ -50,4 +45,6 @@ public void CheckInvalidOrigin(){ NotebookServer server = new NotebookServer(); Assert.assertFalse(server.checkOrigin(new TestHttpServletRequest(), "http://evillocalhost:8080")); } + + } From 47902a6c7d6cfac6d0f5e8932600ae9f6f9bee95 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 07:36:24 -0700 Subject: [PATCH 07/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../apache/zeppelin/server/CorsFilter.java | 17 +- .../zeppelin/socket/NotebookServer.java | 14 +- .../apache/zeppelin/utils/SecurityUtils.java | 41 +++++ .../zeppelin/security/SecurityUtilsTests.java | 50 ++++++ .../src/test/resources/zeppelin-site.xml | 163 ++++++++++++++++++ .../zeppelin/conf/ZeppelinConfiguration.java | 30 +++- .../conf/ZeppelinConfigurationTests.java | 73 ++++++++ .../test/resources/test-zeppelin-site1.xml | 163 ++++++++++++++++++ .../test/resources/test-zeppelin-site2.xml | 163 ++++++++++++++++++ .../src/test/resources/zeppelin-site.xml | 158 +++++++++++++++++ 10 files changed, 850 insertions(+), 22 deletions(-) create mode 100644 zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java create mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java create mode 100644 zeppelin-server/src/test/resources/zeppelin-site.xml create mode 100644 zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java create mode 100644 zeppelin-zengine/src/test/resources/test-zeppelin-site1.xml create mode 100644 zeppelin-zengine/src/test/resources/test-zeppelin-site2.xml create mode 100644 zeppelin-zengine/src/test/resources/zeppelin-site.xml diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java index c2a137f169d..0e39242acd8 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/server/CorsFilter.java @@ -17,9 +17,14 @@ package org.apache.zeppelin.server; +import org.apache.zeppelin.conf.ZeppelinConfiguration; +import org.apache.zeppelin.utils.SecurityUtils; + import java.io.IOException; import java.net.URI; +import java.net.URISyntaxException; import java.text.DateFormat; +import java.util.Arrays; import java.util.Date; import java.util.Locale; @@ -41,11 +46,15 @@ public class CorsFilter implements Filter { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws IOException, ServletException { - String sourceHost = request.getServerName(); - String currentHost = java.net.InetAddress.getLocalHost().getHostName(); + String sourceHost = ((HttpServletRequest) request).getHeader("Origin"); String origin = ""; - if (currentHost.equals(sourceHost) || "localhost".equals(sourceHost)) { - origin = ((HttpServletRequest) request).getHeader("Origin"); + + try { + if (SecurityUtils.isValidOrigin(sourceHost, ZeppelinConfiguration.create())) { + origin = sourceHost; + } + } catch (URISyntaxException e) { + e.printStackTrace(); } if (((HttpServletRequest) request).getMethod().equals("OPTIONS")) { diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java index 8a02b5334ef..036a548061f 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java @@ -42,6 +42,7 @@ import org.apache.zeppelin.scheduler.Job.Status; import org.apache.zeppelin.server.ZeppelinServer; import org.apache.zeppelin.socket.Message.OP; +import org.apache.zeppelin.utils.SecurityUtils; import org.eclipse.jetty.websocket.WebSocket; import org.eclipse.jetty.websocket.WebSocketServlet; import org.quartz.SchedulerException; @@ -67,24 +68,15 @@ private Notebook notebook() { } @Override public boolean checkOrigin(HttpServletRequest request, String origin) { - URI sourceUri = null; - String currentHost = null; try { - sourceUri = new URI(origin); - currentHost = java.net.InetAddress.getLocalHost().getHostName(); + return SecurityUtils.isValidOrigin(origin, ZeppelinConfiguration.create()); } catch (UnknownHostException e) { e.printStackTrace(); - } - catch (URISyntaxException e) { + } catch (URISyntaxException e) { e.printStackTrace(); } - String sourceHost = sourceUri.getHost(); - if (currentHost.equals(sourceHost) || "localhost".equals(sourceHost)) { - return true; - } - return false; } diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java new file mode 100644 index 00000000000..9de407f0a7c --- /dev/null +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java @@ -0,0 +1,41 @@ +/* + * 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.zeppelin.utils; + +import org.apache.zeppelin.conf.ZeppelinConfiguration; + +import java.net.URI; +import java.net.URISyntaxException; +import java.net.UnknownHostException; + +/** + * Created by joelz on 8/19/15. + */ +public class SecurityUtils { + public static Boolean isValidOrigin(String sourceHost, ZeppelinConfiguration conf) + throws UnknownHostException, URISyntaxException { + URI sourceHostUri = new URI(sourceHost.toLowerCase()); + String currentHost = java.net.InetAddress.getLocalHost().getHostName().toLowerCase(); + if (currentHost.equals(sourceHostUri.getHost()) || + "localhost".equals(sourceHostUri.getHost()) || + conf.getAllowedOrigins().contains(sourceHost)) { + return true; + } + + return false; + } +} diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java new file mode 100644 index 00000000000..50d59eda4ba --- /dev/null +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java @@ -0,0 +1,50 @@ +/* + * 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.zeppelin.security; + +import junit.framework.Assert; +import org.apache.commons.configuration.ConfigurationException; +import org.apache.zeppelin.conf.ZeppelinConfiguration; +import org.apache.zeppelin.utils.SecurityUtils; +import org.junit.Test; + +import java.net.URISyntaxException; +import java.net.UnknownHostException; + +/** + * Created by joelz on 8/19/15. + */ +public class SecurityUtilsTests { + @Test + public void isLocalhost() throws URISyntaxException, UnknownHostException { + Assert.assertTrue(SecurityUtils.isValidOrigin("http://localhost", ZeppelinConfiguration.create())); + } + + @Test + public void isLocalMachine() throws URISyntaxException, UnknownHostException { + Assert.assertTrue(SecurityUtils.isValidOrigin( + "http://" + java.net.InetAddress.getLocalHost().getHostName(), + ZeppelinConfiguration.create())); + } + + @Test + public void isValidFromConfig() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertTrue( + SecurityUtils.isValidOrigin("http://otherhost.com", + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); + } +} diff --git a/zeppelin-server/src/test/resources/zeppelin-site.xml b/zeppelin-server/src/test/resources/zeppelin-site.xml new file mode 100644 index 00000000000..ae5cea1deaa --- /dev/null +++ b/zeppelin-server/src/test/resources/zeppelin-site.xml @@ -0,0 +1,163 @@ + + + + + + + + zeppelin.server.addr + 0.0.0.0 + Server address + + + + zeppelin.server.port + 8080 + Server port. + + + + zeppelin.notebook.dir + notebook + path or URI for notebook persist + + + + zeppelin.notebook.homescreen + + id of notebook to be displayed in homescreen. ex) 2A94M5J1Z Empty value displays default home screen + + + + zeppelin.notebook.homescreen.hide + false + hide homescreen notebook from list when this value set to true + + + + + + + + zeppelin.notebook.storage + org.apache.zeppelin.notebook.repo.VFSNotebookRepo + notebook persistence layer implementation + + + + zeppelin.interpreter.dir + interpreter + Interpreter implementation base directory + + + + zeppelin.interpreters + org.apache.zeppelin.spark.SparkInterpreter,org.apache.zeppelin.spark.PySparkInterpreter,org.apache.zeppelin.spark.SparkSqlInterpreter,org.apache.zeppelin.spark.DepInterpreter,org.apache.zeppelin.markdown.Markdown,org.apache.zeppelin.angular.AngularInterpreter,org.apache.zeppelin.shell.ShellInterpreter,org.apache.zeppelin.hive.HiveInterpreter,org.apache.zeppelin.tajo.TajoInterpreter,org.apache.zeppelin.flink.FlinkInterpreter,org.apache.zeppelin.lens.LensInterpreter,org.apache.zeppelin.ignite.IgniteInterpreter,org.apache.zeppelin.ignite.IgniteSqlInterpreter,org.apache.zeppelin.cassandra.CassandraInterpreter,org.apache.zeppelin.geode.GeodeOqlInterpreter,org.apache.zeppelin.postgresql.PostgreSqlInterpreter + Comma separated interpreter configurations. First interpreter become a default + + + + zeppelin.interpreter.connect.timeout + 30000 + Interpreter process connect timeout in msec. + + + + + zeppelin.ssl + false + Should SSL be used by the servers? + + + + zeppelin.ssl.client.auth + false + Should client authentication be used for SSL connections? + + + + zeppelin.ssl.keystore.path + keystore + Path to keystore relative to Zeppelin configuration directory + + + + zeppelin.ssl.keystore.type + JKS + The format of the given keystore (e.g. JKS or PKCS12) + + + + zeppelin.ssl.keystore.password + change me + Keystore password. Can be obfuscated by the Jetty Password tool + + + + + + zeppelin.ssl.truststore.path + truststore + Path to truststore relative to Zeppelin configuration directory. Defaults to the keystore path + + + + zeppelin.ssl.truststore.type + JKS + The format of the given truststore (e.g. JKS or PKCS12). Defaults to the same type as the keystore type + + + + zeppelin.server.allowed.origins + http://onehost:8080,http://otherhost.com, + Allowed sources for REST and WebSocket requests. + + + + + diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java index 6fda2b2c600..fe548b969d6 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java @@ -18,7 +18,7 @@ package org.apache.zeppelin.conf; import java.net.URL; -import java.util.List; +import java.util.*; import org.apache.commons.configuration.ConfigurationException; import org.apache.commons.configuration.XMLConfiguration; @@ -69,7 +69,7 @@ public ZeppelinConfiguration() { /** * Load from resource. - * + *url = ZeppelinConfiguration.class.getResource(ZEPPELIN_SITE_XML); * @throws ConfigurationException */ public static ZeppelinConfiguration create() { @@ -270,9 +270,9 @@ public int getServerPort() { public String getKeyStorePath() { return getRelativeDir( - String.format("%s/%s", - getConfDir(), - getString(ConfVars.ZEPPELIN_SSL_KEYSTORE_PATH))); + String.format("%s/%s", + getConfDir(), + getString(ConfVars.ZEPPELIN_SSL_KEYSTORE_PATH))); } public String getKeyStoreType() { @@ -348,17 +348,30 @@ public String getRelativeDir(ConfVars c) { } public String getRelativeDir(String path) { - if (path != null && path.startsWith("/")) { + if (path != null && path.startsWith("/") || isWindowsPath(path)) { return path; } else { return getString(ConfVars.ZEPPELIN_HOME) + "/" + path; } } + public boolean isWindowsPath(String path){ + return path.matches("^[A-Za-z]:\\\\.*"); + } + public String getConfDir() { return getString(ConfVars.ZEPPELIN_CONF_DIR); } + public List getAllowedOrigins() + { + if (getString(ConfVars.ZEPPELIN_ALLOWED_ORIGINS).isEmpty()) { + return Arrays.asList(new String[0]); + } + + return Arrays.asList(getString(ConfVars.ZEPPELIN_ALLOWED_ORIGINS).toLowerCase().split(",")); + } + /** * Wrapper class. @@ -411,7 +424,10 @@ public static enum ConfVars { ZEPPELIN_INTERPRETER_REMOTE_RUNNER("zeppelin.interpreter.remoterunner", "bin/interpreter.sh"), // Decide when new note is created, interpreter settings will be binded automatically or not. ZEPPELIN_NOTEBOOK_AUTO_INTERPRETER_BINDING("zeppelin.notebook.autoInterpreterBinding", true), - ZEPPELIN_CONF_DIR("zeppelin.conf.dir", "conf"); + ZEPPELIN_CONF_DIR("zeppelin.conf.dir", "conf"), + // Allows a way to specify a ',' separated list of allowed origins for rest and websockets + // i.e. http://localhost:8080 + ZEPPELIN_ALLOWED_ORIGINS("zeppelin.server.allowed.origins", ""); private String varName; @SuppressWarnings("rawtypes") diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java new file mode 100644 index 00000000000..478f4b4937f --- /dev/null +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java @@ -0,0 +1,73 @@ +/* + * 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.zeppelin.conf; + +import junit.framework.Assert; +import org.apache.commons.configuration.ConfigurationException; +import org.junit.Test; + +import java.net.MalformedURLException; +import java.util.List; + + +/** + * Created by joelz on 8/19/15. + */ +public class ZeppelinConfigurationTests { + @Test + public void getAllowedOrigins2Test() throws MalformedURLException, ConfigurationException { + + ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/test-zeppelin-site2.xml")); + List origins = conf.getAllowedOrigins(); + Assert.assertEquals(2, origins.size()); + Assert.assertEquals("http://onehost:8080", origins.get(0)); + Assert.assertEquals("http://otherhost.com", origins.get(1)); + } + + @Test + public void getAllowedOrigins1Test() throws MalformedURLException, ConfigurationException { + + ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/test-zeppelin-site1.xml")); + List origins = conf.getAllowedOrigins(); + Assert.assertEquals(1, origins.size()); + Assert.assertEquals("http://onehost:8080", origins.get(0)); + } + + @Test + public void getAllowedOriginsNoneTest() throws MalformedURLException, ConfigurationException { + + ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")); + List origins = conf.getAllowedOrigins(); + Assert.assertEquals(0, origins.size()); + } + + @Test + public void isWindowsPathTestTrue() throws ConfigurationException { + + ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")); + Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt"); + Assert.assertTrue(isIt); + } + + @Test + public void isWindowsPathTestFalse() throws ConfigurationException { + + ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")); + Boolean isIt = conf.isWindowsPath("~/test/file.xml"); + Assert.assertFalse(isIt); + } +} diff --git a/zeppelin-zengine/src/test/resources/test-zeppelin-site1.xml b/zeppelin-zengine/src/test/resources/test-zeppelin-site1.xml new file mode 100644 index 00000000000..cf83b541e05 --- /dev/null +++ b/zeppelin-zengine/src/test/resources/test-zeppelin-site1.xml @@ -0,0 +1,163 @@ + + + + + + + + zeppelin.server.addr + 0.0.0.0 + Server address + + + + zeppelin.server.port + 8080 + Server port. + + + + zeppelin.notebook.dir + notebook + path or URI for notebook persist + + + + zeppelin.notebook.homescreen + + id of notebook to be displayed in homescreen. ex) 2A94M5J1Z Empty value displays default home screen + + + + zeppelin.notebook.homescreen.hide + false + hide homescreen notebook from list when this value set to true + + + + + + + + zeppelin.notebook.storage + org.apache.zeppelin.notebook.repo.VFSNotebookRepo + notebook persistence layer implementation + + + + zeppelin.interpreter.dir + interpreter + Interpreter implementation base directory + + + + zeppelin.interpreters + org.apache.zeppelin.spark.SparkInterpreter,org.apache.zeppelin.spark.PySparkInterpreter,org.apache.zeppelin.spark.SparkSqlInterpreter,org.apache.zeppelin.spark.DepInterpreter,org.apache.zeppelin.markdown.Markdown,org.apache.zeppelin.angular.AngularInterpreter,org.apache.zeppelin.shell.ShellInterpreter,org.apache.zeppelin.hive.HiveInterpreter,org.apache.zeppelin.tajo.TajoInterpreter,org.apache.zeppelin.flink.FlinkInterpreter,org.apache.zeppelin.lens.LensInterpreter,org.apache.zeppelin.ignite.IgniteInterpreter,org.apache.zeppelin.ignite.IgniteSqlInterpreter,org.apache.zeppelin.cassandra.CassandraInterpreter,org.apache.zeppelin.geode.GeodeOqlInterpreter,org.apache.zeppelin.postgresql.PostgreSqlInterpreter + Comma separated interpreter configurations. First interpreter become a default + + + + zeppelin.interpreter.connect.timeout + 30000 + Interpreter process connect timeout in msec. + + + + + zeppelin.ssl + false + Should SSL be used by the servers? + + + + zeppelin.ssl.client.auth + false + Should client authentication be used for SSL connections? + + + + zeppelin.ssl.keystore.path + keystore + Path to keystore relative to Zeppelin configuration directory + + + + zeppelin.ssl.keystore.type + JKS + The format of the given keystore (e.g. JKS or PKCS12) + + + + zeppelin.ssl.keystore.password + change me + Keystore password. Can be obfuscated by the Jetty Password tool + + + + + + zeppelin.ssl.truststore.path + truststore + Path to truststore relative to Zeppelin configuration directory. Defaults to the keystore path + + + + zeppelin.ssl.truststore.type + JKS + The format of the given truststore (e.g. JKS or PKCS12). Defaults to the same type as the keystore type + + + + zeppelin.server.allowed.origins + http://onehost:8080 + Allowed sources for REST and WebSocket requests. + + + + + diff --git a/zeppelin-zengine/src/test/resources/test-zeppelin-site2.xml b/zeppelin-zengine/src/test/resources/test-zeppelin-site2.xml new file mode 100644 index 00000000000..ae5cea1deaa --- /dev/null +++ b/zeppelin-zengine/src/test/resources/test-zeppelin-site2.xml @@ -0,0 +1,163 @@ + + + + + + + + zeppelin.server.addr + 0.0.0.0 + Server address + + + + zeppelin.server.port + 8080 + Server port. + + + + zeppelin.notebook.dir + notebook + path or URI for notebook persist + + + + zeppelin.notebook.homescreen + + id of notebook to be displayed in homescreen. ex) 2A94M5J1Z Empty value displays default home screen + + + + zeppelin.notebook.homescreen.hide + false + hide homescreen notebook from list when this value set to true + + + + + + + + zeppelin.notebook.storage + org.apache.zeppelin.notebook.repo.VFSNotebookRepo + notebook persistence layer implementation + + + + zeppelin.interpreter.dir + interpreter + Interpreter implementation base directory + + + + zeppelin.interpreters + org.apache.zeppelin.spark.SparkInterpreter,org.apache.zeppelin.spark.PySparkInterpreter,org.apache.zeppelin.spark.SparkSqlInterpreter,org.apache.zeppelin.spark.DepInterpreter,org.apache.zeppelin.markdown.Markdown,org.apache.zeppelin.angular.AngularInterpreter,org.apache.zeppelin.shell.ShellInterpreter,org.apache.zeppelin.hive.HiveInterpreter,org.apache.zeppelin.tajo.TajoInterpreter,org.apache.zeppelin.flink.FlinkInterpreter,org.apache.zeppelin.lens.LensInterpreter,org.apache.zeppelin.ignite.IgniteInterpreter,org.apache.zeppelin.ignite.IgniteSqlInterpreter,org.apache.zeppelin.cassandra.CassandraInterpreter,org.apache.zeppelin.geode.GeodeOqlInterpreter,org.apache.zeppelin.postgresql.PostgreSqlInterpreter + Comma separated interpreter configurations. First interpreter become a default + + + + zeppelin.interpreter.connect.timeout + 30000 + Interpreter process connect timeout in msec. + + + + + zeppelin.ssl + false + Should SSL be used by the servers? + + + + zeppelin.ssl.client.auth + false + Should client authentication be used for SSL connections? + + + + zeppelin.ssl.keystore.path + keystore + Path to keystore relative to Zeppelin configuration directory + + + + zeppelin.ssl.keystore.type + JKS + The format of the given keystore (e.g. JKS or PKCS12) + + + + zeppelin.ssl.keystore.password + change me + Keystore password. Can be obfuscated by the Jetty Password tool + + + + + + zeppelin.ssl.truststore.path + truststore + Path to truststore relative to Zeppelin configuration directory. Defaults to the keystore path + + + + zeppelin.ssl.truststore.type + JKS + The format of the given truststore (e.g. JKS or PKCS12). Defaults to the same type as the keystore type + + + + zeppelin.server.allowed.origins + http://onehost:8080,http://otherhost.com, + Allowed sources for REST and WebSocket requests. + + + + + diff --git a/zeppelin-zengine/src/test/resources/zeppelin-site.xml b/zeppelin-zengine/src/test/resources/zeppelin-site.xml new file mode 100644 index 00000000000..57d1b23b2ce --- /dev/null +++ b/zeppelin-zengine/src/test/resources/zeppelin-site.xml @@ -0,0 +1,158 @@ + + + + + + + + zeppelin.server.addr + 0.0.0.0 + Server address + + + + zeppelin.server.port + 8080 + Server port. + + + + zeppelin.notebook.dir + notebook + path or URI for notebook persist + + + + zeppelin.notebook.homescreen + + id of notebook to be displayed in homescreen. ex) 2A94M5J1Z Empty value displays default home screen + + + + zeppelin.notebook.homescreen.hide + false + hide homescreen notebook from list when this value set to true + + + + + + + + zeppelin.notebook.storage + org.apache.zeppelin.notebook.repo.VFSNotebookRepo + notebook persistence layer implementation + + + + zeppelin.interpreter.dir + interpreter + Interpreter implementation base directory + + + + zeppelin.interpreters + org.apache.zeppelin.spark.SparkInterpreter,org.apache.zeppelin.spark.PySparkInterpreter,org.apache.zeppelin.spark.SparkSqlInterpreter,org.apache.zeppelin.spark.DepInterpreter,org.apache.zeppelin.markdown.Markdown,org.apache.zeppelin.angular.AngularInterpreter,org.apache.zeppelin.shell.ShellInterpreter,org.apache.zeppelin.hive.HiveInterpreter,org.apache.zeppelin.tajo.TajoInterpreter,org.apache.zeppelin.flink.FlinkInterpreter,org.apache.zeppelin.lens.LensInterpreter,org.apache.zeppelin.ignite.IgniteInterpreter,org.apache.zeppelin.ignite.IgniteSqlInterpreter,org.apache.zeppelin.cassandra.CassandraInterpreter,org.apache.zeppelin.geode.GeodeOqlInterpreter,org.apache.zeppelin.postgresql.PostgreSqlInterpreter + Comma separated interpreter configurations. First interpreter become a default + + + + zeppelin.interpreter.connect.timeout + 30000 + Interpreter process connect timeout in msec. + + + + + zeppelin.ssl + false + Should SSL be used by the servers? + + + + zeppelin.ssl.client.auth + false + Should client authentication be used for SSL connections? + + + + zeppelin.ssl.keystore.path + keystore + Path to keystore relative to Zeppelin configuration directory + + + + zeppelin.ssl.keystore.type + JKS + The format of the given keystore (e.g. JKS or PKCS12) + + + + zeppelin.ssl.keystore.password + change me + Keystore password. Can be obfuscated by the Jetty Password tool + + + + + + zeppelin.ssl.truststore.path + truststore + Path to truststore relative to Zeppelin configuration directory. Defaults to the keystore path + + + + zeppelin.ssl.truststore.type + JKS + The format of the given truststore (e.g. JKS or PKCS12). Defaults to the same type as the keystore type + + + + + + From 1f851c07b4368e059727cc6dc500a0c7d7981be4 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 11:24:53 -0700 Subject: [PATCH 08/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- conf/zeppelin-site.xml.template | 6 + .../zeppelin/socket/NotebookServer.java | 1 - .../apache/zeppelin/utils/SecurityUtils.java | 3 +- .../zeppelin/security/SecurityUtilsTests.java | 7 + .../zeppelin/server/CorsFilterTests.java | 2 +- .../src/test/resources/zeppelin-site-star.xml | 163 ++++++++++++++++++ 6 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 zeppelin-server/src/test/resources/zeppelin-site-star.xml diff --git a/conf/zeppelin-site.xml.template b/conf/zeppelin-site.xml.template index 57d1b23b2ce..5d3b97d2116 100644 --- a/conf/zeppelin-site.xml.template +++ b/conf/zeppelin-site.xml.template @@ -154,5 +154,11 @@ --> + + zeppelin.server.allowed.origins + * + Allowed sources for REST and WebSocket requests (i.e. http://onehost:8080,http://otherhost.com). If you leave * you are vulnerable to https://issues.apache.org/jira/browse/ZEPPELIN-173 + + diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java index 1cddc04c053..df9c0369302 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java @@ -325,7 +325,6 @@ private void sendHomeNote(NotebookSocket conn, Notebook notebook) throws IOExcep note = notebook.getNote(noteId); } - Note note = notebook.getNote(noteId); if (note != null) { addConnectionToNote(note.id(), conn); conn.send(serializeMessage(new Message(OP.NOTE).put("note", note))); diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java index 9de407f0a7c..fa245a76ee1 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java @@ -32,7 +32,8 @@ public static Boolean isValidOrigin(String sourceHost, ZeppelinConfiguration con String currentHost = java.net.InetAddress.getLocalHost().getHostName().toLowerCase(); if (currentHost.equals(sourceHostUri.getHost()) || "localhost".equals(sourceHostUri.getHost()) || - conf.getAllowedOrigins().contains(sourceHost)) { + conf.getAllowedOrigins().contains(sourceHost) || + conf.getAllowedOrigins().contains("*")) { return true; } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java index 50d59eda4ba..9518582ef9d 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java @@ -47,4 +47,11 @@ public void isValidFromConfig() throws URISyntaxException, UnknownHostException, SecurityUtils.isValidOrigin("http://otherhost.com", new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); } + + @Test + public void isValidFromStar() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertTrue( + SecurityUtils.isValidOrigin("http://anyhost.com", + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site-star.xml")))); + } } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java index 3c9152d35cc..02da90203bb 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java @@ -54,7 +54,7 @@ public void ValidCorsFilterTest() throws IOException, ServletException { when(mockRequest.getHeader("Origin")).thenReturn("http://localhost:8080"); when(mockRequest.getMethod()).thenReturn("Empty"); when(mockRequest.getServerName()).thenReturn("localhost"); - + count = 0; doAnswer(new Answer() { @Override diff --git a/zeppelin-server/src/test/resources/zeppelin-site-star.xml b/zeppelin-server/src/test/resources/zeppelin-site-star.xml new file mode 100644 index 00000000000..8f3f4c10683 --- /dev/null +++ b/zeppelin-server/src/test/resources/zeppelin-site-star.xml @@ -0,0 +1,163 @@ + + + + + + + + zeppelin.server.addr + 0.0.0.0 + Server address + + + + zeppelin.server.port + 8080 + Server port. + + + + zeppelin.notebook.dir + notebook + path or URI for notebook persist + + + + zeppelin.notebook.homescreen + + id of notebook to be displayed in homescreen. ex) 2A94M5J1Z Empty value displays default home screen + + + + zeppelin.notebook.homescreen.hide + false + hide homescreen notebook from list when this value set to true + + + + + + + + zeppelin.notebook.storage + org.apache.zeppelin.notebook.repo.VFSNotebookRepo + notebook persistence layer implementation + + + + zeppelin.interpreter.dir + interpreter + Interpreter implementation base directory + + + + zeppelin.interpreters + org.apache.zeppelin.spark.SparkInterpreter,org.apache.zeppelin.spark.PySparkInterpreter,org.apache.zeppelin.spark.SparkSqlInterpreter,org.apache.zeppelin.spark.DepInterpreter,org.apache.zeppelin.markdown.Markdown,org.apache.zeppelin.angular.AngularInterpreter,org.apache.zeppelin.shell.ShellInterpreter,org.apache.zeppelin.hive.HiveInterpreter,org.apache.zeppelin.tajo.TajoInterpreter,org.apache.zeppelin.flink.FlinkInterpreter,org.apache.zeppelin.lens.LensInterpreter,org.apache.zeppelin.ignite.IgniteInterpreter,org.apache.zeppelin.ignite.IgniteSqlInterpreter,org.apache.zeppelin.cassandra.CassandraInterpreter,org.apache.zeppelin.geode.GeodeOqlInterpreter,org.apache.zeppelin.postgresql.PostgreSqlInterpreter + Comma separated interpreter configurations. First interpreter become a default + + + + zeppelin.interpreter.connect.timeout + 30000 + Interpreter process connect timeout in msec. + + + + + zeppelin.ssl + false + Should SSL be used by the servers? + + + + zeppelin.ssl.client.auth + false + Should client authentication be used for SSL connections? + + + + zeppelin.ssl.keystore.path + keystore + Path to keystore relative to Zeppelin configuration directory + + + + zeppelin.ssl.keystore.type + JKS + The format of the given keystore (e.g. JKS or PKCS12) + + + + zeppelin.ssl.keystore.password + change me + Keystore password. Can be obfuscated by the Jetty Password tool + + + + + + zeppelin.ssl.truststore.path + truststore + Path to truststore relative to Zeppelin configuration directory. Defaults to the keystore path + + + + zeppelin.ssl.truststore.type + JKS + The format of the given truststore (e.g. JKS or PKCS12). Defaults to the same type as the keystore type + + + + zeppelin.server.allowed.origins + * + Allowed sources for REST and WebSocket requests. + + + + + From 3d6ce2eebfcd20b2a8ac23f38bb8818b1978cbc2 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 11:31:28 -0700 Subject: [PATCH 09/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../apache/zeppelin/security/SecurityUtilsTests.java | 12 ++++++++++++ .../apache/zeppelin/conf/ZeppelinConfiguration.java | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java index 9518582ef9d..131c8d048ce 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java @@ -29,6 +29,18 @@ * Created by joelz on 8/19/15. */ public class SecurityUtilsTests { + @Test + public void isInvalid() throws URISyntaxException, UnknownHostException { + Assert.assertFalse(SecurityUtils.isValidOrigin("http://127.0.1.1", ZeppelinConfiguration.create())); + } + + @Test + public void isInvalidFromConfig() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertFalse( + SecurityUtils.isValidOrigin("http://otherinvalidhost.com", + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); + } + @Test public void isLocalhost() throws URISyntaxException, UnknownHostException { Assert.assertTrue(SecurityUtils.isValidOrigin("http://localhost", ZeppelinConfiguration.create())); diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java index fe548b969d6..2fdaee17e1c 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java @@ -427,7 +427,7 @@ public static enum ConfVars { ZEPPELIN_CONF_DIR("zeppelin.conf.dir", "conf"), // Allows a way to specify a ',' separated list of allowed origins for rest and websockets // i.e. http://localhost:8080 - ZEPPELIN_ALLOWED_ORIGINS("zeppelin.server.allowed.origins", ""); + ZEPPELIN_ALLOWED_ORIGINS("zeppelin.server.allowed.origins", "*"); private String varName; @SuppressWarnings("rawtypes") From bcb1ac15575366f61bd26b5dda335eaa935d5727 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 11:34:22 -0700 Subject: [PATCH 10/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../socket/TestNotebookSocketListener.java | 41 ------------------- 1 file changed, 41 deletions(-) delete mode 100644 zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java deleted file mode 100644 index 8d0637cdbce..00000000000 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/TestNotebookSocketListener.java +++ /dev/null @@ -1,41 +0,0 @@ -/** - * Created by joelz on 8/6/15. - * - * - * 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.zeppelin.socket; - -/** - * Created by joelz on 8/6/15. - * This enables mocking a socket listener. - */ -public class TestNotebookSocketListener implements NotebookSocketListener { - @Override - public void onClose(NotebookSocket socket, int code, String message) { - - } - - @Override - public void onOpen(NotebookSocket socket) { - - } - - @Override - public void onMessage(NotebookSocket socket, String message) { - - } -} From 3795de71e868f99afff9f535ccf5367c7eac2eed Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 11:59:12 -0700 Subject: [PATCH 11/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../test/java/org/apache/zeppelin/server/CorsFilterTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java index 02da90203bb..68af681b4fd 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java @@ -79,7 +79,6 @@ public void InvalidCorsFilterTest() throws IOException, ServletException { when(mockRequest.getMethod()).thenReturn("Empty"); when(mockRequest.getServerName()).thenReturn("evillocalhost"); - doAnswer(new Answer() { @Override public Object answer(InvocationOnMock invocationOnMock) throws Throwable { From 4ae9129709517545e5c45756474f579409a65d07 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 12:47:47 -0700 Subject: [PATCH 12/19] Fixing null reference --- .../java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java index 0da5e0de19e..4d526253ed1 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java @@ -104,9 +104,10 @@ public void testInterpreterAutoBinding() throws IOException { // create note Note note = ZeppelinServer.notebook.createNote(); - // check interpreter is bindded + // check interpreter is binded GetMethod get = httpGet("/notebook/interpreter/bind/"+note.id()); assertThat(get, isAllowed()); + get.addRequestHeader("Origin", "http://localhost"); Map resp = gson.fromJson(get.getResponseBodyAsString(), new TypeToken>(){}.getType()); List> body = (List>) resp.get("body"); assertTrue(0 < body.size()); From b2b418a62de7c91260b3ea0c5953c972e812b4fc Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 12:48:12 -0700 Subject: [PATCH 13/19] Fixing cross origin bug for rest calls that allow a malicious user to issue requests from a site other than the zeppelin server. Adding unit tests and a dependency to mockito to the server project (please comment if that is ok or if there is another preferred mocking framework). Also upgrading the servelet version from 2.5 to 3.0 as this also fixes a security vulnerability with respect to httonly cookies. --- .../apache/zeppelin/utils/SecurityUtils.java | 6 +++++- .../zeppelin/security/SecurityUtilsTests.java | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java index fa245a76ee1..e3a4ce30ba9 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java @@ -28,7 +28,11 @@ public class SecurityUtils { public static Boolean isValidOrigin(String sourceHost, ZeppelinConfiguration conf) throws UnknownHostException, URISyntaxException { - URI sourceHostUri = new URI(sourceHost.toLowerCase()); + if(sourceHost == null){ + return false; + } + + URI sourceHostUri = new URI(sourceHost); String currentHost = java.net.InetAddress.getLocalHost().getHostName().toLowerCase(); if (currentHost.equals(sourceHostUri.getHost()) || "localhost".equals(sourceHostUri.getHost()) || diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java index 131c8d048ce..de3163c0ef9 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java @@ -66,4 +66,25 @@ public void isValidFromStar() throws URISyntaxException, UnknownHostException, C SecurityUtils.isValidOrigin("http://anyhost.com", new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site-star.xml")))); } + + @Test + public void nullOrigin() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertFalse( + SecurityUtils.isValidOrigin(null, + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); + } + + @Test + public void emptyOrigin() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertFalse( + SecurityUtils.isValidOrigin("", + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); + } + + @Test + public void notAURIOrigin() throws URISyntaxException, UnknownHostException, ConfigurationException { + Assert.assertFalse( + SecurityUtils.isValidOrigin("test123", + new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); + } } From b7bb7bf904673ab72073711f67edeb9b8676b78e Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 14:36:01 -0700 Subject: [PATCH 14/19] Fixing Styling --- .../main/java/org/apache/zeppelin/utils/SecurityUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java index e3a4ce30ba9..004f10d84d3 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/utils/SecurityUtils.java @@ -28,8 +28,8 @@ public class SecurityUtils { public static Boolean isValidOrigin(String sourceHost, ZeppelinConfiguration conf) throws UnknownHostException, URISyntaxException { - if(sourceHost == null){ - return false; + if (sourceHost == null){ + return false; } URI sourceHostUri = new URI(sourceHost); From 9260d5da8988171c4d0a9fcc1ee937a9e6b99679 Mon Sep 17 00:00:00 2001 From: joelz Date: Thu, 20 Aug 2015 14:47:08 -0700 Subject: [PATCH 15/19] Fixing adding the origin header for get and post tests. --- .../java/org/apache/zeppelin/rest/AbstractTestRestApi.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java index 393dc7bcabc..aa395aaeb2b 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java @@ -227,7 +227,8 @@ protected static GetMethod httpGet(String path) throws IOException { LOG.info("Connecting to {}", url + path); HttpClient httpClient = new HttpClient(); GetMethod getMethod = new GetMethod(url + path); - httpClient.executeMethod(getMethod); + getMethod.addRequestHeader("Origin", "http://localhost:8080"); + httpClient.executeMethod(getMethod); LOG.info("{} - {}", getMethod.getStatusCode(), getMethod.getStatusText()); return getMethod; } @@ -236,6 +237,7 @@ protected static PostMethod httpPost(String path, String body) throws IOExceptio LOG.info("Connecting to {}", url + path); HttpClient httpClient = new HttpClient(); PostMethod postMethod = new PostMethod(url + path); + postMethod.addRequestHeader("Origin", "http://localhost:8080"); RequestEntity entity = new ByteArrayRequestEntity(body.getBytes("UTF-8")); postMethod.setRequestEntity(entity); httpClient.executeMethod(postMethod); From 2887f0d46538958b3d4209f8d66eb53588f2bec7 Mon Sep 17 00:00:00 2001 From: joelz Date: Fri, 21 Aug 2015 10:25:46 -0700 Subject: [PATCH 16/19] Renaming tests to singular name so plugin can detect and run --- .../{SecurityUtilsTests.java => SecurityUtilsTest.java} | 2 +- .../server/{CorsFilterTests.java => CorsFilterTest.java} | 2 +- .../{NotebookServerTests.java => NotebookServerTest.java} | 2 +- ...inConfigurationTests.java => ZeppelinConfigurationTest.java} | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename zeppelin-server/src/test/java/org/apache/zeppelin/security/{SecurityUtilsTests.java => SecurityUtilsTest.java} (99%) rename zeppelin-server/src/test/java/org/apache/zeppelin/server/{CorsFilterTests.java => CorsFilterTest.java} (99%) rename zeppelin-server/src/test/java/org/apache/zeppelin/socket/{NotebookServerTests.java => NotebookServerTest.java} (97%) rename zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/{ZeppelinConfigurationTests.java => ZeppelinConfigurationTest.java} (98%) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java similarity index 99% rename from zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java rename to zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java index de3163c0ef9..505b049fd78 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java @@ -28,7 +28,7 @@ /** * Created by joelz on 8/19/15. */ -public class SecurityUtilsTests { +public class SecurityUtilsTest { @Test public void isInvalid() throws URISyntaxException, UnknownHostException { Assert.assertFalse(SecurityUtils.isValidOrigin("http://127.0.1.1", ZeppelinConfiguration.create())); diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTest.java similarity index 99% rename from zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java rename to zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTest.java index 68af681b4fd..a3057c83ed2 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/server/CorsFilterTest.java @@ -40,7 +40,7 @@ * @author joelz * */ -public class CorsFilterTests { +public class CorsFilterTest { public static String[] headers = new String[8]; public static Integer count = 0; diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java similarity index 97% rename from zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java rename to zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java index b7a8dfc8f51..23c4d78230b 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTests.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java @@ -35,7 +35,7 @@ * @author joelz * */ - public class NotebookServerTests { + public class NotebookServerTest { @Test public void CheckOrigin() throws UnknownHostException { diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java similarity index 98% rename from zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java rename to zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java index 478f4b4937f..cf703cb3a89 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTests.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java @@ -27,7 +27,7 @@ /** * Created by joelz on 8/19/15. */ -public class ZeppelinConfigurationTests { +public class ZeppelinConfigurationTest { @Test public void getAllowedOrigins2Test() throws MalformedURLException, ConfigurationException { From e9d8384eefa2ba43d493b483b50f3062467a0617 Mon Sep 17 00:00:00 2001 From: joelz Date: Fri, 21 Aug 2015 10:55:48 -0700 Subject: [PATCH 17/19] Retrying due to git download issue with build --- .../java/org/apache/zeppelin/security/SecurityUtilsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java index 505b049fd78..df9f80368e5 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/security/SecurityUtilsTest.java @@ -87,4 +87,4 @@ public void notAURIOrigin() throws URISyntaxException, UnknownHostException, Con SecurityUtils.isValidOrigin("test123", new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")))); } -} +} \ No newline at end of file From 625b54e99e86cb65c8a7bfc381d4d4a598aaf74f Mon Sep 17 00:00:00 2001 From: joelz Date: Fri, 21 Aug 2015 11:34:39 -0700 Subject: [PATCH 18/19] Fixing unit test that reads from a file but initializes to a default value and hence the configuration is present. --- .../org/apache/zeppelin/conf/ZeppelinConfigurationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java index cf703cb3a89..dc13eb02134 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java @@ -52,7 +52,7 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException, Configurat ZeppelinConfiguration conf = new ZeppelinConfiguration(this.getClass().getResource("/zeppelin-site.xml")); List origins = conf.getAllowedOrigins(); - Assert.assertEquals(0, origins.size()); + Assert.assertEquals(1, origins.size()); } @Test From 989f1e06ed9adb0a9da42e4998d97baff606463f Mon Sep 17 00:00:00 2001 From: joelz Date: Fri, 21 Aug 2015 12:47:13 -0700 Subject: [PATCH 19/19] Retrying build as it seems ZeppelinIT failed for not reason. --- .../java/org/apache/zeppelin/socket/NotebookServerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java index 23c4d78230b..34cd4111e6d 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java @@ -49,4 +49,4 @@ public void CheckInvalidOrigin(){ NotebookServer server = new NotebookServer(); Assert.assertFalse(server.checkOrigin(new TestHttpServletRequest(), "http://evillocalhost:8080")); } -} +} \ No newline at end of file