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

ReactiveManagementWebSecurityAutoConfiguration does not back off when oauth2 client configuration present #17949

Closed
jgrandja opened this issue Aug 23, 2019 · 6 comments
Labels
type: bug A general bug
Milestone

Comments

@jgrandja
Copy link

jgrandja commented Aug 23, 2019

ReactiveManagementWebSecurityAutoConfiguration does not configure oauth2Login() even when spring-security-oauth2-client is in classpath and properties have been configured. It only configures httpBasic() and formLogin() for all cases.

The configuration logic should follow the default configuration for SecurityWebFilterChain in WebFluxSecurityConfiguration.springSecurityFilterChain().

Related spring-security#6314

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 23, 2019
@philwebb
Copy link
Member

@jgrandja Does this also apply to ManagementWebSecurityConfigurerAdapter? Should we try and surface something public in Spring Security itself that will allow us to share that configuration code?

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Aug 23, 2019
@jgrandja
Copy link
Author

@philwebb ManagementWebSecurityAutoConfiguration behaves as expected. This issue arises with a Reactive configuration. However, I looked a bit deeper into this and it appears ReactiveManagementWebSecurityAutoConfiguration is not the issue.

I believe the main issue is with ReactiveOAuth2ClientAutoConfiguration as it does not register a default @Bean of SecurityWebFilterChain.

If you look at OAuth2ClientAutoConfiguration on the Servlet side of things, you will see that it (indirectly) registers the default security configuration via OAuth2WebSecurityConfiguration.OAuth2WebSecurityConfigurerAdapter, which makes ManagementWebSecurityAutoConfiguration back off.

I believe the only fix required here is to add the following code to ReactiveOAuth2ClientAutoConfiguration:

@Bean
public SecurityWebFilterChain springSecurityFilterChain(ServerHttpSecurity http) {
	return http
			.authorizeExchange()
				.anyExchange().authenticated()
				.and()
			.oauth2Login()
				.and()
			.oauth2Client()
				.and()
			.build();
}

What are your thoughts?

@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, 2019
@mbhave
Copy link
Contributor

mbhave commented Aug 27, 2019

@jgrandja Instead of duplicating what WebFluxSecurityConfiguration.springSecurityFilterChain() does, I think we can annotate ReactiveOAuth2ClientAutoConfiguration with @EnableWebFluxSecurity which would will create the beans that cause ReactiveManagementWebSecurityAutoConfiguration to back off. WDYT?

@mbhave mbhave added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Aug 27, 2019
@mbhave mbhave changed the title ReactiveManagementWebSecurityAutoConfiguration should account for oauth2Login() ReactiveManagementWebSecurityAutoConfiguration does not back off when oauth2 client configuration present Aug 27, 2019
@mbhave mbhave added this to the 2.1.x milestone Aug 27, 2019
@jgrandja
Copy link
Author

@mbhave Annotating ReactiveOAuth2ClientAutoConfiguration with @EnableWebFluxSecurity seems like a better idea as it will follow the same code path for creating the default SecurityWebFilterChain.

@philwebb
Copy link
Member

philwebb commented Sep 4, 2019

I had an attempt at fixing this in https://github.com/philwebb/spring-boot/tree/gh-17949 but it doesn't feel quite right. Specifically we're now pretty much duplicating ReactiveSecurityAutoConfiguration in ReactiveOAuth2ClientAutoConfiguration.

I have a feeling that the problem does actually lie in ReactiveManagementWebSecurityAutoConfiguration and the springSecurityFilterChain it creates. I think that that class should be duplicating WebFluxSecurityConfiguration.springSecurityFilterChain() and doing what OAuth2ClasspathGuard does in the default configuration.

I also wonder if we need something in ManagementWebSecurityConfigurerAdapter for the Servlet setup (although I can't see any OAuth support in WebSecurityConfigurerAdapter.configure).

@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review status: on-hold We can't start working on this issue yet labels Sep 4, 2019
@mbhave
Copy link
Contributor

mbhave commented Sep 4, 2019

I think the servlet setup is slightly different as you explicitly need to configure .oauth2Login() and .oauth2client() which we do in OAuth2WebSecurityConfiguration. Since there's an explicit bean of type WebSecurityConfigurerAdapter for the OAuth setup, it causes ManagementWebSecurityConfigurerAdapter to back off.

The main reason the separate management configurations exist, is for the health and info endpoints. With the default security configuration, we wanted these endpoints to be accessible without authenticating. When OAuth is added to the classpath we secure everything via OAuth, including the health and info endpoints (this was the plan for the reactive case as well but it doesn't behave this way because of this bug).

Whether the change needs to be in ReactiveOAuth2ClientAutoConfiguration or ReactiveManagementWebSecurityAutoConfiguration depends on if we want the same behavior of opening up health and info endpoints even when OAuth is on the classpath. If we do decide that health and info should be open, the servlet configuration would need to be updated as well. This might potentially lead to two WebSecurityConfigurerAdapters for the servlet case which is something I'd really like to avoid (as deciding when to back off might become tricky).

I'm in favor of having everything secured by OAuth once OAuth is added to the classpath without making an exception for the health and info endpoints. If duplicating @EnableWebFluxSecurity feels odd, we should consider doing what Joe suggested above in ReactiveOAuth2ClientAutoConfiguration. Let's see what the rest of the team thinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants