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

Default user password shouldn't be logged if OAuth2 is being used #10531

Closed
mbhave opened this Issue Oct 5, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@mbhave
Contributor

mbhave commented Oct 5, 2017

No description provided.

@mbhave mbhave added this to the 2.0.0.M6 milestone Oct 5, 2017

@mbhave mbhave self-assigned this Oct 13, 2017

@Quantas

This comment has been minimized.

Show comment
Hide comment
@Quantas

Quantas Oct 23, 2017

Contributor

I also noticed this "regression" when switching from 2.0.0M4 to 2.0.0M5 when not using OAuth but when using Pre Authenticated security such as SiteMinder.

Contributor

Quantas commented Oct 23, 2017

I also noticed this "regression" when switching from 2.0.0M4 to 2.0.0M5 when not using OAuth but when using Pre Authenticated security such as SiteMinder.

@wilkinsona wilkinsona modified the milestones: 2.0.0.M6, 2.0.0.RC1 Oct 25, 2017

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Nov 7, 2017

Contributor

We can make the bean lazy once this is taken care of.

Contributor

mbhave commented Nov 7, 2017

We can make the bean lazy once this is taken care of.

@snicoll snicoll changed the title from Default user password shouldn't be logged if OAuth is being used to Default user password shouldn't be logged if OAuth2 is being used Nov 8, 2017

@philwebb philwebb modified the milestones: 2.0.0.M7, 2.0.0.RC1 Nov 8, 2017

@snicoll

This comment has been minimized.

Show comment
Hide comment
@snicoll

snicoll Jan 8, 2018

Member

@mbhave I was discussing this issue with @rwinch recently and he was mentioning he was expecting zero change in Spring Boot. Can we please double check we're on the same page here? (dunno what you mean with the @Lazy part). Thanks!

Member

snicoll commented Jan 8, 2018

@mbhave I was discussing this issue with @rwinch recently and he was mentioning he was expecting zero change in Spring Boot. Can we please double check we're on the same page here? (dunno what you mean with the @Lazy part). Thanks!

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Jan 8, 2018

Contributor

I think we'd still need the bean to be lazy, so that it only logs the password if InMemoryUserDetailsManager is being used. Unless I'm missing something @rwinch?

Contributor

mbhave commented Jan 8, 2018

I think we'd still need the bean to be lazy, so that it only logs the password if InMemoryUserDetailsManager is being used. Unless I'm missing something @rwinch?

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Jan 8, 2018

Member

@mbhave You are right. It will need to be lazy (I missed that it wasn't lazy already). @jgrandja is looking into it now so we will find out the details soon

Member

rwinch commented Jan 8, 2018

@mbhave You are right. It will need to be lazy (I missed that it wasn't lazy already). @jgrandja is looking into it now so we will find out the details soon

@wilkinsona

This comment has been minimized.

Show comment
Hide comment
@wilkinsona

wilkinsona Jan 19, 2018

Member

@rwinch @jgrandja Given that spring-projects/spring-security#4795 has been declined, can you please describe how we should achieve our goal here?

Member

wilkinsona commented Jan 19, 2018

@rwinch @jgrandja Given that spring-projects/spring-security#4795 has been declined, can you please describe how we should achieve our goal here?

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Jan 20, 2018

Contributor

I think the suggestion is to add

@ConditionalOnMissingBean(type="org.springframework.security.oauth2.client.registration.ClientRegistrationRepository")

to InMemoryUserDetailsManager.

I think it contradicts this slightly, specifically,

http.formLogin() and http.oauth2Login() can be configured at the same time and the UserDetailsService bean would be used when the user logs in via http.formLogin()

Since these two modes of authentication can be used together, it makes me wonder if not creating the InMemoryUserDetailsManager bean when there is a ClientRegistrationRepository is the right call.

Contributor

mbhave commented Jan 20, 2018

I think the suggestion is to add

@ConditionalOnMissingBean(type="org.springframework.security.oauth2.client.registration.ClientRegistrationRepository")

to InMemoryUserDetailsManager.

I think it contradicts this slightly, specifically,

http.formLogin() and http.oauth2Login() can be configured at the same time and the UserDetailsService bean would be used when the user logs in via http.formLogin()

Since these two modes of authentication can be used together, it makes me wonder if not creating the InMemoryUserDetailsManager bean when there is a ClientRegistrationRepository is the right call.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Jan 22, 2018

Member

@mbhave You are right it does contradict that statement. However, determining when to create a UserDetailsService has always been an educated guess. Unfortuantely, there is no way around making an educated guess until SPR-13779 is solved and then we integrate something into Spring Security to leverage that.

Other examples of where creating the UserDetailsService is unnecessary are when a user might leverage Spring Security for just HTTP headers. Yet another example is if the user performs authentication using a third party library that simply sets the SecurityContextHolder without using an AuthenticationManager. There are plenty of reasons that this is not a perfect answer. If we need a perfect answer for when UserDetailsService is created, then we probably cannot create one at all (at least not until SPR-13779 is solved).

In the end we want to ensure that the bean is not even defined if the user does not need it. Therefore, the previous approaches outlined of using @Lazy and Spring Security not looking it up are not sufficient.

Member

rwinch commented Jan 22, 2018

@mbhave You are right it does contradict that statement. However, determining when to create a UserDetailsService has always been an educated guess. Unfortuantely, there is no way around making an educated guess until SPR-13779 is solved and then we integrate something into Spring Security to leverage that.

Other examples of where creating the UserDetailsService is unnecessary are when a user might leverage Spring Security for just HTTP headers. Yet another example is if the user performs authentication using a third party library that simply sets the SecurityContextHolder without using an AuthenticationManager. There are plenty of reasons that this is not a perfect answer. If we need a perfect answer for when UserDetailsService is created, then we probably cannot create one at all (at least not until SPR-13779 is solved).

In the end we want to ensure that the bean is not even defined if the user does not need it. Therefore, the previous approaches outlined of using @Lazy and Spring Security not looking it up are not sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment