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

x/crypto: migrate packages to the standard library #65269

Open
rolandshoemaker opened this issue Jan 24, 2024 · 13 comments
Open

x/crypto: migrate packages to the standard library #65269

rolandshoemaker opened this issue Jan 24, 2024 · 13 comments
Assignees
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jan 24, 2024

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):

  • twofish
  • cast5
  • tea
  • xtea
  • poly1305
  • ripemd160
  • bn256
  • blowfish
  • openpgp
  • md4
  • pkcs12

cc @FiloSottile @golang/security

@rolandshoemaker rolandshoemaker self-assigned this Jan 24, 2024
@gopherbot gopherbot added this to the Proposal milestone Jan 24, 2024
@magical
Copy link
Contributor

magical commented Jan 25, 2024

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 (x/crypto/ed25519 and x/crypto/curve25519), two should not have been exposed at all (x/crypto/salsa20/salsa and x/crypto/ssh/test), and the last 4 implement outdated or unpopular algorithms.

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:

    x/crypto/acme
    x/crypto/acme/autocert
    x/crypto/argon2
    x/crypto/bcrypt
    x/crypto/blake2b
    x/crypto/blake2s
    x/crypto/chacha20
    x/crypto/chacha20poly1305
    x/crypto/cryptobyte
    x/crypto/cryptobyte/asn1
    x/crypto/hkdf
    x/crypto/nacl/box
    x/crypto/nacl/secretbox
    x/crypto/ocsp
    x/crypto/pbkdf2
    x/crypto/salsa20
    x/crypto/scrypt
    x/crypto/sha3
*   x/crypto/ssh
*   x/crypto/ssh/agent
*   x/crypto/ssh/knownhosts

@magical
Copy link
Contributor

magical commented Jan 25, 2024

We may want to take the opportunity to clean up the API for scrypt and argon2 before moving them: #16971

@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jan 25, 2024
@FiloSottile
Copy link
Contributor

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.

@FiloSottile
Copy link
Contributor

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.

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

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 25, 2024

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).

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.

@FiloSottile
Copy link
Contributor

I am somewhat sad at the idea of losing effective or at least convenient git blame. Since this is a once-in-the-project-lifetime occurrence, maybe we should do a git merge instead of a copy-paste, with a git-filter-repo'd version of the x/crypto history as one of the parents.

The git-filter-repo invocation would be as simple as

git filter-repo --to-subdirectory-filter src/crypto/ --path acme/ --path argon2/ [...]

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 git-filter-repo result to the go repo as a temporary branch, though.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2024

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.
Let's make this proposal just about the idea of moving everything worth keeping into the standard library.
I also agree with @magical that we should look for important API changes as part of the move.
As long as no functionality is deleted outright, the x repos can still wrap the new APIs.
Again, that can happen in the proposal for each individual package.

@rsc
Copy link
Contributor

rsc commented Feb 9, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Feb 14, 2024

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.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

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.

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

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.

@rsc rsc changed the title proposal: x/crypto: migrate packages to the standard library x/crypto: migrate packages to the standard library Mar 8, 2024
@rsc rsc modified the milestones: Proposal, Backlog Mar 8, 2024
@dolmen
Copy link
Contributor

dolmen commented Mar 25, 2024

What is the bar to add (remove?) an algorithm to the crypto namespace? I have not found a mention of a policy in the documentation of the package. Is it available elsewhere?

I'm not in the crypto domain, so I've never heard of x/crypto/nacl/... algorithms (in fact I even initially wondered it was related to Google Native Client, that's why those got my attention), so I was wondering if there was some written criteria for inclusion, like the reference in an IETF/NIST/ISO/ECMA... standard. I see no such references in the godoc of x/crypto/nacl/....

@dolmen
Copy link
Contributor

dolmen commented Mar 25, 2024

I'm also wondering about the crypto namespace being crowded: we have both API/utility packages (rand, cipher, subtle), protocol packages (x509, tls, ssh...) and algorithms (sha1, aes, ecdh...) mixed at the same level. Adding more algorithms there will only make API packages less discoverable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

7 participants