[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