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

Disabling credentials erasure on custom AuthenticationManager is not working #15683

Closed
kmartin88 opened this issue Aug 23, 2024 · 7 comments
Closed
Assignees
Labels
in: docs An issue in Documentation or samples type: bug A general bug
Milestone

Comments

@kmartin88
Copy link

Describe the bug
In the documentation there is an example on how to customize the AuthenticationManager:
https://docs.spring.io/spring-security/reference/servlet/authentication/passwords/index.html#customize-global-authentication-manager (below "Publish AuthenticationManager bean for Spring Security").
Even though eraseCredentialsAfterAuthentication is set to false, the credentials get erased. They seem to get erased by another ProviderManager which has an AnonymousAuthenticationProvider.

To Reproduce
Create an empty Spring Boot Project with Spring Security and Spring Web MVC, create a @RestController and a SecurityConfig like in the example. The RestController should have a method which autowires the Authentication and returns the credentials of this Authentication object. Then you call the controller with user credentials and see that the credentials are empty/null.

Expected behavior
The password of the user should be returned when calling the controller.

@kmartin88 kmartin88 added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 23, 2024
@sjohnr sjohnr self-assigned this Aug 23, 2024
@sjohnr
Copy link
Member

sjohnr commented Aug 23, 2024

@kmartin88, thank you for reaching out!

Create an empty Spring Boot Project with Spring Security and Spring Web MVC, create a @RestController and a SecurityConfig like in the example. The RestController should have a method which autowires the Authentication and returns the credentials of this Authentication object. Then you call the controller with user credentials and see that the credentials are empty/null.

Can you please provide more information about the security configuration and rest controller? It would be helpful if you could provide actual code, so there is no chance of misunderstanding.

Currently, I am not seeing information about how you authenticate prior to calling an endpoint, and whether the endpoint you are calling is permitAll() or authenticated() in authorizeHttpRequests(), which I think would be an important detail for reproducing what you're seeing. Please provide full steps to reproduce the issue.

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: docs An issue in Documentation or samples and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 23, 2024
@kmartin88
Copy link
Author

@sjohnr
Hi, I created a sample project: https://github.com/kmartin88/demo
I hope this clarifies what I mean.

@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 Aug 26, 2024
@sjohnr
Copy link
Member

sjohnr commented Aug 26, 2024

Thanks for the sample, @kmartin88! That does indeed help clarify your situation. I do see what the issue is so let me explain that first:

Spring Security has a specific order in which it checks for beans that can be used to construct an AuthenticationProvider and/or AuthenticationManager.

  1. First, it looks to see if there is a unique AuthenticationProvider published by your application as a bean. If so, this is added to a list used to build an internal AuthenticationManager.
  2. Second, it looks to see if there is a unique UserDetailsService published by your application as a bean. If so, this is used to build a DaoAuthenticationProvider, which is then added to a list used to build an internal AuthenticationManager.
  3. Finally, if neither of the above are found (or there are multiple beans to choose from, which is non-unique) it looks to see if there is a unique or @Primary AuthenticationManager published by your application as a bean. If so, this is directly used as the internal AuthenticationManager (in addition to being available for your application as a @Bean.

The reason this ordering of checks is used is historical in Spring Security, so we probably couldn't switch the order to be more intuitive or else risk breaking applications relying on this order. What I believe you are hoping for in this case based on your sample is that you fall into the third (3) case. Unfortunately, you are falling into the second (2) case because you have published a UserDetailsService @Bean. Here is what the docs mention in context (emphasis added):

Publish an AuthenticationManager bean

A fairly common requirement is publishing an AuthenticationManager bean to allow for custom authentication, such as in a @Service or Spring MVC @Controller. For example, you may want to authenticate users via a REST API instead of using Form Login.

In fact, in your sample you are using a built-in authentication mechanism (e.g. HTTP Basic or Form Login).

It could be that the documentation should be modified to suggest a configuration that doesn't fall into the second case. However, I wonder if the documentation needs to be improved in some way to make this clearer or if this is simply something you missed when reading the documentation and modifying the code from there? I feel the current example and text in the docs make the use case fairly clear, but I imagine it could be confusing if the code is modified from there. Can you provide some feedback on this and whether you find the docs clear or confusing and in need of improvement?

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 26, 2024
@kmartin88
Copy link
Author

@sjohnr Thanks for the explanation!

Even if I don't publish a UserDetailsService bean I still see the same effect in my test case. So if I do it like this:

@Bean
public AuthenticationManager authenticationManager(PasswordEncoder passwordEncoder) {
        UserDetails userDetails = User.withDefaultPasswordEncoder()
                .username("user")
                .password("password")
                .roles("USER")
                .build();

        InMemoryUserDetailsManager inMemoryUserDetailsManager = new InMemoryUserDetailsManager(userDetails);
        DaoAuthenticationProvider authenticationProvider = new DaoAuthenticationProvider();
        authenticationProvider.setUserDetailsService(inMemoryUserDetailsManager);
        authenticationProvider.setPasswordEncoder(passwordEncoder);

        ProviderManager providerManager = new ProviderManager(authenticationProvider);
        providerManager.setEraseCredentialsAfterAuthentication(false);

        return providerManager;
}

and remove the UserDetailsService bean in the SecurityConfig, the credentials are still erased in the end.

As to if I find the documentation confusing - I guess so, yes. So first it tells me I can publish an AuthenticationManager bean to be able to use it directly in a RestController.
But then it says:

Normally, Spring Security builds an AuthenticationManager internally composed of a DaoAuthenticationProvider for username/password authentication. In certain cases, it may still be desired to customize the instance of AuthenticationManager used by Spring Security. For example, you may need to simply disable credential erasure for cached users.
The recommended way to do this is to simply publish your own AuthenticationManager bean, and Spring Security will use it. You can publish an AuthenticationManager using the following configuration:

Which sounds to me like it would be the way to go if I need to disable the credentials erasure regardless of how I use the AuthenticationManager.

The following:

Alternatively, you can take advantage of the fact that the AuthenticationManagerBuilder used to build Spring Security’s global AuthenticationManager is published as a bean. You can configure the builder as follows:

suggests (to me at least) that the following example is doing the same thing and both examples are interchangeable.

What I really want to accomplish is (as real life examples always are) a bit more complex. We have several SecurityFilterChains and depending on a configuration file we have two authentication mechanisms out of 4 in total which should be used globally. Currently, we are doing it like this:

@Autowired
public void configureAuthenticationManagerBuilder(AuthenticationManagerBuilder authenticationManagerBuilder) throws Exception {
        authenticationManagerBuilder.eraseCredentials(false);

        if (isFirstAuthMode()) {
            configureFirstAuthentication(authenticationManagerBuilder); //this calls ldapAuthentication() on the builder and configures the resulting configurer
        } else if (isSecondAuthMode()) {
            configureSecondAuthentication(authenticationManagerBuilder); //this creates a new instance of an AuthenticationProvider and adds it via authenticationProvider() to the builder
        } else if (isThirdAuthMode()) {
            configureThirdAuthentication(authenticationManagerBuilder); //this creates a new instance of an AuthenticationProvider and adds it via authenticationProvider() to the builder
        }

        addDefaultAuthentication(authenticationManagerBuilder); //this creates a new instance of an AuthenticationProvider and adds it via authenticationProvider() to the builder
}

and this is working fine. However, when looking at several examples on how to do it since Spring Security 6, it seems to me that configuring AuthenticationManagers like this is not the intended way.

@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 Aug 29, 2024
@sjohnr
Copy link
Member

sjohnr commented Aug 29, 2024

@kmartin88 thanks for the detailed response! It appears that this documentation page is indeed inaccurate and does need to be reworked so thanks for providing this feedback. I'll use this ticket to fix the mistake.

and this is working fine. However, when looking at several examples on how to do it since Spring Security 6, it seems to me that configuring AuthenticationManagers like this is not the intended way.

You are good to keep the config the way you have it. That is currently the most effective way to disable credential erasure.

Note: The reason the other configuration in the docs (publishing a @Bean AuthenticationManager) is not working as expected is because we are only configuring the so-called "global AuthenticationManager" which is a parent to any local authentication managers. The local one (created for each filter chain) only contains AnonymousAuthenticationProvider and falls back to the global one, but remains configured with the default (eraseCredentialsAfterAuthentication=true). After the global manager authenticates, the local one erases credentials. Somehow, I must have missed testing this scenario when updating the docs.

Thanks again!

@sjohnr sjohnr moved this to Prioritized in Spring Security Team Aug 29, 2024
@sjohnr sjohnr added this to the 5.8.15 milestone Aug 29, 2024
@kmartin88
Copy link
Author

@sjohnr Ah okay I see!

You are good to keep the config the way you have it. That is currently the most effective way to disable credential erasure.

Okay this is good to know. However, if I were to say for security reasons I only want to disable credentials erasure for one of my filter chains but aside from that want to use the same authentication mechanism in all of the filter chains - how would I achieve this? The only way I can think of would be to create a new instance of a ProviderManager locally in the "unsecure" filter chain and set it directly as authenticationManager on the HttpSecurity.

Like this:

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http,  
                                                   UserDetailsService userDetailsService,
                                                   PasswordEncoder passwordEncoder) throws Exception {
        DaoAuthenticationProvider authenticationProvider = new DaoAuthenticationProvider();
        authenticationProvider.setUserDetailsService(userDetailsService);
        authenticationProvider.setPasswordEncoder(passwordEncoder);

        ProviderManager providerManager = new ProviderManager(authenticationProvider);
        providerManager.setEraseCredentialsAfterAuthentication(false);
        http
                .authorizeHttpRequests((authorize) -> authorize
                        .requestMatchers("/login").permitAll()
                        .anyRequest().authenticated()
                )
                .httpBasic(Customizer.withDefaults())
                .formLogin(Customizer.withDefaults())
                .authenticationManager(providerManager);

        return http.build();
    }

    @Bean
    public SecurityFilterChain otherSecurityFilterChain(HttpSecurity http) throws Exception {
        //configure something else
        return http.build();
    }

    @Bean
    public AuthenticationManager authenticationManager(
            UserDetailsService userDetailsService,
            PasswordEncoder passwordEncoder) {
        DaoAuthenticationProvider authenticationProvider = new DaoAuthenticationProvider();
        authenticationProvider.setUserDetailsService(userDetailsService);
        authenticationProvider.setPasswordEncoder(passwordEncoder);

        return new ProviderManager(authenticationProvider);
    }

So in this case, otherSecurityFilterChain would use its local AuthenticationManager with my custom AuthenticationManager as parent and erase the credentials and the securityFilterChain would use its custom local AuthenticationManager without erasing credentials if I'm not mistaken.

Or would there be a cleaner way without duplicating the AuthenticationManager?

@sjohnr
Copy link
Member

sjohnr commented Sep 6, 2024

@kmartin88 glad your understanding is growing!

However, if I were to say for security reasons I only want to disable credentials erasure for one of my filter chains but aside from that want to use the same authentication mechanism in all of the filter chains - how would I achieve this?

It feels like this is a question that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements.

Having said that, I think this is just a case of making your code more reusable with a factory method that creates the ProviderManager with either true or false. You are correct however that specifying the "local" authentication manager on the filter chain is necessary for securityFilterChain to use a custom one.

@sjohnr sjohnr removed the status: feedback-provided Feedback has been provided label Sep 13, 2024
@sjohnr sjohnr moved this from Prioritized to In Progress in Spring Security Team Sep 13, 2024
@sjohnr sjohnr closed this as completed in 243f0f8 Sep 13, 2024
sjohnr added a commit that referenced this issue Sep 13, 2024
@sjohnr sjohnr moved this from In Progress to Done in Spring Security Team Sep 13, 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 type: bug A general bug
Projects
Status: Done
Development

No branches or pull requests

3 participants