Skip to content

Commit

Permalink
Allow multiple request matchers to decide if require CSRF (#7123)
Browse files Browse the repository at this point in the history
* Allow multiple request matchers to decide if require CSRF token
* Fix a problem with the Keycloak configuration that was making the server
to require CSRF tokens for GN URLs that shouldn't be protected by CSRF.
To do this, now the GeonetworkCsrfSecurityRequestMatcher allows to be configured
with additional RequestMatchers, for example the KeycloakCsrfRequestMatcher.
If Keycloak security is enabled, then the KeycloakCsrfRequestMatcher is added to the
original GeoNetwork patterns listed in `config-security-core.xml`
  • Loading branch information
juanluisrp authored Jun 2, 2023
1 parent 376e9e6 commit bb48909
Show file tree
Hide file tree
Showing 5 changed files with 240 additions and 20 deletions.
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ git*.properties
*/*/target/
*/target/
.*/
GeoNetwork*
#GeoNetwork*
/geonetwork*
camel-harvesters/wfsfeature-harvester/logs
changes-*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2001-2017 Food and Agriculture Organization of the
* Copyright (C) 2001-2023 Food and Agriculture Organization of the
* United Nations (FAO-UN), United Nations World Food Programme (WFP)
* and United Nations Environment Programme (UNEP)
*
Expand All @@ -24,6 +24,7 @@
package org.fao.geonet.kernel.security.keycloak;

import org.fao.geonet.Constants;
import org.fao.geonet.security.web.csrf.GeonetworkCsrfSecurityRequestMatcher;
import org.keycloak.adapters.spi.UserSessionManagement;
import org.keycloak.adapters.springsecurity.filter.KeycloakCsrfRequestMatcher;
import org.keycloak.adapters.springsecurity.filter.KeycloakPreAuthActionsFilter;
Expand All @@ -41,29 +42,30 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.net.URLEncoder;
import java.io.IOException;
import java.net.URLEncoder;

public class keycloakPreAuthActionsLoginFilter extends KeycloakPreAuthActionsFilter {
public static String signinPath = null;
public class KeycloakPreAuthActionsLoginFilter extends KeycloakPreAuthActionsFilter {

@Autowired
LoginUrlAuthenticationEntryPoint loginUrlAuthenticationEntryPoint;

public keycloakPreAuthActionsLoginFilter(UserSessionManagement userSessionManagement) {
public KeycloakPreAuthActionsLoginFilter(UserSessionManagement userSessionManagement) {
super(userSessionManagement);
}

public keycloakPreAuthActionsLoginFilter(UserSessionManagement userSessionManagement, CsrfFilter csrfFilter) {
public KeycloakPreAuthActionsLoginFilter(UserSessionManagement userSessionManagement, CsrfFilter csrfFilter,
GeonetworkCsrfSecurityRequestMatcher csrfRequestMatcher) {
super(userSessionManagement);
// Set the csrf filter request matcher for Keycloak so that it allows k_* endpoints to be reached without CSRF.
// Without this fix, the backchannel logout was not working due to CSRF failures.
csrfFilter.setRequireCsrfProtectionMatcher(new KeycloakCsrfRequestMatcher());
// Without this fix, the back-channel logout was not working due to CSRF failures.
csrfRequestMatcher.addRequestMatcher(new KeycloakCsrfRequestMatcher());
csrfFilter.setRequireCsrfProtectionMatcher(csrfRequestMatcher);
}

@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
throws IOException, ServletException {
throws IOException, ServletException {
HttpServletRequest servletRequest = (HttpServletRequest) request;
HttpServletResponse servletResponse = (HttpServletResponse) response;

Expand All @@ -76,7 +78,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
if (servletRequest.getPathInfo() != null &&
!KeycloakAuthenticationProcessingFilter.DEFAULT_REQUEST_MATCHER.matches(servletRequest) &&
!isAuthenticated() &&
!(servletRequest.getContextPath() + KeycloakUtil.getSigninPath()).equals(servletRequest.getRequestURI()) &&
!(servletRequest.getContextPath() + KeycloakUtil.getSigninPath()).equals(servletRequest.getRequestURI()) &&
!servletRequest.getRequestURI().endsWith(AdapterConstants.K_LOGOUT) &&
!servletRequest.getRequestURI().endsWith(AdapterConstants.K_PUSH_NOT_BEFORE) &&
!servletRequest.getRequestURI().endsWith(AdapterConstants.K_QUERY_BEARER_TOKEN) &&
Expand All @@ -102,7 +104,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
private boolean isAuthenticated() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null || AnonymousAuthenticationToken.class.
isAssignableFrom(authentication.getClass())) {
isAssignableFrom(authentication.getClass())) {
return false;
}
return authentication.isAuthenticated();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2001-2016 Food and Agriculture Organization of the
* Copyright (C) 2001-2023 Food and Agriculture Organization of the
* United Nations (FAO-UN), United Nations World Food Programme (WFP)
* and United Nations Environment Programme (UNEP)
*
Expand All @@ -25,32 +25,61 @@

import org.springframework.security.web.util.matcher.RegexRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.util.Assert;

import javax.servlet.http.HttpServletRequest;
import java.util.Arrays;
import java.util.LinkedHashSet;
import java.util.Set;
import java.util.regex.Pattern;

/**
* RequestMatcher to exclude the CSRF token from requests.
*
* <p>
* Useful to exclude CSW POST requests for GetRecords.
*
* @author Jose García
* @author Jose García.
*/
public class GeonetworkCsrfSecurityRequestMatcher implements RequestMatcher {
private Pattern allowedMethods = Pattern.compile("^(GET|HEAD|TRACE|OPTIONS)$");
private RegexRequestMatcher unprotectedMatcher;
private Set<RequestMatcher> otherMatchers = new LinkedHashSet<>();

public GeonetworkCsrfSecurityRequestMatcher(Set<String> unprotectedUrlPatterns) {
unprotectedMatcher = new RegexRequestMatcher(String.join("|", unprotectedUrlPatterns), null);
}

/**
* Adds additional RequestMatchers used if the list of unprotectedUrlPatters don't match. The check is done in the
* order in what the matchers have been added.
* The matcher must be negative, that's it, it will return false for the patterns that match the patterns.
*
* @param matcher
*/
public void addRequestMatcher(RequestMatcher... matcher) {
Assert.notNull(matcher, "To add additional matchers the parameter matcher cannot be null");
otherMatchers.addAll(Arrays.asList(matcher));
}

/**
* Return {@code}true{@code} if the request doesn't match the methods and patterns defined.
*
* @param request the request to check for a match
* @return
*/
@Override
public boolean matches(HttpServletRequest request) {
if(allowedMethods.matcher(request.getMethod()).matches()){
return false;
boolean result = true;
if (allowedMethods.matcher(request.getMethod()).matches()) {
result = false;
}
if (result) {
result = !unprotectedMatcher.matches(request);
}

return !unprotectedMatcher.matches(request);
if (result && !otherMatchers.isEmpty()) {
result = otherMatchers.stream().anyMatch(matcher -> matcher.matches(request));
}
return result;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
/*
* Copyright (C) 2001-2023 Food and Agriculture Organization of the
* United Nations (FAO-UN), United Nations World Food Programme (WFP)
* and United Nations Environment Programme (UNEP)
*
* This program 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 of the License, or (at
* your option) any later version.
*
* This program 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 this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
*
* Contact: Jeroen Ticheler - FAO - Viale delle Terme di Caracalla 2,
* Rome - Italy. email: geonetwork@osgeo.org
*/
package org.fao.geonet.security.web.csrf;

import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import org.junit.Before;
import org.junit.Test;
import org.keycloak.adapters.springsecurity.filter.KeycloakCsrfRequestMatcher;
import org.keycloak.constants.AdapterConstants;
import org.springframework.http.HttpMethod;
import org.springframework.mock.web.MockHttpServletRequest;

import java.util.Set;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class GeonetworkCsrfSecurityRequestMatcherTest {

private static final String ROOT_CONTEXT_PATH = "";
private static final String SUB_CONTEXT_PATH = "/foo";

private GeonetworkCsrfSecurityRequestMatcher matcher;
private MockHttpServletRequest request;

@Before
public void setUp() {
request = new MockHttpServletRequest();
Set<String> notCorsPaths = Sets.newLinkedHashSet(Lists.newArrayList(
"/[a-zA-Z0-9_\\-]+/[a-z]{2,3}/csw-publication!?.*",
"/[a-zA-Z0-9_\\-]+/[a-z]{2,3}/csw-.*",
"/[a-zA-Z0-9_\\-]+/[a-z]{2,3}/csw!?.*",
"/[a-zA-Z0-9_\\-]+/api/search/.*",
"/[a-zA-Z0-9_\\-]+/api/site")
);
matcher = new GeonetworkCsrfSecurityRequestMatcher(notCorsPaths);
}

@Test
public void testDefaultConfigHttpMethods() {

request.setMethod(HttpMethod.GET.name());
assertFalse((matcher.matches(request)));

request.setMethod(HttpMethod.HEAD.name());
assertFalse((matcher.matches(request)));
request.setMethod(HttpMethod.TRACE.name());
assertFalse((matcher.matches(request)));
request.setMethod(HttpMethod.OPTIONS.name());
assertFalse((matcher.matches(request)));

request.setMethod(HttpMethod.PATCH.name());
assertTrue(matcher.matches(request));
request.setMethod(HttpMethod.POST.name());
assertTrue(matcher.matches(request));
request.setMethod(HttpMethod.PUT.name());
assertTrue(matcher.matches(request));
request.setMethod(HttpMethod.DELETE.name());
assertTrue(matcher.matches(request));
}

@Test
public void testDefaultMatchesMethodPost() throws Exception {
prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));

prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw-publication");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw-publication");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw-foo");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw-foo");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/api/search/foo");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/api/search/foo");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/api/site");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/api/site");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "k_logout");
assertTrue(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "k_logout");
assertTrue(matcher.matches(request));
}

@Test
public void testKeycloakCorsFilter() {
matcher.addRequestMatcher(new KeycloakCsrfRequestMatcher());

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "some/random/uri");
assertTrue(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw-publication");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw-publication");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw-foo");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw-foo");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/eng/csw");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/eng/csw");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/api/search/foo");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/api/search/foo");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, "srv/api/site");
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, "srv/api/site");
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, AdapterConstants.K_LOGOUT);
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, AdapterConstants.K_LOGOUT);
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, AdapterConstants.K_PUSH_NOT_BEFORE);
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, AdapterConstants.K_PUSH_NOT_BEFORE);
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, AdapterConstants.K_QUERY_BEARER_TOKEN);
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, AdapterConstants.K_QUERY_BEARER_TOKEN);
assertFalse(matcher.matches(request));

prepareRequest(HttpMethod.POST, ROOT_CONTEXT_PATH, AdapterConstants.K_TEST_AVAILABLE);
assertFalse(matcher.matches(request));
prepareRequest(HttpMethod.POST, SUB_CONTEXT_PATH, AdapterConstants.K_TEST_AVAILABLE);
assertFalse(matcher.matches(request));

}

private void prepareRequest(HttpMethod method, String contextPath, String uri) {
request.setMethod(method.name());
request.setContextPath(contextPath);
request.setRequestURI(contextPath + "/" + uri);
request.setServletPath("/" + uri);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>

<!--
~ Copyright (C) 2001-2016 Food and Agriculture Organization of the
~ Copyright (C) 2001-2023 Food and Agriculture Organization of the
~ United Nations (FAO-UN), United Nations World Food Programme (WFP)
~ and United Nations Environment Programme (UNEP)
~
Expand Down Expand Up @@ -76,9 +76,10 @@
<bean id="keycloakPreAuthActionsFilter" class="org.keycloak.adapters.springsecurity.filter.KeycloakPreAuthActionsFilter">
<constructor-arg ref="userSessionManagement" />
</bean>
<bean id="keycloakPreAuthActionsLoginFilter" class="org.fao.geonet.kernel.security.keycloak.keycloakPreAuthActionsLoginFilter" >
<bean id="keycloakPreAuthActionsLoginFilter" class="org.fao.geonet.kernel.security.keycloak.KeycloakPreAuthActionsLoginFilter" >
<constructor-arg ref="userSessionManagement" />
<constructor-arg ref="csrfFilter" />
<constructor-arg ref="geonetworkCsrfSecurityRequestMatcher" />
</bean>
<bean id="keycloakAuthenticationProcessingFilter" class="org.fao.geonet.kernel.security.keycloak.KeycloakAuthenticationProcessingFilter" depends-on="keycloakUtil">
<constructor-arg name="authenticationManager" ref="authenticationManager" />
Expand Down

0 comments on commit bb48909

Please sign in to comment.