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

type: remove rsa type in jwk #136

Merged

Conversation

lukasjhan
Copy link
Member

I accidentally forgot to remove the rsa type.

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

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.41%. Comparing base (38ffb3c) to head (3a4207f).
Report is 2 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #136      +/-   ##
==========================================
- Coverage   97.41%   97.41%   -0.01%     
==========================================
  Files          23       23              
  Lines        1899     1898       -1     
  Branches      273      273              
==========================================
- Hits         1850     1849       -1     
  Misses         49       49              

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

@lukasjhan lukasjhan merged commit 7b8a217 into openwallet-foundation-labs:next Mar 6, 2024
10 checks passed
@berendsliedrecht
Copy link
Contributor

What was the reason for removing rsa here?

@lukasjhan
Copy link
Member Author

@berendsliedrecht We discussed this issue in #130. I thought @cre8 removed and approved it but It was not actually removed, So I opened this PR

@lukasjhan
Copy link
Member Author

In case we add the rsa type, we just need to add:

interface RsaOtherPrimesInfo {
  d?: string;
  r?: string;
  t?: string;
}

@berendsliedrecht
Copy link
Contributor

Ahh okay, yeah that makes sense! If we fully want to delete it would should also get rid of the n and e fields. I don't think they are used by any other algo, but they are the main components for RSA keys.

@lukasjhan
Copy link
Member Author

Ahh okay, yeah that makes sense! If we fully want to delete it would should also get rid of the n and e fields. I don't think they are used by any other algo, but they are the main components for RSA keys.

Oh I see, I'll prepare PR for that :)

@cre8
Copy link
Contributor

cre8 commented Mar 6, 2024

I think we could use the definition from the dom.lib.ts and just copy it. Would this solve all the edge cases?

https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts#L689

@lukasjhan
Copy link
Member Author

I think we could use the definition from the dom.lib.ts and just copy it. Would this solve all the edge cases?

@cre8 Yeah I used that one, but we decided to remove the rsa-related part in #130. If it is okay to add it again, should we change it to add it?

@cre8
Copy link
Contributor

cre8 commented Mar 6, 2024

I would add the fields again. Just because some organisation say RSA usage is not safe anymore, it is still used out there ;)

With my comments on 130 I intended that we have just the Jsonwebkey interface, but inside it all attributes to cover different keys like in the lib.dom.ts

@lukasjhan
Copy link
Member Author

@cre8 @berendsliedrecht I opened a PR for restoring rsa field in JWK. #137

cre8 pushed a commit to cre8/sd-jwt-js 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>
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

4 participants