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

WARN when ignoring antMatchers - please use permitAll #10938

Closed
pblanchardie opened this issue Mar 7, 2022 · 6 comments
Closed

WARN when ignoring antMatchers - please use permitAll #10938

pblanchardie opened this issue Mar 7, 2022 · 6 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply

Comments

@pblanchardie
Copy link

When I use web.ignoring().antMatchers() I'd like to see a DEBUG message instead of a WARNING for each ignored pattern.
I'm confused by the message saying it's not recommended and I should use permitAll instead, as I don't want Spring Security filter chain on these paths.

Since a09f6e1, there is a WARNING for each ignored pattern saying:

You are asking Spring Security to ignore Ant [pattern='/foo.bar']. This is not recommended -- please use permitAll via HttpSecurity#authorizeHttpRequests instead.

Before that, we had no warning.

Why is ignoring antMatchers not recommended anymore? Why should I prefer permitAll for eg. static resources?

@pblanchardie pblanchardie added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Mar 7, 2022
@jzheaux jzheaux self-assigned this Mar 8, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Mar 8, 2022

Hi, @pblanchardie, good question.

web.ignoring() means that Spring Security cannot provide any security headers or other protective measures on those endpoints. Instead, using permitAll allows Spring Security to write headers and otherwise secure the request without requiring authorization. This is why permitAll is recommended. The warning message is intended to alert you to the tradeoff.

If needed, you can use web.ignoring() as a performance optimization for static resources as it avoids looking up the SecurityContext from the session. However, once #10913 is completed, this performance benefit will go away.

In the meantime, if you don't want the warning message and you need the performance optimization, you can introduce a second filter chain for static resources like so:

@Bean 
@Order(0)
SecurityFilterChain resources(HttpSecurity http) throws Exception {
    http
        .requestMatchers((matchers) -> matchers.antMatchers("/static/**"))
        .authorizeHttpRequests((authorize) -> authorize.anyRequest().permitAll())
        .requestCache().disable()
        .securityContext().disable()
        .sessionManagement().disable();

    return http.build();
}

This second filter chain will secure the static resources without reading the SecurityContext from the session.

@jzheaux jzheaux closed this as completed Mar 8, 2022
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Mar 8, 2022
@pblanchardie
Copy link
Author

Hi @jzheaux thanks a lot for your quick answer, explanation and tips. It makes sense now!

@jebeaudet
Copy link

Hi

I've noticed the warnings in my logs today. My problem with the permitAll approach is that if a token (or other mean of authentication) is sent on a request, the code will try to authenticate the request. This could mean a call to an external service, or redis for a session cookie, or some other service. And if that authentication token is not valid, the user will receive a 401.

So, for example, if you have a path /static that you want to be wide open, this will happen
GET -H 'Authorization: validToken' /static -> HTTP 200 but it might call an external service
GET -H 'Authorization: invalidToken' /static -> HTTP 401 but it might call an external service
GET /static -> HTTP 200

From a resiliency perspective, if I use a session that is backed by a redis server and that server is down, it will throw an exception and my users won't be able to access static resources or any path that the session is not strictly required.

Will #10913 fix this behavior?

Thanks!

@marcusdacoregio
Copy link
Contributor

Hi @jebeaudet, have you disabled the following Configurers for your permitAll paths?

http
        .requestMatchers((matchers) -> matchers.antMatchers("/static/**"))
        .authorizeHttpRequests((authorize) -> authorize.anyRequest().permitAll())
        .requestCache().disable()
        .securityContext().disable()
        .sessionManagement().disable();

With those configurations disabled, Spring Security will not look up the session and, therefore, will not try to authenticate the user.

And yes, #10913 will fix that.

@jebeaudet
Copy link

jebeaudet commented Apr 6, 2022

Hi @marcusdacoregio, thanks for chiming in.

Reading your message made me realize that Josh also put those disable() in his message, I missed that originally, sorrya bout that. However, my initial impression is that it's 1. much more verbose and 2. error prone. What if you forget a spring security feature that relies on authentication? What if a new configurer is added retroactively to Spring Security?

The web ignoring way was straightforward. Also, the log message does not mention the disable() requirements to get a 1:1 replacement, applying only the warning message advice will lead to the situation I mentioned in my previous post. If the suggested replacement is the official way of migrating from web.ignoring(), it would be appreciated to get a proper replacement on HttpSecurity directly?

Good news that #10913 will fix that, however I'm curious as to whether it'll cover the requestCache and sessionManagement disabling?

Thanks again

@jebeaudet
Copy link

@marcusdacoregio sorry for the ping but would you have a chance to answer my question? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants