Mercurial > hg > release > thermostat-0.11
changeset 1118:d329d22447f4
Add minimal authorization to command channel actions.
Reviewed-by: vanaltj, ebaron
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-May/006610.html
PR1434
line wrap: on
line diff
--- a/agent/command/src/main/java/com/redhat/thermostat/agent/command/internal/ServerHandler.java Tue May 28 11:23:34 2013 -0400 +++ b/agent/command/src/main/java/com/redhat/thermostat/agent/command/internal/ServerHandler.java Tue May 14 16:34:29 2013 +0200 @@ -130,7 +130,13 @@ ServiceReference storageRef = bCtx.getServiceReference(Storage.class.getName()); Storage storage = (Storage) bCtx.getService(storageRef); if (storage instanceof SecureStorage) { - return authenticateRequest(request, (SecureStorage) storage); + boolean authenticatedRequest = authenticateRequest(request, (SecureStorage) storage); + if (authenticatedRequest) { + logger.finest("Authentication and authorization for request " + request + " succeeded!"); + } else { + logger.finest("Request " + request + " failed to authenticate or authorize"); + } + return authenticatedRequest; } else { return true; } @@ -142,7 +148,15 @@ String authTokenStr = request.getParameter(Request.AUTH_TOKEN); byte[] authToken = Base64.decodeBase64(authTokenStr); AuthToken token = new AuthToken(authToken, clientToken); - return storage.verifyToken(token); + String actionName = request.getParameter(Request.ACTION); + try { + // actionName must not be null here. If we somehow get a bogus request + // at this point where this does not exist, verifyToken will throw a + // NPE. + return storage.verifyToken(token, actionName); + } catch (NullPointerException e) { + return false; + } } @Override
--- a/client/command/src/main/java/com/redhat/thermostat/client/command/cli/PingCommand.java Tue May 28 11:23:34 2013 -0400 +++ b/client/command/src/main/java/com/redhat/thermostat/client/command/cli/PingCommand.java Tue May 14 16:34:29 2013 +0200 @@ -62,6 +62,8 @@ import com.redhat.thermostat.storage.dao.HostInfoDAO; public class PingCommand extends AbstractCommand { + + private static final String PING_ACTION_NAME = "ping"; private static final Translate<LocaleResources> translator = LocaleResources.createLocalizer(); @@ -81,7 +83,11 @@ case ERROR: out.println(translator.localize(LocaleResources.COMMAND_PING_RESPONSE_ERROR, request.getTarget().toString()).getContents()); break; + case AUTH_FAILED: + out.println(translator.localize(LocaleResources.COMMAND_PING_RESPONSE_AUTH_FAILED, request.getTarget().toString())); + break; case OK: + // fallthrough case NOOP: out.println(translator.localize(LocaleResources.COMMAND_PING_RESPONSE_OK, request.getTarget().toString()).getContents()); break; @@ -139,6 +145,7 @@ String [] host = address.split(":"); InetSocketAddress target = new InetSocketAddress(host[0], Integer.parseInt(host[1])); Request ping = new Request(RequestType.RESPONSE_EXPECTED, target); + ping.setParameter(Request.ACTION, PING_ACTION_NAME); ping.setReceiver("com.redhat.thermostat.agent.command.internal.PingReceiver"); final Semaphore responseBarrier = new Semaphore(0); ping.addListener(new PongListener(out, responseBarrier));
--- a/client/command/src/main/java/com/redhat/thermostat/client/command/internal/LocaleResources.java Tue May 28 11:23:34 2013 -0400 +++ b/client/command/src/main/java/com/redhat/thermostat/client/command/internal/LocaleResources.java Tue May 14 16:34:29 2013 +0200 @@ -53,6 +53,7 @@ COMMAND_PING_RESPONSE_OK, COMMAND_PING_RESPONSE_REFUSED, COMMAND_PING_RESPONSE_UNKNOWN, + COMMAND_PING_RESPONSE_AUTH_FAILED, ; static final String RESOURCE_BUNDLE = "com.redhat.thermostat.client.command.internal.strings";
--- a/client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java Tue May 28 11:23:34 2013 -0400 +++ b/client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java Tue May 14 16:34:29 2013 +0200 @@ -95,7 +95,10 @@ private void authenticateRequest(Request request, SecureStorage storage) { try { - AuthToken token = storage.generateToken(); + String actionName = request.getParameter(Request.ACTION); + // actionName must not be null here. + // This is checked in generateToken. + AuthToken token = storage.generateToken(actionName); request.setParameter(Request.CLIENT_TOKEN, Base64.encodeBase64String(token.getClientToken())); request.setParameter(Request.AUTH_TOKEN, Base64.encodeBase64String(token.getToken())); } catch (StorageException ex) {
--- a/client/command/src/main/resources/com/redhat/thermostat/client/command/internal/strings.properties Tue May 28 11:23:34 2013 -0400 +++ b/client/command/src/main/resources/com/redhat/thermostat/client/command/internal/strings.properties Tue May 14 16:34:29 2013 +0200 @@ -9,4 +9,5 @@ COMMAND_PING_RESPONSE_ERROR = Error received from: {0} COMMAND_PING_RESPONSE_OK = Response received from: {0} COMMAND_PING_RESPONSE_REFUSED = The server refused to PONG our PING? -COMMAND_PING_RESPONSE_UNKNOWN = Unknown result from ping command \ No newline at end of file +COMMAND_PING_RESPONSE_UNKNOWN = Unknown result from ping command +COMMAND_PING_RESPONSE_AUTH_FAILED = The server refused to ping due to authentication/authorization issues. \ No newline at end of file
--- a/client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java Tue May 28 11:23:34 2013 -0400 +++ b/client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java Tue May 14 16:34:29 2013 +0200 @@ -77,7 +77,7 @@ when(mockContext.getServiceReference(Storage.class.getName())).thenReturn(mockServiceRef); SecureStorage mockStorage = mock(SecureStorage.class); AuthToken mockToken = mock(AuthToken.class); - when(mockStorage.generateToken()).thenReturn(mockToken); + when(mockStorage.generateToken(any(String.class))).thenReturn(mockToken); when(mockContext.getService(mockServiceRef)).thenReturn(mockStorage); when(mockBundle.getBundleContext()).thenReturn(mockContext); when(FrameworkUtil.getBundle(RequestQueueImpl.class)).thenReturn(mockBundle);
--- a/common/command/src/main/java/com/redhat/thermostat/common/command/Request.java Tue May 28 11:23:34 2013 -0400 +++ b/common/command/src/main/java/com/redhat/thermostat/common/command/Request.java Tue May 14 16:34:29 2013 +0200 @@ -102,6 +102,7 @@ public static final String CLIENT_TOKEN = "client-token"; public static final String AUTH_TOKEN = "auth-token"; + public static final String ACTION = "action-name"; public Request(RequestType type, InetSocketAddress target) { this.type = type;
--- a/killvm/client-swing/src/main/java/com/redhat/thermostat/killvm/client/internal/KillVMAction.java Tue May 28 11:23:34 2013 -0400 +++ b/killvm/client-swing/src/main/java/com/redhat/thermostat/killvm/client/internal/KillVMAction.java Tue May 14 16:34:29 2013 +0200 @@ -59,6 +59,7 @@ public class KillVMAction implements VMContextAction { private static final String RECEIVER = "com.redhat.thermostat.killvm.agent.internal.KillVmReceiver"; + private static final String CMD_CHANNEL_ACTION_NAME = "killvm"; private final AgentInfoDAO agentDao; private final VmInfoDAO vmDao; private final Translate<LocaleResources> t; @@ -91,6 +92,7 @@ String [] host = address.split(":"); InetSocketAddress target = new InetSocketAddress(host[0], Integer.parseInt(host[1])); Request murderer = getKillRequest(target); + murderer.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); murderer.setParameter("vm-id", reference.getIdString()); murderer.setReceiver(RECEIVER); murderer.addListener(listener);
--- a/killvm/client-swing/src/test/java/com/redhat/thermostat/killvm/client/internal/KillVMActionTest.java Tue May 28 11:23:34 2013 -0400 +++ b/killvm/client-swing/src/test/java/com/redhat/thermostat/killvm/client/internal/KillVMActionTest.java Tue May 14 16:34:29 2013 +0200 @@ -40,6 +40,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -104,6 +105,7 @@ RequestQueue queue = mock(RequestQueue.class); final Request req = mock(Request.class); + when(req.getParameter(Request.ACTION)).thenReturn("killvm"); KillVMAction action = new KillVMAction(agentDao, vmInfoDao, queue, agentResponseListener) { @Override Request getKillRequest(InetSocketAddress target) { @@ -111,10 +113,8 @@ } }; action.execute(ref); - ArgumentCaptor<String> vmIdParamCaptor = ArgumentCaptor - .forClass(String.class); - verify(req).setParameter(vmIdParamCaptor.capture(), any(String.class)); - assertEquals("vm-id", vmIdParamCaptor.getValue()); + verify(req).setParameter(eq("vm-id"), any(String.class)); + verify(req).setParameter(eq(Request.ACTION), any(String.class)); verify(req).addListener(agentResponseListener); ArgumentCaptor<String> receiverCaptor = ArgumentCaptor .forClass(String.class);
--- a/storage/core/src/main/java/com/redhat/thermostat/storage/core/AuthToken.java Tue May 28 11:23:34 2013 -0400 +++ b/storage/core/src/main/java/com/redhat/thermostat/storage/core/AuthToken.java Tue May 14 16:34:29 2013 +0200 @@ -42,8 +42,8 @@ public class AuthToken { - private byte[] token; - private byte[] clientToken; + private final byte[] token; + private final byte[] clientToken; public AuthToken(byte[] token, byte[] clientToken) { this.token = token; @@ -58,6 +58,7 @@ return clientToken; } + @Override public String toString() { return "AuthToken: client-token: " + Arrays.toString(clientToken) + ", token: " + Arrays.toString(token); }
--- a/storage/core/src/main/java/com/redhat/thermostat/storage/core/SecureQueuedStorage.java Tue May 28 11:23:34 2013 -0400 +++ b/storage/core/src/main/java/com/redhat/thermostat/storage/core/SecureQueuedStorage.java Tue May 14 16:34:29 2013 +0200 @@ -49,13 +49,13 @@ super(storage); } @Override - public AuthToken generateToken() throws StorageException { - return ((SecureStorage)delegate).generateToken(); + public AuthToken generateToken(String actionName) throws StorageException { + return ((SecureStorage)delegate).generateToken(actionName); } @Override - public boolean verifyToken(AuthToken token) { - return ((SecureStorage)delegate).verifyToken(token); + public boolean verifyToken(AuthToken token, String actionName) { + return ((SecureStorage)delegate).verifyToken(token, actionName); } }
--- a/storage/core/src/main/java/com/redhat/thermostat/storage/core/SecureStorage.java Tue May 28 11:23:34 2013 -0400 +++ b/storage/core/src/main/java/com/redhat/thermostat/storage/core/SecureStorage.java Tue May 14 16:34:29 2013 +0200 @@ -43,7 +43,7 @@ * The protocol works as follows: * * <ol> - * <li> The client asks the SecureStorage for an authentication token using {@link #generateToken()}. + * <li> The client asks the SecureStorage for an authentication token using {@link #generateToken(String)}. * This should happen over an authenticated and secured connection, thus authenticating the client * in the storage. The returned AuthToken will carry a client-token (that has been generated by the * client) and an auth-token (generated by the storage). If authentication fails at this stage @@ -62,17 +62,30 @@ public interface SecureStorage extends Storage { /** - * Generates a token in the storage that can be used to authenticate cmd channel requests. - * - * @throws StorageException if authentication fails at this point + * Generates a token in the storage that can be used to authenticate cmd + * channel requests. + * + * @param actionName + * A unique name of the type of action to be performed. + * + * @throws StorageException + * if authentication fails at this point */ - AuthToken generateToken() throws StorageException; + AuthToken generateToken(String actionName) throws StorageException; /** - * Verifies the specified token in the storage. + * Verifies the specified token and action in the storage. * - * @return <code>true</code> if authentication succeeded, <code>false</code> otherwise + * @param actionName + * A unique name of the type of action to be performed. This + * action name is used during verification. This means if + * verification succeeds, the given action name can be trusted + * and may be used for authorization checks. + * @param token The token to be verified. + * + * @return <code>true</code> if authentication succeeded, <code>false</code> + * otherwise */ - boolean verifyToken(AuthToken token); + boolean verifyToken(AuthToken token, String actionName); }
--- a/thread/client-common/src/main/java/com/redhat/thermostat/thread/client/common/collector/impl/ThreadMXBeanCollector.java Tue May 28 11:23:34 2013 -0400 +++ b/thread/client-common/src/main/java/com/redhat/thermostat/thread/client/common/collector/impl/ThreadMXBeanCollector.java Tue May 14 16:34:29 2013 +0200 @@ -64,6 +64,7 @@ public class ThreadMXBeanCollector implements ThreadCollector { + private static final String CMD_CHANNEL_ACTION_NAME = "thread-harvester"; private static final Logger logger = LoggingUtils.getLogger(ThreadMXBeanCollector.class); private AgentInfoDAO agentDao; @@ -102,6 +103,7 @@ public boolean startHarvester() { Request harvester = createRequest(); + harvester.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); harvester.setParameter(HarvesterCommand.class.getName(), HarvesterCommand.START.name()); harvester.setParameter(HarvesterCommand.VM_ID.name(), ref.getIdString()); @@ -136,6 +138,7 @@ public boolean stopHarvester() { Request harvester = createRequest(); + harvester.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); harvester.setParameter(HarvesterCommand.class.getName(), HarvesterCommand.STOP.name()); harvester.setParameter(HarvesterCommand.VM_ID.name(), ref.getIdString());
--- a/vm-gc/remote-collector-client-common/src/main/java/com/redhat/thermostat/gc/remote/common/GCRequest.java Tue May 28 11:23:34 2013 -0400 +++ b/vm-gc/remote-collector-client-common/src/main/java/com/redhat/thermostat/gc/remote/common/GCRequest.java Tue May 14 16:34:29 2013 +0200 @@ -40,8 +40,8 @@ import com.redhat.thermostat.client.command.RequestQueue; import com.redhat.thermostat.common.command.Request; +import com.redhat.thermostat.common.command.Request.RequestType; import com.redhat.thermostat.common.command.RequestResponseListener; -import com.redhat.thermostat.common.command.Request.RequestType; import com.redhat.thermostat.gc.remote.common.command.GCCommand; import com.redhat.thermostat.storage.core.HostRef; import com.redhat.thermostat.storage.core.VmRef; @@ -49,6 +49,8 @@ public class GCRequest { + private static final String CMD_CHANNEL_ACTION_NAME = "garbage-collect"; + private RequestQueue queue; public GCRequest(RequestQueue queue) { this.queue = queue; @@ -66,6 +68,7 @@ gcRequest.setReceiver(GCCommand.RECEIVER); + gcRequest.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); gcRequest.setParameter(GCCommand.class.getName(), GCCommand.REQUEST_GC.name()); gcRequest.setParameter(GCCommand.VM_ID, vm.getIdString());
--- a/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapHelper.java Tue May 28 11:23:34 2013 -0400 +++ b/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapHelper.java Tue May 14 16:34:29 2013 +0200 @@ -51,6 +51,7 @@ public class DumpHeapHelper { private static final String RECEIVER_CLASS_NAME = "com.redhat.thermostat.vm.heap.analysis.agent.internal.HeapDumpReceiver"; + private static final String CMD_CHANNEL_ACTION_NAME = "dump-heap"; private static final String VM_ID_PARAM = "vmId"; private class HeapDumpListener implements RequestResponseListener { @@ -88,6 +89,7 @@ InetSocketAddress target = new InetSocketAddress(host[0], Integer.parseInt(host[1])); Request req = new Request(RequestType.RESPONSE_EXPECTED, target); req.setReceiver(RECEIVER_CLASS_NAME); + req.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); req.setParameter(VM_ID_PARAM, reference.getIdString()); req.addListener(new HeapDumpListener(heapDumpSuccessAction, heapDumpFailureAction));
--- a/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java Tue May 28 11:23:34 2013 -0400 +++ b/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java Tue May 14 16:34:29 2013 +0200 @@ -52,6 +52,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.UUID; import java.util.logging.Level; import java.util.logging.Logger; @@ -623,26 +624,31 @@ } @Override - public AuthToken generateToken() throws StorageException { - byte[] token = new byte[256]; - random.nextBytes(token); - NameValuePair clientTokenParam = new BasicNameValuePair("client-token", Base64.encodeBase64String(token)); - List<NameValuePair> formparams = Arrays.asList(clientTokenParam); + public AuthToken generateToken(String actionName) throws StorageException { + byte[] clientToken = new byte[256]; + random.nextBytes(clientToken); + NameValuePair clientTokenParam = new BasicNameValuePair("client-token", Base64.encodeBase64String(clientToken)); + NameValuePair actionNameParam = new BasicNameValuePair("action-name", + Objects.requireNonNull(actionName)); + List<NameValuePair> formparams = Arrays.asList(clientTokenParam, actionNameParam); try (CloseableHttpEntity entity = post(endpoint + "/generate-token", formparams)) { byte[] authToken = EntityUtils.toByteArray(entity); - return new AuthToken(authToken, token); + return new AuthToken(authToken, clientToken); } catch (IOException ex) { throw new StorageException(ex); } } @Override - public boolean verifyToken(AuthToken authToken) { + public boolean verifyToken(AuthToken authToken, String actionName) { byte[] clientToken = authToken.getClientToken(); byte[] token = authToken.getToken(); NameValuePair clientTokenParam = new BasicNameValuePair("client-token", Base64.encodeBase64String(clientToken)); NameValuePair tokenParam = new BasicNameValuePair("token", Base64.encodeBase64String(token)); - List<NameValuePair> formparams = Arrays.asList(clientTokenParam, tokenParam); + NameValuePair actionNameParam = new BasicNameValuePair("action-name", + Objects.requireNonNull(actionName)); + List<NameValuePair> formparams = Arrays.asList(clientTokenParam, + tokenParam, actionNameParam); HttpResponse response = null; try { HttpEntity entity = new UrlEncodedFormEntity(formparams, "UTF-8");
--- a/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java Tue May 28 11:23:34 2013 -0400 +++ b/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java Tue May 14 16:34:29 2013 +0200 @@ -53,6 +53,8 @@ import java.io.StringReader; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.Arrays; import java.util.Enumeration; import java.util.HashMap; @@ -491,7 +493,8 @@ public void testGenerateToken() throws UnsupportedEncodingException { responseBody = "flufftoken"; - AuthToken authToken = storage.generateToken(); + final String actionName = "some action"; + AuthToken authToken = storage.generateToken(actionName); assertTrue(requestURI.endsWith("/generate-token")); assertEquals("POST", method); @@ -502,15 +505,13 @@ byte[] clientToken = Base64.decodeBase64(clientTokenParam); assertEquals(256, clientToken.length); - assertTrue(authToken instanceof AuthToken); - AuthToken token = (AuthToken) authToken; - byte[] tokenBytes = token.getToken(); + byte[] tokenBytes = authToken.getToken(); assertEquals("flufftoken", new String(tokenBytes)); - assertTrue(Arrays.equals(clientToken, token.getClientToken())); + assertTrue(Arrays.equals(clientToken, authToken.getClientToken())); // Send another request and verify that we send a different client-token every time. - storage.generateToken(); + storage.generateToken(actionName); requestParts = requestBody.split("="); assertEquals("client-token", requestParts[0]); @@ -538,37 +539,51 @@ } @Test - public void testVerifyToken() throws UnsupportedEncodingException { + public void testVerifyToken() throws UnsupportedEncodingException, NoSuchAlgorithmException { byte[] token = "stuff".getBytes(); - byte[] clientToken = "fluff".getBytes(); - AuthToken authToken = new AuthToken(token, clientToken); + String clientToken = "fluff"; + String someAction = "someAction"; + byte[] tokenDigest = getShaBytes(clientToken, someAction); + + AuthToken authToken = new AuthToken(token, tokenDigest); responseStatus = 200; - boolean ok = storage.verifyToken(authToken); + boolean ok = storage.verifyToken(authToken, someAction); assertTrue(requestURI.endsWith("/verify-token")); assertEquals("POST", method); String[] requestParts = requestBody.split("&"); - assertEquals(2, requestParts.length); + assertEquals(3, requestParts.length); String[] clientTokenParts = requestParts[0].split("="); assertEquals(2, clientTokenParts.length); assertEquals("client-token", clientTokenParts[0]); String urlDecoded = URLDecoder.decode(clientTokenParts[1], "UTF-8"); - String base64decoded = new String(Base64.decodeBase64(urlDecoded)); - assertEquals("fluff", base64decoded); + assertTrue(Arrays.equals(tokenDigest, Base64.decodeBase64(urlDecoded))); String[] authTokenParts = requestParts[1].split("="); assertEquals(2, authTokenParts.length); assertEquals("token", authTokenParts[0]); + String[] actionParts = requestParts[2].split("="); + assertEquals(2, actionParts.length); + assertEquals("action-name", actionParts[0]); urlDecoded = URLDecoder.decode(authTokenParts[1], "UTF-8"); - base64decoded = new String(Base64.decodeBase64(urlDecoded)); + String base64decoded = new String(Base64.decodeBase64(urlDecoded)); assertEquals("stuff", base64decoded); assertTrue(ok); // Try another one in which verification fails. responseStatus = 401; - ok = storage.verifyToken(authToken); + ok = storage.verifyToken(authToken, someAction); assertFalse(ok); } + + private byte[] getShaBytes(String clientToken, String someAction) + throws NoSuchAlgorithmException, UnsupportedEncodingException { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + digest.update(clientToken.getBytes()); + digest.update(someAction.getBytes("UTF-8")); + byte[] tokenDigest = digest.digest(); + return tokenDigest; + } @Test public void verifyConnectFiresEventOnConnectionFailure() {
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/TokenManager.java Tue May 28 11:23:34 2013 -0400 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/TokenManager.java Tue May 14 16:34:29 2013 +0200 @@ -37,11 +37,15 @@ package com.redhat.thermostat.web.server; +import java.io.UnsupportedEncodingException; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Objects; import java.util.Timer; import java.util.TimerTask; @@ -62,11 +66,13 @@ this.timeout = timeout; } - byte[] generateToken(String clientToken) { + byte[] generateToken(byte[] clientToken, String actionName) { byte[] token = new byte[TOKEN_LENGTH]; random.nextBytes(token); - tokens.put(clientToken, token); - scheduleRemoval(clientToken); + final String clientKey = getKey(clientToken, + Objects.requireNonNull(actionName)); + tokens.put(clientKey, token); + scheduleRemoval(clientKey); return token; } @@ -80,11 +86,21 @@ }; timer.schedule(task, timeout); } + + private String getKey(byte[] clientToken, String actionName) { + try { + return getSha256HexString(clientToken, actionName.getBytes("UTF-8")); + } catch (UnsupportedEncodingException e) { + // If this happens, this is clearly a bug. + throw new RuntimeException(e); + } + } - boolean verifyToken(String clientToken, byte[] token) { - if (tokens.containsKey(clientToken)) { - byte[] storedToken = tokens.get(clientToken); - boolean verified = Arrays.equals(token, storedToken); + boolean verifyToken(byte[] clientToken, byte[] candidateToken, String actionName) { + final String clientKey = getKey(clientToken, Objects.requireNonNull(actionName)); + if (tokens.containsKey(clientKey)) { + byte[] storedToken = tokens.get(clientKey); + boolean verified = Arrays.equals(candidateToken, storedToken); if (verified) { tokens.remove(clientToken); } @@ -92,6 +108,39 @@ } return false; } + + private String getSha256HexString(byte[] clientToken, byte[] actionName) { + MessageDigest digest; + try { + digest = MessageDigest.getInstance("SHA-256"); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } + digest.update(clientToken); + digest.update(actionName); + byte[] result = digest.digest(); + return convertBytesToHexString(result); + } + + // package private for testing + String convertBytesToHexString(byte[] shaBytes) { + StringBuffer hexString = new StringBuffer(); + + for (int i = 0; i < shaBytes.length; i++) { + String hex = Integer.toHexString(0xff & shaBytes[i]); + if (hex.length() == 1) { + hexString.append('0'); + } + hexString.append(hex); + } + + return hexString.toString(); + } + + // Used for testing only + byte[] getStoredToken(String sha256) { + return tokens.get(sha256); + } }
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Tue May 28 11:23:34 2013 -0400 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Tue May 14 16:34:29 2013 +0200 @@ -93,6 +93,7 @@ @SuppressWarnings("serial") public class WebStorageEndPoint extends HttpServlet { + static final String CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX = "thermostat-cmdc-grant-"; private static final String TOKEN_MANAGER_TIMEOUT_PARAM = "token-manager-timeout"; private static final String TOKEN_MANAGER_KEY = "token-manager"; @@ -491,8 +492,29 @@ } TokenManager tokenManager = (TokenManager) getServletContext().getAttribute(TOKEN_MANAGER_KEY); assert tokenManager != null; - String clientToken = req.getParameter("client-token"); - byte[] token = tokenManager.generateToken(clientToken); + String clientTokenParam = req.getParameter("client-token"); + byte[] clientToken = Base64.decodeBase64(clientTokenParam); + String actionName = req.getParameter("action-name"); + // Perform pre-authorization: Since it's the client user which issues + // generate-token we have the correct user for which we can check role + // membership - and, thus, determine if this action is allowed to be + // performed. If the action is not allowed to be performed for this user + // a 403 will get returned and further verify-token would also fail, + // since no token gets added into the map. Trustworthiness of the + // action name will be implicitly checked by verify-token. + // + // We authorize based on role membership of this user. I.e. in order + // for a ping request (action-name == "ping") to properly authorize + // the user needs to be a member of role + // "thermostat-cmdc-grant-ping". More generally, membership of role + // "thermostat-cmdc-grant-<actionName>" grants the authenticated + // user the <actionName> command channel action. + String requiredRole = CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX + actionName; + if (! isAuthorized(req, resp, requiredRole)) { + return; + } + // authorization succeeded at this point + byte[] token = tokenManager.generateToken(clientToken, actionName); resp.setContentType("application/octet-stream"); resp.setContentLength(token.length); resp.getOutputStream().write(token); @@ -505,12 +527,31 @@ } TokenManager tokenManager = (TokenManager) getServletContext().getAttribute(TOKEN_MANAGER_KEY); assert tokenManager != null; - String clientToken = req.getParameter("client-token"); + String clientTokenParam = req.getParameter("client-token"); + byte[] clientToken = Base64.decodeBase64(clientTokenParam); + String actionName = req.getParameter("action-name"); byte[] token = Base64.decodeBase64(req.getParameter("token")); - boolean verified = tokenManager.verifyToken(clientToken, token); + // Perform authentication of the request. We can't do authorization for + // the originating client request here, since the only user info we have + // in verify-token is the identity of the agent which the client asked + // to perform the action for. Hence looking up role membership is not + // what we want here in order to limit privileges of the client. + // + // Note that we achieve this by performing authorization checks during + // generate-token. This is something the client user initiates and hence + // there we have the required user information. The entire command + // channel interaction can only succeed if and only if generate-token + // AND verify-token succeeded for the same token. Thus it's OK to only + // verify the token here - which in would only verify successfully if + // generate-token worked properly as a first step. + boolean verified = tokenManager.verifyToken(clientToken, token, actionName); if (! verified) { + logger.log(Level.INFO, "Command channel action " + actionName + " from remote host " + + req.getRemoteAddr() + " FAILED to authenticate!"); resp.setStatus(HttpServletResponse.SC_FORBIDDEN); } else { + logger.log(Level.FINEST, "Command channel action " + actionName + " from remote host " + + req.getRemoteAddr() + " PASSED authentication."); resp.setStatus(HttpServletResponse.SC_OK); } } @@ -519,7 +560,7 @@ if (req.isUserInRole(role)) { return true; } else { - logger.log(Level.FINEST, "Not permitting access to " + req.getPathInfo() + ". User '" + req.getRemoteUser() + "' not in role " + role); + logger.log(Level.INFO, "Not permitting access to " + req.getPathInfo() + ". User '" + req.getRemoteUser() + "' not in role " + role); resp.setStatus(HttpServletResponse.SC_FORBIDDEN); return false; }
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/TokenManagerTest.java Tue May 14 16:34:29 2013 +0200 @@ -0,0 +1,97 @@ +/* + * Copyright 2012, 2013 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.web.server; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import java.io.UnsupportedEncodingException; +import java.util.Arrays; + +import org.junit.Test; + +public class TokenManagerTest { + + /** + * Since we want to make sure that the action-name parameter for + * CMD channel interactions are trustworthy, we SHA256 hash the client + * token + the action name parameter, convert the resulting bytes into + * a hex string and finally put it into the tokens map. This way + * verification of the token should never pass if either the client token + * or the action-name was wrong. + * + * This test verifies that we have SHA256 hex-strings as keys. SHA256 hex + * strings are generated using the the client token first and then the + * action name for the digest. + */ + @Test + public void generateTokenTest() { + TokenManager tokenManager = new TokenManager(); + String clientToken = "something"; + String action = "myAction"; + byte[] token = tokenManager.generateToken(clientToken.getBytes(), action); + String expectedKey = "8d51bc31b39f54a1a12f2f94f09371ad5afe2263c1fdb7a3785aaacea6a386ef"; + byte[] actualToken = tokenManager.getStoredToken(expectedKey); + assertNotNull(actualToken); + assertTrue(Arrays.equals(token, actualToken)); + } + + @Test + public void canConvertBytesToHexString() throws UnsupportedEncodingException { + byte[] expected = new byte[] { + (byte)0xff, 0x6f, 0x6d, 0x65, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x57, 0x65, 0x48, 0x61, 0x73, 0x68 + }; + TokenManager tokenManager = new TokenManager(); + String actual = tokenManager.convertBytesToHexString(expected); + assertEquals(expected.length * 2, actual.length()); + assertEquals("ff6f6d65537472696e67576548617368", actual); + } + + @Test + public void generateAndVerifyTokenTest() { + TokenManager tokenManager = new TokenManager(); + String clientToken = "something"; + String action = "myAction"; + byte[] token = tokenManager.generateToken(clientToken.getBytes(), action); + assertTrue(tokenManager.verifyToken(clientToken.getBytes(), token, action)); + // try again with different action name, which should not verify + String wrongAction = "someAction"; + assertFalse(tokenManager.verifyToken(clientToken.getBytes(), token, wrongAction)); + } +}
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java Tue May 28 11:23:34 2013 -0400 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java Tue May 14 16:34:29 2013 +0200 @@ -37,6 +37,7 @@ package com.redhat.thermostat.web.server; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; @@ -895,8 +896,40 @@ @Test public void authorizedGenerateToken() throws Exception { + String actionName = "testing"; String[] roleNames = new String[] { - Roles.CMD_CHANNEL_GENERATE + Roles.CMD_CHANNEL_GENERATE, + // grant the "testing" action + WebStorageEndPoint.CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX + actionName + }; + String testuser = "testuser"; + String password = "testpassword"; + final LoginService loginService = new TestLoginService(testuser, password, roleNames); + port = FreePortFinder.findFreePort(new TryPort() { + + @Override + public void tryPort(int port) throws Exception { + startServer(port, loginService); + } + }); + verifyAuthorizedGenerateToken(testuser, password, actionName); + } + + @Test + public void unauthorizedGenerateToken() throws Exception { + String failMsg = "thermostat-cmdc-generate role missing, expected Forbidden!"; + String[] insufficientRoles = new String[0]; + doUnauthorizedTest("generate-token", failMsg, insufficientRoles, false); + } + + @Test + public void authorizedGenerateVerifyToken() throws Exception { + String actionName = "someAction"; + String[] roleNames = new String[] { + Roles.CMD_CHANNEL_GENERATE, + Roles.CMD_CHANNEL_VERIFY, + // grant "someAction" to be performed + WebStorageEndPoint.CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX + actionName, }; String testuser = "testuser"; String password = "testpassword"; @@ -908,20 +941,53 @@ startServer(port, loginService); } }); - verifyAuthorizedGenerateToken(testuser, password); + byte[] token = verifyAuthorizedGenerateToken(testuser, password, actionName); + + String endpoint = getEndpoint(); + URL url = new URL(endpoint + "/verify-token"); + + HttpURLConnection conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("POST"); + conn.setRequestProperty("Content-type", "application/x-www-form-urlencoded"); + sendAuthentication(conn, testuser, password); + conn.setDoOutput(true); + conn.setDoInput(true); + OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream()); + out.write("client-token=fluff&action-name=" + actionName + "&token=" + + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); + out.flush(); + assertEquals(200, conn.getResponseCode()); } - + @Test - public void unauthorizedGenerateToken() throws Exception { - String failMsg = "thermostat-cmdc-generate role missing, expected Forbidden!"; - String[] insufficientRoles = new String[0]; - doUnauthorizedTest("generate-token", failMsg, insufficientRoles, false); - } - - @Test - public void authorizedGenerateVerifyToken() throws Exception { + public void unAuthorizedGenerateVerifyToken() throws Exception { + String testuser = "testuser"; + String password = "testpassword"; + String actionName = "someAction"; String[] roleNames = new String[] { Roles.CMD_CHANNEL_GENERATE, + Roles.CMD_CHANNEL_VERIFY, + // missing the thermostat-cmdc-grant-someAction role + }; + final LoginService loginService = new TestLoginService(testuser, password, roleNames); + port = FreePortFinder.findFreePort(new TryPort() { + + @Override + public void tryPort(int port) throws Exception { + startServer(port, loginService); + } + }); + + byte[] result = verifyAuthorizedGenerateToken(testuser, password, actionName, 403); + assertNull(result); + } + + @Test + public void authenticatedGenerateVerifyTokenWithActionNameMismatch() throws Exception { + String actionName = "someAction"; + String[] roleNames = new String[] { + Roles.CMD_CHANNEL_GENERATE, + WebStorageEndPoint.CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX + actionName, Roles.CMD_CHANNEL_VERIFY }; String testuser = "testuser"; @@ -934,7 +1000,7 @@ startServer(port, loginService); } }); - byte[] token = verifyAuthorizedGenerateToken(testuser, password); + byte[] token = verifyAuthorizedGenerateToken(testuser, password, actionName); String endpoint = getEndpoint(); URL url = new URL(endpoint + "/verify-token"); @@ -946,16 +1012,22 @@ conn.setDoOutput(true); conn.setDoInput(true); OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream()); - out.write("client-token=fluff&token=" + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); + // expected action-name parameter is "someAction". This should not + // verify => 403. + out.write("client-token=fluff&action-name=wrongAction&token=" + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); out.flush(); - assertEquals(200, conn.getResponseCode()); + assertEquals(403, conn.getResponseCode()); } + @Test - public void authorizedTokenTimeout() throws Exception { + public void authenticatedTokenTimeout() throws Exception { + String actionName = "someAction"; String[] roleNames = new String[] { Roles.CMD_CHANNEL_GENERATE, - Roles.CMD_CHANNEL_VERIFY + Roles.CMD_CHANNEL_VERIFY, + // Grant "someAction", this test tests the time-out + WebStorageEndPoint.CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX + actionName }; String testuser = "testuser"; String password = "testpassword"; @@ -967,7 +1039,7 @@ startServer(port, loginService); } }); - byte[] token = verifyAuthorizedGenerateToken(testuser, password); + byte[] token = verifyAuthorizedGenerateToken(testuser, password, actionName); Thread.sleep(700); // Timeout is set to 500ms for tests, 700ms should be enough for everybody. ;-) @@ -981,13 +1053,14 @@ conn.setDoOutput(true); conn.setDoInput(true); OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream()); - out.write("client-token=fluff&token=" + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); + out.write("client-token=fluff&action-name=" + actionName + "&token=" + + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); out.flush(); assertEquals(403, conn.getResponseCode()); } @Test - public void authorizedVerifyNonExistentToken() throws Exception { + public void authenticatedVerifyNonExistentToken() throws Exception { String[] roleNames = new String[] { Roles.CMD_CHANNEL_VERIFY }; @@ -1014,7 +1087,7 @@ conn.setDoOutput(true); conn.setDoInput(true); OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream()); - out.write("client-token=fluff&token=" + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); + out.write("client-token=fluff&action-name=someAction&token=" + URLEncoder.encode(Base64.encodeBase64String(token), "UTF-8")); out.flush(); assertEquals(403, conn.getResponseCode()); } @@ -1054,7 +1127,17 @@ } } - private byte[] verifyAuthorizedGenerateToken(String username, String password) throws IOException { + private byte[] verifyAuthorizedGenerateToken(String username, String password, String actionName) throws IOException { + return verifyAuthorizedGenerateToken(username, password, actionName, 200); + } + + private byte[] verifyAuthorizedGenerateToken(String username, String password, String actionName, int expectedResponseCode) throws IOException { + return verifyAuthorizedGenerateToken(username, password, expectedResponseCode, actionName); + } + + private byte[] verifyAuthorizedGenerateToken(String username, + String password, int expectedResponseCode, String actionName) + throws IOException { String endpoint = getEndpoint(); URL url = new URL(endpoint + "/generate-token"); @@ -1064,21 +1147,27 @@ conn.setDoOutput(true); conn.setDoInput(true); OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream()); - out.write("client-token=fluff"); + out.write("client-token=fluff&action-name=" + actionName); out.flush(); - InputStream in = conn.getInputStream(); - int length = conn.getContentLength(); - byte[] token = new byte[length]; - assertEquals(256, length); - int totalRead = 0; - while (totalRead < length) { - int read = in.read(token, totalRead, length - totalRead); - if (read < 0) { - fail(); + int actualResponseCode = conn.getResponseCode(); + assertEquals(expectedResponseCode, actualResponseCode); + if (actualResponseCode == 200) { + InputStream in = conn.getInputStream(); + int length = conn.getContentLength(); + byte[] token = new byte[length]; + assertEquals(256, length); + int totalRead = 0; + while (totalRead < length) { + int read = in.read(token, totalRead, length - totalRead); + if (read < 0) { + fail(); + } + totalRead += read; } - totalRead += read; + return token; + } else { + return null; } - return token; } private static class TestLoginService extends MappedLoginService {