Skip to content

Commit 611cdad

Browse files
joaquinfelicijoaquinfelici
committed
fix: isolate CSRF cookie per physical-tenant scope
1 parent bdf4081 commit 611cdad

6 files changed

Lines changed: 230 additions & 23 deletions

File tree

‎spring-boot-starter/src/main/java/io/camunda/security/spring/scope/ScopedSecurityChainRegistrar.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
final class ScopedSecurityChainRegistrar implements BeanDefinitionRegistryPostProcessor {
4848

4949
static final String SESSION_COOKIE_PREFIX = "camunda-session-";
50+
static final String CSRF_COOKIE_PREFIX = "camunda-csrf-";
5051
static final int MAX_COOKIE_NAME_LENGTH =
5152
200; // well under the RFC 6265 4096-byte name=value budget
5253

@@ -273,7 +274,8 @@ private OrderedSecurityFilterChainWrapper buildWebappChain(
273274
descriptor.basePath(),
274275
descriptor.authentication(),
275276
sessionFilter,
276-
sessionCookieName(descriptor.basePath()));
277+
sessionCookieName(descriptor.basePath()),
278+
csrfCookieName(descriptor.basePath()));
277279
return new OrderedSecurityFilterChainWrapper(chain, ORDER_WEBAPP_API);
278280
} catch (final IllegalStateException ex) {
279281
throw ex;
@@ -288,6 +290,11 @@ static String sessionCookieName(final String basePath) {
288290
return SESSION_COOKIE_PREFIX + sanitizeBasePath(basePath);
289291
}
290292

293+
/** The per-scope CSRF cookie name: {@code camunda-csrf-<sanitize(basePath)>}. */
294+
static String csrfCookieName(final String basePath) {
295+
return CSRF_COOKIE_PREFIX + sanitizeBasePath(basePath);
296+
}
297+
291298
static void rejectCookieNameCollisions(final List<ScopedSecurityDescriptor> descriptors) {
292299
final var seen = new HashSet<String>();
293300
final var collisions = new LinkedHashSet<String>();

‎spring-boot-starter/src/main/java/io/camunda/security/spring/security/ScopedWebappSecurityChainBuilder.java‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,8 @@ public SecurityFilterChain buildScopedWebappChain(
306306
final String basePath,
307307
final AuthenticationConfiguration authentication,
308308
final SessionRepositoryFilter<?> sessionRepositoryFilter,
309-
final String scopedSessionCookieName)
309+
final String scopedSessionCookieName,
310+
final String scopedCsrfCookieName)
310311
throws Exception {
311312
Objects.requireNonNull(http, "http must not be null");
312313
Objects.requireNonNull(basePath, "basePath must not be null");
@@ -316,6 +317,7 @@ public SecurityFilterChain buildScopedWebappChain(
316317
Objects.requireNonNull(authentication.getMethod(), "authentication.method must not be null");
317318
Objects.requireNonNull(sessionRepositoryFilter, "sessionRepositoryFilter must not be null");
318319
Objects.requireNonNull(scopedSessionCookieName, "scopedSessionCookieName must not be null");
320+
Objects.requireNonNull(scopedCsrfCookieName, "scopedCsrfCookieName must not be null");
319321
Objects.requireNonNull(
320322
authorizedClientManagerFactory, "authorizedClientManagerFactory must not be null");
321323
Objects.requireNonNull(
@@ -335,10 +337,15 @@ public SecurityFilterChain buildScopedWebappChain(
335337
return switch (authentication.getMethod()) {
336338
case OIDC ->
337339
buildOidcWebappChainInternal(
338-
http, prefix, authentication, sessionRepositoryFilter, scopedSessionCookieName);
340+
http,
341+
prefix,
342+
authentication,
343+
sessionRepositoryFilter,
344+
scopedSessionCookieName,
345+
scopedCsrfCookieName);
339346
case BASIC ->
340347
buildBasicWebappChainInternal(
341-
http, prefix, sessionRepositoryFilter, scopedSessionCookieName);
348+
http, prefix, sessionRepositoryFilter, scopedSessionCookieName, scopedCsrfCookieName);
342349
default ->
343350
throw new IllegalStateException(
344351
"Unsupported authentication method: " + authentication.getMethod());
@@ -404,7 +411,8 @@ private SecurityFilterChain buildOidcWebappChainInternal(
404411
final String prefix,
405412
final AuthenticationConfiguration authentication,
406413
final SessionRepositoryFilter<?> sessionRepositoryFilter,
407-
final String scopedSessionCookieName)
414+
final String scopedSessionCookieName,
415+
final String scopedCsrfCookieName)
408416
throws Exception {
409417

410418
final var matchers = pathPort.webappPaths().stream().map(p -> prefix + p).toList();
@@ -490,7 +498,7 @@ private SecurityFilterChain buildOidcWebappChainInternal(
490498
.addLogoutHandler(
491499
pathScopedCookieClearingLogoutHandler(scopedSessionCookieName, prefix))
492500
.addLogoutHandler(
493-
pathScopedCookieClearingLogoutHandler(X_CSRF_TOKEN, prefix));
501+
pathScopedCookieClearingLogoutHandler(scopedCsrfCookieName, prefix));
494502
logoutSuccessHandlerProvider.ifAvailable(logout::logoutSuccessHandler);
495503
});
496504

@@ -499,7 +507,7 @@ private SecurityFilterChain buildOidcWebappChainInternal(
499507
final var logoutHandler =
500508
new CompositeLogoutHandler(
501509
pathScopedCookieClearingLogoutHandler(scopedSessionCookieName, prefix),
502-
pathScopedCookieClearingLogoutHandler(X_CSRF_TOKEN, prefix),
510+
pathScopedCookieClearingLogoutHandler(scopedCsrfCookieName, prefix),
503511
new SecurityContextLogoutHandler());
504512
filterChainBuilder.addFilterAfter(
505513
new OAuth2RefreshTokenFilter(
@@ -515,7 +523,7 @@ private SecurityFilterChain buildOidcWebappChainInternal(
515523
}
516524

517525
SecurityFilterChainSupport.applyCsrfConfiguration(
518-
filterChainBuilder, properties, pathPort, prefix);
526+
filterChainBuilder, properties, pathPort, prefix, scopedCsrfCookieName);
519527
SecurityFilterChainSupport.setupSecureHeaders(filterChainBuilder, properties.getHttpHeaders());
520528

521529
// Install the multi-IdP login picker (GH-269): the custom entry point trips
@@ -532,7 +540,8 @@ private SecurityFilterChain buildBasicWebappChainInternal(
532540
final HttpSecurity http,
533541
final String prefix,
534542
final SessionRepositoryFilter<?> sessionRepositoryFilter,
535-
final String scopedSessionCookieName)
543+
final String scopedSessionCookieName,
544+
final String scopedCsrfCookieName)
536545
throws Exception {
537546

538547
final var matchers = pathPort.webappPaths().stream().map(p -> prefix + p).toList();
@@ -572,7 +581,7 @@ private SecurityFilterChain buildBasicWebappChainInternal(
572581
.addLogoutHandler(
573582
pathScopedCookieClearingLogoutHandler(scopedSessionCookieName, prefix))
574583
.addLogoutHandler(
575-
pathScopedCookieClearingLogoutHandler(X_CSRF_TOKEN, prefix)))
584+
pathScopedCookieClearingLogoutHandler(scopedCsrfCookieName, prefix)))
576585
.exceptionHandling(
577586
eh ->
578587
eh.authenticationEntryPoint(authFailureHandler)
@@ -590,7 +599,7 @@ private SecurityFilterChain buildBasicWebappChainInternal(
590599
}
591600

592601
SecurityFilterChainSupport.applyCsrfConfiguration(
593-
filterChainBuilder, properties, pathPort, prefix);
602+
filterChainBuilder, properties, pathPort, prefix, scopedCsrfCookieName);
594603
SecurityFilterChainSupport.setupSecureHeaders(filterChainBuilder, properties.getHttpHeaders());
595604

596605
return filterChainBuilder.build();

‎spring-boot-starter/src/main/java/io/camunda/security/spring/security/SecurityFilterChainSupport.java‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,19 +77,28 @@ static Set<String> csrfAllowedPaths(
7777

7878
public static CookieCsrfTokenRepository cookieCsrfTokenRepository(
7979
final CamundaSecurityLibraryProperties properties) {
80-
return cookieCsrfTokenRepository(properties, null);
80+
return buildCookieCsrfTokenRepository(properties, null, X_CSRF_TOKEN);
8181
}
8282

8383
public static CookieCsrfTokenRepository cookieCsrfTokenRepository(
8484
final CamundaSecurityLibraryProperties properties, final String cookiePath) {
85-
return buildCookieCsrfTokenRepository(properties, resolveCookiePath(cookiePath));
85+
return buildCookieCsrfTokenRepository(properties, resolveCookiePath(cookiePath), X_CSRF_TOKEN);
86+
}
87+
88+
public static CookieCsrfTokenRepository cookieCsrfTokenRepository(
89+
final CamundaSecurityLibraryProperties properties,
90+
final String cookiePath,
91+
final String cookieName) {
92+
return buildCookieCsrfTokenRepository(properties, resolveCookiePath(cookiePath), cookieName);
8693
}
8794

8895
private static CookieCsrfTokenRepository buildCookieCsrfTokenRepository(
89-
final CamundaSecurityLibraryProperties properties, final String resolvedCookiePath) {
96+
final CamundaSecurityLibraryProperties properties,
97+
final String resolvedCookiePath,
98+
final String cookieName) {
9099
final CookieCsrfTokenRepository repository = new CookieCsrfTokenRepository();
91100
repository.setHeaderName(X_CSRF_TOKEN);
92-
repository.setCookieName(X_CSRF_TOKEN);
101+
repository.setCookieName(cookieName);
93102
final boolean httpOnly = properties.getCsrf().isCookieHttpOnly();
94103
repository.setCookieCustomizer(builder -> builder.httpOnly(httpOnly));
95104
if (resolvedCookiePath != null) {
@@ -122,7 +131,7 @@ public static void applyCsrfConfiguration(
122131
final CamundaSecurityLibraryProperties properties,
123132
final SecurityPathPort pathPort)
124133
throws Exception {
125-
applyCsrfConfiguration(http, properties, pathPort, null);
134+
applyCsrfConfiguration(http, properties, pathPort, null, X_CSRF_TOKEN);
126135
}
127136

128137
public static void applyCsrfConfiguration(
@@ -131,6 +140,16 @@ public static void applyCsrfConfiguration(
131140
final SecurityPathPort pathPort,
132141
final String cookiePath)
133142
throws Exception {
143+
applyCsrfConfiguration(http, properties, pathPort, cookiePath, X_CSRF_TOKEN);
144+
}
145+
146+
public static void applyCsrfConfiguration(
147+
final HttpSecurity http,
148+
final CamundaSecurityLibraryProperties properties,
149+
final SecurityPathPort pathPort,
150+
final String cookiePath,
151+
final String csrfCookieName)
152+
throws Exception {
134153
if (!properties.getCsrf().isEnabled()) {
135154
http.csrf(AbstractHttpConfigurer::disable);
136155
return;
@@ -140,7 +159,7 @@ public static void applyCsrfConfiguration(
140159

141160
final String resolvedCookiePath = resolveCookiePath(cookiePath);
142161
final CookieCsrfTokenRepository repo =
143-
buildCookieCsrfTokenRepository(properties, resolvedCookiePath);
162+
buildCookieCsrfTokenRepository(properties, resolvedCookiePath, csrfCookieName);
144163
final CsrfTokenRepository csrfTokenRepository =
145164
(resolvedCookiePath != null)
146165
? new ContextPathScopedCsrfTokenRepository(repo, resolvedCookiePath)

‎spring-boot-starter/src/test/java/io/camunda/security/spring/scope/ScopedSecurityChainRegistrarTest.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,19 @@ void distinctBasePathsCanSanitiseToTheSameFragment() {
7171
.isEqualTo("a-b");
7272
}
7373

74+
@Test
75+
void csrfCookieNameIsPrefixedWithCamundaCsrf() {
76+
assertThat(ScopedSecurityChainRegistrar.csrfCookieName("/physical-tenants/t1"))
77+
.isEqualTo("camunda-csrf-physical-tenants-t1");
78+
}
79+
80+
@Test
81+
void csrfAndSessionCookieNamesAreDifferentForTheSameBasePath() {
82+
final var basePath = "/physical-tenants/t1";
83+
assertThat(ScopedSecurityChainRegistrar.csrfCookieName(basePath))
84+
.isNotEqualTo(ScopedSecurityChainRegistrar.sessionCookieName(basePath));
85+
}
86+
7487
@Test
7588
void rejectsBasePathsThatSanitizeToCollidingCookieNames() {
7689
final var descriptors =

‎spring-boot-starter/src/test/java/io/camunda/security/spring/security/ScopedWebappSecurityChainBuilderScopedTest.java‎

Lines changed: 121 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -251,16 +251,75 @@ void scopedBasicChainLogoutClearsScopedCookiesAtBasePath() throws Exception {
251251
&& h.contains("Max-Age=0")
252252
&& h.contains("Path=" + BASE_PATH));
253253

254+
final var csrfCookieName = "camunda-csrf-physical-tenants-t1";
254255
assertThat(cookieHeaders)
255-
.as("logout must emit a Set-Cookie clearing the CSRF cookie at basePath")
256+
.as("logout must emit a Set-Cookie clearing the per-scope CSRF cookie")
256257
.anyMatch(
257258
h ->
258-
h.contains("X-CSRF-TOKEN=")
259+
h.contains(csrfCookieName + "=")
259260
&& h.contains("Max-Age=0")
260261
&& h.contains("Path=" + BASE_PATH));
261262
});
262263
}
263264

265+
@Test
266+
void twoScopedChainsHaveIndependentCsrfCookiesThatDoNotCrossContaminate() throws Exception {
267+
new WebApplicationContextRunner()
268+
.withUserConfiguration(
269+
ObjectMapperConfig.class,
270+
StubPaths.class,
271+
StubUserDetailsPortConfig.class,
272+
TwoScopedBasicChainsConfig.class)
273+
.withConfiguration(
274+
AutoConfigurations.of(
275+
CamundaSecurityConfiguration.class,
276+
BaseSecurityConfiguration.class,
277+
AuthFailureHandlerConfiguration.class,
278+
io.camunda.security.spring.user.UserConfiguration.class,
279+
ScopedOidcInfrastructureConfiguration.class,
280+
ScopedWebappSecurityChainBuilderConfiguration.class))
281+
.run(
282+
ctx -> {
283+
final var chainT1 = ctx.getBean("scopedBasicChainForT1", SecurityFilterChain.class);
284+
final var chainT2 = ctx.getBean("scopedBasicChainForT2", SecurityFilterChain.class);
285+
286+
final var requestT1 =
287+
new MockHttpServletRequest("POST", "/physical-tenants/t1/logout");
288+
final var responseT1 = new MockHttpServletResponse();
289+
new FilterChainProxy(List.of(chainT1))
290+
.doFilter(requestT1, responseT1, new MockFilterChain());
291+
292+
final var requestT2 =
293+
new MockHttpServletRequest("POST", "/physical-tenants/t2/logout");
294+
final var responseT2 = new MockHttpServletResponse();
295+
new FilterChainProxy(List.of(chainT2))
296+
.doFilter(requestT2, responseT2, new MockFilterChain());
297+
298+
final var cookiesT1 = responseT1.getHeaders("Set-Cookie");
299+
final var cookiesT2 = responseT2.getHeaders("Set-Cookie");
300+
301+
assertThat(cookiesT1)
302+
.as("t1 logout must clear its own per-scope CSRF cookie")
303+
.anyMatch(
304+
h ->
305+
h.contains("camunda-csrf-physical-tenants-t1=")
306+
&& h.contains("Max-Age=0"));
307+
assertThat(cookiesT1)
308+
.as("t1 logout must not touch t2's CSRF cookie")
309+
.noneMatch(h -> h.contains("camunda-csrf-physical-tenants-t2="));
310+
311+
assertThat(cookiesT2)
312+
.as("t2 logout must clear its own per-scope CSRF cookie")
313+
.anyMatch(
314+
h ->
315+
h.contains("camunda-csrf-physical-tenants-t2=")
316+
&& h.contains("Max-Age=0"));
317+
assertThat(cookiesT2)
318+
.as("t2 logout must not touch t1's CSRF cookie")
319+
.noneMatch(h -> h.contains("camunda-csrf-physical-tenants-t1="));
320+
});
321+
}
322+
264323
@Test
265324
void buildScopedWebappChainRejectsRootBasePath() {
266325
runner.run(
@@ -275,7 +334,12 @@ void buildScopedWebappChainRejectsRootBasePath() {
275334
.isThrownBy(
276335
() ->
277336
builder.buildScopedWebappChain(
278-
http, "/", authentication, sessionFilter, "session-cookie"))
337+
http,
338+
"/",
339+
authentication,
340+
sessionFilter,
341+
"session-cookie",
342+
"csrf-cookie"))
279343
.withMessageContaining("must not be the root path");
280344
});
281345
}
@@ -329,7 +393,12 @@ SecurityFilterChain scopedOidcTestChain(
329393
final var authentication = buildOidcAuthentication("oidc");
330394
final var sessionFilter = buildSessionFilter();
331395
return builder.buildScopedWebappChain(
332-
http, BASE_PATH, authentication, sessionFilter, "camunda-session-physical-tenants-t1");
396+
http,
397+
BASE_PATH,
398+
authentication,
399+
sessionFilter,
400+
"camunda-session-physical-tenants-t1",
401+
"camunda-csrf-physical-tenants-t1");
333402
}
334403

335404
private static SessionRepositoryFilter<?> buildSessionFilter() {
@@ -376,7 +445,12 @@ SecurityFilterChain scopedOidcMultiIdpTestChain(
376445
final var sessionFilter =
377446
new SessionRepositoryFilter<>(new MapSessionRepository(new ConcurrentHashMap<>()));
378447
return builder.buildScopedWebappChain(
379-
http, BASE_PATH, authentication, sessionFilter, "camunda-session-physical-tenants-t1");
448+
http,
449+
BASE_PATH,
450+
authentication,
451+
sessionFilter,
452+
"camunda-session-physical-tenants-t1",
453+
"camunda-csrf-physical-tenants-t1");
380454
}
381455

382456
private static AuthenticationConfiguration buildOidcAuthentication(
@@ -425,7 +499,48 @@ SecurityFilterChain scopedBasicTestChain(
425499
final var sessionFilter =
426500
new SessionRepositoryFilter<>(new MapSessionRepository(new ConcurrentHashMap<>()));
427501
return builder.buildScopedWebappChain(
428-
http, BASE_PATH, authentication, sessionFilter, "camunda-session-physical-tenants-t1");
502+
http,
503+
BASE_PATH,
504+
authentication,
505+
sessionFilter,
506+
"camunda-session-physical-tenants-t1",
507+
"camunda-csrf-physical-tenants-t1");
508+
}
509+
}
510+
511+
@Configuration
512+
static class TwoScopedBasicChainsConfig {
513+
514+
@Bean("scopedBasicChainForT1")
515+
SecurityFilterChain scopedBasicChainForT1(
516+
final HttpSecurity http, final ScopedWebappSecurityChainBuilder builder) throws Exception {
517+
final var authentication = new AuthenticationConfiguration();
518+
authentication.setMethod(AuthenticationMethod.BASIC);
519+
final var sessionFilter =
520+
new SessionRepositoryFilter<>(new MapSessionRepository(new ConcurrentHashMap<>()));
521+
return builder.buildScopedWebappChain(
522+
http,
523+
"/physical-tenants/t1",
524+
authentication,
525+
sessionFilter,
526+
"camunda-session-physical-tenants-t1",
527+
"camunda-csrf-physical-tenants-t1");
528+
}
529+
530+
@Bean("scopedBasicChainForT2")
531+
SecurityFilterChain scopedBasicChainForT2(
532+
final HttpSecurity http, final ScopedWebappSecurityChainBuilder builder) throws Exception {
533+
final var authentication = new AuthenticationConfiguration();
534+
authentication.setMethod(AuthenticationMethod.BASIC);
535+
final var sessionFilter =
536+
new SessionRepositoryFilter<>(new MapSessionRepository(new ConcurrentHashMap<>()));
537+
return builder.buildScopedWebappChain(
538+
http,
539+
"/physical-tenants/t2",
540+
authentication,
541+
sessionFilter,
542+
"camunda-session-physical-tenants-t2",
543+
"camunda-csrf-physical-tenants-t2");
429544
}
430545
}
431546
}

0 commit comments

Comments
 (0)