ConsumerDataStandardsAustralia / infosec

Work space for the consumer data right information security profile development in Australia
MIT License
16 stars 5 forks source link

spec is a little unclear as to what is normative #15

Closed jogu closed 6 years ago

jogu commented 6 years ago

(I'm not sure if general feedback is welcome or if this is the correct way to provide it; feel free to point me in the right direction if not!)

Having read https://consumerdatastandardsaustralia.github.io/infosec/ I find it a little hard going.

To give specific examples:

"Data Holder Metadata" section, "issuer" field - it looks like this really exactly repeats the requirements in https://openid.net/specs/openid-connect-discovery-1_0.html - the spec might be easier to read if it called out just what it was trying to say that's different to that already required by the discovery specification.

"Client Authentication - private_key_jwt" - repeats much of https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication - I'm unsure if anything is different. (Although it, presumably accidentally, doesn't use the word 'must' anymore when describing 'iss' and so on.)

It might be helpful to split the document up into two parts - a normative section that says "Follow OAuth2, OIDCC, OIDD, FAPI part 2, but with these additional requirements" and another non-normative section that provides further useless text to the reader, for example explaining exactly what fields are required in a client assertion by the time once you've combined the relevant standards.

lukepopp commented 6 years ago

Hi @jogu

https://openid.net/specs/openid-connect-discovery-1_0.html doesn't address things like CIBA or FAPI discovery directly which is why there is a detailed section on this in the profile - it lists mandatory values for fields (required by FAPI for example) and additional fields (which is likely to grow). The private jwt section does mention those fields are mandatory but does so in an inconsistent way so will address that. Furthermore, samples will help as well and they are to be added. Thanks for feedback - a normative section could be the way to go but we want to avoid being ambiguous.

jogu commented 6 years ago

Thanks @lukepopp !

The danger I see with restating what's in existing specs in a normative fashion is that it gives rise to confusion when the Australian standard appears to conflict with what what the upstream specs say, which is easily done (we've seen it happen quite a few times with the UK OB standards). To give one example 0.0.1 says "response_mode_supported REQUIRED Must contain one or more of these values: fragment query" but FAPI/OIDC restrict this so that response_mode_supported must only contain 'fragment' (as the query mode is explicitly not allowed to be used in the hybrid flow). I suspect the Australian spec does not mean to override the upstream specs to allow the use of query but some people may interpret it that way unless it's clear that text intended to simply restate what the upstream standards say; it's then clear in the case of a conflict that following the upstream spec is the correct approach.

Another example is that the authorisation endpoint request appears to require that prompt is passed as a parameter in the query string, but I think this is unnecessary (as prompt must presumably also be passed inside the request object as per FAPI part 2 5.2.3-8) and passing it outside as well would be unnecessary and overly long urls can be an issue for some browsers/web servers. The request object presumably also requires an exp claim as per FAPI part 2 5.2.2-13 but the omission of that leaves me wondering if request objects without the exp field are allowed in the Australian standard.

I suspect there could be more examples but it takes quite a bit of time and expertise to surface those kind of discrepancies.

BTW: I'm writing from the perspective of a vendor that already provides FAPI & UK OpenBanking compliant authorisation servers, looking at what changes would be necessary to support the Australian ecosystem.

lukepopp commented 6 years ago

Thanks @jogu. It's interesting that the attempt to be explicit has confused you. Thanks for picking up that bug (query and fragment option - this is genuinely a bug). We will flush out the Request object as this is one area where I feel OIDC needs improving - I find the essential and voluntary claims sections confusing and misleading. However, I think you're right: it would be better to point to the standards (normative) and focus on the deltas. However, the profile is also an educational instrument for those that are not across OIDC/FAPI so we just need to keep that in mind.

jogu commented 6 years ago

Thanks @lukepopp

I can likely guess what you mean, but having had a lot of discussions with banks about what standards mean over the last year it is very hard to get them to stick to a standard if there's ambiguity in the text - a mistake made in what should have been non-normative text in the OB profile meant we had to accept two banks as certified & compliant with the standard even though their behaviour was not actually what was intended and was not behaviour permitted by OpenID Connect Core. (Doing otherwise would have created a precarious legal position.)

I agree with the about the claims section being fraught in OIDC with fraught with danger and commonly misunderstood. If you have any improved text that fixes that (but doesn't change behaviour) there is currently work underway to apply errata to OIDC; you can file suggestions at https://bitbucket.org/openid/connect/issues?status=new&status=open - a particular bit to be careful about is the acr claim, which explicitly has different behaviour in OIDCC to other essential: yes claims.

Other changes improvements you've made in the Australian standard may well make sense to push upto FAPI (if it's in FAPI you're likely to be getting vendor support and it's also going to make life easier for people trying to consume the APIs using open source libraries) - eg. the Australian standard makes aud & iss mandatory in the request object; I've already raised a defect that I think FAPI should make these mandatory: https://bitbucket.org/openid/fapi/issues/190/aud-iss-should-be-mandatory-in-requests - if there's other changes FAPI needs we could likely get a third implementors draft of the FAPI spec published on a timeline that makes sense for Australia. If there's other things you've spotted that make sense to include in FAPI let me know, I can raise them with the WG if that would be helpful.

lukepopp commented 6 years ago

@jogu, I'll be creating a normative vs non normative feature soon to clear up the confusion. Thanks again for your comments.