[Openid-specs-ab] Issue #1372: SIOP v2 -06 review (pre implementer's draft) (openid/connect)

[David Waite]

New issue 1372: SIOP v2 -06 review (pre implementer's draft)
https://bitbucket.org/openid/connect/issues/1372/siop-v2-06-review-pre-implementers-draft

David Waite:

From Section 6: 

> \(based on the openid: custom scheme\)

Recommend “\(based on custom URL schemes or claimed “https” URIs\)”

This section seems like where we would describe response\_mode=post

This section needs reference to security considerations, e.g. “this has reduced security properties see security considerations for more information”

‌

From Section 9.1:

The authorization\_endpoint should be specified as “openid://”

‌

From Section 9.2:

This basically specifies a recovery for no jwks\_uri, which means it goes from a must not specify to a must ignore. If we want to support specifying a jwks\_uri, we MUST have another metadata attribute to differentiate SIOP from non-SIOP processing behavior \(e.g. an i\_am\_siop metadata property\)

Note that if we have an i\_am\_siop metadata property, we no arguably no longer require an i\_am\_siop claim in the id\_token. 

‌

Section 9.2.2:

Should reference the jwk-thumbprint urn scheme for the thumbprint rather than just saying to b64url the value?

‌

Section 10.2:

* Signed requests can either be OpenID Federation entity statements or other URI which can be resolved to a set of keys.
* That clarification is needed to indicate that when the client\_id represents a federation entity statement, registration parameters MUST NOT be present.

‌

Section 10.2.2.1:

> When Relying Party's client\_id is expressed as an https URI

this also needs to state that the request must be signed for federation entity statements

‌

Section 10.2.3:

> If the RP uses more than one Redirection URI, the redirect\_uris parameter would be used to register them.

10\.2 forbids redirect\_uris  in the \`registration\` and \`registration\_uri\`, so we should probably just eliminate this text. Multiple redirect\_uris are still allowed when using entity statements.

Also, `subject_syntax_types_supported` is atypical from other registration parameters, where the RP specifies exactly what configuration it wants \(see [https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#client-metadata](https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#client-metadata) \). Is there a reason this shouldn’t be `subject_syntax_type`?

‌

Section Section 11:

* Scope is listed as required, but should be forbidden in cross-device flows
* The registration and registration\_uri parameter descriptions should just point to 10.2
* “To prevent duplication…” - remove registration\(\_uri\) from list, since described in 10.2

I’d recommend for a future edit - remove request/request\_uri from the parameters list, since they describe \_how\_ to send the other parameters. Instead, describe them in the subsequent text of the section, along with the list of query parameters which MUST still be sent with them on the uri.

I put up also for consideration whether we should recommend against `registration_uri` when `request_uri` is being used.  

Section 11.1:

For the following text:

> Note that the authentication request might only include request parameters and not be targeted to a particular authorization\_endpoint, in which case, the user must use a particular Self-Issued OP application to scan the QR code with such request.

I’d recommend:

“Note that the authentication request URI may be insufficient to target the intended SIOP using a platform or third-party QR code scanning system. In this case, the user will need to use the Self-Issued OP application to scan the QR code”

‌

Section 12.2:

> Note that HTTP error codes do not work in the cross-device Self-Issued OP flows.

I’d recommend something more like  
”When response\_mode=post is used to submit the response directly to the RP redirect URI, the HTTP response is not used to convey information to the SIOP about success or failure and MUST be ignored”

Also, recommend removing `user_cencelled` as an error code as it seems to be a non-SIOP-specific error, and we do not define any usage criteria.  

Section 12.3:

I’d recommend just making `i_am_siop` required for all SIOP id\_tokens, rather than special-casing it.

‌

Section 13, Bullet \(1\):

> The Relying Party \(RP\) MUST determine whether the ID Token has been issued by the Self-Issued OP. When static Self-Issued OP Discovery metadata has been used, iss MUST be [https://self-issued.me/v2](https://self-issued.me/v2). When Self-Issued OP Discovery metadata has been obtained dynamically, an additional i\_am\_siop Claim MUST be present in the ID Token and iss MUST exactly match the issuer identifier specified in the Self-Issued OP Discovery Metadata MUST be included in the ID Token as a way for the RP to determine if the, when dynamic Self-Issued OpenID Provider discovery has been used.

Recommend changing this to something like:

1. Verify the issuer of the id\_token. If the OP metadata, whether negotiated out-of-band or retrieved dynamically, has associated signing keys \(such as those referenced via `jwks_uri`, the OP is not Self-Issued and processing should proceed using the verification steps defined by OpenID Core.
2. If the ID Token is encrypted, decrypt it using the keys and algorithms that the Client specified during Registration that the Self-Issued OP was to use to encrypt the ID Token. If encryption was negotiated with the Self-Issued OP at registration time and the ID Token is not encrypted, the RP SHOULD reject it.
3. The Issuer Identifier for the Self-Issued OpenID Provider from the request MUST exactly match the value of the `iss` \(issuer\) Claim.
4. Verify the id\_token has an `i_am_siop` claim with value `true`, indicating it is intended to be processed as a Self-Issued ID Token

Section 13, Bullet \(2\):

> The RP MUST validate that the aud \(audience\) Claim contains the value of the client\_id that the RP sent in the Authentication Request as an audience. When the request has been signed, the value might be an HTTPS URL, or a Decentralized Identifier.

There is no need for that second sentence. We also need to define how to process audience as an array. Recommend:

“The RP MUST validate that the aud \(audience\) Claim contains the value of the client\_id that the RP sent in the Authentication Request as an audience. This MUST be specified either as a string, or as an array containing a single string value.”

Section 13, Bullet \(3\):

“The RP MUST identify which Subject Syntax Type is used based on the URI of the sub Claim. Valid values defined in this specification are urn:ietf:params:oauth:jwk-thumbprint for JWK Thumbprint Subject Syntax Type and did: for Decentralized Identifier Subject Syntax Type.”

… and types with a “did:” prefix, indicating particular Decentralized Identifier methods.  

Section 13, Bullet \(4 & 5\):

> The RP MUST validate the signature of the ID Token. When Subject Syntax Type is JWK Thumbprint, validation is done according to JWS \[RFC7515\] using the algorithm specified in the `alg` header parameter of the JOSE Header, using the key in the `sub_jwk` Claim. The key MUST be a bare key in JWK format \(not an X.509 certificate value\). The RP MUST validate that the algorithm is one of the allowed algorithms \(as in `id_token_signing_alg_values_supported`\). When Subject Syntax Type is Decentralized Identifier, validation is performed against the key obtained from a DID Document. DID Document MUST be obtained by resolving a Decentralized Identifier included in the `sub` Claim using DID Resolution as defined by a DID Method specification of the DID Method used. Since `verificationMethod` property in the DID Document may contain multiple public key sets, public key identified by a key identifier `kid` in a Header of a signed ID Token MUST be used to validate that ID Token.
>
> The RP MUST validate the `sub` value. When Subject Syntax Type is JWK Thumbprint, the RP MUST validate that the `sub` Claim value equals the base64url encoded representation of the thumbprint of the key in the `sub_jwk` Claim, as specified in Section 12. When Subject Syntax Type is Decentralized Identifier, the RP MUST validate that the `sub` Claim value equals the `id` property in the DID Document.

The ordering should be resolve, check sub value, validate signature.

We should define the format of `sub_jwk` separately so we do not need to define it here. We should also define the DID mechanism separately so we do not need to define it here.

The MUST on checking id\_token\_signing\_alg\_values\_supported is stronger than OpenID Core. Is there a reason for this?

With these changes in mind, including describing sub\_jwk and DID requirements elsewhere

4: The RP MUST resolve an appropriate public key for the ID token signature, based on the information conveyed via the `sub` claim. 

4\.a: When the subject type is a JWK thumbprint, the public material MUST be extracted from the `sub_jwk` element. The `sub_jwk` MUST only contain the required values for a given `kty`. The subject identifier MUST be validated to be a thumbprint matching the specified key, using the mechanism defined in \[RFC7638\].

4\.b:  When the subject type is a decentralized identifier, the RP MUST resolve the DID to a DID document. The `id` of the DID document MUST exactly match the `sub` value. A key from the DID document MUST be specified by exact match with the `kid` protected header of the id\_token. The key specified MUST be appropriate for assertion signing.

5\. The Client MUST validate the signature of the ID Tokens using the resolved subject key, according to [JWS \[JWS\] using the algorithm specified in the JWT ](https://openid.net/specs/openid-connect-core-1_0.html#JWS)`alg` Header Parameter. The Client MUST NOT use any keys associated with the issuer.

‌

Section 13.1:

There is no reason that the nonce claim should be validated in any different manner from same-device flows. This likely makes this section redundant.  

Section 14.1:

Title should call out it is specifically the security considerations for cross-device usage