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

Update lower bound for RSA key sizes #218

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Update lower bound for RSA key sizes #218

merged 3 commits into from
Mar 25, 2024

Conversation

ptoffy
Copy link
Contributor

@ptoffy ptoffy commented Feb 14, 2024

Update lower bound for RSA key sizes to 2048 rather than 1024

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary (didn't find any documentation to update about this)

Motivation:

Since 2015, NIST recommends a minimum key size of 2048 bits as stated in https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r5.pdf
This is also stated in the JWA specification (RFC7518 3.3 and 3.5) and in the RSA SSH spec, that's why this PR enforces the use of key sizes >=2048 rather than >=1024 as before.

Modifications:

Update lower bound for RSA key sizes to 2048 rather than 1024, specifically in the RSA key initialisers. Also update tests which use 1024 bits-sized keys and make sure they throw

Result:

Creating RSA keys of sizes <2048 will throw

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for this change! However, as written I don’t think we should take it.

RSA support is not provided for new uses, but for compatibility with old ones. To that compatibility end, some support for weaker key sizes is likely to be necessary.

However, I do think we could make the case for this change if we introduced an explicitly marked unsafe or unchecked initializer that reinstated the current bounds. Are you open to that?

@ptoffy
Copy link
Contributor Author

ptoffy commented Feb 15, 2024

@Lukasa that seems sensible. I've updated the PR with those changes, let me know what you think!

@ptoffy ptoffy requested a review from Lukasa February 15, 2024 09:45
@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Mar 25, 2024
@Lukasa
Copy link
Collaborator

Lukasa commented Mar 25, 2024

@swift-server-bot test this please

@Lukasa Lukasa merged commit 89876ab into apple:main Mar 25, 2024
9 checks passed
@ptoffy ptoffy deleted the rsa-keysize branch March 25, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants