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

Move ArbitraryPrecisionInteger type to CryptoBoringWrapper module #236

Conversation

simonjbeaumont
Copy link
Contributor

@simonjbeaumont simonjbeaumont commented Jun 3, 2024

Motivation

Before this PR, the Crypto module had an internal ArbitraryPrecisionInteger type that manages the lifetime of a BoringSSL BIGNUM and exposes several BIGNUM operations. We'd like to make use of this from the _CryptoExtras module and also for code that will run on Darwin platforms.

Modifications

  • Move ArbitraryPrecisionInteger to the CryptoBoringWrapper module.
  • Remove compilation guards so it's available on all platforms.
  • Mark most operations public.
  • Provide RawPointer and MutableRawPointer APIs (and hide typed pointer APIs).
  • Provide shim in Crypto for typed BIGNUM pointer APIs.
  • Move ArbitraryPrecisionIntegerTests to (new) CryptoWrapperTests test target.

There are a number of other ways this could have been achieved but none of them seemed great given the requirements, specifically:

  • We require that any import of the CCryptoBoringSSL module is imported using @_implementationOnly.
  • We don't want to expose ArbitraryPrecisionInteger (or, of course, BIGNUM) in the public API of Crypto.
  • We would like to not remove the existing @inlineable and @usableFromInline annotations on existing APIs that make use of ArbitraryPrecisionInteger, which itself was @usableFromInline before this PR.

Result

ArbitraryPrecisionInteger is now in a shared place that can be used from the _CryptoExtras module.

@simonjbeaumont simonjbeaumont force-pushed the sb/move-arbitrary-precicision-integer-to-wrapper branch from 2407071 to 6cfa683 Compare June 3, 2024 15:48
@simonjbeaumont
Copy link
Contributor Author

  • We require that any import of the CCryptoBoringSSL module is imported using @_implementationOnly.

@Lukasa based on our discussion, we believe that this is not a requirement and that we can actually drop the @_implementationOnly on our imports, which we can front-run in its own, targeted PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI I think we can drop the proposed raw pointer APIs on ArbitraryPrecisionInteger and these shims back to typed pointers if we're willing to drop @_implementationOnly import in this project.

@simonjbeaumont
Copy link
Contributor Author

@swift-server-bot test this please

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Jun 18, 2024
@simonjbeaumont simonjbeaumont marked this pull request as ready for review June 18, 2024 13:57
@simonjbeaumont simonjbeaumont force-pushed the sb/move-arbitrary-precicision-integer-to-wrapper branch from 3708319 to 50e358f Compare June 19, 2024 06:57
@Lukasa
Copy link
Contributor

Lukasa commented Jun 19, 2024

Thanks for this Si, much appreciated.

@Lukasa Lukasa merged commit a3866d4 into apple:main Jun 21, 2024
9 checks passed
simonjbeaumont added a commit to simonjbeaumont/swift-crypto that referenced this pull request Jul 2, 2024
simonjbeaumont added a commit to simonjbeaumont/swift-crypto that referenced this pull request Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants