From 5215580cd75557ad1cc95401ee7a4c992cdba3ae Mon Sep 17 00:00:00 2001 From: "Marcel Mager (INST-IOT/ESB)" Date: Thu, 15 Sep 2016 09:45:28 +0200 Subject: [PATCH] Fix code review findings: * adapt return type of #getPreAuthenticatedPrincipal to the #getPreAuthenticatedCredentials return type (both Object) * #splitMultiHash returns list instead of array Signed-off-by: Marcel Mager (INST-IOT/ESB) --- ...SourceTrustAuthenticationProviderTest.java | 32 +++++++++------- ...bstractControllerAuthenticationFilter.java | 2 +- ...rPreAuthenticatedSecurityHeaderFilter.java | 35 ++++++++---------- ...okenSourceTrustAuthenticationProvider.java | 14 ++++--- .../security/PreAuthentificationFilter.java | 2 +- ...AuthenticatedSecurityHeaderFilterTest.java | 37 ++++++++++++++++++- 6 files changed, 79 insertions(+), 43 deletions(-) diff --git a/hawkbit-http-security/src/test/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProviderTest.java b/hawkbit-http-security/src/test/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProviderTest.java index eb4cacf33..28e9708ac 100644 --- a/hawkbit-http-security/src/test/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProviderTest.java +++ b/hawkbit-http-security/src/test/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProviderTest.java @@ -21,6 +21,8 @@ import org.springframework.security.authentication.InsufficientAuthenticationExc import org.springframework.security.core.Authentication; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; +import com.google.common.collect.Lists; + import ru.yandex.qatools.allure.annotations.Description; import ru.yandex.qatools.allure.annotations.Features; import ru.yandex.qatools.allure.annotations.Stories; @@ -45,8 +47,8 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { public void principalAndCredentialsNotTheSameThrowsAuthenticationException() { final String principal = "controllerIdURL"; final String credentials = "controllerIdHeader"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); // test, should throw authentication exception @@ -64,11 +66,12 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { public void principalAndCredentialsAreTheSameWithNoSourceIpCheckIsSuccessful() { final String principal = "controllerId"; final String credentials = "controllerId"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); - final Authentication authenticate = underTestWithoutSourceIpCheck.authenticate(token); + final Authentication authenticate = underTestWithoutSourceIpCheck + .authenticate(token); assertThat(authenticate.isAuthenticated()).isTrue(); } @@ -78,8 +81,8 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { final String remoteAddress = "192.168.1.1"; final String principal = "controllerId"; final String credentials = "controllerId"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); when(webAuthenticationDetailsMock.getRemoteAddress()).thenReturn(remoteAddress); @@ -99,14 +102,15 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { public void priniciapAndCredentialsAreTheSameAndSourceIpIsTrusted() { final String principal = "controllerId"; final String credentials = "controllerId"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); when(webAuthenticationDetailsMock.getRemoteAddress()).thenReturn(REQUEST_SOURCE_IP); // test, should throw authentication exception - final Authentication authenticate = underTestWithSourceIpCheck.authenticate(token); + final Authentication authenticate = underTestWithSourceIpCheck + .authenticate(token); assertThat(authenticate.isAuthenticated()).isTrue(); } @@ -116,8 +120,8 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { "192.168.1.3" }; final String principal = "controllerId"; final String credentials = "controllerId"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); when(webAuthenticationDetailsMock.getRemoteAddress()).thenReturn(REQUEST_SOURCE_IP); @@ -135,8 +139,8 @@ public class PreAuthTokenSourceTrustAuthenticationProviderTest { final String[] trustedIPAddresses = new String[] { "192.168.1.1", "192.168.1.2", "192.168.1.3" }; final String principal = "controllerId"; final String credentials = "controllerId"; - final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, - credentials); + final PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken( + principal, Lists.newArrayList(credentials)); token.setDetails(webAuthenticationDetailsMock); when(webAuthenticationDetailsMock.getRemoteAddress()).thenReturn(REQUEST_SOURCE_IP); diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/AbstractControllerAuthenticationFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/AbstractControllerAuthenticationFilter.java index c45cfb5e4..cf64e6c59 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/AbstractControllerAuthenticationFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/AbstractControllerAuthenticationFilter.java @@ -47,7 +47,7 @@ public abstract class AbstractControllerAuthenticationFilter implements PreAuthe } @Override - public abstract HeaderAuthentication getPreAuthenticatedPrincipal(TenantSecurityToken secruityToken); + public abstract Object getPreAuthenticatedPrincipal(TenantSecurityToken secruityToken); @Override public abstract Object getPreAuthenticatedCredentials(TenantSecurityToken secruityToken); diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilter.java index 6fd13a1c6..2f0ecaf6a 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilter.java @@ -10,6 +10,7 @@ package org.eclipse.hawkbit.security; import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Set; import org.eclipse.hawkbit.dmf.json.model.TenantSecurityToken; @@ -79,10 +80,9 @@ public class ControllerPreAuthenticatedSecurityHeaderFilter extends AbstractCont } @Override - public HeaderAuthentication getPreAuthenticatedPrincipal(final TenantSecurityToken secruityToken) { + public Object getPreAuthenticatedPrincipal(final TenantSecurityToken secruityToken) { // retrieve the common name header and the authority name header from - // the http request and - // combine them together + // the http request and combine them together final String commonNameValue = secruityToken.getHeader(caCommonNameHeader); final String knownSslIssuerConfigurationValue = tenantAware.runAsTenant(secruityToken.getTenant(), sslIssuerNameConfigTenantRunner); @@ -106,23 +106,17 @@ public class ControllerPreAuthenticatedSecurityHeaderFilter extends AbstractCont sslIssuerNameConfigTenantRunner); String controllerId = secruityToken.getControllerId(); // in case of legacy download artifact, the controller ID is not in the - // URL path, so then - // we just use the common name header + // URL path, so then we just use the common name header if (controllerId == null || "anonymous".equals(controllerId)) { controllerId = secruityToken.getHeader(caCommonNameHeader); } - String[] knownHashes = splitMultiHash(authorityNameConfigurationValue); - if (knownHashes.length > 1) { - Set multiHashes = new HashSet<>(); - final String cntlId = controllerId; - Arrays.asList(knownHashes) - .forEach(hashItem -> multiHashes.add(new HeaderAuthentication(cntlId, hashItem))); - return multiHashes; + List knownHashes = splitMultiHash(authorityNameConfigurationValue); - } else { - return new HeaderAuthentication(controllerId, authorityNameConfigurationValue); - } + Set multiHashes = new HashSet<>(); + final String cntlId = controllerId; + knownHashes.forEach(hashItem -> multiHashes.add(new HeaderAuthentication(cntlId, hashItem))); + return multiHashes; } /** @@ -132,12 +126,15 @@ public class ControllerPreAuthenticatedSecurityHeaderFilter extends AbstractCont * this request for this tenant. */ private String getIssuerHashHeader(final TenantSecurityToken secruityToken, final String knownIssuerHashes) { + // there may be several knownIssuerHashes configured for the tenant + // separated by a semicolon + List knownHashes = splitMultiHash(knownIssuerHashes); + // iterate over the headers until we get a null header. - String[] knownHashes = splitMultiHash(knownIssuerHashes); int iHeader = 1; String foundHash; while ((foundHash = secruityToken.getHeader(String.format(sslIssuerHashBasicHeader, iHeader))) != null) { - if (Arrays.asList(knownHashes).contains(foundHash)) { + if (knownHashes.contains(foundHash)) { if (LOGGER.isTraceEnabled()) { LOGGER.trace("Found matching ssl issuer hash at position {}", iHeader); } @@ -164,7 +161,7 @@ public class ControllerPreAuthenticatedSecurityHeaderFilter extends AbstractCont } } - private static String[] splitMultiHash(String knownIssuerHashes) { - return knownIssuerHashes.split(";"); + private static List splitMultiHash(String knownIssuerHashes) { + return Arrays.asList(knownIssuerHashes.split(";")); } } diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProvider.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProvider.java index c653108a0..7f69180be 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProvider.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthTokenSourceTrustAuthenticationProvider.java @@ -97,14 +97,16 @@ public class PreAuthTokenSourceTrustAuthenticationProvider implements Authentica throw new BadCredentialsException("The provided principal and credentials are not match"); } - // check if principal equals credentials because we want to check if - // e.g. controllerId - // containing in the URL equals the controllerId in the special header - // set by the reverse - // proxy which extracted the CN from the certificate + // The credentials may either be of type HeaderAuthentication or of type + // Collection depending on the authentication mode + // in use (the latter is used in case of trusted reverse-proxy). + // It is checked whether principal equals credentials (respectively if + // credentials contains principal in case of collection) because we want + // to check if e.g. controllerId containing in the URL equals the + // controllerId in the special header set by the reverse-proxy which + // extracted the CN from the certificate. if (principal.equals(credentials)) { successAuthentication = checkSourceIPAddressIfNeccessary(tokenDetails); - } else if (Collection.class.isAssignableFrom(credentials.getClass())) { final Collection multiValueCredentials = (Collection) credentials; if (multiValueCredentials.contains(principal)) { diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthentificationFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthentificationFilter.java index f0801ff4d..72f362fa0 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthentificationFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthentificationFilter.java @@ -36,7 +36,7 @@ public interface PreAuthentificationFilter { * the secruityToken * @return the extracted tenant and controller id */ - HeaderAuthentication getPreAuthenticatedPrincipal(TenantSecurityToken secruityToken); + Object getPreAuthenticatedPrincipal(TenantSecurityToken secruityToken); /** * Extract the principal credentials from the current secruityToken. diff --git a/hawkbit-security-integration/src/test/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilterTest.java b/hawkbit-security-integration/src/test/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilterTest.java index eb34d5253..dc99df7e7 100644 --- a/hawkbit-security-integration/src/test/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilterTest.java +++ b/hawkbit-security-integration/src/test/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedSecurityHeaderFilterTest.java @@ -12,6 +12,8 @@ import static org.junit.Assert.*; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.when; +import java.util.Collection; + import org.eclipse.hawkbit.dmf.json.model.TenantSecurityToken; import org.eclipse.hawkbit.dmf.json.model.TenantSecurityToken.FileResource; import org.eclipse.hawkbit.repository.TenantConfigurationManagement; @@ -102,10 +104,41 @@ public class ControllerPreAuthenticatedSecurityHeaderFilterTest { assertNull(underTest.getPreAuthenticatedPrincipal(securityToken)); } + @Test + @Description("Tests different values for issuer hash header and inspects the credentials") + public void useDifferentValuesForIssuerHashHeader() { + + // prepare security token + TenantSecurityToken securityToken = prepareSecurityToken(); + securityToken.getHeaders().put(X_SSL_ISSUER_HASH_1, "hash1"); + + when(tenantConfigurationManagementMock.getConfigurationValue( + eq(TenantConfigurationKey.AUTHENTICATION_MODE_HEADER_AUTHORITY_NAME), eq(String.class))) + .thenReturn(CONFIG_VALUE_MULTI_HASH); + + HeaderAuthentication expected = new HeaderAuthentication("box1", "hash1"); + Collection credentials = (Collection) underTest + .getPreAuthenticatedCredentials(securityToken); + assertTrue(credentials.contains(expected)); + + Object principal = underTest.getPreAuthenticatedPrincipal(securityToken); + assertEquals(expected, principal); + + securityToken = prepareSecurityToken(); + securityToken.getHeaders().put(X_SSL_ISSUER_HASH_1, "hash2"); + expected = new HeaderAuthentication("box1", "hash2"); + credentials = (Collection) underTest.getPreAuthenticatedCredentials(securityToken); + assertTrue(credentials.contains(expected)); + + principal = underTest.getPreAuthenticatedPrincipal(securityToken); + assertEquals(expected, principal); + + } + private static TenantSecurityToken prepareSecurityToken() { - final TenantSecurityToken securityToken = new TenantSecurityToken("default", "1234", + final TenantSecurityToken securityToken = new TenantSecurityToken("DEFAULT", "box1", FileResource.createFileResourceBySha1("12345")); - securityToken.getHeaders().put(CA_COMMON_NAME, "any"); + securityToken.getHeaders().put(CA_COMMON_NAME, "box1"); return securityToken; }