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

fix: externalRepresentation condition to validate if key is private should be d not prime #1060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

beatt83
Copy link

@beatt83 beatt83 commented Dec 6, 2024

Fixes #

I got to this issue recently where the library was giving me a public key DER when the key had a d and no primes, this is misleading since an RSA key can have d and no primes. This function should acess if the key is private through the d, if it requires the primes it should return an error.

As part of this PR I also made the primes public, they are already a let, making them public makes the API more accessible to deal for example if I want to build a JWK with primes included.
It is quite difficult right now if you want to expose this keys to other coding types since the primes are private, and as far as I know there is no reason for that.

Checklist:

  • Correct file headers (see CONTRIBUTING.md).
  • Formatted with SwiftFormat.
  • Tests added.

Changes proposed in this pull request:

  • RSA.externalRepresentation() private of public condition should be made validate through the variable d not primes.
  • Change primes to public.

…hould be d not prime

I got to this issue recently where the library was giving me a public key DER when the key had a d and no primes,
this is misleading since an RSA key can have d and no primes.
This function should acess if the key is private through the d, if it requires the primes it should return an error.

As part of this PR I also made the primes public, they are already a let, making them public makes the API more
accessible to deal for example if I want to build a JWK with primes included.
beatt83 added a commit to beatt83/jose-swift that referenced this pull request Dec 7, 2024
beatt83 added a commit to beatt83/jose-swift that referenced this pull request Dec 7, 2024
beatt83 added a commit to beatt83/jose-swift that referenced this pull request Dec 7, 2024
@krzyzanowskim
Copy link
Owner

@beatt83 why this PR got closed?

@krzyzanowskim
Copy link
Owner

I reopen and let @nathanfallet to weight in about the change.

@krzyzanowskim krzyzanowskim reopened this Dec 9, 2024
@beatt83
Copy link
Author

beatt83 commented Dec 9, 2024

Hey @krzyzanowskim. It was a weird behavior with GitHub, since I tagged this PR in a commit and then merged on my library that PR it closed this one.

Thanks for reopening. :)

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.

2 participants