Skip to content

Conversation

@deshmukhrajvardhan
Copy link
Contributor

@deshmukhrajvardhan deshmukhrajvardhan commented Jul 17, 2024

PR Checkpoints

  • official html rendering, which is generated using mmark - Download it from the GitHub action tab, https://github.com/openid/OpenID4VP/pull//checks - by clicking on 'artifacts' and then 'output'
  • >=4 approvals OR >=2 if editorial PR
  • >=1 week since PR's final state
  • Only Chairs can merge the PR

Summary

I used https://jwt.io/ to create the signed and encoded payload for request.
Paste the payload in there to decode it and verify the signature

Related Issue(s)

Closes #78

Special notes for your reviewer

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit, but overall, looks good to me, thank you!

it is an editorial PR, so no need for 4 >= approvals :)

authorization request defined in rfc9101

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
@deshmukhrajvardhan
Copy link
Contributor Author

Thanks @Sakurann !
I've committed your suggested change :)
Ah! gotcha. How many approvals do we need for editorial change?

Comment on lines 305 to 307
response_type=vp_token
&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&request=eyJhbGciOiJSUzI1NiIsImtpZCI6ImsyYmRjIn0.eyJpc3MiOiJzNkJoZFJrcXQzIiwiYXVkIjoiaHR0cHM6Ly9zZWxmLWlzc3VlZC5tZS92MiIsInJlc3BvbnNlX3R5cGUiOiJ2cF90b2tlbiIsImNsaWVudF9pZCI6InM2QmhkUmtxdDMiLCJyZWRpcmVjdF91cmkiOiJodHRwcy8vY2xpZW50LmV4YW1wbGUub3JnL2NiIiwic2NvcGUiOiJvcGVuaWQiLCJwcmVzZW50YXRpb25fZGVmaW5pdGlvbiI6eyJpZCI6ImV4YW1wbGVfand0X3ZjIiwiaW5wdXRfZGVzY3JpcHRvcnMiOlt7ImlkIjoiaWRfY3JlZGVudGlhbCIsImZvcm1hdCI6eyJqd3RfdmNfanNvbiI6eyJwcm9vZl90eXBlIjpbIkpzb25XZWJTaWduYXR1cmUyMDIwIl19fSwiY29uc3RyYWludHMiOnsiZmllbGRzIjpbeyJwYXRoIjpbIiQudmMudHlwZSJdLCJmaWx0ZXIiOnsidHlwZSI6ImFycmF5IiwiY29udGFpbnMiOnsiY29uc3QiOiJJRENyZWRlbnRpYWwifX19XX19XX0sIm5vbmNlIjoibi0wUzZfV3pBMk1qIn0.m1KYXT4r5Pj7Jh8gUlngpvjUoEZnOcbL8lXuDxbLbQyhLG9mro9EpT3I4YOm25tnxFx2mrKb42dFPxXOwLO5NbBmoYmY8wxH85H98RtpPGaTwCD6duxbp4eZm6LO-fdxpSvjzCAKoU3kzloh1Q1ijkP_ZQgwmS-uQba9qpu61hMjF6KIIjz59sJHpZOI0cEz4LqrLwXXIht0hyZXdE_vJIBEwfafoLZgggF9adNGUQnE3dXLi_is6XrDk7BS0VZds6IaNTVlIlS5NfHg6Lp1IpbiFta_6CWrFxtC1QoCIrhIzbJgnF4GRboy0SfYRXD8Vt8qbPzLIeUvE-LyObGN5g
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with other examples we should use two space indents here:

Suggested change
response_type=vp_token
&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&request=eyJhbGciOiJSUzI1NiIsImtpZCI6ImsyYmRjIn0.eyJpc3MiOiJzNkJoZFJrcXQzIiwiYXVkIjoiaHR0cHM6Ly9zZWxmLWlzc3VlZC5tZS92MiIsInJlc3BvbnNlX3R5cGUiOiJ2cF90b2tlbiIsImNsaWVudF9pZCI6InM2QmhkUmtxdDMiLCJyZWRpcmVjdF91cmkiOiJodHRwcy8vY2xpZW50LmV4YW1wbGUub3JnL2NiIiwic2NvcGUiOiJvcGVuaWQiLCJwcmVzZW50YXRpb25fZGVmaW5pdGlvbiI6eyJpZCI6ImV4YW1wbGVfand0X3ZjIiwiaW5wdXRfZGVzY3JpcHRvcnMiOlt7ImlkIjoiaWRfY3JlZGVudGlhbCIsImZvcm1hdCI6eyJqd3RfdmNfanNvbiI6eyJwcm9vZl90eXBlIjpbIkpzb25XZWJTaWduYXR1cmUyMDIwIl19fSwiY29uc3RyYWludHMiOnsiZmllbGRzIjpbeyJwYXRoIjpbIiQudmMudHlwZSJdLCJmaWx0ZXIiOnsidHlwZSI6ImFycmF5IiwiY29udGFpbnMiOnsiY29uc3QiOiJJRENyZWRlbnRpYWwifX19XX19XX0sIm5vbmNlIjoibi0wUzZfV3pBMk1qIn0.m1KYXT4r5Pj7Jh8gUlngpvjUoEZnOcbL8lXuDxbLbQyhLG9mro9EpT3I4YOm25tnxFx2mrKb42dFPxXOwLO5NbBmoYmY8wxH85H98RtpPGaTwCD6duxbp4eZm6LO-fdxpSvjzCAKoU3kzloh1Q1ijkP_ZQgwmS-uQba9qpu61hMjF6KIIjz59sJHpZOI0cEz4LqrLwXXIht0hyZXdE_vJIBEwfafoLZgggF9adNGUQnE3dXLi_is6XrDk7BS0VZds6IaNTVlIlS5NfHg6Lp1IpbiFta_6CWrFxtC1QoCIrhIzbJgnF4GRboy0SfYRXD8Vt8qbPzLIeUvE-LyObGN5g
response_type=vp_token
&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&request=eyJhbGciOiJSUzI1NiIsImtpZCI6ImsyYmRjIn0.eyJpc3MiOiJzNkJoZFJrcXQzIiwiYXVkIjoiaHR0cHM6Ly9zZWxmLWlzc3VlZC5tZS92MiIsInJlc3BvbnNlX3R5cGUiOiJ2cF90b2tlbiIsImNsaWVudF9pZCI6InM2QmhkUmtxdDMiLCJyZWRpcmVjdF91cmkiOiJodHRwcy8vY2xpZW50LmV4YW1wbGUub3JnL2NiIiwic2NvcGUiOiJvcGVuaWQiLCJwcmVzZW50YXRpb25fZGVmaW5pdGlvbiI6eyJpZCI6ImV4YW1wbGVfand0X3ZjIiwiaW5wdXRfZGVzY3JpcHRvcnMiOlt7ImlkIjoiaWRfY3JlZGVudGlhbCIsImZvcm1hdCI6eyJqd3RfdmNfanNvbiI6eyJwcm9vZl90eXBlIjpbIkpzb25XZWJTaWduYXR1cmUyMDIwIl19fSwiY29uc3RyYWludHMiOnsiZmllbGRzIjpbeyJwYXRoIjpbIiQudmMudHlwZSJdLCJmaWx0ZXIiOnsidHlwZSI6ImFycmF5IiwiY29udGFpbnMiOnsiY29uc3QiOiJJRENyZWRlbnRpYWwifX19XX19XX0sIm5vbmNlIjoibi0wUzZfV3pBMk1qIn0.m1KYXT4r5Pj7Jh8gUlngpvjUoEZnOcbL8lXuDxbLbQyhLG9mro9EpT3I4YOm25tnxFx2mrKb42dFPxXOwLO5NbBmoYmY8wxH85H98RtpPGaTwCD6duxbp4eZm6LO-fdxpSvjzCAKoU3kzloh1Q1ijkP_ZQgwmS-uQba9qpu61hMjF6KIIjz59sJHpZOI0cEz4LqrLwXXIht0hyZXdE_vJIBEwfafoLZgggF9adNGUQnE3dXLi_is6XrDk7BS0VZds6IaNTVlIlS5NfHg6Lp1IpbiFta_6CWrFxtC1QoCIrhIzbJgnF4GRboy0SfYRXD8Vt8qbPzLIeUvE-LyObGN5g

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

"nonce": "n-0S6_WzA2Mj"
}
```
With these keys:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's only one key, so "keys" is wrong. Suggest a slightly more verbose wording too:

Suggested change
With these keys:
The example request object above is signed with this key:

```
GET /authorize?
client_id=client.example.org
response_type=vp_token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response_type isn't legal here as I mentioned here: #78 (comment)

Suggested change
response_type=vp_token

response_type=vp_token
&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&client_id_scheme=x509_san_dns
&client_metadata=...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client_metadata also isn't legal here (see link in previous comment)

&request_uri=https%3A%2F%2Fclient.example.org%2Frequest%2Fvapof4ql2i7m41m68uep
&request_uri_method=post HTTP/1.1
```
And, later use the `request_uri` for the below request:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what is meant?

Suggested change
And, later use the `request_uri` for the below request:
And, later the wallet might send the following non-normative example request to the `request_uri`:

```
And, later use the `request_uri` for the below request:
```
POST /request HTTP/1.1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To match the above example it needs the full path that was passed in request_uri parameter:

Suggested change
POST /request HTTP/1.1
POST /request/vapof4ql2i7m41m68uep HTTP/1.1

"response_type": "vp_token",
"client_id": "s6BhdRkqt3",
"redirect_uri": "https//client.example.org/cb",
"scope": "openid",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have probably preferred if all three examples showed the same request so that we were just highlighting the differences of the 3 formats (e.g. this request has scope=openid and the above one doesn't) but I don't feel strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Agreed!

deshmukhrajvardhan and others added 2 commits July 23, 2024 14:10
Formatting space

Co-authored-by: Joseph Heenan <joseph@authlete.com>
```
{
"kty":"RSA",
"kid":"k2bdc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this key sure does bring back some memories, thanks for that, but this is not really consistent with how examples are provided in the rest of this document, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use existing key

{
  "kty": "EC",
  "crv": "P-256",
  "x": "MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4",
  "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM",
  "use": "enc",
  "kid": "1",
  "alg": "PS256"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, what, the encryption key from one of the examples in the W3C Digital Credentials API section? that doesn't seem good. and where did you find the private key? so many questions...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow the private key is apparently here https://datatracker.ietf.org/doc/html/rfc7517#appendix-A.2 which is fun

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but maybe that's not even relevant

sorry

i am genuinely confused

Copy link
Member

@bc-pi bc-pi Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and by confused #218 (comment) i mean I don't understand what's going on here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review and I hope to learn!

I found the key in this repo https://github.com/openid/OpenID4VP/blob/main/examples/digital_credentials_api/signed_request_payload.json#L19 and the draft https://openid.github.io/OpenID4VP/openid-4-verifiable-presentations-wg-draft.html
you are right it is the same one as in that doc in section A1 https://datatracker.ietf.org/doc/html/rfc7517#appendix-A.1 and A.2 section has the private key.

Lmk, if I should use a different key as its private key is readily available? Or do I need to create a new key pair and mention them in the doc? or something else?

Happy to get feedback and incorporate changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of providing the key in a spec to allow for verification of examples is nice. But I don't think that's something that's been done much or at all in the 4VC family of documents. So doing it here is kind of out of place. And there are already plenty of opportunities for that in PAR, JAR, JWT, etc so it's arguably not necessary or even particularly useful to do here.

It's pretty well accepted that using the same key pair for both singing and encryption is a bad and potentially dangerous thing. So we should avoid doing anything that looks like it's condoning or encouraging the practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the guidance! That makes sense!
I have removed the key and shortened the jwt to be eyJrd... as no key is specified.

Let me know if this looks good.

@deshmukhrajvardhan deshmukhrajvardhan requested review from bc-pi and jogu July 26, 2024 18:11
&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&redirect_uri=https%3A%2F%2Fclient.example.org%2Fcb
&presentation_definition=...
&scope=openid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced it's a good idea to add scope=openid here, it makes it not a pure VP request and I think we want to show a basic VP request here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should i remove it from all 3 examples then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes, please - it's not there in any of the examples currently in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

&client_id=https%3A%2F%2Fclient.example.org%2Fcb
&request=eyJrdHkiOiJFQyIsImNydiI6IlAtMjU2IiwieCI6Ik1LQkNUTkljS1VTRGlpMTF5U3MzNTI2aURaOEFpVG83VHU2S1BBcXY3R
DQiLCJ5IjoiNEV0bDZTUlcyWWlMVXJONXZmdlZIdWhwN3g4UHhsdG1XV2xiYk00SUZ5TSIsInVzZSI6ImVuYyIsImtpZCI6IjEiLCJhbGc
iOiJQUzI1NiJ9.eyJpc3MiOiJzNkJoZFJrcXQzIiwiYXVkIjoiaHR0cHM6Ly9zZWxmLWlzc3VlZC5tZS92MiIsInJlc3BvbnNlX3R5cGU
Copy link
Member

@bc-pi bc-pi Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the header of this thing decodes to the following, which isn't ok

{
  "kty": "EC",
  "crv": "P-256",
  "x": "MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4",
  "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM",
  "use": "enc",
  "kid": "1",
  "alg": "PS256"
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the key and shortened the jwt to be eyJrd... as no key is specified.

Let me know if this looks good.

Copy link
Member

@bc-pi bc-pi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friday night after IETF week is probably not an ideal time to be reviewing PRs, sorry, but this needs some care and feeding

@deshmukhrajvardhan
Copy link
Contributor Author

Friday night after IETF week is probably not an ideal time to be reviewing PRs, sorry, but this needs some care and feeding

No problem and thanks for the review. I am happy to nurture it as much as needed with help from reviewers and reiterate until we are at a good spot!

&request_uri_method=post HTTP/1.1
&scope=openid
```
And, later the wallet might send the following non-normative example request to the `request_uri`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
And, later the wallet might send the following non-normative example request to the `request_uri`:
Later, the wallet might send the following non-normative example request to the `request_uri`:

@bc-pi
Copy link
Member

bc-pi commented Jul 31, 2024

No problem and thanks for the review. I am happy to nurture it as much as needed with help from reviewers and reiterate until we are at a good spot!

Appreciate that attitude, thank you! I'm attempting to help as a reviewer but, like all of us, have only limited cycles and have to try and manage where they are applied.

@cyberphone
Copy link

F.Y.I.
Another wallet scheme uses Deterministically Encoded CBOR and an enveloped signature, here shown in diagnostic nation. You may try it yourself: https://test.webpki.org/csf-lab/home

{
  1: {
    1: "Space Shop",
    2: "435.00",
    3: "USD"
  },
  2: "spaceshop.com",
  3: "FR7630002111110020050014382",
  4: "https://banknet2.org",
  5: "05768401",
  6: "2022-09-29T09:34:08-05:00",
  7: {
    1: 38.8882,
    2: 77.0199
  },
  / Enveloped signature object /
  -1: {
    / Signature algorithm = ES256 /
    1: -7,
    / Public key descriptor in COSE format /
    4: {
      / kty = EC /
      1: 2,
      / crv = P-256 /
      -1: 1,
      / x /
      -2: h'e812b1a6dcbc708f9ec43cc2921fa0a14e9d5eadcc6dc63471dd4b680c6236b5',
      / y /
      -3: h'9826dcbd4ce6e388f72edd9be413f2425a10f75b5fd83d95fa0cde53159a51d8'
    },
    / Signature value /
    6: h'5f21112b9f5c1a93743409596421e673f88e59cf34610d53a7cd00f4017327c8f603ea7344a9286db26c52c2a661dff6e4fabc0f2384fe7d1de6c0a23a3f5c21'
  }
}

I could not find anything in the example that looked like payment transaction data.

@bc-pi
Copy link
Member

bc-pi commented Aug 19, 2024

@cyberphone, how exactly is that comment helping progress this PR?

@cyberphone
Copy link

cyberphone commented Aug 19, 2024

@bc-pi Pardon me for disturbing the order. Anyway, the EUIDW payment project currently seems to lack a sequence diagram making transaction request data PRs somewhat less interesting. I haven't found any specification of virtual payment credentials either. In addition, I believe deterministically encoded CBOR would be a better mousetrap than JSON.

#188

@bc-pi
Copy link
Member

bc-pi commented Aug 19, 2024

@cyberphone, how exactly does that comment help answer the question about how exactly the previous comment is helping progress this PR?

@cyberphone
Copy link

@bc-pi Is this PR supposed to cover payment requests as well? If so, I would put it on hold because it doesn't seem to match this use case. See also: https://www.w3.org/TR/payment-request/

@bc-pi
Copy link
Member

bc-pi commented Aug 19, 2024

I commend the relentlessly consistent approach @cyberphone takes here and in many other forums. I apologize to anyone reading these PR comments, and especially folks subscribed and receiving notifications, for my momentary laps of judgement in thinking that engaging in this context could possibly have had a different outcome.

```
GET /authorize?
response_type=vp_token%20id_token
&scope=openid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scope=openid should actually stay, as this section is showing an example of a SIOP request which does require openid. (openid should still be omitted in the new examples you're adding)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! i wasn't sure about this earlier.
I've made the change.

The following is a non-normative example of Authorization Request with request object as value:
```
GET /authorize?
response_type=vp_token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response_type isn't legal here, when request= is used response_type can only be inside the request object, it should be removed:

Suggested change
response_type=vp_token

Copy link
Contributor Author

@deshmukhrajvardhan deshmukhrajvardhan Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! good catch! I've made the change

@jogu jogu added the editorial label Sep 27, 2024
@Sakurann Sakurann added this to the 1.1 milestone Mar 4, 2025
@jogu jogu self-requested a review March 18, 2025 02:36
@Sakurann
Copy link
Collaborator

@deshmukhrajvardhan looks like the content is good to go and can be merged but there are so many conflicts, it might be easier to do a new PR to the most recent version of the spec and add content from this PR to it? (we can merge it without additional review period based on the reviews on this PR if you can do a new PR) (you can also resolve conflicts on this PR) please let us know how you would like to proceed.

@deshmukhrajvardhan
Copy link
Contributor Author

@deshmukhrajvardhan looks like the content is good to go and can be merged but there are so many conflicts, it might be easier to do a new PR to the most recent version of the spec and add content from this PR to it? (we can merge it without additional review period based on the reviews on this PR if you can do a new PR) (you can also resolve conflicts on this PR) please let us know how you would like to proceed.

@Sakurann I agree with your suggestion. A new PR would be easier. Will create and attach the link in the next comment.

@deshmukhrajvardhan
Copy link
Contributor Author

@Sakurann
Copy link
Collaborator

replaced by #464

@Sakurann Sakurann closed this Mar 20, 2025
Sakurann pushed a commit that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specification is missing examples for request objects

7 participants