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

extras: Provide initializers for RSA keys from RSA parameters #247

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

We currently offer APIs to construct RSA keys from PEM and DER representations but we have no way of constructing they key from its constituent RSA parameters: n and e for public keys, and n, e, d, p, and q for private keys. Sometimes these are what you have to hand, e.g. in a JSON Web Key.

Modifications

Provide initializers for RSA keys from RSA parameters for all the RSA key types.

To implement this, we leverage the fact that we are making use of BoringSSL in _CryptoExtras on all platforms. We need this because there are no APIs to construct the underlying key type on Darwin platforms from these parameters. So we do this by first creating a BoringSSL key, serializing it to PEM format, and then constructing a platform-specific key from the PEM representation.

Result

New APIs to construct RSA keys from RSA parameters.

@simonjbeaumont simonjbeaumont marked this pull request as ready for review July 4, 2024 15:10
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jul 5, 2024
Sources/_CryptoExtras/RSA/RSA.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/RSA/RSA.swift Outdated Show resolved Hide resolved
@simonjbeaumont simonjbeaumont force-pushed the sb/rsa-key-init-from-rsa-numbers branch from 945d5dc to cc4f80f Compare July 9, 2024 08:28
Copy link
Contributor Author

@simonjbeaumont simonjbeaumont 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 the feedback @Lukasa. I think this is ready for another pass 🙏

Sources/_CryptoExtras/RSA/RSA.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/RSA/RSA.swift Outdated Show resolved Hide resolved
@simonjbeaumont simonjbeaumont force-pushed the sb/rsa-key-init-from-rsa-numbers branch from cc4f80f to 09a8cc3 Compare July 9, 2024 09:02
Copy link
Contributor

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

Very nice, just a little note.

Sources/_CryptoExtras/RSA/RSA_security.swift Outdated Show resolved Hide resolved
@0xTim
Copy link
Contributor

0xTim commented Jul 15, 2024

cc @ptoffy since we've implemented that ourselves in JWTKit - we should either wait for this or ensure our APIs are internal so we can switch over without an API break

@ptoffy
Copy link
Contributor

ptoffy commented Jul 15, 2024

@0xTim we have our own key wrapping SwiftCrypto's so we should be fine. We can switch to this without breaking anything

@simonjbeaumont simonjbeaumont force-pushed the sb/rsa-key-init-from-rsa-numbers branch from 09a8cc3 to 7690a99 Compare July 15, 2024 12:32
@simonjbeaumont simonjbeaumont requested a review from Lukasa July 15, 2024 12:32
Copy link
Contributor

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

Great, looks good to me, minus the inlinable thing.

///
/// On Darwin platforms, this will serialize it to PEM format, and then construct a platform-specific key from
/// the PEM representation.
public init(n: some ContiguousBytes, e: some ContiguousBytes) throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generalised nit: @inlinable on generic methods, as far as possible. This feedback applies to any new generic function or type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWICT there is a very deliberate use of private for the backing key types and fileprivate for the type aliases which is why existing generic initialisers are not currently inlinable either (e.g. the PEM and DER initialisers).

I think we'd probably need to take a wider view to achieve this so should we defer this to a future PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@simonjbeaumont
Copy link
Contributor Author

@swift-server-bot test this please

@simonjbeaumont simonjbeaumont requested a review from Lukasa July 22, 2024 10:55
@Lukasa Lukasa enabled auto-merge (squash) July 22, 2024 10:59
@Lukasa Lukasa merged commit 8dafe0f into apple:main Jul 22, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants