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

Add CNG crypto backend #645

Merged
merged 9 commits into from
Jul 28, 2022
Merged

Add CNG crypto backend #645

merged 9 commits into from
Jul 28, 2022

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 19, 2022

This PR integrates https://github.com/microsoft/go-crypto-winnative as a Go backend.

Tips for the reviewers:

  • The first commit is just adding the CNG backend as if it was 100% compatible with Go crypto. You can review this fast because it is the same as we already did for openssl and boring.
  • The second commit adds a new CI job that builds and test Go using goexperiment=cngcrypto.
  • The third commit is the important one. It contains all the hacks and fallbacks I had to do to fit CNG into Go.

As agreed, we will fallback to Go crypto when CNG does not implement an algorithm or a parameter required by Go. The complete list can be found at microsoft/go-crypto-winnative#4.

⚠️ I have not implemented a fallback for unsupported RSA key lengths. This is a special case, as CNG might want to restrict even more the supported key lengths based on the configured security provider or when running in FIPS mode. If we fallback to Go crypto in such case, we would be bypassing an important security policy.

I'll add an exhaustive documentation of what to expect and how to use CNG backend once this PR lands.

@dagood @jaredpar @chsienki how does goexperiment=cngcrypto resonates to you? People will understand that cngcrypto is the CNG Windows backend? Alternatives?

Closes #476

@dagood
Copy link
Member

dagood commented Jul 19, 2022

how does goexperiment=cngcrypto resonates to you? People will understand that cngcrypto is the CNG Windows backend? Alternatives?

I think it works--it parallels nicely with boringcrypto and opensslcrypto being named after the native library they use rather than the platform. Anyone who wants to use the mode should be looking in our docs and understand it fine from there. And even if someone sees it in the wild and doesn't understand, I was able to plop cngcrypto into a few search engines and see the Microsoft CNG docs at or near the top.

@jaredpar
Copy link
Member

I like cngcrypto

I was able to plop cngcrypto into a few search engines and see the Microsoft CNG docs at or near the top.

This makes me like it more 😄

@qmuntal qmuntal force-pushed the dev/qmuntal/cng branch 3 times, most recently from c649559 to 41e174a Compare July 20, 2022 09:56

- if boring.Enabled {
+ if boring.Enabled &&
+ (!goexperiment.CNGCrypto || (len(priv.Primes) == 2 && hash != crypto.MD5SHA1)) {
Copy link
Member

Choose a reason for hiding this comment

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

Here's what I was mentioning in the Go sync--the inversion:

 (!goexperiment.CNGCrypto || (len(priv.Primes) == 2 && hash != crypto.MD5SHA1))
!( goexperiment.CNGCrypto && (len(priv.Primes) != 2 || hash == crypto.MD5SHA1))

I think of the right side as a list of unsupported/fallback cases and it's quite a bit easier to read as positives rather than negatives:

  • len(priv.Primes) == 2 && hash != crypto.MD5SHA1) "does every unsupported case not match"
  • len(priv.Primes) != 2 || hash == crypto.MD5SHA1) "does any unsupported case match"

Maybe it's just me. I can certainly figure it out either way, so I'm happy to go with whatever makes the most sense for anyone else. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! 0236a2e

@qmuntal qmuntal marked this pull request as ready for review July 21, 2022 07:02
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.

LGTM, just a comment/doc suggestion and a question.

Comment on lines 472 to 474
+ // CNGCrypto has too many stdlib fallbacks,
+ // so Unreachable is not always true.
+ if !goexperiment.CNGCrypto {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth elaborating a bit here... my understanding from poking around is that this is because CNGCrypto has more fallbacks than boring/openssl, and I think this is saying that there are enough extra fallbacks that if we were to remove boring.Unreachable() from each of those code paths, Unreachable becomes useless. Is that right?

I assume it's also not worthwhile to make backend-specific Unreachable calls, so they can be tailored to the fallbacks necessary for each backend?

Copy link
Contributor Author

@qmuntal qmuntal Jul 27, 2022

Choose a reason for hiding this comment

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

I give it another thought, and it was easier than expected to make backend-specific Unreachable calls:
358a6e6.

patches/0006-Add-CNG-crypto-backend.patch Outdated Show resolved Hide resolved
@qmuntal qmuntal merged commit 2ebdc73 into microsoft/main Jul 28, 2022
@qmuntal qmuntal deleted the dev/qmuntal/cng branch July 28, 2022 18:34
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.

Implement FIPS-compatible crypto for Windows using platform-native crypto library
4 participants