Implement crypto backend for openssl #363
Conversation
qmuntal
requested review from
microsoft-golang-review-bot and
a team
as code owners
| ) | ||
|
|
||
| // Test that func init does not panic. | ||
| func TestInit(t *testing.T) {} |
There was a problem hiding this comment.
Did you mean to call Init in this test function?
There was a problem hiding this comment.
Hmm, this test is a leftover and does not make sense because init is called before executing the test. I'll remove it.
| } | ||
| return strings.TrimSpace(string(buf)) == "1" | ||
| // FIPS returns true if OpenSSL is running in FIPS mode, else returns false. | ||
| func FIPS() bool { |
There was a problem hiding this comment.
What is the diff between FIPS() and Enabled after this PR?
There was a problem hiding this comment.
openssl.FIPS() just check if OpenSSL is running on FIPS mode by calling the appropriate OpenSSL function. It can return either because FIPS is enabled system-wide by default or because SetFIPS has been called successfully. So the FIPS state does not belong to the openssl Go package but to OpenSSL itself.
backend.Enabled means that OpenSSL has been initialized and is running in FIPS mode. In other words, the init function has successfully called openssl.Init() and either openssl.FIPS() == true or openssl.SetFIPS(true) == nil.
| func VerifyRSAPSS(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte, saltLen int) error { | ||
| panic("boringcrypto: not available") | ||
| } | ||
| // Copyright 2017 The Go Authors. All rights reserved. |
There was a problem hiding this comment.
It looks like this file changed from LF to CRLF line endings. (That's why the diff doesn't find any similar lines. Go explicitly turns off autocrlf in .gitattributes, so need to be careful with editors. The .gitattributes file suggests there's some logic in the CL submission tool that verifies LF endings.)
These files currently have CRLF as of this PR:
$ find src/crypto -not -type d -exec file "{}" ";" | grep CRLF
src/crypto/internal/backend/openssl_linux.go: ASCII text, with CRLF line terminators
src/crypto/internal/backend/backend_test.go: ASCII text, with CRLF line terminators
src/crypto/internal/backend/nobackend.go: ASCII text, with CRLF line terminators
src/crypto/internal/backend/internal/openssl/goopenssl.c: C source, ASCII text, with CRLF line terminators
src/crypto/internal/backend/internal/openssl/apibridge_1_1.h: ASCII text, with CRLF line terminators
src/crypto/internal/backend/internal/openssl/openssl_funcs.h: C source, ASCII text, with CRLF line terminators
src/crypto/internal/backend/internal/openssl/apibridge_1_1.c: C source, ASCII text, with CRLF line terminators
Turning on GitHub's "ignore whitespace" option does work to make this reviewable, though!
There was a problem hiding this comment.
Wow, that's unexpected. I'll fix it 👍
| } | ||
| } | ||
|
|
||
| func hasSuffix(s, t string) bool { |
There was a problem hiding this comment.
why don't we use strings.HasSuffix?
There was a problem hiding this comment.
My initial answer was going to be:
Go is very conscious about binary sizes and they don't want to add bloat to final apps is not strictly necessary.
stringsimportsunicode/utf8, which is huge, so better just copy the HasSuffix implementation and avoid that dependency for apps that usecryptobut do not importstrings(or anything that ends up importing it). In fact we are importingoswhen we shouldn't: #360
But I then realized that I already introduced strings some lines above to call strings.TrimSpace, which was an oversight, but my argument falls apart.
Anyway, I'll keep it as is for now as when I remove the os dependency will no longer need strings.TrimSpace, and I plan to do so once the active FIPS PRs are merged.
Co-authored-by: Davis Goodin <[email protected]>
Co-authored-by: Davis Goodin <[email protected]>
qmuntal
changed the base branch from
dev/qmuntal/port-openssl
to
microsoft/dev.boringcrypto.go1.17
Co-authored-by: Davis Goodin <[email protected]>
Co-authored-by: Davis Goodin <[email protected]>
…soft/go into dev/qmuntal/crypto-backend
|
It looks like a lot of commits got duplicated. (Pull/merge rather than force push after a rebase?) |
Important: This PR will be rebased to
microsoft/dev.boringcrypto.go1.17once #361 is merged.This PR contains two major changes:
crypto/internal/backend/internal/openssldoes not have aninitnorEnabledfunction anymore. It exposes instead three new functions:Init() error,FIPS() bool,SetFIPS(bool) error. It is now up to the caller to initialize the package and track if OpenSSL is enabled or not. Tests and documentation has changed accordingly.crypto/internal/backend. This package acts as a façade between Go crypto and whatever crypto backend we want to implement, at the moment just OpenSSL. It was a linux-onlyinitfunction that decides is OpenSSL and FIPS should be used or not using the criteria previously implemented.There are also some minor refactors:
NewOpenSSLError()as it is not useful externally.opensslpackage no longer needs so many build tags as part of the logic has been moved tobackend. Header files don't need build tags as they are only included if a .go or .c imports them.