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

feat: fixing exclude _sd_alg when no disclosure #120

Merged

Conversation

lukasjhan
Copy link
Member

I always set _sd_alg in payload even if there is no disclosure. But I think it's better not to include when there is nothing.
I guess it's more complaint.

When used, this claim MUST appear at the top level of the SD-JWT payload.

The payload MAY contain the _sd_alg key described in Section 5.1.1.

Signed-off-by: Lukas <Lukas@hopae.io>
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (d503996) to head (43f4b0f).

Additional details and impacted files
@@           Coverage Diff           @@
##             next     #120   +/-   ##
=======================================
  Coverage   95.54%   95.54%           
=======================================
  Files          21       21           
  Lines        1705     1705           
  Branches      246      247    +1     
=======================================
  Hits         1629     1629           
  Misses         72       72           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cre8
Copy link
Contributor

cre8 commented Feb 28, 2024

I reached out to the authors since the paragraph is confusing.

"If the _sd_alg claim is not present at the top level, a default value of sha-256 MUST be used."

I understand that the _sd_alg needs only be included in case I want to use a other algorithm than _sd_alg

@lukasjhan
Copy link
Member Author

I reached out to the authors since the paragraph is confusing.

"If the _sd_alg claim is not present at the top level, a default value of sha-256 MUST be used."

I understand that the _sd_alg needs only be included in case I want to use a other algorithm than _sd_alg

FYI: @berendsliedrecht 's implementation doesn't have _sd_alg when there is no disclosure.

hmm... I thought that is for backward compatibility. I think excluding _sd_alg when using sha-256, It can be good. It's more compact

@cre8
Copy link
Contributor

cre8 commented Feb 28, 2024

I reached out to the authors since the paragraph is confusing.
"If the _sd_alg claim is not present at the top level, a default value of sha-256 MUST be used."
I understand that the _sd_alg needs only be included in case I want to use a other algorithm than _sd_alg

FYI: @berendsliedrecht 's implementation doesn't have _sd_alg when there is no disclosure.

hmm... I thought that is for backward compatibility. I think excluding _sd_alg when using sha-256, It can be good. It's more compact

Your change is not violating the spec. And the small amount of chars in the JWT will have no negative effect. So from my point we can merge this and move on :)

@lukasjhan lukasjhan merged commit c1b701d into openwallet-foundation-labs:next Feb 28, 2024
10 checks passed
@berendsliedrecht
Copy link
Contributor

I did not include it, because when I started to implement on a lower draft version it basically stated that when no selectively disclosable items are included, it should be treated as a normal JWT. However, I think the point more is for verification as @cre8 mentioned, if it is not included assume sha2-256, but it does not always have to be set by default to sha2-256. If that makes sense.

cre8 pushed a commit to cre8/sd-jwt-js that referenced this pull request Mar 8, 2024
…n-labs#120)

Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
cre8 pushed a commit that referenced this pull request Mar 8, 2024
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
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.

None yet

3 participants