[Openid-specs-ab] OpenID Connect Federation draft 09 ready for your review

Marcos Sanz sanz at denic.de
Tue Oct 29 15:43:31 UTC 2019


Hi Mike,
hi all,

here's my detailed review of the doc, as promised last week during the 
call (sorry, this mail's got much longer than I expected).

Issues

- Section 2.1, about "aud": it leaves open the value of "aud" to be 
something else than the entity identifier of the audience, and I wouldn't 
see room for that. Section 6.1.1 clearly calls "aud" to be "The entity 
identifier of the requester. ". I'd change the sentence in 2.1 to the 
following: "If present, the entity identifier for that entity MUST appear 
in this claim".
- Section 2.1, "metadata": It says "If the entity is a non-leaf entity it 
MUST contain a metadata object with a federation_entity object inside". 
This leaves open if a _leaf entity_ is allowed to publish a metadata with 
a "federation_entity' inside. This is specially relevant now that section 
3.6 has defined leaf entities to be also participants of the federation. 
Btw: the example in section 2.1 is lacking the now mandatory metadata 
element.
- Section 2.1: "metadata_policy": It says "If the metadata type identifier 
is federation_entity, then the policy MUST be applied to the immediate 
subordinate in the trust chain _unless that is a leaf entity_". Again: 
leaf entities are now also federation entities. The exclusion seems a bit 
arbitrary to me (see point before).
- Section 2.2: If I get the counting of i and j right, then the algorithm 
of verification is missing the final (and pretty important) "the jwks 
claim in ES[0] can be used to verify the signature of ES[0]"
- Section 4.1: It's not clear to me what should happen if application of 
the policy language results (or would result) in incorrect entity 
metadata. While 4.1.2 and 4.1.3 state "applying the policy MUST lead to 
failure" (this suggests process interruption and thus, probably, that the 
policy being processed wouldn't change the metadata of the whole entity 
under application), 4.4 says that in case of some claims having all their 
values removed (a subset of the possible reasons for an incorrect end 
state) only the responsible metadata statement will be removed. I think 
that a clear/coherent statement for failure processing is needed for all 
elements of the policy language if we expect this to be implemented 
consistently.
- Section 7.1: It says "Once the Consumer has found a trust anchor it 
wants to use, it MUST complete the trust chain by fetching the trust 
anchor's self-signed entity statement" This sentence is in contradiction 
with other parts of the spec: Section 2.2 clearly states that the trust 
chain "ends with an entity statement issued by the trust anchor about the 
top-most intermediate [...] or the leaf entity [...]", and it does thus 
exclude from the chain the (certainly most probably existing) trust 
anchor's self-signed entity statement. Verification algorithm of 2.2 
ignores the TA self-singed ES, so does Appendix A.1.7.
- Section 7.2: It says "using the rules laid out in that [Section 2.2] 
section" and it's not clear to me now what section is authoritative for 
such an important thing as the trust chain validation algorithm. In any 
case, duplication is definitely the worst option: A proof of that is that 
the pseudo-code in 7.2 does not match completely what 2.2 says, even after 
the aforementioned ammendment about ES[0] verfication. That's described in 
next point.
- Section 7.2: The chain validation algorithm in 7.2 is broken. It looks 
like it considers the TA self-signed statement to be part of the chain (cf 
"for j=0,i verify that iss==sub"), but if that's the case, you'll easily 
see by setting i=1 (no intermediaries, just one leaf and a TA) that you 
only perform 2 signature verifications instead of the 3 that'd be needed 
(self-signed leaf ES, TA about leaf ES, self-signed TA ES).
- Section 8.1: It says "If a leaf entity uses jwks_uri...", but I don't 
see how jwks_uri would be honored at all in this spec, so I think the 
statement about key rotation is void.
- Section 8.2: "A trust anchor must publish a self-signed entity statement 
about itself. As described above in Section 2.2, it should be at the end 
of the trust chain". Section 2.2 says actually the opposite, see further 
up.

Minor/Typos/Nits

- Section 1: It's a bit awkward that the term "trust issuer" is 
introduced/defined in the itnroduction, but then it's not used at all in 
the rest of the document. Drop it/rename it?
- Section 1: I'd move the sentence "All entities [...] MUST have a 
globally unique identifier" from the introduction further down, e.g. to 
the "Entity" definition in 1.2. It's a bit strange to have normative 
language in the intro, even before RFC2119 terms are presented.
- Section 2.1, "authority hints": There are still some instances overall 
in the document of "entity ID" that should be expanded to "entity 
identifier" and "trust root" that should be changed to "trust anchor". 
Further, it says that the value of "authority hints" is "an array of 
strings", but that's not the syntax being used in the examples, s. for 
instance section 6.1.2 and later in the Appendix A

  "authority_hints": {
    "https://edugain.org/federation": [
      "https://edugain.org/federation"
    ]
  }
- Section 2.1, "authority hints": It says "entity IDs of intermediate 
entities that may issue [...]", but it should say "entity identifiers of 
intermediate entities or trust anchors that may issue [...]". Further, 
when it says "this is REQUIRED" I would enforce "this is REQUIRED and MUST 
NOT be the empty list []" (stating this here is much more important than 
doing so for optional elements of the entity statement, like crit or 
policy_language_crit). Finally, this is just a nit, I'd change "absent in 
an entity statement that belongs to" to "absent in an entity statement 
issued by".
- Section 2.2: It says "The signing key that MUST be used to verify ES[i] 
is distributed from the trust anchors to the leaf entities [...]". I don't 
understand why the leaf entities have to have that information at all. 
Only the entity/consumer trying to verify the trust chain needs (the 
public part of) that key.
- Section 3.4: Second sentence is no sentence. Add "are applicable." at 
the end.
- Section 3.5: Are the parameters of this type defined in some RFC or 
somewhere else? Reference needed.
- Section 3.6, "federation_api_endpoint": This is new and I like it, it 
completely addresses my point of the last review about introducing one 
more level of indirection for the API discovery. The text leaves open 
whether leaf entities are allowed to publish such an endpoint. Is that on 
purpose?
- Section 3.6, "trust_anchor_id": two typos, missing dot after "OPTIONAL" 
and s/MUST used this/MUST use this/. However: what is "this" referencing? 
What is the formal definition of the possible syntax of a trust_anchor_id? 
And later in section 9.2 I see no mention of this parameter at all.
- Section 3.6, "max_path_length" and "naming_constraints": this is new and 
interesting. However, there's no explanation later about when/how these 
parameters are to be processed. Further: I am not sure these parameters 
are well-placed as "metadata", since they are not information from the 
entity about the entity itself, but it's a kind of policy imposed by the 
relevant entity further down the tree. So: What about moving these two to 
"metadata_policy"?
- Section 4: s/the information in a leaf/the metadata information in a 
leaf/
- Section 4.1.3: We need an explicit statement at the end on whether to 
deal strictly or not with superset operation (mathematical superset 
operation includes equality as a special case, strict superset doesn't). I 
am for the non-strict variant.
- Section 4.1.4: We need an explicit statement on what to do with add if 
the metadata statement claim doesn't exist at all. I am for instantiating 
the claim with the value to be added.
- Section 4.1.6: Capitalize "If" at the beginning of the sentence.
- Section 4.3: Missing trailing dot in second paragraph.
- Section 4.3.1, 2nd JSON example: There's a spurious comma at the end of 
the "add" statement
- Section 4.6, 1st JSON example: Missing closing quote marks for 
"response_type" name
- Section 4.6: s/federations policy/federation's policy/
- Section 4.6, last JSON example: Missing comma after "tos_uri" value
- Section 5: After the sentence "found using the Well-Known URIs 
specification" a reference to RFC 8615 would help.
- Section 5: It says "Therefore, if using the Federation API at the 
URL..." and that was right before the last changes, but now it should say 
"Therefore, if using the configuration endpoint at the URL..."
- Section 5: It says "Federation Entities MUST make a metadata statement 
available", but it'd be better to say "Federation Entities MUST make an 
Entity Configuration Document available" (it's clear to me that an entity 
configuration document is nothing but a metadata statement, but that'd be 
a spoiler for the next section ;-)
- Section 5.2: It says "The response is an Entity Statement, as described 
in entity statement". It'd be helpful if the word "self-signed" would 
appear somewhere and the "as described" attachment doesn't help. What 
about "The response is a self-signed Entity Statement"?
- Section 5.2: s/and the content type MUST/, the content type MUST/
- Section 5.2, example: I'd drop "jti" from the ES, not listed in 2.1
- Section 6: Spurious comma in the first sentence.
- Section 6.1.1, "aud": it's not clear from the text whether the issuer 
"may choose to include this" in the ES or it rather "SHOULD be present", 
these are not compatible. I'd go for the latter and rather say: "If the 
aud parameter is present in the request, the aud claim SHOULD be present 
in the entity statement response and take exactly that value". Which 
reminds me that we should add some cautionary text about making decissions 
based on the "aud" query string parameter, since the API is not 
authenticated and aud might take any value I'd like it to take. Maybe 
security considerations?
- Section 6.1.2, example: Missing comma after "redirect_uris", missing 
closing quote marks for "organization_name" value.
- Section 6.2.1: Regarding anchor query parameter, it has to be specified 
what format/syntax it's expected to have.
- Section 6.2.2: At first I was irritated that the response example is 
application/json instead of application/jose (differently from section 
6.1.1). Then I thought this is on purpose, but some explicit normative 
text would help. Btw, also in Section 6.3.2
- Section 6.3.1, request example: Rename "op" parameter to "operation", 
and drop "type"
- Section 7: The phrase "trusted trust anchors" is irritating, since it 
invites to think about the existence of "non-trusted trust anchors". I'd 
just write "trust anchors".
- Section 7.2: I think we need a final statement like "Consumers MAY cache 
Entity Statements or signature verification results for a given time until 
they expire (s. Section 7.4)" so that section 7.4 makes sense.
- Section 7.3: I think we need a final statement like "Consumers MAY 
follow other rules according to local policy".
- Section 7.4: I'd change the name of the section to "Calculating the 
Expiration Time of a Trust Chain" (expiration time is a recurring term in 
the doc, lifetime is not)
- Section 8: s/SHOULD/should/
- Section 9: s/an RP and an OP that has/an RP and an OP that have/ and 
s/RP uses dynamic/RP uses automatic/
- Section 9.1.1, example: the "Host" HTTP header value shouldn't carry any 
schema "https://"
- Section 9.2.2.1, step 4: It says "the entity statement is sent to the 
federation_registration_endpoint" and it left me wondering how. If this is 
a POST, it has to be clarified (specially since Section 6 says "all 
operations in the specification make use of a GET request"). An example 
would also help.
- Section 9.2.2.2: Though much will be inherited from Dynamic Client 
Registration spec, I'm missing some statement about response HTTP code, 
content-type, example, possible error codes (some from Dynamic Client 
Registration section 3.3, maybe some additional)...
- Appendix A.1, third bullet: Final sentence "Asking for information 
[·..]" is no sentence.
- Appendix A.1.1, example: "kid" top-most element is not defined. Also in 
some of the following sections. Delete it.
- Appendix A.1.3, example: "metadata" element is missing. Also in some of 
the following sections.
- Appendix A.1.5, last sentence: Maybe this is nitpicking, but it says 
"This is the entity statement of a trust anchor". I don't get the meaning 
(or maybe just its relevance here). How can the consumer know this 
(swamid.se) is the statement of a trust anchor? A trust anchor is whatever 
LIGO Wiki has configured as such, so it could've been "umu.se" in case of 
a different configuration but it's not.
- Appendix A.1.7, 1st bullet: Strange marriage of an opening regular 
bracket and a closing angle bracket
- Appendix A.2: s/which you can used/which can be used/
- Appendix A.2.1.2: "I will not list" is a bit harsh of the editor :-) 
Maybe better "we" or passive form.
- Appendix A.2.1.2: It says "There are two metadata policies" and it lists 
edugain.geant.org twice. One should be incommon.org.

Best regards,
Marcos

"Openid-specs-ab" <openid-specs-ab-bounces at lists.openid.net> wrote on 
21/10/2019 18:19:10:

> Von: "Mike Jones via Openid-specs-ab" <openid-specs-ab at lists.openid.net>
> An: "openid-specs-ab at lists.openid.net" 
<openid-specs-ab at lists.openid.net>
> Kopie: "Mike Jones" <Michael.Jones at microsoft.com>
> Datum: 21/10/2019 18:19
> Betreff: [Openid-specs-ab] OpenID Connect Federation draft 09 ready for 
your review
> Gesendet von: "Openid-specs-ab" 
<openid-specs-ab-bounces at lists.openid.net>
> 
> Draft 09 of the OpenID Connect Federation specification has been 
published at 
https://openid.net/specs/openid-connect-federation-1_0-09.html
> .  Major changes were:
> Separated entity configuration discovery from operations provided by the 
federation API.
> Defined new authentication error codes.
> 
> The authors believe that this version should become an Implementer’s 
Draft, in preparation for interop testing in the coming year.
> Please review!
> 
>                                                        Thanks,
>                                                        -- Mike
>  _______________________________________________
> Openid-specs-ab mailing list
> Openid-specs-ab at lists.openid.net
> http://lists.openid.net/mailman/listinfo/openid-specs-ab



More information about the Openid-specs-ab mailing list