[Openid-specs-ab] SIOP v2 Review

[Kristina Yasuda]

Hi Torsten,

Thank you so much for the review and the feedback.
I started a PR that incorporates your feedback: https://bitbucket.org/openid/connect/pull-requests/68
It still needs to be worked on, but it shows the suggestions of how to update the text as I also summarized in-line in blue below.

Best Regards,
Kristina

________________________________
From: Torsten Lodderstedt <torsten at lodderstedt.net>
Sent: Sunday, November 7, 2021 4:21
To: Artifact Binding/Connect Working Group <openid-specs-ab at lists.openid.net>
Cc: Kristina Yasuda <Kristina.Yasuda at microsoft.com>
Subject: SIOP v2 Review

Hi all,

I reviewed the current master revision of SIOP v2 with a special focus on SIOP discovery, client id resolution and sub types.

Generally, I agree with the direction of travel. I think we need to find the right balance between flexibility and simplicity, esp. related to client ids and authentication requests.

Here is my detailed feedback:

- "Relying Parties typically cannot pre-establish registration with a Self-Issued OP, as each End-user might be represented by a different, locally-controlled Self-Issued OP instance. This specification extends the authentication request with a mechanism with additional dynamic registration techniques for feature negotiation.“

Other models are possible, e.g. all instances of a wallet app sharing the same RP registration data. This should be mentioned in the spec.
-> below text added in this scope section. Still need to think how to add this to other sections such as the registration metadata section.
"In another model, when all instances of Sel-Issued OPs share the same RP registration data, it is possible for the RPs to pre-establish registration with that set of Self-Issued OPs."
Assuming that the method used by the RP in this case would be out-of-band or Dynamic Client Registration..?

- „## Self-Issued OpenID Provider Invocation“

I this section should also mention that the current custom URI scheme `openid://` does not work with web wallets.
-> text added

"In the first scenario, request is encoded in a QR code, and the End-user scans it with the camera via the application that is intended to handle the request from the RP. In this scenario, the request does not need to be intended for a specfiic `authorization_endpoint` of a Self-Issued OP. Note that this will not work in a same-device Self-Issued OP flow, when the RP and the Self-Issued OP are on the same device.“

Having the authorization endpoint in the QR code allows scanning with an arbitrary cam app whereas. Just sending the parameters requires the user to use the wallet app for that purpose. I think we need to support both options.
Note: the „just parameters approach“ is not reflect the in the cross device section of the spec yet.
-> added a text with this meaning to the cross-device section too.

- "### Common identifier for one or more Self-Issued OPs (better title needed)

This identifier should be communicated to the RP as an `authorization_endpoint` URI which every Self-Issued OP supports“

How is this use case any different from "### Separate identifier per Self-Issued OP (better title needed)“ from a RP perspective? I’m asking since I don’t understand why this section deviates from the rest of the section by using the authorization endpoint as identifier instead of the issuer URL?
-> merged these two sections and clarified the text, but needs to be discussed if we want to keep this descriptive text.

- "## Relying Party Registration“

The new revision supports entity statements and DIDs as only client id. This mandates signed requests for SIOP, which makes SIOP significantly more complex to implement. How is this justified?

I personally would feel better to retain the simple "client id is redirect uri“ approach as third option.
-> The intention was not to remove "client id is redirect uri“ option. I added the text clarifying that when the request is not signed, "client id is redirect uri“.
-> I also added a summary of the options in the very beginning of the registration and discovery section.

- "# Self-Issued OpenID Provider Response {#siop-authentication-response}" does not describe handling of SIOP responses with other than "https://self-issued.me/v2<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fself-issued.me%2Fv2&data=04%7C01%7CKristina.Yasuda%40microsoft.com%7Ce8687928bf7848967c2c08d9a1e912f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637718844724683333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=l1sGYdWOSvIDqjHKCJDSUJcai47TQ%2FVTAKL9hCXvVeE%3D&reserved=0>" iss values

- "1. The Relying Party (RP) MUST validate that the value of the `iss` (issuer) Claim equals to the `authorization_endpoint` in the Self-Issued OP metadata. When static Self-Issued OP Discovery metadata has been used, `iss` MUST be `https://self-issued.me/v2`<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fself-issued.me%2Fv2%2560&data=04%7C01%7CKristina.Yasuda%40microsoft.com%7Ce8687928bf7848967c2c08d9a1e912f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637718844724693333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5LktXgKYqbRVK3UT9dJzpg6eF%2FDYXZkdHj6VHNOayWw%3D&reserved=0>. When dynamic Self-Issued OP Discovery metadata has been performed, `iss` MUST exactly match the `issuer` identifier pre-obtained by the RP.“

The first sentence contradicts the last sentence (which I think ist correct). I suggest to remove the first sentence.
-> Your understanding is correct. I removed the first sentence.

- "1. The RP MUST validate that the `aud` (audience) Claim contains the value of the `redirect_uri` that the RP sent in the Authentication Request as an audience.“

I think this must be changed to incoporate the new possible values (DID, entity statement id).
-> good catch! text updated.

- "1. The RP MUST validate the signature of the ID Token. When Subject Syntax Type is `jkt`, …"

It ist unclear to me how the RP determines what subject syntax type is used. Is it the existence of the „sub_jwk“ claim in the id token?
-> Currently, if subject syntax type is did, sub=did:<method name>:<identifier>; if subject syntax type is jwk, sub_jwk is present. I added a text to this meaning, but feedback welcome if this is enough and makes sense.

General findings

- The draft currently lacks a couple of references to other specs just some examples: JWK, DID, Claims Aggregation specification, DID-CORE, VC-DATA, RFC6749 and OpenID-Core
-> thank you for reminding. will do an editorial PR.
- I would strongly encourage to authors to add more examples.
-> I added some examples in the text and added a placeholder for a separate examples sections, and what examples I think should go there - feedback welcome.

best regards,
Torsten.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-ab/attachments/20211108/5ca2e425/attachment.html>