-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
x/crypto: migrate packages to the standard library #65269
Comments
For anyone who, like me, is wondering exactly which packages would be moved the full list is in #65250 (copied at the end of this post, for completeness). Whether or not #65250 is not accepted, i do not think that any of the packages it proposes to deprecate/freeze should be included to the standard library. Two are wrappers for packages that were already moved to the standard library ( Considering that x/crypto/ssh seems to be under active development, maybe it would be worth postponing its move for a couple releases? No need to make a new module - if the rest of x/crypto is deprecated then x/crypto/ssh would de-facto be its own module. And although a hypothetical "mechanism for updating individual sections of the standard library" would be nice, i don't think one is likely to materialize anytime soon. The remaining packages generally seem like worthwhile additions to the standard library:
|
We may want to take the opportunity to clean up the API for scrypt and argon2 before moving them: #16971 |
We are very tempted to take the opportunity to rework half the APIs, but really it should be orthogonal to this move. First we move the packages maintaining perfect compatibility, so that imports can just be rewritten and it can be done in one fell swoop, then we think about any v2 that's worth doing. |
A lot of the standard library is under active development. The release cycle is not perfect, but it's also not hostile to x/crypto/ssh in particular. There are actually good sides to bringing x/crypto/ssh into the stdlib: it would allow us to make use of the godebug mechanism to start doing security deprecations, which we have so far been resistant to doing in x/crypto because we don't have the equivalent escape hatches and the baking time of release candidates. /cc @drakkan |
I think it's unavoidable that we will have to do the occasional security or bugfix backport (with a similar bar to what we would backport to a minor release) until the compatibility wrappers are removed, so IMHO there is no need to freeze the standard library copy in the first cycle. When we do the move we freeze the x/crypto side, and it enters the state in which it will stay for the next two releases, before the compatibility implementations are removed. |
I am somewhat sad at the idea of losing effective or at least convenient The
and we could use Gerrit to review the merge commit, no need to push to master. It would need someone from @golang/release to take elevated privileges push the |
I agree with this in principle but there should be review of individual packages rather than a blank check to usher significant amounts of never-reviewed API into the standard library. |
This proposal has been added to the active column of the proposals project |
Have all remaining concerns about this proposal been addressed? This proposal is about the idea of moving everything worth keeping from x/crypto into the standard library. There would be a followup proposal for each individual package to decide what form comes in and whether it’s worth keeping. |
Based on the discussion above, this proposal seems like a likely accept. This proposal is about the idea of moving everything worth keeping from x/crypto into the standard library. There would be a followup proposal for each individual package to decide what form comes in and whether it’s worth keeping. |
No change in consensus, so accepted. 🎉 This proposal is about the idea of moving everything worth keeping from x/crypto into the standard library. There would be a followup proposal for each individual package to decide what form comes in and whether it’s worth keeping. |
What is the bar to add (remove?) an algorithm to the I'm not in the crypto domain, so I've never heard of |
Change https://go.dev/cl/602855 mentions this issue: |
For golang/go#68723 Updates golang/go#65269 Change-Id: Ied7a96ab990abe257c4b8c295e292f92a745a4a7 GitHub-Last-Rev: 33f25dd GitHub-Pull-Request: #51 Reviewed-on: https://go-review.googlesource.com/c/proposal/+/602855 Commit-Queue: Nicola Murino <nicola.murino@gmail.com> Reviewed-by: Nicola Murino <nicola.murino@gmail.com> Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Change https://go.dev/cl/616717 mentions this issue: |
For now just internally, pending a dedicated proposal for the exposed package API. In this CL the code is copied verbatim, for ease of review. Only the imports were replaced with the corresponding internal ones, and crypto.RegisterHash calls were disabled. DO NOT SUBMIT until CL 616635 is submitted, and this CL is synced, then specify here what commit was imported. Updates golang#65269 For golang#69536 Change-Id: Ia4735b50c99b9573a5c4889733c4a119930fe658
Change https://go.dev/cl/619477 mentions this issue: |
For now just internally, pending a dedicated proposal for the exposed package API. In this CL the code is copied verbatim, for ease of review. Only the imports were replaced with the corresponding internal ones, and crypto.RegisterHash calls were disabled. DO NOT SUBMIT until CL 616635 is submitted, and this CL is synced, then specify here what commit was imported. Updates golang#65269 For golang#69536 Change-Id: Ia4735b50c99b9573a5c4889733c4a119930fe658
For now the package contents are added internally pending a dedicated proposal for an exposed API. The code is copied verbatim for ease of review with a few small exceptions: * The hash.Hash interface usage was replaced with fips.Hash. * Import paths were replaced with corresponding internal FIPS package paths, and the hmac.New() arguments updated accordingly. * The pbkdf2_test was made external to pbkdf2 so it will be easier to move to the public package in the future. Updates golang#65269 For golang#69536 Change-Id: Ief8f42a88bad165010e7844186f306ea0db34c2e
For now just internally, pending a dedicated proposal for the exposed package API. In this CL the code is copied verbatim, for ease of review. Only the imports were replaced with the corresponding internal ones, and crypto.RegisterHash calls were disabled. Also, the 0.5MB keccakkats file was dropped, supplanted by TestCSHAKEAccumulated and ACVP tests. Updates #65269 Updates #69982 For #69536 Change-Id: Ia4735b50c99b9573a5c4889733c4a119930fe658 Reviewed-on: https://go-review.googlesource.com/c/go/+/616717 Reviewed-by: Daniel McCarney <daniel@binaryparadox.net> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Roland Shoemaker <roland@golang.org> Auto-Submit: Filippo Valsorda <filippo@golang.org> Reviewed-by: Carlos Amedee <carlos@golang.org>
Change https://go.dev/cl/629176 mentions this issue: |
Implement the SHA-3 hash algorithms and the SHAKE extendable output functions defined in FIPS 202. This is a wrapper for crypto/internal/fips/sha3 which in turn was ported from x/crypto/sha3 in CL 616717 as part of #65269. Fixes #69982 Change-Id: I64ce7f362c1a773f7f5b05f7e0acb4110e52a329 Reviewed-on: https://go-review.googlesource.com/c/go/+/629176 Reviewed-by: Russ Cox <rsc@golang.org> Auto-Submit: Filippo Valsorda <filippo@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
The Go cryptographic libraries are currently split between the standard library and the golang.org/x/crypto module. This document discusses the issues with this split and proposes a plan to move the majority of the golang.org/x/crypto module into the standard library.
Current State
Currently, the Go cryptographic libraries are split across the standard library and the golang.org/x/crypto module. Why something is in x/crypto, versus the standard library, is often hard to explain (in many cases it is simply an artifact of how the golang.org/x tree was used historically), and seems consistently confusing to users (a persistent misconception is that the x/ tree is for “experimental” code), often dissuading them from relying on code in the module because of assumptions about quality or API stability.
The boundary between the standard library and x/crypto is further blurred as the standard library currently relies on a number of x/crypto packages (currently 7), which need to be vendored. This vendoring relationship additionally creates a complex security patching problem, requiring a special third process for how we patch these particular packages (separate from how we patch just the standard library, or just x/crypto).
The x/crypto module is, in theory, useful as a place to develop code that, while still abiding by the API backwards compatibility rules, is somewhat more experimental (despite x/ not meaning that) in that it may require a more fast paced development cycle than the standard library can reasonably provide (for example when developing implementations of new ciphers or protocols for which the specifications are still fluid).
In reality though, the x/crypto module is not generally used this way, and if we were to start doing this it would only encode the misconceptions people already have about the module. Building experimental or semi-unstable code in the module would also lead to the question of what to do with code once it stabilizes. If we decide to move stable code into the standard library we would end up with a series of wrapper packages in x/crypto which only serve to forward “completed” packages to their eventual standard library counterparts.
One exception to this is the x/crypto/ssh package, which has recently seen very rapid development. For many users, a lot of the newly introduced features and bug fixes are things they would like to start using immediately, and as such the release cadence of the standard library is likely too slow. Moving this package into the standard library could therefore create friction for these users. That said it isn’t clear that this is a strong enough reason not to migrate this package. We could split x/crypto/ssh into its own module, but it’s quite likely that the development cadence of this package will decrease significantly over time, at which point we’d likely want to revisit this decision and consider integrating it into the standard library. This seems like one more case in which it would be useful for the standard library to provide some mechanism for updating individual sections of the standard library out of step with the larger release cadence.
Proposal
The code in x/crypto has no discernable difference in quality or API stability than any code in the standard library (except for a small handful of notable exceptions, which will be addressed later). The vast majority of packages in x/crypto would be perfectly at home in the standard library crypto/ tree, and as such, they really should be in the standard library.
Moving the x/crypto packages into the standard library would communicate to users that we are confident in their quality and stability, and encourage their widespread adoption and usage, dispelling the commonly held misconceptions about the contents of x/crypto, and reducing the complexity that supporting this special additional module creates.
Migrating and Freezing x/crypto
The vast majority of packages in x/crypto should be moved, with little to no modification, into the crypto/ tree. This should be done during a single standard library release cycle (note: this should be done as close to the end of the release cycle as possible, to avoid having to keep the two copies of packages in sync. Ideally once we copy the packages into the standard library we should freeze both the x/crypto and standard library copy, and only unfreeze once the standard library tree re-opens, only accepting changes to the standard library version. The exact process for this is still up for debate).
Once the packages have been migrated to the standard library, the module should be split using build tags, leaving the current implementation as pre_go1{version} (i.e.
+build !go1.24
) and adding wrappers which forward to the standard library version as post_go1{version} (i.e.+build go1.24
), and tagged as v1.0.0. This will allow users who don’t update to the newest version of Go to continue to use the x/crypto module, and allow us to backport security changes to x/crypto until at least two major versions into the future. We will not backport any non-security changes to x/crypto.Two major versions after the transition (i.e., assuming we land this in 1.24, in 1.26), we will remove the build tagged prior implementation, leaving just the forwarders, tagging the (hopefully) final version of x/crypto.
Remaining Packages
There are a handful of packages which we do not want to move into the standard library, either because they require an update/release cadence that does not align with the standard library, or because they are frozen/deprecated, and this provides an opportunity to make a clean break.
In particular the release cadence for the x/crypto/x509roots package does not align with that of the standard library. This package needs to be updated on an arbitrary basis, and changes should be pulled in by users as quickly as possible, which is currently not possible for packages within the standard library. This package should be moved into its own standalone module, golang.org/x/x509roots (an argument could be made for leaving it in x/crypto but this goes against one of the main arguments for this move in the first place, which is that the x/crypto module is confusing, and leaving only one thing there which continues to be maintained is likely to only make that distinction even more confusing).
The following already deprecated/frozen packages will not be moved into the standard library, and the implementations will remain as-is in the v1 tagged golang.org/x/crypto module (#65250 proposes to deprecate/freeze a number of additional packages, if that proposal is accepted I will update this list to mirror that):
cc @FiloSottile @golang/security
The text was updated successfully, but these errors were encountered: