Skip to content

Commit

Permalink
Small security improvements (#1412)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
avgustinmm committed Aug 16, 2023
1 parent a5dba29 commit acff82f
Show file tree
Hide file tree
Showing 10 changed files with 71 additions and 69 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,18 @@ public Message onAuthenticationRequest(final Message message) {
* 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);
}
Expand Down Expand Up @@ -198,15 +198,15 @@ private static DmfArtifact convertDbArtifact(final Artifact dbArtifact) {
@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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,14 @@ private void resolveTenant(final DmfTenantSecurityToken securityToken) {
}

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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,13 @@ public void doFilter(final ServletRequest request, final ServletResponse respons
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 {
Expand Down Expand Up @@ -129,7 +129,7 @@ protected void successfulAuthentication(final HttpServletRequest request, final
* 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)) {
Expand All @@ -142,7 +142,7 @@ protected DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpSe
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<String, String> extractUriTemplateVariables = pathExtractor.extractUriTemplateVariables(
Expand All @@ -151,7 +151,7 @@ protected DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpSe
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,
Expand All @@ -161,33 +161,33 @@ protected DmfTenantSecurityToken createTenantSecruityTokenVariables(final HttpSe
}
}

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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ protected AbstractControllerAuthenticationFilter(final TenantConfigurationManage
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<Boolean> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ public ControllerPreAuthenticateSecurityTokenFilter(
}

@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);
return new HeaderAuthentication(controllerId, authHeader.substring(OFFSET_TARGET_TOKEN));
}
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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ public ControllerPreAuthenticatedAnonymousDownload(
}

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,17 @@ public ControllerPreAuthenticatedAnonymousFilter(final DdiSecurityProperties ddi
}

@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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
* 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.}
*
Expand Down Expand Up @@ -55,25 +55,27 @@ public ControllerPreAuthenticatedGatewaySecurityTokenFilter(
}

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,26 @@ public interface PreAuthenticationFilter {
/**
* 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
*/
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,18 +165,18 @@ UIComponentIdProvider.DOWNLOAD_ANONYMOUS_CHECKBOX, getBinder(),

@Override
public void save() {
if (getBinderBean().isGatewaySecToken()) {
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_KEY,
getBinderBean().getGatewaySecurityToken());
}

writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_TARGET_SECURITY_TOKEN_ENABLED,
getBinderBean().isTargetSecToken());
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_ENABLED,
getBinderBean().isGatewaySecToken());
writeConfigOption(TenantConfigurationKey.ANONYMOUS_DOWNLOAD_MODE_ENABLED,
getBinderBean().isDownloadAnonymous());

if (getBinderBean().isGatewaySecToken()) {
writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_GATEWAY_SECURITY_TOKEN_KEY,
getBinderBean().getGatewaySecurityToken());
}

writeConfigOption(TenantConfigurationKey.AUTHENTICATION_MODE_HEADER_ENABLED,
getBinderBean().isCertificateAuth());
if (getBinderBean().isCertificateAuth()) {
Expand Down

0 comments on commit acff82f

Please sign in to comment.