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

Allow the ability to deactivate the device authorization grant #1454

Open
ddittmar opened this issue Nov 20, 2023 · 13 comments
Open

Allow the ability to deactivate the device authorization grant #1454

ddittmar opened this issue Nov 20, 2023 · 13 comments
Labels
type: enhancement A general enhancement

Comments

@ddittmar
Copy link

ddittmar commented Nov 20, 2023

Expected Behavior

I would like to have the ability to deactivate certain flows (or endpoints).

Current Behavior

There is no way to deactivate certain flows (or endpoints)

Context

Specifically I want to deactivate the "device authorization flow". I don't want my server to have the ability.

The "OpenID Connect 1.0 Client Registration endpoint" is disabled by default. Why can't I deactive other endpoints?

@ddittmar ddittmar added the type: enhancement A general enhancement label Nov 20, 2023
@jgrandja
Copy link
Collaborator

@ddittmar

I want to deactivate the "device authorization flow". I don't want my server to have the ability.

Can you please provide a detailed reason why you need to deactivate the device authorization endpoint?

As an FYI, if there are no RegisterClient's that are configured with .authorizationGrantType(AuthorizationGrantType.DEVICE_CODE) then the endpoint will never process the flow anyway.

I would like to have the ability to deactivate certain flows (or endpoints).

Which other flows? Please be specific and provide a detailed reason.

@jgrandja jgrandja added the status: waiting-for-feedback We need additional information before we can continue label Nov 25, 2023
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Dec 2, 2023
@ddittmar
Copy link
Author

ddittmar commented Dec 4, 2023

@jgrandja sorry for the late reply ...

You're right that the flow is never processed if the RegisterdClient does not have the AuthorizationGrantType.DEVICE_CODE configured... but then the endpoint is open and will never do anything... additionally the filter is in the securty chain without any need...

I just thought it would be good to deactive the flow completly because (in my case) there is no need for a "device authorization flow" and I think (especally) the device flow is a more exotic case.

@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 status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Dec 4, 2023
@jgrandja jgrandja changed the title add the ability to deactivate certain authorization flows (or endpoints) Allow the ability to deactivate the device authorization grant Dec 5, 2023
@jgrandja jgrandja removed the status: feedback-provided Feedback has been provided label Dec 5, 2023
@jgrandja
Copy link
Collaborator

jgrandja commented Dec 5, 2023

Thanks for the explanation @ddittmar. I updated the issue title to be more specific.

@gregecho
Copy link

gregecho commented Dec 20, 2023

@jgrandja ,

Do we need to support this feature? If so, can I work on this?

@jgrandja
Copy link
Collaborator

@gregecho Thanks for your interest. It's a lower priority item at the moment but if you would like to work on it please go ahead. Just a heads up that I'm heading out for the holidays and will be back Jan 8.

gregecho pushed a commit to gregecho/spring-authorization-server that referenced this issue Jan 8, 2024
Disable device authorization grant by default. Such as, deviceAuthorizationEndpoint and deviceVerificationEndpoint
gregecho pushed a commit to gregecho/spring-authorization-server that referenced this issue Jan 8, 2024
Disable device authorization grant by default. Such as, deviceAuthorizationEndpoint and deviceVerificationEndpoint
gregecho pushed a commit to gregecho/spring-authorization-server that referenced this issue Jan 15, 2024
Provide a configuration "enableDeviceAuthorizationEndpoint" to support enable/disable device authorization grant. The default value of enableDeviceAuthorizationEndpoint is true for backward capability.
@jgrandja
Copy link
Collaborator

@ddittmar Just circling back to this and I have a temporary workaround until we come up with a solution.

Assuming the @Bean name for the Authorization Server SecurityFilterChain is authorizationServerSecurityFilterChain, registering the following BeanPostProcessor will remove the device_code grant Filter's:

@Bean
public BeanPostProcessor authorizationServerSecurityFilterChainPostProcessor() {
	return new BeanPostProcessor() {
		public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
			if (beanName.equals("authorizationServerSecurityFilterChain")) {
				DefaultSecurityFilterChain securityFilterChain = (DefaultSecurityFilterChain)bean;
				List<Filter> filters = new ArrayList<>(securityFilterChain.getFilters());
				filters.removeIf((filter) ->
						filter instanceof OAuth2DeviceAuthorizationEndpointFilter ||
								filter instanceof OAuth2DeviceVerificationEndpointFilter);

				return new DefaultSecurityFilterChain(
						securityFilterChain.getRequestMatcher(), filters);
			}

			return bean;
		}
	};
}

@dlehammer
Copy link

Hi @jgrandja ,

I thought I would add my 2c 🤓

While your authorizationServerSecurityFilterChainPostProcessor suggestion disables access to the endpoints, this change isn't reflected in the exposed .well-known configuration.

Ie. like @ddittmar, I was also quite surprised that the auth-server not only advertises new endpoints and grant_types_supported by default after upgrading to v1.1, but also exposes these endpoints.
From a security risk perspective I would greatly prefer white-listing functionality instead.

Example below from the unexpected changes to coverage for v1.1:

SNIP..
--- a/src/functional-test/groovy/authorization/services/app/well_known/OpenidConfigurationEndpointFunctionalSpec.groovy
+++ b/src/functional-test/groovy/authorization/services/app/well_known/OpenidConfigurationEndpointFunctionalSpec.groovy
@@ -23,10 +23,12 @@ class OpenidConfigurationEndpointFunctionalSpec extends AbstractFunctionalSpec {
       responseEntity.statusCode == HttpStatus.OK
 
       Map response = responseEntity.body
-      response.size() == 15
+      response.size() == 17
 
       response.authorization_endpoint == "http://localhost:$serverPort/oauth2/authorize"
-      response.grant_types_supported.sort() == ['authorization_code', 'client_credentials', 'refresh_token']
+      response.device_authorization_endpoint == "http://localhost:$serverPort/oauth2/device_authorization"
+      response.end_session_endpoint == "http://localhost:$serverPort/connect/logout"
+      response.grant_types_supported.sort() == ['authorization_code', 'client_credentials', 'refresh_token', 'urn:ietf:params:oauth:grant-type:device_code']
       response.id_token_signing_alg_values_supported.sort() == ['RS256']
       response.introspection_endpoint == "http://localhost:$serverPort/oauth2/introspect"
SNIP..

The BeanPostProcessor suggestion, also exposes a disconnect between advertised support vs. actual support, which could be avoided if the auth-server used the same configuration source for advertising and exposing capabilities.

In addition, the current approach also presents a challenge with regard to auto-configured clients that relies on the .well-known configuration, since not all advertised capabilities are available to the client 🤔

@jgrandja
Copy link
Collaborator

@dlehammer

While your authorizationServerSecurityFilterChainPostProcessor suggestion disables access to the endpoints, this change isn't reflected in the exposed .well-known configuration.

The "temporary workaround" I provided is not a complete sample configuration. Agreed, there is a disconnect between "advertised support vs. actual support", however, this can be customized in the OpenID Connect 1.0 Provider Configuration Endpoint using a custom providerConfigurationCustomizer() to ensure consistency between advertised vs. actual support.

From a security risk perspective I would greatly prefer white-listing functionality instead.

Sounds like you would also prefer that device_code was disabled by default? Is there any other functionality that you would prefer disabled by default? Or are you just concerned with the device_code?

@dlehammer
Copy link

Hi @jgrandja,

Thanks for taking the time to reply.

Regarding

.. using a custom providerConfigurationCustomizer() to ensure consistency between advertised vs. actual support.

In my opinion, this approach is what I perceive as the source of the disconnect.
Consider an alternative scenario, where the .well-known endpoint transparently reflects the runtime endpoint-configuration of the auth-server.
In essence where the advertised and exposed functionality "draws from the same well", ensuring consistency and adhering to the principle of least astonishment.

Sounds like you would also prefer that device_code was disabled by default?

I think my surprise for the default enabled support for this particular flow, is because it's an addition to the OIDC core spec.
I applaud extended OIDC support, just not enabled by default :)

Is there any other functionality that you would prefer disabled by default?

In my opinion the auth-server is a critical component, and hence must be as hardened as possible by default.
Implicitly exposing new endpoints after an update, negates a secure-by-default approach.

Or are you just concerned with the device_code?

I only discovered these new endpoints, because I had .well-known coverage breakage.
Hence it follows, that unless a framework user has similar breakage, these updated auth-servers all have increased attack surface after updating 🫣

Ie. I get that there might not be any obvious exploits for these endpoints now - but as time goes by, exploits will most-likely be discovered/introduced, if history is any indication.
And when that time comes, as a framework user it would be preferable to only have the bare minimum exposed 🙏

@jgrandja
Copy link
Collaborator

@dlehammer

Consider an alternative scenario, where the .well-known endpoint transparently reflects the runtime endpoint-configuration of the auth-server.

Great idea but that kind of capability would require a much bigger change and is not related to this issue on hand.

In my opinion the auth-server is a critical component, and hence must be as hardened as possible by default.
Implicitly exposing new endpoints after an update, negates a secure-by-default approach.

Agree on all points. But you didn't really answer my question. Other than the device_code endpoint enabled by default, what other enabled endpoints/features are you concerned with?

as a framework user it would be preferable to only have the bare minimum exposed

No doubt. I'm confident we have only exposed what is needed to be exposed by default and fully adhere to secure-by-default principle. We might re-consider disabling device_code in 2.0 (breaking change). I am very curious to hear your thoughts if there are other features/endpoints that should be disabled by default.

@dlehammer
Copy link

Hi @jgrandja,

Agree on all points. But you didn't really answer my question.

I'm glad we're agreeing 🙏
I was trying to voice my support for this issue + voice my concern; for what I deemed a risky new direction for optional features in the Spring Authorization server.
(Simultaneously attempting to avoid hijacking this issue 😊)

.. what other enabled endpoints/features are you concerned with?

None pt. AFAIK

I'm confident we have only exposed what is needed to be exposed by default and fully adhere to secure-by-default principle.

Thank you for that unambiguous statement, that's reassuring 💪

We might re-consider disabling device_code in 2.0 (breaking change).

That would be much appreciated 🙏

@jgrandja
Copy link
Collaborator

@dlehammer I logged gh-1709

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

Successfully merging a pull request may close this issue.

5 participants