search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.63k stars 17.61k forks source link

crypto: consider moving boring to a non-internal location #53497

Closed marten-seemann closed 2 years ago

marten-seemann commented 2 years ago

What version of Go are you using (go version)?

$ go version
1.19beta1

Does this issue reproduce with the latest release?

Yes.

What did you do?

I'm the author of quic-go, a QUIC implementation written in Go. Since crypto/tls doesn't provide the APIs necessary for QUIC, we maintain a fork of crypto/tls, for every Go version (example: https://github.com/marten-seemann/qtls-go1-18). We are very much aware that this is a suboptimal solution, and this has been discussed extensively (https://github.com/lucas-clemente/quic-go/issues/2727), but this is currently the only way.

With Go 1.19, crypto/tls is now using the crypto/internal/boring package. This poses a challenge for forking crypto/tls, since we can't import an internal package. One option would be forking crypto/internal/boring as well, but I'd very much like to avoid that.

Would it be possible to move that package to a non-internal location, either in the standard library or maybe in x/crypto?

What did you expect to see?

crypto/internal/boring cannot be imported. Rightly so, because it's an internal package.

What did you see instead?

The code being accessible, i.e. not in an internal package.

qmuntal commented 2 years ago

I was just going to create an issue for this very same thing, as I already asked for it in the boringcrypto umbrella issue (https://github.com/golang/go/issues/51940#issuecomment-1080733997) without luck.

My use-case is to facilitate forking the crypto package to support other out-of-tree FIPS 140-2 modules, such as go-crypto-openssl. If we moved boringcrypto to x/crypto it would set a precedent and its interface with other crypto packages would be more out-of-tree friendly.

seankhliao commented 2 years ago

cc @golang/security

FiloSottile commented 2 years ago

I don't think any version of FIPS 140 allows QUIC, so really you can just replace the boring package with a stub.

I realize we're causing the issue by not having worked out a set of QUIC-friendly APIs, but exposing some other APIs that applications otherwise don't need just to make forking easier seems like the wrong answer.

@qmuntal That's a separate issue. You want pluggable engines, which turned out to be an architectural nightmare in any cryptography library that adopted them (NSS, OpenSSL 3.0). I specifically don't want the BoringCrypto hooks to set precedent for pluggable engines. The patch was merged for release engineering reasons, not as a signal of progressing status. Red Hat had a look at how much code it would be to support OpenSSL too, I was hoping it would be a couple hundred lines of linker code, instead it was a couple thousand lines adding a wholly separate backend that would have its own object lifetime and behavior alignment issues.

qmuntal commented 2 years ago

@qmuntal That's a separate issue. You want pluggable engines

@FiloSottile If you refer to the #33281, I'm not advocating for it. I agree with you that having runtime swappable crypto engines brings exponential complexity and hurts flexibility.

I did not +1 thinking it sets precedence for pluggable cryptos, there are no hidden agendas here. We (Microsoft) are well staffed to keep our own compile-time-swappable OpenSSL and Windows CNG engines for Go crypto in a separate Go fork.

We have chosen the path of keeping our engines in separate repos (i.e. go-crypto-openssl) and then vendoring them into the main tree. So, if @marten-seemann request is implemented, it would make our approach, which is also followed by RedHat (openssl-fips) easier to maintain and reduce the likelihood of conflicts. I do think this is a fair ask to the Go team, but agree that it deserve its own issue.

marten-seemann commented 2 years ago

@FiloSottile Thank you for the explanation. I went ahead and removed all imports of the boring package in https://github.com/marten-seemann/qtls-go1-19/commit/10a6fc69b670e95e1927bdc28888e470421777d0, so I'd consider this issue as resolved. Unless you're planning to extend usage of that package in future releases ;)

Feel free to close this issue.

FiloSottile commented 2 years ago

@qmuntal My apologies, I misinterpreted "If we moved boringcrypto to x/crypto it would set a precedent". I am still skeptical that the BoringCrypto hooks can generalize properly to "the hooks where you can inject any crypto backend at compile time" without major architectural overhead. It's also not very clear to me what good making the package public (and so subject to a compatibility promise) would do compared to being a separate internal package like it already is: the hooks would still need to be patched at compile time, even if you achieve perfect API compatibility with your backends, since you can't replace a stdlib dependency like you would a go.mod one (yet? This came up a few times but has not made much progress). Anyway, this can probably be discussed in a separate issue, and it would help to see concrete examples of 1) the changes it would require and 2) the concrete benefits.

@marten-seemann I don't think the usage of that package will ever extend to non-BoringCrypto modes. You might want to leave the hooks in and replace imports with a example.com/internal/stub/boring package where Enabled is a const false to minimize merge conflicts in the future, by the way. Closing as requested.