Adding client credentials authorization filter - Still In Progress #63

paurav-munshi · 2020-04-23T06:18:49Z

Closes #5

Adding a templated OAuth filter which performs following actions
- Identifying if Filter will support an incoming request using RequestMatcher
- Validating the request using RequestValidator
- Converting the request to Authentication using AuthenticationConverter
- Performing the authenticaion using supplied AuthenticaionManager
Adding a configuration which will build the templated OAuth filter to
work as Client Credentials filter using custom validators, converters,
matchers and auth managers

- Adding a templated OAuth filter which performs following actions - Identifying if support a request using RequestMatcher - Validating the request using RequestValidator - Converting the request to Authentication using AuthenticationConverter - Performing the authenticaion using supplied AuthenticaionManager - Adding a configuration which will build the templated OAuth filter to work as Client Credentials filter using custom validators, converters, matchers and auth managers

pivotal-issuemaster · 2020-04-23T06:18:51Z

@paurav-munshi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-04-23T06:20:21Z

@paurav-munshi Thank you for signing the Contributor License Agreement!

Captain-P-Goldfish · 2020-04-25T11:00:46Z

src/main/java/org/springframework/authz/server/config/SecurityConfigurer.java

+    	http.authorizeRequests()
+				.antMatchers("/token").permitAll();
+
+    	http.addFilterBefore(clientCredentialsFilter, BasicAuthenticationFilter.class);


You are not validating the client credentials yourself which leaves me to the conclusion that you are relying on the BasicAuthenticationFilter from the spring-security-filterchain.
This is actually a problem. RFC6749 defines an additional operation to the basic authenticaton scheme that is not directly mentioned in RFC7617 (Basic Auth Scheme). This causes problems with libraries like nimbus that do honor this additional operation.
https://tools.ietf.org/html/rfc6749#section-2.3.1
Simplified, if a client uses the basic authentication scheme the client_id parameter and the client_secret must be url-encoded. the BasicAuthenticationFilter of spring does not decode such authentication details and thus the authentication will fail.

Actually I am not relying on the BasicAuthorizationFilter in my flow. The configuration of the client credentials filter that I have developed could be seen in the AuthorizationFilterConfigurations.java in my PR. But for extracting the client id and secret from authorization header, I am using BasicAuthenticationConverter which I can replace with a custom class and inject.

As far as the basic auth is concerned, current implementation will extract the authorization header, make sure its Basic, then decode the base64 encoded string and split the result by colon to obtain client id and secret.

Now for the missing piece, do you mean that client id and secret obtained from header should further be url-decoded or do you mean that client id and secret passed in request body should be url-decoded ? If you mean the latter then that is not yet developed. I am a bit confused because the spec link you mentiond specifies application/x-www-form-urlencoded which is more relevant to content of the body rather the header (but correct me if I am wrong).

Once that is clear I can make changes accordingly.

Thanks for your inputs.

Paurav.

I mean the base64 decoded client_id and client_secret from the header. I recently made the experience that if this is not done it might come to a clash with the nimbus library. As pointed out in section 2.3.1 of RFC6749 the client_id and client_secret must be encoded.

Clients in possession of a client password MAY use the HTTP Basic
authentication scheme as defined in [RFC2617] to authenticate with
the authorization server. The client identifier is encoded using the
"application/x-www-form-urlencoded" encoding algorithm per
Appendix B, and the encoded value is used as the username; the client
password is encoded using the same algorithm and used as the
password.

and then from https://www.w3.org/TR/1999/REC-html401-19991224/interact/forms.html#h-17.13.4.1

application/x-www-form-urlencoded

This is the default content type. Forms submitted with this content type must be encoded as follows:

Control names and values are escaped. Space characters are replaced by +', and then reserved characters are escaped as described in [RFC1738], section 2.2: Non-alphanumeric characters are replaced by %HH', a percent sign and two hexadecimal digits representing the ASCII code of the character. Line breaks are represented as "CR LF" pairs (i.e., `%0D%0A').

The control names/values are listed in the order they appear in the document. The name is separated from the value by =' and name/value pairs are separated from each other by &'.

at least the nimbus developers implemented it this way and I had a short discussion with them about this topic and agreed eventually on their solution

jgrandja

Thanks for the PR @paurav-munshi. There is code in this PR that is not related to the issue we need to address. This issue simply addresses client authentication. So the minimal amount of code to achieve this is Filter, Authentication and AuthenticationProvider.

Take a look at #39. You will see the implementation should include OAuth2ClientAuthenticationFilter creates OAuth2ClientAuthenticationToken and passes to OAuth2ClientAuthenticationProvider. The Filter should encapsulate the request matching and credentials parsing logic - we will expose public API's for allowing customizations in a later story/issue. The plan is to add the minimal amount of code for each iteration to get things working. We will iterate a few times to add additional functionality as we build things out.

I'm going to close this PR based on my comments, as well, another PR has been previously submitted for this as per comment.

jgrandja · 2020-04-27T10:13:29Z

src/main/java/org/springframework/authz/server/config/AuthorizationFilterConfigurations.java

+	}
+
+	@Bean
+	public UserDetailsService clientCredentialsUserDetailsService() {


UserDetailsService will not be used for Client Registrations. The UserDetailsService is really meant for the user database. See #40

The main intention was to have an authentication manager in the filter. But to provide a temporary function which could work and store client details I used user details service.

Actually my ultimate goal was to have an client cred authentication provider similar to what is there in #39 but the purpose of the PR at that point in time was to get the Design Pattern validated.

jgrandja · 2020-04-27T10:40:47Z

src/main/java/org/springframework/authz/server/filter/OAuthGrantBasedAuthenticationFilter.java

+import org.springframework.security.web.util.matcher.RequestMatcher;
+import org.springframework.util.Assert;
+
+public class OAuthGrantBasedAuthenticationFilter extends AbstractAuthenticationProcessingFilter implements InitializingBean {


AbstractAuthenticationProcessingFilter is meant to be used for user authentication (login). Client authentication is different as the client is not capable (or required) of handling redirect based flows, which the AbstractAuthenticationProcessingFilter is design for. Instead we should use OncePerRequestFilter.

Got it. I was not aware of that. Thanks for clarifying.

jgrandja · 2020-04-27T10:49:24Z

src/main/java/org/springframework/authz/server/filter/matcher/GrantTypeReqMatcher.java

+	@Override
+	public boolean matches(HttpServletRequest request) {
+		String grantType = request.getParameter(OAuthConstants.GARNT_TYPE_PARAM);
+		return grantsAllowed.contains(grantType);


Client authentication should not rely on the type of grant in the request. It should authenticate if there are credentials in the request and the target is the token endpoint.

Hmmm. Got it. Thanks for clarification.

paurav-munshi

@jgrandja

Thanks a lot for your valuable inputs.

Thanks for the PR @paurav-munshi. There is code in this PR that is not related to the issue we need to address. This issue simply addresses client authentication. So the minimal amount of code to achieve this is Filter, Authentication and AuthenticationProvider.

Take a look at #39. You will see the implementation should include OAuth2ClientAuthenticationFilter creates OAuth2ClientAuthenticationToken and passes to OAuth2ClientAuthenticationProvider. The Filter should encapsulate the request matching and credentials parsing logic - we will expose public API's for allowing customizations in a later story/issue. The plan is to add the minimal amount of code for each iteration to get things working. We will iterate a few times to add additional functionality as we build things out.

I'm going to close this PR based on my comments, as well, another PR has been previously submitted for this as per comment.

I agree with your point that we can add customizations in a later story/issue. But if we observe closely even the OAuth2AuthorizationEndpointFilter from #66 has been designed with similar ideology of composable components constituting a filter. So won't it be useful if we setup and prescribe a design pattern, early on, which will be followed by all the Filter implementation ?

jgrandja · 2020-04-27T20:20:47Z

@paurav-munshi

But if we observe closely even the OAuth2AuthorizationEndpointFilter from #66 has been designed with similar ideology of composable components constituting a filter. So won't it be useful if we setup and prescribe a design pattern, early on, which will be followed by all the Filter implementation ?

The Spring Security OAuth 2.0 re-write was a huge undertaking and we've established the coding conventions and patterns and want to ensure contributions to this project follow the same. Simplicity in design, reusability and consistency is key - but of course very hard to achieve. For more details please see getting started.

This PR introduced extra code that was not relevant to the issue being addressed. As outlined in #39, it really should contain OAuth2ClientAuthenticationFilter, OAuth2ClientAuthenticationToken and OAuth2ClientAuthenticationProvider only (and tests).

Our goal is to iterate fast with this project so we can deliver 1.0.0 sooner than later. In order to achieve this, features (epics) will break the work into smaller units (issues) so it's quicker to implement and easier on the review end of things. The bigger the PR, the more dialogue back-and-forth, and the slower it gets merged.

I hope this makes sense?

paurav-munshi · 2020-04-28T19:10:24Z

@paurav-munshi

But if we observe closely even the OAuth2AuthorizationEndpointFilter from #66 has been designed with similar ideology of composable components constituting a filter. So won't it be useful if we setup and prescribe a design pattern, early on, which will be followed by all the Filter implementation ?

The Spring Security OAuth 2.0 re-write was a huge undertaking and we've established the coding conventions and patterns and want to ensure contributions to this project follow the same. Simplicity in design, reusability and consistency is key - but of course very hard to achieve. For more details please see getting started.

This PR introduced extra code that was not relevant to the issue being addressed. As outlined in #39, it really should contain OAuth2ClientAuthenticationFilter, OAuth2ClientAuthenticationToken and OAuth2ClientAuthenticationProvider only (and tests).

Our goal is to iterate fast with this project so we can deliver 1.0.0 sooner than later. In order to achieve this, features (epics) will break the work into smaller units (issues) so it's quicker to implement and easier on the review end of things. The bigger the PR, the more dialogue back-and-forth, and the slower it gets merged.

I hope this makes sense?

Thanks for a detailed response. Yes it makes a lot of sense.

Captain-P-Goldfish suggested changes Apr 25, 2020

View reviewed changes

Captain-P-Goldfish mentioned this pull request Apr 26, 2020

Client authentication filter #64

Closed

jgrandja reviewed Apr 27, 2020

View reviewed changes

jgrandja closed this Apr 27, 2020

jgrandja self-assigned this Apr 27, 2020

jgrandja added the status: duplicate A duplicate of another issue label Apr 27, 2020

jgrandja mentioned this pull request Apr 27, 2020

Implement Client Authentication #39

Closed

paurav-munshi commented Apr 27, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding client credentials authorization filter - Still In Progress #63

Adding client credentials authorization filter - Still In Progress #63

paurav-munshi commented Apr 23, 2020 •

edited

Loading

pivotal-issuemaster commented Apr 23, 2020

pivotal-issuemaster commented Apr 23, 2020

Captain-P-Goldfish Apr 25, 2020

paurav-munshi Apr 27, 2020

Captain-P-Goldfish Apr 27, 2020

jgrandja left a comment

jgrandja Apr 27, 2020

paurav-munshi Apr 27, 2020

paurav-munshi Apr 27, 2020

jgrandja Apr 27, 2020

paurav-munshi Apr 27, 2020

jgrandja Apr 27, 2020

paurav-munshi Apr 27, 2020

paurav-munshi left a comment •

edited

Loading

jgrandja commented Apr 27, 2020 •

edited

Loading

paurav-munshi commented Apr 28, 2020

Adding client credentials authorization filter - Still In Progress #63

Adding client credentials authorization filter - Still In Progress #63

Conversation

paurav-munshi commented Apr 23, 2020 • edited Loading

pivotal-issuemaster commented Apr 23, 2020

pivotal-issuemaster commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paurav-munshi left a comment • edited Loading

Choose a reason for hiding this comment

jgrandja commented Apr 27, 2020 • edited Loading

paurav-munshi commented Apr 28, 2020

paurav-munshi commented Apr 23, 2020 •

edited

Loading

paurav-munshi left a comment •

edited

Loading

jgrandja commented Apr 27, 2020 •

edited

Loading