Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unintuitive behavior of multiple servlet contexts and HttpSecurity#securityMatcher pattern #15004

Closed
arvyy opened this issue May 3, 2024 · 5 comments
Assignees
Labels
in: docs An issue in Documentation or samples status: invalid An issue that we don't feel is valid type: enhancement A general enhancement

Comments

@arvyy
Copy link

arvyy commented May 3, 2024

Expected Behavior

http.securityMatcher("/actuator/**")

I expected above would match all actuator endpoints and apply the security filter configuration

Current Behavior

If there are more servlet contexts deployed (?) under subpath, security matcher tries to match against relative path from that other servlet context. Specifically, after adding hawtio project, I can see that during request to /actuator/hawtio/keycloak/enabled, the configured matcher is compared to enabled path, and since enabled doesn't match /actuator/**, the current filter chain is skipped and next one is tried.

Context

I've marked this as enhancement instead of a bug, because I presume this could be considered a desired behavior. The solution perhaps could be to explicitly specify this in javadoc of securityMatcher?

A workaround in my case is to use http.securityMatcher(request -> request.getServletPath().startsWith("/actuator"))

@arvyy arvyy added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels May 3, 2024
@sjohnr
Copy link
Member

sjohnr commented May 9, 2024

@arvyy thanks for reaching out!

There's a few things going on in your comment and I feel there might be missing information. I'd like to make sure I (or anyone else reviewing this issue) understands clearly. Can you please provide a minimal, reproducible sample that demonstrates a) the issue you are facing and/or b) what you're trying to do via the workaround? You can provide either steps to reproduce or a MockMvc-style test.

@sjohnr sjohnr self-assigned this May 9, 2024
@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels May 9, 2024
@arvyy
Copy link
Author

arvyy commented May 16, 2024

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 16, 2024
@sjohnr
Copy link
Member

sjohnr commented May 16, 2024

Thanks for providing the sample, @arvyy! That is very helpful in clarifying what you're trying to do. In Spring Security 6.x, the matching has indeed changed such that MVC-style matching is automatically applied when Spring MVC is on the classpath. This has the implication that other servlets need to be specifically matched, otherwise the Spring MVC servlet is being matched.

The servlet path for actuator endpoints varies by the endpoint. In your case, it will be /actuator/hawtio/keycloak. I'm not very familiar with hawtio actuator integration, so I don't know why /keycloak is part of the servlet path, but it appears that it is. Therefore, you should use MvcRequestMatcher.Builder (as suggested in the docs, see also gh-13562) like this:

@Bean
@Order(1)
SecurityFilterChain filterChainActuator(HttpSecurity http, HandlerMappingIntrospector introspector) throws Exception {

    MvcRequestMatcher.Builder mvc =
        new MvcRequestMatcher.Builder(introspector);

    MvcRequestMatcher hawtioMatcher =
        mvc.servletPath("/actuator/hawtio/keycloak")
            .pattern("/**");

    http
        .securityMatcher(hawtioMatcher)
        .authorizeHttpRequests(auth -> auth
            .anyRequest().permitAll()
        );

    return http.build();
}

Note that depending on what else you're trying to do with multiple SecurityFilterChains, you could actually combine them into a single filter chain (which would be preferable in simple cases) like this:

@Bean
SecurityFilterChain securityFilterChain(HttpSecurity http, HandlerMappingIntrospector introspector) throws Exception {

    MvcRequestMatcher.Builder mvc =
        new MvcRequestMatcher.Builder(introspector);

    MvcRequestMatcher hawtioMatcher =
        mvc.servletPath("/actuator/hawtio/keycloak")
            .pattern("/**");

    http
        .authorizeHttpRequests(auth -> auth
            .requestMatchers(hawtioMatcher).permitAll()
            .anyRequest().authenticated()
        )
        .formLogin(Customizer.withDefaults());

    return http.build();
}

I don't think the startsWith workaround would make a good recommendation here since there is a specific servlet in play that you need to match on and we don't usually want to expose all actuator endpoints by default.

I do feel like the information for how to do this is quite findable, for example starting from Security Matchers under Authorize HttpServletRequests. Perhaps just improving javadoc would be in order. Or do you feel more could be done with reference docs?

Would you be interested in submitting a PR for one or both? I'm happy to work through it with you.

@arvyy
Copy link
Author

arvyy commented May 29, 2024

I don't think the startsWith workaround would make a good recommendation here since there is a specific servlet in play that you need to match on and we don't usually want to expose all actuator endpoints by default.

I do want anything under /actuator to have its own security configuration, regardless what servlets are involved there. Config here is simplified because it's just a reproduction example

Perhaps just improving javadoc would be in order

yes, that's my initial assumption as well. Although maybe hawtio is exceeding edge case, and maybe it's not worth it overall? I don't know. Maybe it's not worth addressing afterall

@sjohnr
Copy link
Member

sjohnr commented May 30, 2024

I do want anything under /actuator to have its own security configuration, regardless what servlets are involved there. Config here is simplified because it's just a reproduction example

Understood. So in that case you would want to do

http.securityMatcher(AntPathRequestMatcher.antMatcher("/actuator/**"))

which is documented with an example here.

Perhaps just improving javadoc would be in order.

As I review the javadoc, I actually feel it has quite enough to give context and pointers for both MVC-style and ant-style matching, so I'm not sure what more we could add there either.

maybe it's not worth it overall?

I think enhancing docs is always worth it if something is missing or has room for improvement. In this case, I'm not finding anything missing, but if you have a specific enhancement in mind, please feel free to suggest it or even open a pull request (referencing this issue) and I'll be happy to work with you to get it incorporated.

For now however, I'm going to close this issue as I don't find anything actionable. If you find or think of something we can reopen this and address it at that point. Thanks again for reaching out!

@sjohnr sjohnr closed this as completed May 30, 2024
@sjohnr sjohnr added status: invalid An issue that we don't feel is valid in: docs An issue in Documentation or samples and removed status: feedback-provided Feedback has been provided in: web An issue in web modules (web, webmvc) labels May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples status: invalid An issue that we don't feel is valid type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants