-
Notifications
You must be signed in to change notification settings - Fork 27
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: Use openssl instead of boring #364
crypto: Use openssl instead of boring #364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On another topic,
TestAlertFlushing
was failing on Fedora's fork and they skipped it. I've reenabled and fixed this test in 288c23b.
I can't find any reenabling in that commit, is it more subtle than removing a skip? (I also don't see a skip in that test, though, so I'm not sure what's going on.)
The skip does not exist in this branch because it was added by Fedora in their fork, and we inherited it in our go/src/crypto/tls/handshake_client_test.go Lines 2145 to 2146 in e0ab90c
As |
Edit: The problem has been fixed in 35ebace. It seems that the toolchain requires openssl package to use internal linking even if cgo is used. @dagood I can't reproduce the CI failures locally. It is complaining that some packages used for bootstrapping Go are stale (whatever it means): go tool dist: unexpected stale targets reported by /__w/1/s/pkg/tool/linux_amd64/go_bootstrap list -gcflags="" -ldflags="" for [cmd/asm cmd/cgo cmd/compile cmd/link runtime/internal/sys] (consider rerunning with GOMAXPROCS=1 GODEBUG=gocachehash=1):
STALE cmd/asm: stale dependency: internal/cpu
STALE cmd/cgo: stale dependency: internal/cpu
STALE cmd/compile: stale dependency: internal/cpu
STALE cmd/link: stale dependency: internal/cpu
STALE runtime/internal/sys: build ID mismatch Doing a quick search if found a similar issue in Go's builders: golang/go#33598, which for what I can see affects packages that contain C code. It has been fixed in go1.18, so we may be suffering the same issue. The CI works for windows, so it is probably related to the changes in this PR, which are only applied to linux. Yet the CI also fails on ubuntu, which does not have OpenSSL enabled: https://dev.azure.com/dnceng/internal/_build/results?buildId=1552873&view=logs&j=3ec15cda-97e1-50e8-54b7-d443ee2fe707&t=9abecb8d-598c-5cc1-6611-4513c327c7d5 |
I had to skip |
Co-authored-by: Davis Goodin <[email protected]>
8689911
to
fe31c8f
Compare
} | ||
// We could return here, but the Go standard library test expects DecryptRSANoPadding to verify the result | ||
// in order to defend against errors in the CRT computation. | ||
var n, e, d *C.GO_BIGNUM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this affect our guarantee's around compliance etc.? It's calling into OpenSSl but still doing some stuff that hasn't necessarily been vetted for security.
IAMNA security person, but I know its bad any time you 'roll anything' yourself. Can we guarantee the implementation works correctly with BIGNUM for instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point. I can't be sure that this code protects against all kind of errors in the CRT computation, but it does provide some safety net, at least as much as is required by the Go crypto unit tests.
To the best of my knowledge the FIPS 140-2 standard does not mention this protection, and even OpenSSL does not provide it. So this should not affect our guarantee around FIPS 140-2 compliance.
Important: This PR will be rebased to microsoft/dev.boringcrypto.go1.17 once #363 is merged.
This is THE PR that switch from using boring to our brand new openssl in all FIPS-enabled crypto APIs.
crypto/internal/backend
is 99% API compatible withcrypto/internal/boring
so this PR contains mostly import renames. In order to minimize upstream diffs the backend package is renamed toboring
on every import line.The only thing that is not API compatible is
backend.Enabled
which is defined asconst
insideboring
but openssl defines it asvar
platforms supporting OpenSSL and asconst
for the others. This is because we implemented a feature to enable or disable openssl backend via environment variable and kernel mode, sobackend.Enabled
is not known at compile time.On another topic,
TestAlertFlushing
was failing on Fedora's fork and they skipped it. I've reenabled and fixed this test in 288c23b.I've finally changed the
TestDependency
expectations in 1d81a96 to make sure we are not usingcrypto/internal/boring
anywhere. With this test we make sure that our Go toolchain won't be bloated by boring files (pun intended😄), such asgoboringcrypto_linux_amd64.syso
, as it won't be included in the final binary.