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: Add EC toolbox abstractions and OPRF(P-384, SHA-384) VOPRF API #292

Merged
merged 13 commits into from
Nov 22, 2024

Conversation

simonjbeaumont
Copy link
Contributor

@simonjbeaumont simonjbeaumont commented Nov 6, 2024

Motivation

We would like to provide support for the P384-SHA384 Verifiable Oblivious Pseudorandom Function (VOPRF) as defined in RFC 9497: VOPRF Protocol.

Modifications

  • Add protocols for some "ECToolbox" abstractions: Group, GroupElement, GroupScalar, HashToGroup.
  • Add implementations backed by BoringSSL.
  • Duplicate some utilities from Crypto into _CryptoExtras: hexadecimal bytes and I2OSP.
  • Add VOPRF API, rooted at enum P384._VOPRF.

Results

  • New internal abstractions that make it easier to bring up new implementations.
  • New API for OPRF(P-384, SHA-384) in VOPRF mode.

Tests

  • Test vectors for internal HashToField implementation.
  • Test vectors for internal VOPRF implementation.
  • Tests of the VOPRF public API.

@simonjbeaumont simonjbeaumont marked this pull request as draft November 6, 2024 11:01
@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 8, 2024 13:37
@simonjbeaumont
Copy link
Contributor Author

@Lukasa PTAL when you have time 🙏

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Nov 20, 2024
@@ -72,6 +72,14 @@ extension BoringSSLEllipticCurveGroup {
return try! ArbitraryPrecisionInteger(copying: baseOrder)
}

@usableFromInline
package var generator: EllipticCurvePoint { get throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth not squishing this in, get throws { can be on its own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I moved this onto its own line.

Aside: I have a task to add the swift-format job to this repo soon, which will squash all these inconsistencies away.

try group.withUnsafeGroupPointer { groupPtr in
guard
let pointPtr = CCryptoBoringSSL_EC_POINT_new(groupPtr),
CCryptoBoringSSL_EC_POINT_mul(groupPtr, pointPtr, scalarPtr, nil, nil, nil) == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a new issue, but this leaks the pointPtr if the mul fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this by factoring out the EC_POINT_new into a designated init(_pointAtInfinityOn group:) and made all these convenience initialisers.

try domainSeparationTag.withUnsafeBytes { dstPtr in
guard
let pointPtr = CCryptoBoringSSL_EC_POINT_new(groupPtr),
hashToCurveFunction(
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails, we leak the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch CCryptoBoringSSL_EC_POINT_cmp(groupPtr, selfPtr, rhsPtr, nil) {
case 0: return true
case 1: return false
default: fatalError("Unexpected error testing EC point equality")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that we need to be this strict. 0 is true, everything else is false. Happy to have an assertion for the other codes, but probably shouldn't fatal error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's not quite right since the docs say that EC_POINT_cmp can return an error:

EC_POINT_cmp returns 1 if the points are not equal, 0 if they are, or -1 on error.

I think this can happen if you provide a point on a different group for an equality test, for instance.

This function isn't throwing so the best we can do is fatal error, which seems to be in line with what we do elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need the error to be this loud, though. Perhaps we drop this to an assertion and then return false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further consideration, given we allow folks to pass in the group, we shouldn't even assert, but just return false if comparing two points from different groups.

I've updated the code to clear the BoringSSL error and return false in this scenario.

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've addressed your comments. Could you take another look when you get a minute.

switch CCryptoBoringSSL_EC_POINT_cmp(groupPtr, selfPtr, rhsPtr, nil) {
case 0: return true
case 1: return false
default: fatalError("Unexpected error testing EC point equality")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's not quite right since the docs say that EC_POINT_cmp can return an error:

EC_POINT_cmp returns 1 if the points are not equal, 0 if they are, or -1 on error.

I think this can happen if you provide a point on a different group for an equality test, for instance.

This function isn't throwing so the best we can do is fatal error, which seems to be in line with what we do elsewhere?

@@ -72,6 +72,14 @@ extension BoringSSLEllipticCurveGroup {
return try! ArbitraryPrecisionInteger(copying: baseOrder)
}

@usableFromInline
package var generator: EllipticCurvePoint { get throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I moved this onto its own line.

Aside: I have a task to add the swift-format job to this repo soon, which will squash all these inconsistencies away.

try group.withUnsafeGroupPointer { groupPtr in
guard
let pointPtr = CCryptoBoringSSL_EC_POINT_new(groupPtr),
CCryptoBoringSSL_EC_POINT_mul(groupPtr, pointPtr, scalarPtr, nil, nil, nil) == 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this by factoring out the EC_POINT_new into a designated init(_pointAtInfinityOn group:) and made all these convenience initialisers.

try domainSeparationTag.withUnsafeBytes { dstPtr in
guard
let pointPtr = CCryptoBoringSSL_EC_POINT_new(groupPtr),
hashToCurveFunction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Nice one, thanks @simonjbeaumont!

@simonjbeaumont
Copy link
Contributor Author

Thanks @Lukasa :) please merge when convenient.

@Lukasa Lukasa enabled auto-merge (squash) November 22, 2024 09:26
@Lukasa Lukasa merged commit ff0f781 into apple:main Nov 22, 2024
31 of 34 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.

2 participants