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

Port openssl package #1

Merged
merged 7 commits into from
Jan 24, 2022
Merged

Port openssl package #1

merged 7 commits into from
Jan 24, 2022

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jan 19, 2022

This PR copies the openssl package located at crypto/internal/backend/internal/openssl into this repo as a standalone module.

Additional changes:

  • Update README.md (this is the only file to review in this PR!)
  • Remove OpenSSL license as we are distributing it
  • Use Microsoft copyright header everywhere except in openssl/internal/subtle, as it is a direct copy of golang.org/x/crypto/internal/subtle.

Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of README.md. A few things I missed in the earlier doc location, and CGO -> cgo.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

OpenSSL is used for a given build only in limited circumstances:

- The platform must be GOOS=linux.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do you know if this will probably work on arm64 etc.? Nothing comes to mind that would prevent it from working just like it does on amd64, but this made me curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work on any platform that is supported by OpenSSL and that can use dlopen and pthreads. To the best of my knowledge almost any linux box, except Android (see this), fulfill those requirements.

README.md Outdated Show resolved Hide resolved
qmuntal and others added 2 commits January 20, 2022 09:03
Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>

> The status of FIPS 140 for Go itself remains "no plans, basically zero chance".

On the other hand, Google maintains a branch that uses cgo and BoringSSL to implement various crypto primitives: https://github.com/golang/go/blob/dev.boringcrypto/README.boringcrypto.md. As BoringSSL is FIPS 140-2 certified, an application using that branch is more likely to be FIPS 140-2 compliant, yet Google does not provide any liability about the suitability of this code in relation to the FIPS 140-2 standard.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end of this sentence sort of implies that we do provide liability, but it's not explicitly called out. Should we clarify our stance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least, OpenSSL does. Either way, as a consumer, it's a bit confusing as to what exactly the guarantees are here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that we should be more explicit regarding the FIPS guarantees when using this package, but I'm afraid we don't have them clear yet. Meanwhile I've added a disclaimer into the README which should help on creating the right expectations.

@jaredpar do you have any insights here?

Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion for the DecryptRSANoPadding comment, and I think the question about what we currently (and maybe plan to) guarantee with our implementation is valid. After that, LGTM.

openssl/rsa.go Outdated Show resolved Hide resolved
qmuntal and others added 2 commits January 21, 2022 08:49
Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>
@qmuntal qmuntal mentioned this pull request Jan 21, 2022
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea on the disclaimer intro. LGTM, plus nitpicky grammar suggestion.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>
@qmuntal qmuntal merged commit 183c44c into main Jan 24, 2022
@dagood dagood deleted the dev/qmuntal/openssl branch January 24, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants