add data integrity conversion for vp token #42
Conversation
Ryanmtate
requested review from
timothee-haudebourg,
cobward and
Joey-Silberman
Ryanmtate
force-pushed
the
feat/vp-data-integrity-cryptosuite
branch
from
0c3ad3b to
453c31d
Compare
| // This bypass is for unencoded JWT requests, but we will need to change this later | ||
| // so that trust is preserved when receiving unencoded requests | ||
| // NOTE: This requires that `Algorithm::None` is permitted in the wallet metadata | ||
| // Otherwise, this function will error in the previous assertion. | ||
| if alg.contains("none") { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the spec, I think we should check whether the kid exists before we check for alg is set to none.
See: https://datatracker.ietf.org/doc/rfc9101/
The authorization server MUST validate the signature of the JWS-
signed [RFC7515] Request Object. If a "kid" Header Parameter is
present, the key identified MUST be the key used and MUST be a key
associated with the client. The signature MUST be validated using a
key associated with the client and the algorithm specified in the
"alg" Header Parameter. Algorithm verification MUST be performed, as
specified in Sections 3.1 and 3.2 of [RFC8725].
If the key is not associated with the client or if signature
validation fails, the authorization server MUST return an
"invalid_request_object" error to the client in response to the
authorization request.
There was a problem hiding this comment.
I think this requirement from openid4vp supercedes this requirement:
Since DID Document may include multiple public keys, a particular public key used to sign the request in question MUST be identified by the kid in the JOSE Header.
There was a problem hiding this comment.
I agree this requirement takes precedence. The question I have now, though, is that it seems the missing kid value in the header is in nonconformance with the oid4vp specification.
We are currently bypassing the kid check IFF the wallet metadata supports a None algorithm for signing.
This preserves the explicit check kid, but allows the wallet metadata to control bypassing this assertion.
@cobward What suggested changes do you think we should make here?
@Joey-Silberman anything you want to add?
There was a problem hiding this comment.
I would suggest don't make the changes here. This library implements DID client id scheme request verification in the standard way. If we need to have a workaround for a particular integration then we can write a non-standard implementation of the RequestVerifier trait: https://github.com/spruceid/openid4vp/blob/main/src/core/authorization_request/verification/mod.rs#L29
There was a problem hiding this comment.
If time doesn't permit making the update elsewhere (in mobile-sdk), then I don't think we should block merging this in. Just ensure that we clarify the issue with the relevant parties and fix this at the earliest opportunity.
| // This bypass is for unencoded JWT requests, but we will need to change this later | ||
| // so that trust is preserved when receiving unencoded requests | ||
| // NOTE: This requires that `Algorithm::None` is permitted in the wallet metadata | ||
| // Otherwise, this function will error in the previous assertion. | ||
| if alg.contains("none") { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this requirement from openid4vp supercedes this requirement:
Since DID Document may include multiple public keys, a particular public key used to sign the request in question MUST be identified by the kid in the JOSE Header.
Signed-off-by: Ryan Tate <[email protected]>
* use serde deserialize serialize derive for authorization response Signed-off-by: Ryan Tate <[email protected]> * update url encoding for auth response Signed-off-by: Ryan Tate <[email protected]> * fix unit tests Signed-off-by: Ryan Tate <[email protected]> * use custom struct for json string encoded authorization response inner values Signed-off-by: Ryan Tate <[email protected]> --------- Signed-off-by: Ryan Tate <[email protected]>
Signed-off-by: Ryan Tate <[email protected]>
Ryanmtate
force-pushed
the
feat/add-req-obj-signing-alg-setter-wallet-metadata
branch
from
9c71c21 to
942a9a4
Compare
…sentation Signed-off-by: Ryan Tate <[email protected]> Co-Authored-By: Joey Silberman <[email protected]>
Signed-off-by: Ryan Tate <[email protected]>
Additionally adds a check for authorization request vp formats supported to check cryptosuite against expected response formats. Signed-off-by: Ryan Tate <[email protected]>
Signed-off-by: Ryan Tate <[email protected]>
* Add temporary support for unencoded JWT authorization requests * Remove unnecessary comment * use serde deserialize serialize derive for authorization response Signed-off-by: Ryan Tate <[email protected]> * update url encoding for auth response Signed-off-by: Ryan Tate <[email protected]> * fix unit tests Signed-off-by: Ryan Tate <[email protected]> * use custom struct for json string encoded authorization response inner values Signed-off-by: Ryan Tate <[email protected]> * allow unencoded authorization request Signed-off-by: Ryan Tate <[email protected]> --------- Signed-off-by: Ryan Tate <[email protected]> Co-authored-by: Joey Silberman <[email protected]> Co-authored-by: Ryan Tate <[email protected]>
Co-authored-by: Jacob <[email protected]> Signed-off-by: Ryan Tate <[email protected]>
Ryanmtate
force-pushed
the
feat/vp-data-integrity-cryptosuite
branch
from
92eeb53 to
bcc5b5d
Compare
| // This bypass is for unencoded JWT requests, but we will need to change this later | ||
| // so that trust is preserved when receiving unencoded requests | ||
| // NOTE: This requires that `Algorithm::None` is permitted in the wallet metadata | ||
| // Otherwise, this function will error in the previous assertion. | ||
| if alg.contains("none") { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
If time doesn't permit making the update elsewhere (in mobile-sdk), then I don't think we should block merging this in. Just ensure that we clarify the issue with the relevant parties and fix this at the earliest opportunity.
Additionally adds a check for authorization request vp formats supported to check cryptosuite against
expected response formats.