Skip to content
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

Add text on hash in KB-JWT #353

Merged
merged 24 commits into from
Oct 23, 2023
Merged

Add text on hash in KB-JWT #353

merged 24 commits into from
Oct 23, 2023

Conversation

danielfett
Copy link
Member

@danielfett danielfett commented Oct 17, 2023

Fixes Issue #346

Let's discuss whether we want to make the hash REQUIRED, OPTIONAL or RECOMMENDED. Current text is for REQUIRED. I'm leaning towards that in order to reduce optionality. It also means that there will be less situations where a Verifier accidentally accepts a KB-JWT without the hash (and we don't need to discuss mitigations against that).

@danielfett danielfett marked this pull request as ready for review October 17, 2023 14:55
Copy link
Collaborator

@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.

Overall this looks good. Thanks! And REQUIRED seems right. I even manually checked one example _sd_hash value and got a matching value, which is good :)
I'm "requesting changes" just for a couple of minor(ish) text suggestions. One of which was already mentioned in slack.

danielfett and others added 2 commits October 18, 2023 16:22
Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
@bc-pi
Copy link
Collaborator

bc-pi commented Oct 18, 2023

ugh, sorry about the trailing whitespace!

Co-authored-by: Orie Steele <orie@or13.io>
@@ -30,7 +30,7 @@ jobs:
- name: "Install SD-JWT tooling"
run: |
python3 -m pip install --upgrade pip
python3 -m pip install git+https://github.com/openwallet-foundation-labs/sd-jwt-python.git
python3 -m pip install git+https://github.com/openwallet-foundation-labs/sd-jwt-python.git@danielfett/kb-jwt-hash
Copy link

Choose a reason for hiding this comment

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

Maybe you want to avoid this repository to rely on personal forks?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the branch (not a personal fork) where this feature was developed. The reference to the branch will be removed again as soon as the feature has been merged to main.

@@ -25,7 +25,7 @@ jobs:
- name: "Install SD-JWT tooling"
run: |
python3 -m pip install --upgrade pip
python3 -m pip install git+https://github.com/openwallet-foundation-labs/sd-jwt-python.git
python3 -m pip install git+https://github.com/openwallet-foundation-labs/sd-jwt-python.git@danielfett/kb-jwt-hash
Copy link

Choose a reason for hiding this comment

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

Maybe you want to avoid this repository to rely on personal forks?

(same as above)

Copy link
Member Author

Choose a reason for hiding this comment

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

as above

@danielfett danielfett changed the base branch from danielfett/fix-290-new-structure to master October 23, 2023 15:33
@danielfett danielfett merged commit 2b68756 into master Oct 23, 2023
2 checks passed
@danielfett danielfett deleted the danielfett/hash-in-kb-jwt branch October 27, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants