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

Jump to bottom

x/crypto/curve25519: consider removing dependency on "fmt" if worthwhile #48154

Closed
dmitshur opened this issue Sep 2, 2021 · 9 comments
Closed

x/crypto/curve25519: consider removing dependency on "fmt" if worthwhile #48154

dmitshur opened this issue Sep 2, 2021 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 2, 2021 •
edited

The curve25519 package imports fmt and uses it 3 times:

return nil, fmt.Errorf("bad scalar length: %d, expected %d", l, 32)
return nil, fmt.Errorf("bad point length: %d, expected %d", l, 32)
return nil, fmt.Errorf("bad input point: low order point")

If it makes sense to remove fmt, then the golang.org/x/crypto/curve25519 can be moved in go/build.TestDependencies from:

# CRYPTO-MATH is core bignum-based crypto - no cgo, net; fmt now ok.

To:

# CRYPTO is core crypto algorithms - no cgo, fmt, net.

Similarly to how crypto/ed25519/internal/edwards25519 was moved in CL 276272.

@FiloSottile, do you think doing so would be worthwhile?
@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 2, 2021
@dmitshur dmitshur added this to the Backlog milestone Sep 2, 2021
@FiloSottile
Copy link
Contributor

Yeah, why not, those can be simple strconv.Itoa and errors.New calls. I'll send a CL.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 2, 2021
@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 2, 2021

Sounds good. Please include a small test in x/crypto that this package doesn't import fmt, so this property doesn't accidentally regress without the benefit of go/build's test.

@dmitshur dmitshur modified the milestones: Backlog, Go1.18 Sep 2, 2021
@gopherbot
Copy link

Change https://golang.org/cl/348069 mentions this issue: x/crypto/curve25519: remove dependency on "fmt"

@aaqaishtyaq
Copy link

Hey @FiloSottile, I would like to work on this. I have removed dependency on fmt from curve25519/curve25519.go.
Let me know if I am going right and additional changes I need to make.

Thanks

@ianlancetaylor
Copy link
Contributor

This doesn't seem urgent, moving milestone to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Backlog Jan 29, 2022
gopherbot pushed a commit to golang/crypto that referenced this issue Jun 22, 2022
@aaqaishtyaq @gopherbot
curve25519: remove dependency on fmt
0559593 
For golang/go#48154

Change-Id: If7e99bd1159edc2e3deeb3a4e3d8fb048bc591ab
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/348069
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/413794 mentions this issue: go/build: test no fmt import for crypto/curve25519

@gopherbot
Copy link

Change https://go.dev/cl/414114 mentions this issue: all: update vendored golang.org/x/crypto

@aaqaishtyaq
Copy link

@FiloSottile @ianlancetaylor Can we close the issue, please?
https://go-review.googlesource.com/c/go/+/413794/comment/babcbae2_b09a307d/

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 1, 2022

golang.org/x/crypto/curve25519 no longer imports fmt as of CL 348069, and it's not vendored into standard library at all as of CL 398914, so everything here is done. Thanks @aaqaishtyaq.

@dmitshur dmitshur closed this as completed Oct 1, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Oct 1, 2022
maisem pushed a commit to tailscale/golang-x-crypto that referenced this issue Nov 15, 2022
@aaqaishtyaq @maisem
curve25519: remove dependency on fmt
51fd58e 
For golang/go#48154

Change-Id: If7e99bd1159edc2e3deeb3a4e3d8fb048bc591ab
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/348069
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
maisem pushed a commit to tailscale/golang-x-crypto that referenced this issue Nov 15, 2022
@aaqaishtyaq @maisem
curve25519: remove dependency on fmt
f963101 
For golang/go#48154

Change-Id: If7e99bd1159edc2e3deeb3a4e3d8fb048bc591ab
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/348069
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@aaqaishtyaq @gopherbot
curve25519: remove dependency on fmt
83793fb 
For golang/go#48154

Change-Id: If7e99bd1159edc2e3deeb3a4e3d8fb048bc591ab
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/348069
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@aaqaishtyaq @gopherbot
curve25519: remove dependency on fmt
78b5003 
For golang/go#48154

Change-Id: If7e99bd1159edc2e3deeb3a4e3d8fb048bc591ab
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/348069
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@FiloSottile @dmitshur @ianlancetaylor @gopherbot @aaqaishtyaq