-
Notifications
You must be signed in to change notification settings - Fork 15
Require use of (most of) FAPI2 with VCI #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The requirements to use PAR and PKCE are already present in FAPI2 so can be removed here. closes #198
Sakurann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re PKCE and PAR
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
|
Discussed on today's WG call: Oliver to review then we'll merge. |
paulbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAPI seems to pull in some requirements like mTLS and specific TLS versions that many people implementing HAIP likely not support currently.
At the same time, some things of FAPI do not apply well, e.g. OIDC, CIBA or are already defined in OpenID4VCI (e.g. RFC8414)
So I'm unsure if we do ourselves a favor of pulling in all of FAPI 2.0 or just re-phrase the relevant things in HAIP or reference specific sections of FAPI.
The requirements are just the best current practices for TLS. If people implementing HAIP don't currently support at least TLS 1.2 and the IETF recommended ciphers, that's something those people really need to look at resolving.
CIBA isn't part of the FAPI2 Security Profile document that's linked to. OIDC isn't required by FAPI2 (and is barely mentioned). Authorization Servers are not currently required to support RFC8414 by OpenID4VCI; mandating FAPI2 like this is HAIP will mean that HAIP compliant ASes (and OAuth Clients) are required to support RFC8414. |
Also just for clarity, this PR does not pull in any requirements for mTLS - the two places where mTLS is permitted in FAPI2 is for client authentication and for sender constraining of access tokens. For the former we say wallet attestations must be used, and for the latter we say dpop must be used, so mtls is never applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a deeper look into FAPI 2 security profile, it doesn't seem as clear to me, so here are some considerations:
- Section 5.1.2 states: "However, for a profile to be compliant with this document, the profile shall not remove or override mandatory behaviors, as doing so is likely to invalidate the formal security analysis and reduce security in potentially unpredictable ways." -> unclear if we can apply this with making changes
- Section 5.2 Network layer protections, VCI and VP already point to BCP195, isn't that enough?
- I would not want to mandate MTLS
- Section 5.3 listing 9 technologies from which 2 not apply and 3 are already defined by VCI and VP itself, so this is only about DPoP, PKCE, PAR and RFC9207
- Section 5.3.2.1 most of these points are already defined within VCI / VP or do not apply imo
- Section 5.3.2.2 most of these things do apply, this seems like the most helpful section, maybe reference this one directly or copy the relevant points to HAIP?
- Sections 5.3.3 for clients are very similar to points above
- Section 5.4 on cryptography, this is an open point, I believe we will end up with a different choice than FAPI
- some parts are mentioning OIDC, which also do not apply
- people are already arguing that our ecosystem consists of too many standards to comply to, mandating FAPI 2 Security profile seems to feed this argument, while we also have define exceptions on top
- how many implementations do we have that are fully compliant to this?
tldr: I would rather define our requirements for DPoP, PKCE, PAR and RFC9207 in HAIP, maybe mention FAPI2 as SHOULD for additional considerations
Thanks Paul. Our profile would not be compliant with FAPI (because of the requirement to use client attestations). (I've suggested to FAPI WG that a later revision of FAPI could allow the use of client attestations, which may mean we are, at some later point in time, entirely compliant.)
Generally no - a lot of the points are covered by BCP 195, though some ('should' use DNSSEC, and I think 1 or 2 others) aren't covered.
Agreed. Although our experience with FAPI is that people do tend to mandate MTLS on top of protocols that don't strictly need it so I suspect some people (particularly when doing server to server) will do it and that section of FAPI provides some guidance on how to di that in an interoperable manner.
A lot of them aren't required in VCI/VP. e.g. the use of sender-constrained access tokens isn't required in VCI.
I wouldn't want to copy, but referring to them is an option. It doesn't necessarily improve things without careful wording (e.g. the question "can I just use a FAPI compliant AS" becomes a lot harder to figure out the answer for).
Yes, deliberately so - both sides need to do the correct thing. That's the kind of thing the current HAIP text isn't very good at (e.g. it says both sides must support PKCE, but it doesn't require the AS to reject requests without PKCE).
We may need something here, agreed.
Those parts are (or should all be) "if you are using OIDC, ....". So they're not harmful.
I think the question is whether we want a similar security level to FAPI2 or not.
I suspect zero - one of the battles in the interop testing was getting wallets to start doing PKCE for example, so I suspect live implementations are very rough and are not compliant with the current HAIP text even. That would actually be an argument in favour of using FAPI2, as it would mean a set of conformance tests could be shipped very quickly that'd give pretty decent coverage.
FAPI2 is already a SHOULD in VCI itself, so we wouldn't need to (and probably shouldn't) say that again in HAIP. |
|
Discussed on today's EU working group call - consensus on the call to progress in this direction. But adding a note that the sections about openid and mtls probably aren't too important could help people. And we want to make sure we don't encourage the application of mtls everywhere. Also we may need extra text about signing algorithms depending on conclusion of #112 - in particular if we end up allowing different for wallet attestation / proof / DPoP signing algs than the text here https://openid.net/specs/fapi-security-profile-2_0.html#section-5.4.1 :
(The current HAIP text requires support for ES256 so no action needed right now I think.) |
As per discussion on yesterday's working group call
|
I've added a note about some parts of FAPI not being applicable as per discussion on yesterday's working group call. @paulbastian could you take another look and see if you're okay withdrawing your request changes please? |
| * MUST comply with the [@!FAPI2_Security_Profile], including but not limited to using PKCE [@!RFC7636] with `S256` as the code challenge method, Pushed Authorization Requests (PAR) [@!RFC9126] and the `iss` value in the Authorization response [@!RFC9207]. The following exception to [@!FAPI2_Security_Profile] applies: | ||
| * Client authentication: Wallet Attestation as defined in (#wallet-attestation) in used. | ||
|
|
||
| Note that some parts of [@!FAPI2_Security_Profile] are not required when using only OpenID for Verifiable Credential Issuance - there is no need to use MTLS or OpenID Connect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
| * MUST comply with the [@!FAPI2_Security_Profile], including but not limited to using PKCE [@!RFC7636] with `S256` as the code challenge method, Pushed Authorization Requests (PAR) [@!RFC9126] and the `iss` value in the Authorization response [@!RFC9207]. The following exception to [@!FAPI2_Security_Profile] applies: | |
| * Client authentication: Wallet Attestation as defined in (#wallet-attestation) in used. | |
| Note that some parts of [@!FAPI2_Security_Profile] are not required when using only OpenID for Verifiable Credential Issuance - there is no need to use MTLS or OpenID Connect. | |
| * MUST support PKCE [@!RFC7636], Pushed Authorization Requests (PAR) [@!RFC9126] and the `iss` value in the Authorization response [@!RFC9207] as defined in [@!FAPI2_Security_Profile]. ``` |
As per WG discussion yesterday, and based on Paul's suggestion. closes #283
|
I've incorporated a suggestion based on @paulbastian wording, please recheck. (I didn't use the 'as defined in wording' as I think that resulted in it being arguable that other requirements in FAPI wouldn't apply, and I believe we do want those to apply.) |
|
WG discussion:
|
Sakurann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the WG discussion, would like to re read and undertand how to bring clarity
The requirements to use PAR and PKCE are already present in FAPI2 so can be removed here.
closes #198
closes #283