-
Notifications
You must be signed in to change notification settings - Fork 161
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: Add APIs for RSA Blind Signatures #232
extras: Add APIs for RSA Blind Signatures #232
Conversation
) == 1 else { | ||
throw CryptoKitError.internalBoringSSLError() | ||
} | ||
precondition(outputCount == signatureByteCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this on enough key sizes that it's always true? (eg. if first bytes are 0, they are not getting trimmed)
Just want to make sure we don't crash if we have an oddly formatted private key generated some day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Let me double check on this, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked again at this and we compute signatureByteCount
using BoringSSL RSA_size
which returns the size of the modulus in bytes (as opposed to the key size). I also confirmed that BoringSSL RSA_sign_raw
allocates a buffer of that size and sets out_len
to the same.
dbe6e60
to
60f26f0
Compare
606f74d
to
7c37d7a
Compare
@Lukasa this is ready for another pass when you have time. |
4c25c87
to
ec0d4da
Compare
@Lukasa OK, thanks for the latest round of feedback. I've addressed all that now, so it's ready for another pass. |
@Lukasa as we discussed, now that we have |
@swift-server-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking really lovely, just a few quick notes here.
} | ||
|
||
@inlinable | ||
public static func random(inclusiveMin: UInt64, exclusiveMax: ArbitraryPrecisionInteger) throws -> ArbitraryPrecisionInteger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we'd spell this with a range, rather than the explicit bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is an interesting one! I initially reached for a Range
but a range needs to have a homogeneous type and the underlying call we're wrapping does not take two BIGNUM*
, but instead one BN_ULONG
and one BIGNUM*
.
This left me with two options:
- Write it in terms of a
Range<ArbitraryPrecisionInteger>
and fail if the lower bound is too large. - Write it like this.
When possible I thought we should be providing API that prohibits invalid use if we can leverage the type system to do so.
guard let inv = try finiteField.inverse(r) else { continue } | ||
return (r, inv) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably just as nice to pre-declare r
and inv
before the loop as var
and then bind them repeatedly throughout the loop than to have this anonymous self-invoking closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've updated it to use var
, although I had this construction to ensure a let
binding because during the last round of review, in addition to dealing with pointers, one of the comments was keeping track of which state had been computed and which was mutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa addressed most of the feedback. The outstanding items that need further discussion are:
- https://github.com/apple/swift-crypto/pull/232/files#r1651172719, where I'm not sure we should provide the computed properties.
- https://github.com/apple/swift-crypto/pull/232/files#r1651153611, where a range isn't expressible without some sort of fallible API surface.
guard let inv = try finiteField.inverse(r) else { continue } | ||
return (r, inv) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've updated it to use var
, although I had this construction to ensure a let
binding because during the last round of review, in addition to dealing with pointers, one of the comments was keeping track of which state had been computed and which was mutable.
} | ||
|
||
@inlinable | ||
public static func random(inclusiveMin: UInt64, exclusiveMax: ArbitraryPrecisionInteger) throws -> ArbitraryPrecisionInteger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is an interesting one! I initially reached for a Range
but a range needs to have a homogeneous type and the underlying call we're wrapping does not take two BIGNUM*
, but instead one BN_ULONG
and one BIGNUM*
.
This left me with two options:
- Write it in terms of a
Range<ArbitraryPrecisionInteger>
and fail if the lower bound is too large. - Write it like this.
When possible I thought we should be providing API that prohibits invalid use if we can leverage the type system to do so.
|
||
guard result.withUnsafeMutableBignumPointer({ resultPtr in | ||
exclusiveMax.withUnsafeBignumPointer { exclusiveMaxPtr in | ||
CCryptoBoringSSL_BN_rand_range_ex(resultPtr, inclusiveMin, exclusiveMaxPtr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't compile on Android armv7, a 32-bit platform, with the following error from my Android CI:
Sources/CryptoBoringWrapper/ArbitraryPrecisionInteger.swift:444:62: error: cannot convert value of type 'UInt64' to expected argument type 'BN_ULONG' (aka 'UInt32')
CCryptoBoringSSL_BN_rand_range_ex(resultPtr, inclusiveMin, exclusiveMaxPtr)
^
BN_ULONG( )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@finagolfin sorry about that oversight. I'm sure the fix should be pretty minimal but unfortunately I don't have a way of validating it and it's not part of our CI. Presumably the fix here is to take a UInt
instead of a UInt64
.
Are you able to test this and open a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I can reproduce this locally when building for watchOS simulator. Let me put up a patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation
RFC 9474 defines the RSA Blind Signatures protocol1, which are a useful building block of privacy-preserving schemes.
Modifications
This PR adds the following public API surface to implement the RSA Blind Signatures protocol:
It also adds tests using the test vectors from the RFC, where possible. That is, for each of the operations except for
blind
, which we don't have the BoringSSL APIs that would allow us to inject the fixed salt value from the test vectors.While it's possible to implement the server-side operations using Security framework, it does not expose the APIs we would need to implement the client-side operations so, because the goal is to also provide the client-side operations in a subsequent PR, the implementation uses BoringSSL on all platforms.
In order to construct the RSA keys from the test vector parameters, some BoringSSL helpers were added.
Result
New API in the
_CryptoExtras
module for the the RSA Blind Signatures protocol as defined in RFC 9474, with support for all named variants: for theRSABSSA_SHA384_PSS_Randomized
,RSABSSA_SHA384_PSS_Deterministic
,RSABSSA_SHA384_PSSZERO_Randomized
, andRSABSSA_SHA384_PSSZERO_Deterministic
.Footnotes
https://datatracker.ietf.org/doc/rfc9474/ ↩