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 <Avgustin.Marinov@bosch.com>
This commit is contained in:
Avgustin Marinov
2023-08-16 14:25:17 +03:00
committed by GitHub
parent a5dba29e74
commit acff82f60f
10 changed files with 71 additions and 69 deletions

View File

@@ -119,18 +119,18 @@ public class AmqpAuthenticationMessageHandler extends BaseAmqpService {
* this file because it's not assigned to an action and not assigned to this * this file because it's not assigned to an action and not assigned to this
* controller. Otherwise no controllerId is set = anonymous download * controller. Otherwise no controllerId is set = anonymous download
* *
* @param secruityToken * @param securityToken
* the security token which holds the target ID to check on * the security token which holds the target ID to check on
* @param sha1Hash * @param sha1Hash
* of the artifact to verify if the given target is allowed to * of the artifact to verify if the given target is allowed to
* download it * download it
*/ */
private void checkIfArtifactIsAssignedToTarget(final DmfTenantSecurityToken secruityToken, final String sha1Hash) { private void checkIfArtifactIsAssignedToTarget(final DmfTenantSecurityToken securityToken, final String sha1Hash) {
if (secruityToken.getControllerId() != null) { if (securityToken.getControllerId() != null) {
checkByControllerId(sha1Hash, secruityToken.getControllerId()); checkByControllerId(sha1Hash, securityToken.getControllerId());
} else if (secruityToken.getTargetId() != null) { } else if (securityToken.getTargetId() != null) {
checkByTargetId(sha1Hash, secruityToken.getTargetId()); checkByTargetId(sha1Hash, securityToken.getTargetId());
} else { } else {
LOG.info("anonymous download no authentication check for artifact {}", sha1Hash); LOG.info("anonymous download no authentication check for artifact {}", sha1Hash);
} }
@@ -198,15 +198,15 @@ public class AmqpAuthenticationMessageHandler extends BaseAmqpService {
@SuppressWarnings("squid:S1166") @SuppressWarnings("squid:S1166")
private Message handleAuthenticationMessage(final Message message) { private Message handleAuthenticationMessage(final Message message) {
final DmfDownloadResponse authenticationResponse = new DmfDownloadResponse(); final DmfDownloadResponse authenticationResponse = new DmfDownloadResponse();
final DmfTenantSecurityToken secruityToken = convertMessage(message, DmfTenantSecurityToken.class); final DmfTenantSecurityToken securityToken = convertMessage(message, DmfTenantSecurityToken.class);
final FileResource fileResource = secruityToken.getFileResource(); final FileResource fileResource = securityToken.getFileResource();
try { try {
SecurityContextHolder.getContext().setAuthentication(authenticationManager.doAuthenticate(secruityToken)); SecurityContextHolder.getContext().setAuthentication(authenticationManager.doAuthenticate(securityToken));
final Artifact artifact = findArtifactByFileResource(fileResource) final Artifact artifact = findArtifactByFileResource(fileResource)
.orElseThrow(EntityNotFoundException::new); .orElseThrow(EntityNotFoundException::new);
checkIfArtifactIsAssignedToTarget(secruityToken, artifact.getSha1Hash()); checkIfArtifactIsAssignedToTarget(securityToken, artifact.getSha1Hash());
final DmfArtifact dmfArtifact = convertDbArtifact(artifact); final DmfArtifact dmfArtifact = convertDbArtifact(artifact);

View File

@@ -145,14 +145,14 @@ public class AmqpControllerAuthentication {
} }
private static PreAuthenticatedAuthenticationToken createAuthentication(final PreAuthenticationFilter filter, private static PreAuthenticatedAuthenticationToken createAuthentication(final PreAuthenticationFilter filter,
final DmfTenantSecurityToken secruityToken) { final DmfTenantSecurityToken securityToken) {
if (!filter.isEnable(secruityToken)) { if (!filter.isEnable(securityToken)) {
return null; return null;
} }
final Object principal = filter.getPreAuthenticatedPrincipal(secruityToken); final Object principal = filter.getPreAuthenticatedPrincipal(securityToken);
final Object credentials = filter.getPreAuthenticatedCredentials(secruityToken); final Object credentials = filter.getPreAuthenticatedCredentials(securityToken);
if (principal == null) { if (principal == null) {
LOGGER.debug("No pre-authenticated principal found in message"); LOGGER.debug("No pre-authenticated principal found in message");

View File

@@ -92,13 +92,13 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac
return; return;
} }
final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables((HttpServletRequest) request); final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables((HttpServletRequest) request);
if (secruityToken == null) { if (securityToken == null) {
chain.doFilter(request, response); chain.doFilter(request, response);
return; return;
} }
abstractControllerAuthenticationFilter = createControllerAuthenticationFilter(); abstractControllerAuthenticationFilter = createControllerAuthenticationFilter();
if (abstractControllerAuthenticationFilter.isEnable(secruityToken) if (abstractControllerAuthenticationFilter.isEnable(securityToken)
&& SecurityContextHolder.getContext().getAuthentication() == null) { && SecurityContextHolder.getContext().getAuthentication() == null) {
super.doFilter(request, response, chain); super.doFilter(request, response, chain);
} else { } else {
@@ -129,7 +129,7 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac
* request does not match the pattern and no variables could be * request does not match the pattern and no variables could be
* extracted * extracted
*/ */
protected DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpServletRequest request) { protected DmfTenantSecurityToken createTenantSecurityTokenVariables(final HttpServletRequest request) {
final String requestURI = request.getRequestURI(); final String requestURI = request.getRequestURI();
if (pathExtractor.match(request.getContextPath() + CONTROLLER_REQUEST_ANT_PATTERN, requestURI)) { 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, LOG.trace("Parsed tenant {} and controllerId {} from path request {}", tenant, controllerId,
requestURI); requestURI);
} }
return createTenantSecruityTokenVariables(request, tenant, controllerId); return createTenantSecurityTokenVariables(request, tenant, controllerId);
} else if (pathExtractor.match(request.getContextPath() + CONTROLLER_DL_REQUEST_ANT_PATTERN, requestURI)) { } else if (pathExtractor.match(request.getContextPath() + CONTROLLER_DL_REQUEST_ANT_PATTERN, requestURI)) {
LOG.debug("retrieving path variables from URI request {}", requestURI); LOG.debug("retrieving path variables from URI request {}", requestURI);
final Map<String, String> extractUriTemplateVariables = pathExtractor.extractUriTemplateVariables( final Map<String, String> extractUriTemplateVariables = pathExtractor.extractUriTemplateVariables(
@@ -151,7 +151,7 @@ public abstract class AbstractHttpControllerAuthenticationFilter extends Abstrac
if (LOG.isTraceEnabled()) { if (LOG.isTraceEnabled()) {
LOG.trace("Parsed tenant {} from path request {}", tenant, requestURI); LOG.trace("Parsed tenant {} from path request {}", tenant, requestURI);
} }
return createTenantSecruityTokenVariables(request, tenant, "anonymous"); return createTenantSecurityTokenVariables(request, tenant, "anonymous");
} else { } else {
if (LOG.isTraceEnabled()) { if (LOG.isTraceEnabled()) {
LOG.trace("request {} does not match the path pattern {}, request gets ignored", requestURI, 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, private DmfTenantSecurityToken createTenantSecurityTokenVariables(final HttpServletRequest request,
final String tenant, final String controllerId) { final String tenant, final String controllerId) {
final DmfTenantSecurityToken secruityToken = new DmfTenantSecurityToken(tenant, null, controllerId, null, final DmfTenantSecurityToken securityToken = new DmfTenantSecurityToken(tenant, null, controllerId, null,
FileResource.createFileResourceBySha1("")); FileResource.createFileResourceBySha1(""));
Collections.list(request.getHeaderNames()) 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 @Override
protected Object getPreAuthenticatedPrincipal(final HttpServletRequest request) { protected Object getPreAuthenticatedPrincipal(final HttpServletRequest request) {
final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables(request); final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables(request);
if (secruityToken == null) { if (securityToken == null) {
return null; return null;
} }
return abstractControllerAuthenticationFilter.getPreAuthenticatedPrincipal(secruityToken); return abstractControllerAuthenticationFilter.getPreAuthenticatedPrincipal(securityToken);
} }
@Override @Override
protected Object getPreAuthenticatedCredentials(final HttpServletRequest request) { protected Object getPreAuthenticatedCredentials(final HttpServletRequest request) {
final DmfTenantSecurityToken secruityToken = createTenantSecruityTokenVariables(request); final DmfTenantSecurityToken securityToken = createTenantSecurityTokenVariables(request);
if (secruityToken == null) { if (securityToken == null) {
return null; return null;
} }
return abstractControllerAuthenticationFilter.getPreAuthenticatedCredentials(secruityToken); return abstractControllerAuthenticationFilter.getPreAuthenticatedCredentials(securityToken);
} }
} }

View File

@@ -43,8 +43,8 @@ public abstract class AbstractControllerAuthenticationFilter implements PreAuthe
protected abstract String getTenantConfigurationKey(); protected abstract String getTenantConfigurationKey();
@Override @Override
public boolean isEnable(final DmfTenantSecurityToken secruityToken) { public boolean isEnable(final DmfTenantSecurityToken securityToken) {
return tenantAware.runAsTenant(secruityToken.getTenant(), configurationKeyTenantRunner); return tenantAware.runAsTenant(securityToken.getTenant(), configurationKeyTenantRunner);
} }
private final class SecurityConfigurationKeyTenantRunner implements TenantAware.TenantRunner<Boolean> { private final class SecurityConfigurationKeyTenantRunner implements TenantAware.TenantRunner<Boolean> {

View File

@@ -61,9 +61,9 @@ public class ControllerPreAuthenticateSecurityTokenFilter extends AbstractContro
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) {
final String controllerId = resolveControllerId(secruityToken); final String controllerId = resolveControllerId(securityToken);
final String authHeader = secruityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); final String authHeader = securityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER);
if ((authHeader != null) && authHeader.startsWith(TARGET_SECURITY_TOKEN_AUTH_SCHEME)) { if ((authHeader != null) && authHeader.startsWith(TARGET_SECURITY_TOKEN_AUTH_SCHEME)) {
LOGGER.debug("found authorization header with scheme {} using target security token for authentication", LOGGER.debug("found authorization header with scheme {} using target security token for authentication",
TARGET_SECURITY_TOKEN_AUTH_SCHEME); TARGET_SECURITY_TOKEN_AUTH_SCHEME);
@@ -71,7 +71,7 @@ public class ControllerPreAuthenticateSecurityTokenFilter extends AbstractContro
} }
LOGGER.debug( LOGGER.debug(
"security token filter is enabled but requst does not contain either the necessary path variables {} or the authorization header with scheme {}", "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; return null;
} }

View File

@@ -41,13 +41,13 @@ public class ControllerPreAuthenticatedAnonymousDownload extends AbstractControl
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) {
return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId());
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) {
return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId());
} }
@Override @Override

View File

@@ -28,17 +28,17 @@ public class ControllerPreAuthenticatedAnonymousFilter implements PreAuthenticat
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) {
return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId());
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) {
return new HeaderAuthentication(secruityToken.getControllerId(), secruityToken.getControllerId()); return new HeaderAuthentication(securityToken.getControllerId(), securityToken.getControllerId());
} }
@Override @Override
public boolean isEnable(final DmfTenantSecurityToken secruityToken) { public boolean isEnable(final DmfTenantSecurityToken securityToken) {
return ddiSecurityConfiguration.getAuthentication().getAnonymous().isEnabled(); return ddiSecurityConfiguration.getAuthentication().getAnonymous().isEnabled();
} }

View File

@@ -19,7 +19,7 @@ import org.slf4j.LoggerFactory;
* configuration) the possibility to authenticate a target based through a * configuration) the possibility to authenticate a target based through a
* gateway security token. This is commonly used for targets connected * gateway security token. This is commonly used for targets connected
* indirectly via a gateway. This gateway controls multiple targets under the * 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 * header. {@code Example Header: Authorization: GatewayToken
* 5d8fSD54fdsFG98DDsa.} * 5d8fSD54fdsFG98DDsa.}
* *
@@ -55,25 +55,27 @@ public class ControllerPreAuthenticatedGatewaySecurityTokenFilter extends Abstra
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedPrincipal(final DmfTenantSecurityToken securityToken) {
final String authHeader = secruityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER); final String authHeader = securityToken.getHeader(DmfTenantSecurityToken.AUTHORIZATION_HEADER);
if ((authHeader != null) && authHeader.startsWith(GATEWAY_SECURITY_TOKEN_AUTH_SCHEME)) { 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", LOGGER.debug("found authorization header with scheme {} using target security token for authentication",
GATEWAY_SECURITY_TOKEN_AUTH_SCHEME); GATEWAY_SECURITY_TOKEN_AUTH_SCHEME);
return new HeaderAuthentication(secruityToken.getControllerId(), return new HeaderAuthentication(securityToken.getControllerId(),
authHeader.substring(OFFSET_GATEWAY_TOKEN)); authHeader.substring(OFFSET_GATEWAY_TOKEN));
} }
LOGGER.debug( LOGGER.debug(
"security token filter is enabled but request does not contain either the necessary secruity token {} or the authorization header with scheme {}", "security token filter is enabled but request does not contain either the necessary security token {} or the authorization header with scheme {}",
secruityToken, GATEWAY_SECURITY_TOKEN_AUTH_SCHEME); securityToken, GATEWAY_SECURITY_TOKEN_AUTH_SCHEME);
return null; return null;
} }
@Override @Override
public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken secruityToken) { public HeaderAuthentication getPreAuthenticatedCredentials(final DmfTenantSecurityToken securityToken) {
final String gatewayToken = tenantAware.runAsTenant(secruityToken.getTenant(), final String gatewayToken = tenantAware.runAsTenant(securityToken.getTenant(),
gatewaySecurityTokenKeyConfigRunner); gatewaySecurityTokenKeyConfigRunner);
return new HeaderAuthentication(secruityToken.getControllerId(), gatewayToken); return new HeaderAuthentication(securityToken.getControllerId(), gatewayToken);
} }
@Override @Override

View File

@@ -22,26 +22,26 @@ public interface PreAuthenticationFilter {
/** /**
* Check if the filter is enabled. * Check if the filter is enabled.
* *
* @param secruityToken the secruity info * @param securityToken the secruity info
* @return <code>true</code> is enabled <code>false</code> diabled * @return <code>true</code> is enabled <code>false</code> 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 * @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 * @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. * Allows to add additional authorities to the successful authenticated token.

View File

@@ -165,6 +165,11 @@ public class AuthenticationConfigurationView extends BaseConfigurationView<Proxy
@Override @Override
public void save() { public void save() {
if (getBinderBean().isGatewaySecToken()) {
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_KEY,
getBinderBean().getGatewaySecurityToken());
}
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_TARGET_SECURITY_TOKEN_ENABLED, writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_TARGET_SECURITY_TOKEN_ENABLED,
getBinderBean().isTargetSecToken()); getBinderBean().isTargetSecToken());
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_ENABLED, writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_ENABLED,
@@ -172,11 +177,6 @@ public class AuthenticationConfigurationView extends BaseConfigurationView<Proxy
writeConfigOption(TenantConfigurationKey.ANONYMOUS_DOWNLOAD_MODE_ENABLED, writeConfigOption(TenantConfigurationKey.ANONYMOUS_DOWNLOAD_MODE_ENABLED,
getBinderBean().isDownloadAnonymous()); getBinderBean().isDownloadAnonymous());
if (getBinderBean().isGatewaySecToken()) {
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_KEY,
getBinderBean().getGatewaySecurityToken());
}
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_HEADER_ENABLED, writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_HEADER_ENABLED,
getBinderBean().isCertificateAuth()); getBinderBean().isCertificateAuth());
if (getBinderBean().isCertificateAuth()) { if (getBinderBean().isCertificateAuth()) {