feat: Public api for OIDC dynamic client registration #2568

fjvierap · 2021-06-15T07:39:37Z

Related issue

Proposed changes

Create a separate endpoint for create, get, update and delete clients form public api. The new endpoint will only be available when SERVE_PUBLIC_ALLOW_DYNAMIC_REGISTRATION is set to true or when allow_dynamic_registration configuration is set to true.

Checklist

[ x] I have read the contributing guidelines.
[ x] I have read the security policy.
[ x] I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
[x ] I have added tests that prove my fix is effective or that my feature
works.
[x ] I have added or changed the documentation.

Further comments

Please let me know if something else needs to be done in order to build successfully without changes mentioned above.

aeneasr · 2021-06-17T14:52:32Z

Ready for 👀 ? :) If so you can always request a review from me so it shows up on my radar :)

fjvierap · 2021-06-17T15:36:46Z

Ready for 👀 ? :) If so you can always request a review from me so it shows up on my radar :)

Yes it is ready for review ;-)... Please check when you have some time

aeneasr

Thank you! This is a great starting point! I have some ideas how to improve the PR.

One thing noteworthy is the question if we want to implement OpenID Connect Dynamic Client Registration OR OAuth2 Dynamic Registration or both. Depending on that, the authentication mechanisms and payloads might change.

I think it makes more sense for us to implement the OIDC spec because that's what we've implemented already, plus we can easily certify it.

client/handler.go

client/validator.go

client/handler.go

aeneasr · 2021-06-18T15:37:42Z

client/handler.go

+	}
+
+	c.Secret = ""
+	if c.Metadata != nil {


Metadata has a different meaning. It's purpose is not to store configuration for the client, but to store any arbitrary data. Adding access_token_ttl here is not metadata, imo, but configuration. Therefore I think that it would be better to have, for example, on the admin port:

POST /clients/<id>/settings

which would allow to change the config of clients - so for example the access token TTL.

Repurposing this field for some other functionality makes Ory Hydra vulnerable. Say you have Hydra 1.10 running. Someone scans you and sees that you use Ory Hydra 1.10 and have this endpoint exposed. He/she adds config options to the client in the metadata field. One day, you upgrade to Ory Hydra 1.11. Now, the attacker was able to change configuration which he was not supposed to change.

Therefore, this has to go somewhere else! It's generally a good idea to never use untrusted data as trusted data. Validation might help but it often fails to cover all areas :)

I understand..... How about have and access_token_ttl or access_token_ttl as specific fields in the root level? As now we will different endpoints for Dynamic Client registration and the /client ones in the admin API. Then we can have different client representations and we can include those access_token_ttl or access_token_ttl only in the one for admin API... Then hydra master can keep that configuration in sync... Does it make sense?

Separate the client representation for Dynamic Client registration and the admin api maybe can be done in separate PR?

Javier, I'm somehow lost by your question:

Separate the client representation for Dynamic Client registration and the admin api maybe can be done in separate PR?

The purpose of #2549 was exactly to achieve this - have two independent APIs (for dynamic client registration according to OIDC spec and for client management). And this also means specific client data representations.

@dadrus I mean to have access_token_ttl or access_token_ttl as dedicated fields fields for the client entity and not as part of metadata...

For this PR we will separate the public and admin endpoints... making public ones for Dynamic Client Registration to do no accept any metadata.

Does it make sense?

fjvierap · 2021-06-22T14:58:27Z

@aeneasr I added the adjustments required. Please have a look when you can ;)

aeneasr

Thank you! A few more things but this is definitely going in the right direction :)

Sorry for being so slow with reviews these past days...

internal/config/config.yaml

aeneasr · 2021-06-28T14:48:59Z

client/handler.go

+		h.r.Writer().WriteError(w, r, err)
+		return
+	}
+	token, _, err := h.r.OpenIDJWTStrategy().Generate(r.Context(), jwtgo.MapClaims{


I think the idea is good :) Unfortunately I would say that we shouldn't use an ID Token as proof of authorization for the client. I think the basic auth model you have used is enough security. The problem here is that this token could be used as an ID token for the application which would be quite confusing to the apps.

Additionally, it would only be valid for an hour and I don't see a way of getting a new token for this?

So I think relying on basic auth is enough! It would also de-complicate the codebase.

@aeneasr I did mentioned changes :) Please review when you have some time

fjvierap · 2021-06-29T15:43:45Z

Thank you! A few more things but this is definitely going in the right direction :)

Sorry for being so slow with reviews these past days...

@aeneasr I did mentioned changes :) Please review when you have some time

aeneasr

Oh man, I had this review started but apparently never clicked on submit. Sorry :(

aeneasr · 2021-07-05T09:49:01Z

client/handler.go

+func (h *Handler) validateDynClientRegistrationAuthorization(r *http.Request, c Client) error {
+
+	basicAuth := getBasicAuth(r)
+	if basicAuth == "" {
+		return herodot.ErrUnauthorized.WithReason("Invalid authorization")
+	}
+	sDec, err := base64.StdEncoding.DecodeString(basicAuth)
+	if err != nil {
+		return herodot.ErrUnauthorized.WithReason("Invalid authorization")
+	}
+	split := strings.SplitN(string(sDec), ":", 2)
+	if len(split) != 2 {
+		return herodot.ErrUnauthorized.WithReason("Invalid authorization")
+	}
+	if c.OutfacingID != split[0] {
+		return herodot.ErrUnauthorized.WithReason("Invalid authorization")
+	}
+	_, err = h.r.ClientManager().Authenticate(r.Context(), split[0], []byte(split[1]))
+	if err != nil {
+		return herodot.ErrUnauthorized.WithReason("Invalid authorization")
+	}
+	return nil
+}
+
+func getBasicAuth(req *http.Request) string {
+	auth := req.Header.Get("Authorization")
+	split := strings.SplitN(auth, " ", 2)
+	if len(split) != 2 || !strings.EqualFold(split[0], "Basic") {
+		// Nothing in Authorization header
+		return ""
+	}
+
+	return split[1]


Could you please add unit tests for this? Specifically for the decoding part :) Also, I think it makes sense to reuse the authentication from here: https://github.com/ory/fosite/blob/a4ce3544c2996a99b65350d4b200967df9fc0d45/client_authentication.go#L311

@aeneasr I create a unit test for this... I think https://github.com/ory/fosite/blob/a4ce3544c2996a99b65350d4b200967df9fc0d45/client_authentication.go#L311 is for getting client credentials form request body we have basic authentication, we have in Authorization header. right?

quickstart-postgres.yml

cypress/integration/openid/dynamic_client_registration.js

fjvierap · 2021-07-13T13:43:55Z

@aeneasr I update the PR based on your comments... Could you please have a look when you have some time? :)

aeneasr · 2021-07-14T09:05:03Z

Could you please enable write access for maintainers? 🤔

Thank you! 🙏

aeneasr · 2021-07-14T09:06:07Z

I think one problem also is that this PR is against your fork's master branch, maybe that's why I get push denied. Do you have some branch protection enabled in your fork?


To github.com:fjvierap/hydra.git
 ! [remote rejected]     fjvierap/master -> fjvierap/master (permission denied)
error: Fehler beim Versenden einiger Referenzen nach 'github.com:fjvierap/hydra.git'

codecov · 2021-07-14T12:21:23Z

Codecov Report

Merging #2568 (7d770e0) into master (beb72e4) will decrease coverage by 3.73%.
The diff coverage is 10.29%.

@@            Coverage Diff             @@
##           master    #2568      +/-   ##
==========================================
- Coverage   52.89%   49.15%   -3.74%     
==========================================
  Files         235      244       +9     
  Lines       14132    15295    +1163     
==========================================
+ Hits         7475     7519      +44     
- Misses       6027     7119    +1092     
- Partials      630      657      +27

Impacted Files	Coverage Δ
.../client/admin/accept_consent_request_parameters.go	`35.71% <0.00%> (-1.79%)`	⬇️
...t/client/admin/accept_consent_request_responses.go	`19.23% <0.00%> (ø)`
...nt/client/admin/accept_login_request_parameters.go	`35.71% <0.00%> (-1.79%)`	⬇️
...ent/client/admin/accept_login_request_responses.go	`23.80% <0.00%> (ø)`
...t/client/admin/accept_logout_request_parameters.go	`29.16% <0.00%> (-2.09%)`	⬇️
...nt/client/admin/accept_logout_request_responses.go	`19.23% <0.00%> (ø)`
...client/admin/create_json_web_key_set_parameters.go	`32.07% <0.00%> (-1.89%)`	⬇️
.../client/admin/create_json_web_key_set_responses.go	`14.70% <0.00%> (ø)`
...t/client/admin/create_o_auth2_client_parameters.go	`26.08% <0.00%> (-2.18%)`	⬇️
...nt/client/admin/create_o_auth2_client_responses.go	`14.70% <0.00%> (ø)`
... and 138 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beb72e4...7d770e0. Read the comment docs.

fjvierap · 2021-07-19T08:44:26Z

@aeneasr I did the mentioned changed and merger ory/hydra master into my repo... But It fails because a vulnerability issue It also fails in my local in ory hydra repository but it worked in the pipeline

I think this issue has been fixed in https://github.com/gobuffalo/packr v1.30.1 I will create and additional PR in https://github.com/ory/x to upgrade to v1.30.1 and then adjust this one.

Anyways it would be good to know why with the same dependency check passed in master pipeline. Maybe something cached?

…4-7bd1-4056-8fb0-450a7e4904b6\?component-type\=golang\&component-name\=github.com%2Fgobuffalo%2Fpackr\&utm_source\=nancy-client\&utm_medium\=integration\&utm_content\=1.0.22

fjvierap · 2021-07-19T13:15:56Z

@aeneasr Can you please review when you have some time?

aeneasr · 2021-07-23T14:47:07Z

Hello! Sorry again for the long review time - I did not have any time to look into open source the past 10 days. I will be able to review this next week. My goal is to merge it by then!

aeneasr · 2021-07-30T10:45:22Z

It looks like my changes to the PR got lost. Have you tried enabling write access for me?

#2568 (comment)

I will have to redo all the changes again but first we need to ensure that this is working. :/

aeneasr · 2021-07-30T10:50:15Z

Looks like I still don't have write rights :/ I will have to figure out another way to complete and review this PR.

aeneasr · 2021-09-20T11:03:09Z

@dadrus @fjvierap how can we proceed with this? Unfortunately, I am still lacking the rights to push to the repository which would greatly help in merging this PR.

fjvierap · 2021-09-23T11:19:53Z

@dadrus @fjvierap how can we proceed with this? Unfortunately, I am still lacking the rights to push to the repository which would greatly help in merging this PR.

Hi @aeneasr I checked you have access to this repo... So I am wandering what the problem is..

BTW I merged from hydra repo into for fork repository

raman-nbg · 2021-12-16T09:38:23Z

Somehow this PR got stuck. Is there anything where I could help to finish this?

aeneasr · 2021-12-29T17:41:29Z

See: #2909

This feature adds first-class support for OpenID Connect Dynamic Client Registration. To enable this feature, which is disabled by default, set ```yaml oidc: dynamic_client_registration: enabled: true ``` in your Ory Hydra configuration. Once enabled, endpoints `POST`, `GET`, `PUT`, and `DELETE` for `/openid/register` will be available at the public port! Closes ory#2568 Closes ory#2549

aeneasr · 2022-01-03T07:42:12Z

@fjvierap thank you for your hard work on this PR!

I am continuing the work in #2909 . I think it makes sense to follow OAuth2 Dynamic Client Registration Protocol as well as OpenID Connect Dynamic Client Registration as closely as possible and add a large set of tests to ensure everything works as needed. This means that there still needs to be quite a lot of work done, so it will take more time for this to get merged.

I will be closing this PR and you can follow the work in #2909

…lient Registration Protocol (#2909) This feature adds first-class support for two IETF RFCs and one OpenID Spec: - [OpenID Connect Dynamic Client Registration 1.0](https://openid.net/specs/openid-connect-registration-1_0.html) - [OAuth 2.0 Dynamic Client Registration Protocol](https://tools.ietf.org/html/rfc7591) - [OAuth 2.0 Dynamic Client Registration Management Protocol](https://tools.ietf.org/html/rfc7592) To enable this feature, which is disabled by default, set ```yaml oidc: dynamic_client_registration: enabled: true ``` in your Ory Hydra configuration. Once enabled, endpoints `POST`, `GET`, `PUT`, and `DELETE` for `/connect/register` will be available at the public port! Closes #2568 Closes #2549 BREAKING CHANGES: Endpoint `PUT /clients` now returns a 404 error when the OAuth2 Client to be updated does not exist. It returned 401 previously. This change requires you to run SQL migrations! Co-authored-by: fjviera <javier.viera@mindcurv.com>

fjvierap added 6 commits June 14, 2021 17:59

Add dynamick client registration

dc1eee9

Add dynamic client registration

37b05c0

Add dynamic client registration

4a08095

Add dynamic client registration

dcad33b

Add dynamic client registration

1c56455

Add dynamic client registration

8722e2a

fjvierap changed the title ~~Public api for dynamic client registration~~ feat: Public api for dynamic client registration Jun 15, 2021

fjvierap added 7 commits June 15, 2021 09:45

Add dynamic client registration

e5b9ded

Merge branch 'master' into master

8b1d509

Add dynamic client registration

db7dc63

Merge branch 'master' of github.com:fjvierap/hydra

8286aec

Add dynamic client registration

dd9ec52

Add dynamic client registration

4111c9f

Add dynamic client registration

2bd1399

fjvierap changed the title ~~feat: Public api for dynamic client registration~~ feat: Public api for OIDC dynamic client registration Jun 15, 2021

fjvierap added 3 commits June 15, 2021 11:00

Update config.yaml

16a3f69

Generate sdk

8d96fc7

Adjust sdk

33e09ac

fjvierap mentioned this pull request Jun 15, 2021

Separate client management API from the API used for dynamic client registration and configuration. #2549

Closed

aeneasr reviewed Jun 18, 2021

View reviewed changes

fjvierap added 3 commits June 22, 2021 15:55

Adjustments for dynamic client registration

fc59688

run prettier

30bfa8a

fix golang-ci violations

86ebb7d

aeneasr reviewed Jun 28, 2021

View reviewed changes

Dynamic client registration only with basisc auth

b5f9991

aeneasr reviewed Jul 12, 2021

View reviewed changes

fjvierap added 2 commits July 13, 2021 13:18

Add unit test to handler.go

7c0bebe

Fix prettier issue

ad3af1a

Fix merge confict

66f2100

fjvierap added 3 commits July 19, 2021 08:44

Fix merge confict

bfa82a1

Merge remote-tracking branch 'upstream/master'

4c71898

Merge remote/origin

054bbbc

fjvierap added 2 commits July 19, 2021 10:51

Fix vulnerability https://ossindex.sonatype.org/vulnerability/1a11ab2…

b6ff5cb

…4-7bd1-4056-8fb0-450a7e4904b6\?component-type\=golang\&component-name\=github.com%2Fgobuffalo%2Fpackr\&utm_source\=nancy-client\&utm_medium\=integration\&utm_content\=1.0.22

Add coverage for client handler

5b01e58

Merge from origin

7d770e0

aeneasr mentioned this pull request Dec 29, 2021

feat: OpenID Connect Dynamic Client Registration and OAuth2 Dynamic Client Registration Protocol #2909

Merged

10 tasks

StarAurryon mentioned this pull request Dec 30, 2021

feat: TLS certs auto-reload #2910

Closed

7 tasks

aeneasr closed this Jan 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Public api for OIDC dynamic client registration #2568

feat: Public api for OIDC dynamic client registration #2568

fjvierap commented Jun 15, 2021 •

edited

Loading

aeneasr commented Jun 17, 2021

fjvierap commented Jun 17, 2021

aeneasr left a comment

aeneasr Jun 18, 2021

aeneasr Jun 18, 2021

fjvierap Jun 21, 2021 •

edited

Loading

dadrus Jun 22, 2021 •

edited

Loading

fjvierap Jun 22, 2021

fjvierap commented Jun 22, 2021

aeneasr left a comment

aeneasr Jun 28, 2021

fjvierap Jun 29, 2021

fjvierap commented Jun 29, 2021

aeneasr left a comment

aeneasr Jul 5, 2021

fjvierap Jul 13, 2021

fjvierap commented Jul 13, 2021

aeneasr commented Jul 14, 2021

aeneasr commented Jul 14, 2021

codecov bot commented Jul 14, 2021 •

edited

Loading

fjvierap commented Jul 19, 2021 •

edited

Loading

fjvierap commented Jul 19, 2021

aeneasr commented Jul 23, 2021

aeneasr commented Jul 30, 2021

aeneasr commented Jul 30, 2021

aeneasr commented Sep 20, 2021

fjvierap commented Sep 23, 2021 •

edited

Loading

raman-nbg commented Dec 16, 2021

aeneasr commented Dec 29, 2021

aeneasr commented Jan 3, 2022

feat: Public api for OIDC dynamic client registration #2568

feat: Public api for OIDC dynamic client registration #2568

Conversation

fjvierap commented Jun 15, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

aeneasr commented Jun 17, 2021

fjvierap commented Jun 17, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jun 18, 2021

Choose a reason for hiding this comment

aeneasr Jun 18, 2021

Choose a reason for hiding this comment

fjvierap Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

dadrus Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

fjvierap Jun 22, 2021

Choose a reason for hiding this comment

fjvierap commented Jun 22, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jun 28, 2021

Choose a reason for hiding this comment

fjvierap Jun 29, 2021

Choose a reason for hiding this comment

fjvierap commented Jun 29, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jul 5, 2021

Choose a reason for hiding this comment

fjvierap Jul 13, 2021

Choose a reason for hiding this comment

fjvierap commented Jul 13, 2021

aeneasr commented Jul 14, 2021

aeneasr commented Jul 14, 2021

codecov bot commented Jul 14, 2021 • edited Loading

Codecov Report

fjvierap commented Jul 19, 2021 • edited Loading

fjvierap commented Jul 19, 2021

aeneasr commented Jul 23, 2021

aeneasr commented Jul 30, 2021

aeneasr commented Jul 30, 2021

aeneasr commented Sep 20, 2021

fjvierap commented Sep 23, 2021 • edited Loading

raman-nbg commented Dec 16, 2021

aeneasr commented Dec 29, 2021

aeneasr commented Jan 3, 2022

fjvierap commented Jun 15, 2021 •

edited

Loading

fjvierap Jun 21, 2021 •

edited

Loading

dadrus Jun 22, 2021 •

edited

Loading

codecov bot commented Jul 14, 2021 •

edited

Loading

fjvierap commented Jul 19, 2021 •

edited

Loading

fjvierap commented Sep 23, 2021 •

edited

Loading