From acff82f60fa8d5e54f25ddf8d8d18e0097ed043d Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Wed, 16 Aug 2023 14:25:17 +0300 Subject: [PATCH] Small security improvements (#1412) Typos fixed Disables empty string gateway token for sure. Test if the gateway token is not empty string ecplicitly. Empty string is the default value and if accepted could be a security vulnerability (e.g. enabling gateway token authentication and using empty string as token). According to https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.4 the header value shall not have trailing spaces and the http server shall already have trimmed them. So if execution passes start with "GatewayToken " then token shall not be empty. But but let's check anyway In UI first set key then enable the gateway token authentication. Otherwise the key might be left empty (default). This however shall not be really problem since (because of token trimming) the empty token will be rejected anyway. Signed-off-by: Marinov Avgustin --- .../AmqpAuthenticationMessageHandler.java | 20 +++++------ .../amqp/AmqpControllerAuthentication.java | 8 ++--- ...actHttpControllerAuthenticationFilter.java | 34 +++++++++---------- ...bstractControllerAuthenticationFilter.java | 4 +-- ...lerPreAuthenticateSecurityTokenFilter.java | 8 ++--- ...llerPreAuthenticatedAnonymousDownload.java | 8 ++--- ...rollerPreAuthenticatedAnonymousFilter.java | 10 +++--- ...thenticatedGatewaySecurityTokenFilter.java | 22 ++++++------ .../security/PreAuthenticationFilter.java | 16 ++++----- .../AuthenticationConfigurationView.java | 10 +++--- 10 files changed, 71 insertions(+), 69 deletions(-) diff --git a/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpAuthenticationMessageHandler.java b/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpAuthenticationMessageHandler.java index 770f1f982..b019dd280 100644 --- a/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpAuthenticationMessageHandler.java +++ b/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpAuthenticationMessageHandler.java @@ -119,18 +119,18 @@ public class AmqpAuthenticationMessageHandler extends BaseAmqpService { * this file because it's not assigned to an action and not assigned to this * controller. Otherwise no controllerId is set = anonymous download * - * @param secruityToken + * @param securityToken * the security token which holds the target ID to check on * @param sha1Hash * of the artifact to verify if the given target is allowed to * download it */ - private void checkIfArtifactIsAssignedToTarget(final DmfTenantSecurityToken secruityToken, final String sha1Hash) { + private void checkIfArtifactIsAssignedToTarget(final DmfTenantSecurityToken securityToken, final String sha1Hash) { - if (secruityToken.getControllerId() != null) { - checkByControllerId(sha1Hash, secruityToken.getControllerId()); - } else if (secruityToken.getTargetId() != null) { - checkByTargetId(sha1Hash, secruityToken.getTargetId()); + if (securityToken.getControllerId() != null) { + checkByControllerId(sha1Hash, securityToken.getControllerId()); + } else if (securityToken.getTargetId() != null) { + checkByTargetId(sha1Hash, securityToken.getTargetId()); } else { LOG.info("anonymous download no authentication check for artifact {}", sha1Hash); } @@ -198,15 +198,15 @@ public class AmqpAuthenticationMessageHandler extends BaseAmqpService { @SuppressWarnings("squid:S1166") private Message handleAuthenticationMessage(final Message message) { final DmfDownloadResponse authenticationResponse = new DmfDownloadResponse(); - final DmfTenantSecurityToken secruityToken = convertMessage(message, DmfTenantSecurityToken.class); - final FileResource fileResource = secruityToken.getFileResource(); + final DmfTenantSecurityToken securityToken = convertMessage(message, DmfTenantSecurityToken.class); + final FileResource fileResource = securityToken.getFileResource(); try { - SecurityContextHolder.getContext().setAuthentication(authenticationManager.doAuthenticate(secruityToken)); + SecurityContextHolder.getContext().setAuthentication(authenticationManager.doAuthenticate(securityToken)); final Artifact artifact = findArtifactByFileResource(fileResource) .orElseThrow(EntityNotFoundException::new); - checkIfArtifactIsAssignedToTarget(secruityToken, artifact.getSha1Hash()); + checkIfArtifactIsAssignedToTarget(securityToken, artifact.getSha1Hash()); final DmfArtifact dmfArtifact = convertDbArtifact(artifact); diff --git a/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpControllerAuthentication.java b/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpControllerAuthentication.java index f6ed125b9..182999735 100644 --- a/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpControllerAuthentication.java +++ b/hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpControllerAuthentication.java @@ -145,14 +145,14 @@ public class AmqpControllerAuthentication { } private static PreAuthenticatedAuthenticationToken createAuthentication(final PreAuthenticationFilter filter, - final DmfTenantSecurityToken secruityToken) { + final DmfTenantSecurityToken securityToken) { - if (!filter.isEnable(secruityToken)) { + if (!filter.isEnable(securityToken)) { return null; } - final Object principal = filter.getPreAuthenticatedPrincipal(secruityToken); - final Object credentials = filter.getPreAuthenticatedCredentials(secruityToken); + final Object principal = filter.getPreAuthenticatedPrincipal(securityToken); + final Object credentials = filter.getPreAuthenticatedCredentials(securityToken); if (principal == null) { LOGGER.debug("No pre-authenticated principal found in message"); diff --git a/hawkbit-http-security/src/main/java/org/eclipse/hawkbit/security/AbstractHttpControllerAuthenticationFilter.java b/hawkbit-http-security/src/main/java/org/eclipse/hawkbit/security/AbstractHttpControllerAuthenticationFilter.java index d5fb98493..dabf93074 100644 --- a/hawkbit-http-security/src/main/java/org/eclipse/hawkbit/security/AbstractHttpControllerAuthenticationFilter.java +++ b/hawkbit-http-security/src/main/java/org/eclipse/hawkbit/security/AbstractHttpControllerAuthenticationFilter.java @@ -92,13 +92,13 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac return; } - final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables((HttpServletRequest) request); - if (secruityToken == null) { + final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables((HttpServletRequest) request); + if (securityToken == null) { chain.doFilter(request, response); return; } abstractControllerAuthenticationFilter = createControllerAuthenticationFilter(); - if (abstractControllerAuthenticationFilter.isEnable(secruityToken) + if (abstractControllerAuthenticationFilter.isEnable(securityToken) && SecurityContextHolder.getContext().getAuthentication() == null) { super.doFilter(request, response, chain); } else { @@ -129,7 +129,7 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac * request does not match the pattern and no variables could be * extracted */ - protected DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpServletRequest request) { + protected DmfTenantSecurityToken createTenantSecurityTokenVariables(final HttpServletRequest request) { final String requestURI = request.getRequestURI(); if (pathExtractor.match(request.getContextPath() + CONTROLLER_REQUEST_ANT_PATTERN, requestURI)) { @@ -142,7 +142,7 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac LOG.trace("Parsed tenant {} and controllerId {} from path request {}", tenant, controllerId, requestURI); } - return createTenantSecruityTokenVariables(request, tenant, controllerId); + return createTenantSecurityTokenVariables(request, tenant, controllerId); } else if (pathExtractor.match(request.getContextPath() + CONTROLLER_DL_REQUEST_ANT_PATTERN, requestURI)) { LOG.debug("retrieving path variables from URI request {}", requestURI); final Map extractUriTemplateVariables = pathExtractor.extractUriTemplateVariables( @@ -151,7 +151,7 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac if (LOG.isTraceEnabled()) { LOG.trace("Parsed tenant {} from path request {}", tenant, requestURI); } - return createTenantSecruityTokenVariables(request, tenant, "anonymous"); + return createTenantSecurityTokenVariables(request, tenant, "anonymous"); } else { if (LOG.isTraceEnabled()) { LOG.trace("request {} does not match the path pattern {}, request gets ignored", requestURI, @@ -161,33 +161,33 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac } } - private DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpServletRequest request, - final String tenant, final String controllerId) { - final DmfTenantSecurityToken secruityToken = new DmfTenantSecurityToken(tenant, null, controllerId, null, + private DmfTenantSecurityToken createTenantSecurityTokenVariables(final HttpServletRequest request, + final String tenant, final String controllerId) { + final DmfTenantSecurityToken securityToken = new DmfTenantSecurityToken(tenant, null, controllerId, null, FileResource.createFileResourceBySha1("")); Collections.list(request.getHeaderNames()) - .forEach(header -> secruityToken.putHeader(header, request.getHeader(header))); + .forEach(header -> securityToken.putHeader(header, request.getHeader(header))); - return secruityToken; + return securityToken; } @Override protected Object getPreAuthenticatedPrincipal(final HttpServletRequest request) { - final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables(request); - if (secruityToken == null) { + final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables(request); + if (securityToken == null) { return null; } - return abstractControllerAuthenticationFilter.getPreAuthenticatedPrincipal(secruityToken); + return abstractControllerAuthenticationFilter.getPreAuthenticatedPrincipal(securityToken); } @Override protected Object getPreAuthenticatedCredentials(final HttpServletRequest request) { - final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables(request); - if (secruityToken == null) { + final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables(request); + if (securityToken == null) { return null; } - return abstractControllerAuthenticationFilter.getPreAuthenticatedCredentials(secruityToken); + return abstractControllerAuthenticationFilter.getPreAuthenticatedCredentials(securityToken); } } 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 43e75126f..35d22e947 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 @@ -43,8 +43,8 @@ public abstract class AbstractControllerAuthenticationFilter implements PreAuthe protected abstract String getTenantConfigurationKey(); @Override - public boolean isEnable(final DmfTenantSecurityToken secruityToken) { - return tenantAware.runAsTenant(secruityToken.getTenant(), configurationKeyTenantRunner); + public boolean isEnable(final DmfTenantSecurityToken securityToken) { + return tenantAware.runAsTenant(securityToken.getTenant(), configurationKeyTenantRunner); } private final class SecurityConfigurationKeyTenantRunner implements TenantAware.TenantRunner { diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticateSecurityTokenFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticateSecurityTokenFilter.java index 4f90d9e2f..bbc70f491 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticateSecurityTokenFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticateSecurityTokenFilter.java @@ -61,9 +61,9 @@ public class ControllerPreAuthenticateSecurityTokenFilter extends AbstractContro } @Override - public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { - final String controllerId = resolveControllerId(secruityToken); - final String authHeader = secruityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); + public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) { + final String controllerId = resolveControllerId(securityToken); + final String authHeader = securityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); if ((authHeader != null) && authHeader.startsWith(TARGET_SECURITY_TOKEN_AUTH_SCHEME)) { LOGGER.debug("found authorization header with scheme {} using target security token for authentication", TARGET_SECURITY_TOKEN_AUTH_SCHEME); @@ -71,7 +71,7 @@ public class ControllerPreAuthenticateSecurityTokenFilter extends AbstractContro } LOGGER.debug( "security token filter is enabled but requst does not contain either the necessary path variables {} or the authorization header with scheme {}", - secruityToken, TARGET_SECURITY_TOKEN_AUTH_SCHEME); + securityToken, TARGET_SECURITY_TOKEN_AUTH_SCHEME); return null; } diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousDownload.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousDownload.java index 572bdb561..10bb1cb86 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousDownload.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousDownload.java @@ -41,13 +41,13 @@ public class ControllerPreAuthenticatedAnonymousDownload extends AbstractControl } @Override - public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { - return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); + public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) { + return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId()); } @Override - public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { - return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); + public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) { + return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId()); } @Override diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousFilter.java index 3555a32d0..6134e6b1f 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedAnonymousFilter.java @@ -28,17 +28,17 @@ public class ControllerPreAuthenticatedAnonymousFilter implements PreAuthenticat } @Override - public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { - return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); + public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) { + return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId()); } @Override - public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { - return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); + public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) { + return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId()); } @Override - public boolean isEnable(final DmfTenantSecurityToken secruityToken) { + public boolean isEnable(final DmfTenantSecurityToken securityToken) { return ddiSecurityConfiguration.getAuthentication().getAnonymous().isEnabled(); } diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedGatewaySecurityTokenFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedGatewaySecurityTokenFilter.java index 2b669ef39..df351d2be 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedGatewaySecurityTokenFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/ControllerPreAuthenticatedGatewaySecurityTokenFilter.java @@ -19,7 +19,7 @@ import org.slf4j.LoggerFactory; * configuration) the possibility to authenticate a target based through a * gateway security token. This is commonly used for targets connected * indirectly via a gateway. This gateway controls multiple targets under the - * gateway security token which can be set via the {@code TenantSecruityToken} + * gateway security token which can be set via the {@code TenantsecurityToken} * header. {@code Example Header: Authorization: GatewayToken * 5d8fSD54fdsFG98DDsa.} * @@ -55,25 +55,27 @@ public class ControllerPreAuthenticatedGatewaySecurityTokenFilter extends Abstra } @Override - public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { - final String authHeader = secruityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); - if ((authHeader != null) && authHeader.startsWith(GATEWAY_SECURITY_TOKEN_AUTH_SCHEME)) { + public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) { + final String authHeader = securityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); + if (authHeader != null && + authHeader.startsWith(GATEWAY_SECURITY_TOKEN_AUTH_SCHEME) && + authHeader.length() > OFFSET_GATEWAY_TOKEN) { // disables empty string token LOGGER.debug("found authorization header with scheme {} using target security token for authentication", GATEWAY_SECURITY_TOKEN_AUTH_SCHEME); - return new HeaderAuthentication(secruityToken.getControllerId(), + return new HeaderAuthentication(securityToken.getControllerId(), authHeader.substring(OFFSET_GATEWAY_TOKEN)); } LOGGER.debug( - "security token filter is enabled but request does not contain either the necessary secruity token {} or the authorization header with scheme {}", - secruityToken, GATEWAY_SECURITY_TOKEN_AUTH_SCHEME); + "security token filter is enabled but request does not contain either the necessary security token {} or the authorization header with scheme {}", + securityToken, GATEWAY_SECURITY_TOKEN_AUTH_SCHEME); return null; } @Override - public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { - final String gatewayToken = tenantAware.runAsTenant(secruityToken.getTenant(), + public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) { + final String gatewayToken = tenantAware.runAsTenant(securityToken.getTenant(), gatewaySecurityTokenKeyConfigRunner); - return new HeaderAuthentication(secruityToken.getControllerId(), gatewayToken); + return new HeaderAuthentication(securityToken.getControllerId(), gatewayToken); } @Override diff --git a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthenticationFilter.java b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthenticationFilter.java index eb55b1fc3..672e0b416 100644 --- a/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthenticationFilter.java +++ b/hawkbit-security-integration/src/main/java/org/eclipse/hawkbit/security/PreAuthenticationFilter.java @@ -22,26 +22,26 @@ public interface PreAuthenticationFilter { /** * Check if the filter is enabled. * - * @param secruityToken the secruity info + * @param securityToken the secruity info * @return true is enabled false diabled */ - boolean isEnable(DmfTenantSecurityToken secruityToken); + boolean isEnable(DmfTenantSecurityToken securityToken); /** - * Extract the principal information from the current secruityToken. + * Extract the principal information from the current securityToken. * - * @param secruityToken the secruityToken + * @param securityToken the securityToken * @return the extracted tenant and controller id */ - HeaderAuthentication getPreAuthenticatedPrincipal(DmfTenantSecurityToken secruityToken); + HeaderAuthentication getPreAuthenticatedPrincipal(DmfTenantSecurityToken securityToken); /** - * Extract the principal credentials from the current secruityToken. + * Extract the principal credentials from the current securityToken. * - * @param secruityToken the secruityToken + * @param securityToken the securityToken * @return the extracted tenant and controller id */ - Object getPreAuthenticatedCredentials(DmfTenantSecurityToken secruityToken); + Object getPreAuthenticatedCredentials(DmfTenantSecurityToken securityToken); /** * Allows to add additional authorities to the successful authenticated token. diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/tenantconfiguration/AuthenticationConfigurationView.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/tenantconfiguration/AuthenticationConfigurationView.java index e3379cb7a..ed69fbb63 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/tenantconfiguration/AuthenticationConfigurationView.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/tenantconfiguration/AuthenticationConfigurationView.java @@ -165,6 +165,11 @@ public class AuthenticationConfigurationView extends BaseConfigurationView