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

BoringSSLRSAPublicKey use EVP_PKEY API #205

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Oct 19, 2023

Switch BoringSSLRSAPublicKey encrypt/decrypt methods to use EVP_PKEY_* API

Motivation

Increased versatility of the BoringSSLRSAPublicKey encrypt and decrypt methods to make future extensions possible.

Modifications

Switch the BoringSSLRSAPublicKey encrypt and decrypt methods to use the EVP_PKEY_* API directly rather than going through the RSA abstractions.

Result

Increased versatility of the BoringSSLRSAPublicKey encrypt and decrypt methods.

Motivation

Increased versatility of the `BoringSSLRSAPublicKey` encrypt and decrypt
methods to make future extensions possible.

Modifications

Switch the `BoringSSLRSAPublicKey` encrypt and decrypt methods to use
the `EVP_PKEY_*` API directly rather than going through the RSA
abstractions.

Result

Increased versatility of the `BoringSSLRSAPublicKey` encrypt and decrypt
methods.
CInt(dataPtr.count),
dataPtr.baseAddress,

let pkey = CCryptoBoringSSL_EVP_PKEY_new()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should amend the type to only hold this as an EVP_PKEY, instead of holding an RSA pointer at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously you have more context here but I'm not sure that's the best thing to do.

The RSA pointer is currently used in a few different locations outside of the initializers e.g. pkcs1DERRepresentation, derRepresentation, keySizeInBits, isValidSignature. Each of these would then need a call to get the RSA pointer unless we also stored that (which is an option). I looked for alternative functions but there are a few including BIO operations which now have annotations in the docs which look like deprecations (here) which I think make this complex enough to warrant a separate piece of work if we want to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the get0 functions offer a cheap way to access the underlying RSA object we have opted to just keep hold of the EVP_KEY

Sources/_CryptoExtras/RSA/RSA_boring.swift Show resolved Hide resolved
)
return rc

return rc == 0 ? CInt(-1) : CInt(writtenLength)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not use -1 as a sentinel, let's just throw.

@rnro rnro requested a review from Lukasa October 23, 2023 12:12
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.

Nice one, this LGTM.

@Lukasa Lukasa merged commit 7644bdc into apple:main Oct 24, 2023
8 checks passed
@Lukasa Lukasa added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants