-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
crypto/elliptic: deprecate big.Int interface #52182
Comments
Change https://go.dev/cl/382994 mentions this issue: |
Change https://go.dev/cl/390514 mentions this issue: |
Change https://go.dev/cl/315273 mentions this issue: |
Change https://go.dev/cl/396255 mentions this issue: |
Change https://go.dev/cl/390754 mentions this issue: |
Change https://go.dev/cl/395775 mentions this issue: |
Change https://go.dev/cl/382995 mentions this issue: |
Change https://go.dev/cl/396414 mentions this issue: |
Change https://go.dev/cl/396935 mentions this issue: |
Change https://go.dev/cl/390134 mentions this issue: |
Change https://go.dev/cl/360015 mentions this issue: |
Change https://go.dev/cl/398674 mentions this issue: |
Change https://go.dev/cl/398914 mentions this issue: |
@FiloSottile I have collected numbers to compare your nistec changes + Paul's carry improvements against your nistec changes + asm.
It looks like there are several cases where the assembler is better, so I don't think we can abandon the asm implementation this release, especially given that we are running out of time. If we can get the remaining Go optimizations in place so the benchmarks are closer then we can use Go instead of asm at that point maybe in the next release. |
Thank you @laboger for the benchmark! The most important line is the ScalarMult one, because that's the one that I can only marginally improve myself, and that one is within 10% of the assembly which is amazing! I can make ScalarBaseMult dramatically faster, and everything else is downstream of that. I filed #52424 to keep track of that. I am also happy to land the updated assembly first to relieve the time pressure. Are the compiler improvements going to land in Go 1.19? |
@pmur is working on getting his changes into Go 1.19. Currently he is waiting on reviews. |
Change https://go.dev/cl/353849 mentions this issue: |
Change https://go.dev/cl/402554 mentions this issue: |
Change https://go.dev/cl/402555 mentions this issue: |
Change https://go.dev/cl/402556 mentions this issue: |
Not quite golang.org/wiki/TargetSpecific compliant, but almost. The only substantial code change is in randFieldElement: it used to use Params().BitSize instead of Params().N.BitLen(), which is semantically incorrect, even if the two values are the same for all named curves. For #52182 Change-Id: Ibc47450552afe23ea74fcf55d1d799d5d7e5487c Reviewed-on: https://go-review.googlesource.com/c/go/+/315273 Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
There was no way to use an interface because the methods on the Point types return concrete Point values, as they should. A couple somewhat minor annoyances: - Allocations went up due to #48849. This is fine here, where math/big causes allocations anyway, but would probably not be fine in nistec itself. - Carrying the newPoint/newGenerator functions around as a field is a little weird, even if type-safe. It also means we have to make what were functions methods so they can access newPoint to return the zero value. This is #35966. For #52182 Change-Id: I050f3a27f15d3f189818da80da9de0cba0548931 Reviewed-on: https://go-review.googlesource.com/c/go/+/360015 Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Marshal behavior for invalid points is undefined, so don't use it to check if points are valid. For #52182 Change-Id: If167893bc4b029f71bb2528564f2bd96bee7221c Reviewed-on: https://go-review.googlesource.com/c/go/+/382994 Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
This makes Gerrit recognize the rename of the field implementation and facilitates the review. No code changes. For #52182 Change-Id: I827004e175db1ae2fcdf17d0f586ff21503d27e3 Reviewed-on: https://go-review.googlesource.com/c/go/+/390754 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Russ Cox <rsc@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> Auto-Submit: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
Change https://go.dev/cl/399755 mentions this issue: |
Change https://go.dev/cl/360114 mentions this issue: |
Change https://go.dev/cl/404174 mentions this issue: |
For #52182 Change-Id: I4dedd8ed9f57f6fc394c71cd20c3b27c3ea29a95 Reviewed-on: https://go-review.googlesource.com/c/go/+/396414 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
For #52182 Change-Id: I8d8b4c3d8299fbd59b0bf48e5c8b7b41c533a2cc Reviewed-on: https://go-review.googlesource.com/c/go/+/360114 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Fernando Lobato Meeser <felobato@google.com>
The goal of this CL is to move the implementation to the new interface with the least amount of changes possible. A follow-up CL will add documentation and cleanup the assembly API. * SetBytes does the element and point validity checks now, which were previously implemented with big.Int. * p256BaseMult would return (0:0:1) if the scalar was zero, which is not a valid encoding of the point at infinity, but would get flattened into (0,0) by p256PointToAffine. The rest of the code can cope with any encoding with Z = 0, not just (t²:t³:0) with t != 0. * CombinedMult was only avoiding the big.Int and affine conversion overhead, which is now gone when operating entirely on nistec types, so it can be implemented entirely in the crypto/elliptic wrapper, and will automatically benefit all NIST curves. * Scalar multiplication can't operate on arbitrarily sized scalars (it was using big.Int to reduce them), which is fair enough. Changed the nistec point interface to let ScalarMult and ScalarBaseMult reject scalars. The crypto/elliptic wrapper still does the big.Int reduction as needed. The ppc64le/s390x assembly is disabled but retained to make review of the change that will re-enable it easier. Very small performance changes, which we will more then recoup when crypto/ecdsa moves to invoking nistec directly. name old time/op new time/op delta pkg:crypto/elliptic goos:darwin goarch:arm64 ScalarBaseMult/P256-8 11.3µs ± 0% 11.4µs ± 0% +0.87% (p=0.000 n=8+10) ScalarMult/P256-8 42.2µs ± 0% 42.2µs ± 0% ~ (p=0.825 n=10+9) MarshalUnmarshal/P256/Uncompressed-8 801ns ± 1% 334ns ± 0% -58.29% (p=0.000 n=9+10) MarshalUnmarshal/P256/Compressed-8 798ns ± 0% 334ns ± 0% -58.13% (p=0.000 n=10+10) pkg:crypto/ecdsa goos:darwin goarch:arm64 Sign/P256-8 19.3µs ± 1% 19.4µs ± 0% +0.81% (p=0.003 n=8+9) Verify/P256-8 56.6µs ± 0% 56.3µs ± 1% -0.48% (p=0.003 n=7+10) GenerateKey/P256-8 11.9µs ± 0% 12.0µs ± 0% +1.22% (p=0.000 n=7+9) For #52182 Change-Id: I0690a387e20018f38da55141c0d2659280b1a630 Reviewed-on: https://go-review.googlesource.com/c/go/+/395775 Reviewed-by: Fernando Lobato Meeser <felobato@google.com> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org>
For #52182 Change-Id: I8a68fda3e54bdea48b0dfe528fe293d47bdcd145 Reviewed-on: https://go-review.googlesource.com/c/go/+/396255 Reviewed-by: Fernando Lobato Meeser <felobato@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org>
name old time/op new time/op delta pkg:crypto/ecdsa goos:darwin goarch:amd64 Sign/P224-16 250µs ± 2% 91µs ± 2% -63.42% (p=0.000 n=10+9) Sign/P384-16 955µs ± 3% 311µs ± 2% -67.48% (p=0.000 n=10+10) Sign/P521-16 2.74ms ± 2% 0.82ms ± 2% -69.95% (p=0.000 n=10+10) Verify/P224-16 440µs ± 3% 282µs ± 5% -35.94% (p=0.000 n=9+10) Verify/P384-16 1.72ms ± 2% 1.07ms ± 1% -38.02% (p=0.000 n=10+9) Verify/P521-16 5.10ms ± 2% 3.18ms ± 3% -37.70% (p=0.000 n=10+10) GenerateKey/P224-16 225µs ± 3% 67µs ± 4% -70.42% (p=0.000 n=9+10) GenerateKey/P384-16 881µs ± 1% 241µs ± 2% -72.67% (p=0.000 n=10+10) GenerateKey/P521-16 2.62ms ± 3% 0.69ms ± 3% -73.78% (p=0.000 n=10+9) pkg:crypto/elliptic/internal/nistec goos:darwin goarch:amd64 ScalarMult/P224-16 219µs ± 4% 209µs ± 3% -4.57% (p=0.003 n=10+10) ScalarMult/P384-16 838µs ± 2% 823µs ± 1% -1.72% (p=0.004 n=10+9) ScalarMult/P521-16 2.48ms ± 2% 2.45ms ± 2% ~ (p=0.052 n=10+10) ScalarBaseMult/P224-16 214µs ± 4% 54µs ± 4% -74.88% (p=0.000 n=10+10) ScalarBaseMult/P384-16 828µs ± 2% 196µs ± 3% -76.38% (p=0.000 n=10+10) ScalarBaseMult/P521-16 2.50ms ± 3% 0.55ms ± 2% -77.96% (p=0.000 n=10+10) Updates #52424 For #52182 Change-Id: I2be3c2b8cdeead512063ef489e43805f4ee71d0f Reviewed-on: https://go-review.googlesource.com/c/go/+/404174 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Fernando Lobato Meeser <felobato@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
For #52182 Change-Id: If9eace36b757ada6cb5123cc60f1e10d4e8280c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/396935 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Fernando Lobato Meeser <felobato@google.com> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
Fixes #50975 For #52182 Change-Id: I4a98d965436c7034877b8c0146bb0bd5b802d6fa Reviewed-on: https://go-review.googlesource.com/c/go/+/382995 Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
So it's reachable from crypto/ecdsa and the upcoming crypto/ecdh. No code changes. For #52182 Change-Id: Ie3216052f46c6ef7ec64d8b87a233a9c50c4b16a Reviewed-on: https://go-review.googlesource.com/c/go/+/398674 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
…25519 This will allow us to use crypto/internal/edwards25519/field from crypto/ecdh to implement X25519, dropping the dependency on golang.org/x/crypto/curve25519. For #52182 Change-Id: I3be9debc6e13bf06944b98668f34313a975914d0 Reviewed-on: https://go-review.googlesource.com/c/go/+/402556 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
Add support for ppc64le assembler to p256. Most of the changes are due to the change in nistec interfaces. There is a change to p256MovCond based on a reviewer's comment. LXVD2X replaces the use of LXVW4X in one function. In addition, some refactoring has been done to this file to reduce size and improve readability: - Eliminate the use of defines to switch between V and VSX registers. V regs can be used for instructions some that previously required VSX. - Use XXPERMDI instead of VPERM to swap bytes loaded and stored with LXVD2X and STXVD2X instructions. This eliminates the need to load the byte swap string into a vector. - Use VMRGEW and VMRGOW instead of VPERM in the VMULT macros. This also avoids the need to load byte strings to swap the high and low values. These changes reduce the file by about 10% and shows an improvement of about 2% at runtime. For #52182 Change-Id: Ic48050fc81bb273b7b4023e54864f4255dcc2a4f Reviewed-on: https://go-review.googlesource.com/c/go/+/399755 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Filippo Valsorda <valsorda@google.com> Reviewed-by: Paul Murphy <murp@ibm.com>
CC @golang/security What is the current status here? This issue is currently in the 1.19 milestone. Should it move to 1.20? To Backlog? Thanks. |
If a program only uses ecdh.P256(), the implementation of the other curves shouldn't end up in the binary. This mostly required moving some operations from init() time. Small performance hit in uncompressed Bytes/SetBytes, but not big enough to show up in higher-level benchmarks. If it becomes a problem, we can fix it by pregenerating the p-1 bytes representation in generate.go. For #52182 Updates #52221 Change-Id: I64460973b59ee3df787d7e967a6c2bcbc114ba65 Reviewed-on: https://go-review.googlesource.com/c/go/+/402555 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Fernando Lobato Meeser <felobato@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org>
Change https://go.dev/cl/459977 mentions this issue: |
Exposing
big.Int
s in crypto/elliptic was a mistake (#52075, #50974, #37294), and the package is slow and not constant time.I've been running a large remediation project for a few releases to replace the guts of the elliptic curve implementations.
This issue tracks related efforts for Go 1.19. Some related issues:
The text was updated successfully, but these errors were encountered: