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

Adjust the primer towards OIDC Federation #208

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

elf-pavlik
Copy link
Member

@elf-pavlik elf-pavlik commented Nov 21, 2022

Part of #207
This draft is intended as collaborative scratchpad. We can use inline suggestions to propose and discuss changes.

TODO

  • OP advertises automatic registration
  • authentication request should use Request Object or PAR
  • RP should publish the trust chain
  • OP should validate the trust chain
  • in the abstract en intro, refer to OpenID Federation, next to OpenID Connect;
  • switch "Client ID Document" to "Entity Statement" or "Entity Configuration".

Copy link
Member

@acoburn acoburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start. How much detail should go into the validation steps involved? i.e. the Resolving Trust Chain and Metadata section

@@ -235,6 +235,8 @@ Response Body:
}
```

Issue: OIDC Federation defines `client_registration_types_supported`, should it be addedhere, to `.well-known/oidc-federation` or both?
Copy link
Member

@acoburn acoburn Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be added to OpenID Metadata resource (/.well-known/openid-configuration) for OPs that support clients from a federation.

If the OpenID Provider participates in a federation, the OP federation metadata would also contain this information (note the resource path: .well-known/openid-federation).

As I have been considering this change, I have been imagining the relying parties participating in an OIDC federation, but that it would entirely optional for an OpenID Provider to participate in a federation. Or are you thinking that this would be a requirement for Solid OIDC Providers as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm going to add it to /.well-known/openid-configuration.

I think we should start with the minimum required, probably enough for RPs to act as entities in federation.

Copy link
Contributor

@woutermont woutermont Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I agreed, but now I'm fairly certain that the Federation spec only means to add that to the .well-known/openid-federation config, not change anything about the OIDC configuration. As long as we're not talking about OPs from a federation perspective, I thus think it should not be added anywhere.

@elf-pavlik
Copy link
Member Author

@acoburn I think we should have a simplest case with all the details. Since this is a primer I don't see a problem with showcasing something which isn't specified directly in Solid-OIDC but in OIDC Federation or DPoP.

Copy link
Contributor

@woutermont woutermont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some inline questions, and two things more general still to do:

  • in the abstract en intro, refer to OpenID Federation, next to OpenID Connect;
  • switch "Client ID Document" to "Entity Statement" or "Entity Configuration".

@@ -235,6 +235,8 @@ Response Body:
}
```

Issue: OIDC Federation defines `client_registration_types_supported`, should it be addedhere, to `.well-known/oidc-federation` or both?
Copy link
Contributor

@woutermont woutermont Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I agreed, but now I'm fairly certain that the Federation spec only means to add that to the .well-known/openid-federation config, not change anything about the OIDC configuration. As long as we're not talking about OPs from a federation perspective, I thus think it should not be added anywhere.

- `client_id=https%3A%2F%2Fdecentphotos.example%2Fclient_id`: Usually the client id of a Solid
application is the app's URI (in our case `https://decentphotos.example/client_id`) as seen here.
- `client_id=https%3A%2F%2Fdecentphotos.example`: Usually the client id of a Solid
application is the app's URI (in our case `https://decentphotos.example`) as seen here.
- `code_challenge_method=S256`: Tells the OP that our code challenge was transformed using SHA-256.
- `code_challenge=HSi9dwlvRpNHCDm-L8GOdM16qcb0tLHPZqQSvaWXTI0`: The [=code challenge=] we generated before.

Note: If the app doesn't have a URI, you can either register an app using static registration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep mentioning static registration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I'm farily certain that the Federation spec only means to add that to the .well-known/openid-federation config

For an OP that is not part of a federation, how would a client know that the OP supports automatic registration? Unless you are suggesting that all such OPs must also participate in a federation? I had not been making that assumption.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how that is related to (not) mentioning static registration? Did you mean to respons to the other conversation on client_registration_types_supported, @acoburn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer to your question: I was assuming that a Solid-OIDC compliant OP would always adhere to the OpenID Federation spec, regardless of it being part of an actual federation or not; so every OP would always have a .well-known/openid-federation with the client_registration_types_supported list. Since that list has no defined meaning in OpenID Connect, however, I don't think it has any role in .well-known/oidc-configuration. But that's just how I initially interpreted it; might be wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: @woutermont this is in a different thread because the GH interface, for some inexplicable reason, will not allow me to comment on the relevant thread.

One perspective is that, if a Solid-OIDC OP is going to support the processing of RP Federation trust chains, it would make sense to also require the OP to participate in a Federation. After all, the OP is already implementing most of OIDC Federation, it could sure support the remaining APIs easily enough.

The other perspective is that the specific need on the part of Solid-OIDC is to be able to uniquely and securely identify RPs. Adding requirements to OPs above and beyond the needs of Solid only adds a burden to implementers.

I suspect that many Solid-OIDC OP implementations will also participate in a Federation, but I am wary of adding requirements that are not strictly necessary. It will be important to ensure that there is nothing in the text of the specification that prevents an IdP implementer from adding full OP support for federation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree, @acoburn! We should not make it any more difficult for OPs than necessary.

I just don't see how my suggestion (not including client_registration_types_supported in .well-known/oidc-configuration) would do that. As I see it, the only thing we really require from OPs is that they support Automatic Registration, and declare that in their .well-known/openid-federation statement. There is no need to also declare it in .well-known/oidc-configuration, because it has no meaning for clients reading that configuration.

Unless I am mistaken, this entails no other requirements from the OP.

- `code_challenge_method=S256`: Tells the OP that our code challenge was transformed using SHA-256.
- `code_challenge=HSi9dwlvRpNHCDm-L8GOdM16qcb0tLHPZqQSvaWXTI0`: The [=code challenge=] we generated before.

Note: If the app doesn't have a URI, you can either register an app using static registration
via some UI on the OP or use [dynamic registration](https://openid.net/specs/openid-connect-registration-1_0.html).

<h4 id="authorization-code-pkce-flow-step-7" class="no-num">7. Fetch RP Client ID Document</h4>
<h4 id="authorization-code-pkce-flow-step-7" class="no-num">7. Fetch RP Metadata</h4>

If an app URI is provided as the client id (see note above to see other options), we must
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect a break or header here to indicate that we're now switching from the app perspective to the OP perspective. Strange I haven't missed that before.

"client_id": "https://decentphtos.example/client_id",
"client_name": "DecentPhotos",
"redirect_uris": [ "https://decentphotos.example/callback" ],
"post_logout_redirect_uris": [ "https://decentphotos.example/logout" ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-logout redirect URI disappeared?

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

Successfully merging this pull request may close these issues.

3 participants