oauth-wg / oauth-selective-disclosure-jwt

https://datatracker.ietf.org/doc/draft-ietf-oauth-selective-disclosure-jwt/
Other
56 stars 29 forks source link

Example 4a - address array disclosure #337

Closed sschulz-t closed 11 months ago

sschulz-t commented 1 year ago

I was wondering if there might be a mistake in the encoding of the address given in example 4:

<snip>
"birthdate": "1973-01-01",
  "address": {
    "postal_code": "12345",
    "locality": "Irgendwo",
    "street_address": "Sonnenstrasse 23",
    "country_code": "DE"
  },
  "is_over_18": true,
<snip>

The documentation says:

SHA-256 Hash: Y1urWJV_-HBGnSf9tFOwvH4cICRBCiKwEHfkXFSfjpo
Disclosure:
WyItNmhDZVJFV3JjRWg0Q2JzQ2RjVTVRIiwgImFkZHJlc3MiLCB7Il9zZCI6
IFsiOHo4ejlYOWpVdGI5OWdqZWpDd0ZBR3o0YXFsSGYtc0NxUTZlTV9xbXBV
USIsICJDeHE0ODcyVVhYbmdHVUxUX2tsOGZkd1ZGa3lLNkFKZlBaTHk3TDVf
MGtJIiwgImxjMk8weER4WXdiZ2M4cjJERXc3eWhfSURXZXhXOENiZ3R6WVBR
RlJpNGMiXSwgInBvc3RhbF9jb2RlIjogIjEyMzQ1IiwgImxvY2FsaXR5Ijog
IklyZ2VuZHdvIiwgInN0cmVldF9hZGRyZXNzIjogIlNvbm5lbnN0cmFzc2Ug
MjMiLCAiY291bnRyeV9jb2RlIjogIkRFIn1d
Contents: ["-6hCeREWrcEh4CbsCdcU5Q", "address", {"_sd":
["8z8z9X9jUtb99gjejCwFAGz4aqlHf-sCqQ6eM_qmpUQ",
"Cxq4872UXXngGULT_kl8fdwVFkyK6AJfPZLy7L5_0kI",
"lc2O0xDxYwbgc8r2DEw7yh_IDWexW8CbgtzYPQFRi4c"],
"postal_code": "12345", "locality": "Irgendwo",
"street_address": "Sonnenstrasse 23", "country_code": "DE"}]

I assume this should be SD-JWT with Recursive Disclosures?

It looks like the claims where added to the list on error and there is a missing hash. I would expect to see some additional disclosure elements for postal_code, ... (each with a nonce) similar to section-5.7.3 instead.

bc-pi commented 1 year ago

Thanks for catching that! There is definitely a mistake in Example 4a. But it looks like the problem has to do with decoy digests in that example. The decoys listed at https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-05.html#appendix-A.3-30 got pulled from the wrong place and so don't match up with the other example content, which includes the address claim Disclosure you've pointed. That all makes parts of that Example 4a problematic and hard to follow.

I think it'd be best addressed by not including decoys in Example 4a.

This is related: https://github.com/danielfett/sd-jwt/issues/13 FWIW

bc-pi commented 1 year ago

PR #338 aims to fix this. A preview from that PR of that example: https://drafts.oauth.net/oauth-selective-disclosure-jwt/bc-fix-ex-4a/draft-ietf-oauth-selective-disclosure-jwt.html#appendix-A.3

and specifically the Disclosure for address: https://drafts.oauth.net/oauth-selective-disclosure-jwt/bc-fix-ex-4a/draft-ietf-oauth-selective-disclosure-jwt.html#appendix-A.3-22

sschulz-t commented 12 months ago

Thanks for the quick response! That looks much better. I was expecting the address to be passed in a way that allows the selective disclosure of single items (e.g., only the post code). Maybe you can add an additional comment which of the three options in 5.7 every example uses (here 5.7.1).

To me, having three options in 5.7 makes it unnecessarily complicated. 5.7.1 and 5.7.3 are basically the same when you would allow having non-selectively disclosable items in this as well. If that would be the case, this could probably also handle all cases 5.7.2 would be used for.

danielfett commented 12 months ago

Thanks for reporting this and @bc-pi thanks for the quick fix!

@sschulz-t The options in 5.7 are meant to show what is possible with SD-JWT and are not meant to restrict or define how address can be encoded. It is up to the issuer to decide for each claim whether it should be SD or not. Options 1-3 just show some of the possible results.

I'm not sure I'm following what you mean by "having non-selectively disclosable items in this as well". Could you give an example?

danielfett commented 12 months ago

I created this PR to ensure that our wording around the options is clear in that the options are non-normative and not exhaustive: https://github.com/oauth-wg/oauth-selective-disclosure-jwt/pull/340

sschulz-t commented 12 months ago

I was wondering, if 5.7.1 would allow to have non-selectively disclosable items in the address claim:

["Qg_O64zqAxe412a108iroA",  "address", 
  {
   "_sd": ["6vh9bq-zS4GKM_7GpggVbYzzu6oOGXrmNVGPHP75Ud0","9gjVuXtdFROCgRrtNcGUXmF65rdezi_6Er_j76kmYyM","KURDPh4ZC19-3tiz-Df39V8eidy1oV3a3H1Da2N0g88"],
   "country": "DE"
  }
]

This single definition could then cover all use cases of 5.7.1,5.7.2, and 5.7.3.

But i see what you are up to (thanks for #340 👍). You do want to leave a maximum flexibility. However, i am a bit concerned of "creative" uses of what is technically possible and how to make sure to be able to parse/process all possible combinations in the openid4vp context.

danielfett commented 12 months ago

Yes, this mixture is possible within the current specification.

With the algorithm defined in Section 6.3 there is no need (for the Holder) to be aware of all possible combinations. (In other words, such assumptions should never be hard coded.)

It is just the Issuer that has to make a decision on which pattern to use.

sschulz-t commented 12 months ago

Sure, this should never be hard coded. However you need e.g., to come up with a user friendly way of displaying that kind of information in a wallet context. It is a challenge to find an intuitive/understandable way to inform the user which information will or can be disclosed and so on. But this is becoming way to off-topic ;) Thanks for your quick response!

bc-pi commented 11 months ago

https://github.com/oauth-wg/oauth-selective-disclosure-jwt/pull/338 merged