impl(common+storage): remove OpenSSL dependency on Windows #13785

teo-tsirpanis · 2024-03-16T17:56:26Z

Fixes #13746

This PR reimplements the cryptography routines on top of Windows APIs, removing the dependency to OpenSSL on Windows¹.

Validated by successfully running the relevant tests locally. I also did some minor cleanups where applicable.

This change is

At least for the REST client libraries; gRPC has its own dependency on OpenSSL. ↩

…gle::cloud::internal::SignUsingSha256` These functions are doing the same thing.

… well

devbww · 2024-03-16T18:06:02Z

/gcbrun

google/cloud/BUILD.bazel

google/cloud/google_cloud_cpp_common.cmake

google/cloud/storage/internal/openssl_util.cc

codecov · 2024-03-16T20:31:16Z

Codecov Report

Attention: Patch coverage is 89.34426% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 93.04%. Comparing base (4832108) to head (d2444bd).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...internal/openssl/parse_service_account_p12_file.cc	79.03%	13 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13785   +/-   ##
=======================================
  Coverage   93.04%   93.04%           
=======================================
  Files        2176     2178    +2     
  Lines      185186   185178    -8     
=======================================
- Hits       172308   172302    -6     
+ Misses      12878    12876    -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coryan

First, thank you for preparing this PR. These are difficult APIs and the changes look good.

Second, I think we should take some time to refactor the code so these changes are easier to maintain in the future. Most of my comments below go along those lines.

Third, I think we need to think about the test coverage consequences. For example, our ASAN builds only run on Linux, and that makes sense because the code is (or was) largely identical on Linux, macOS and Windows. This change introduces a number of tricky allocations / deallocations that (to my eyes) look correct, but I would feel better if they were also tested by some dynamic analyzer. @scotthart our infrastructure cannot handle that at the moment, we should consider if (and how) we would support code that is more specialized for Windows.

@teo-tsirpanis you may want to hold off on making more changes until we had a chance to discuss this internally.

google/cloud/BUILD.bazel

google/cloud/google_cloud_cpp_common.cmake

google/cloud/google_cloud_cpp_rest_internal.cmake

google/cloud/internal/curl_wrappers_locking_already_present_test.cc

google/cloud/storage/google_cloud_cpp_storage.cmake

google/cloud/internal/openssl_util.cc

google/cloud/storage/internal/hash_function_impl.cc

We depend on it only when we need to include a header. Bazel takes care of linking.

coryan · 2024-03-22T13:31:15Z

Thanks for being patient with us, and thanks for sending this PR. I think we really want to break the dependency on OpenSSL on Windows, and your changes are in the right direction.

I think we want to make some refactoring before making these changes. I will be doing the refactoring and CC you on the PRs so you can see where things are going. Feel free to rebase this PR or create a new PR at the end, whatever works for you.

The changes I have in mind include:

Remove the duplicate implementation of SingUsingSha256 (see refactor(storage): de-duplicate SignWithPem() #13825).
Rename google/cloud/internal/openssl_utils.h to something that does not speak to the library we use to implement these things, maybe google/cloud/internal/sign_using_sha256.{h,cc}.
Split the google/cloud/storage/internal/openssl_utils.* into files by function, e.g., .../internal/base64.* and .../internal/md5.*

After you merge your changes I will probably reorganize the code to reduce the number of #ifdefs. I think we would prefer files that are only compiled when needed, or that have a single #ifdef for the whole file. Those changes are not as urgent, and require a deeper understanding of our build scripts, so we will make them. I just mention them because you may find the code changed after your PR.

Again, thanks for this contribution, and let me know how you would like to proceed.

coryan · 2024-03-22T15:29:43Z

@teo-tsirpanis more refactoring in #13826

coryan · 2024-03-28T13:38:01Z

It seems to be Bazel:

https://github.com/bazelbuild/bazel/blob/3ce7424e7f48780c659d63de40132dccf9e026c8/tools/cpp/windows_cc_toolchain_config.bzl#L662

teo-tsirpanis · 2024-03-28T13:47:31Z

Thanks for finding out. Is there a way to work around it?

google/cloud/BUILD.bazel

google/cloud/storage/BUILD.bazel

coryan · 2024-03-28T17:32:05Z

/gcbrun

teo-tsirpanis · 2024-03-28T18:01:52Z

google/cloud/storage/internal/grpc/object_request_parser_test.cc

Running clang-format on this file caused all these changes. I initially did not commit them because they were unrelated, but the checkers CI job keeps failing.

No other file changed by this PR gets changed after running clang-format.

You probably have a slightly different version of clang-format. The checkers job is failing for other reasons, the diff is available at

https://github.com/googleapis/google-cloud-cpp/pull/13785/checks?check_run_id=23211048683

There are problems are (mostly) formatting of the CMake files.

google/cloud/google_cloud_cpp_common.cmake

google/cloud/google_cloud_cpp_rest_internal.cmake

google/cloud/storage/google_cloud_cpp_storage.cmake

coryan · 2024-03-28T18:13:30Z

google/cloud/storage/internal/grpc/object_request_parser_test.cc

You probably have a slightly different version of clang-format. The checkers job is failing for other reasons, the diff is available at

https://github.com/googleapis/google-cloud-cpp/pull/13785/checks?check_run_id=23211048683

There are problems are (mostly) formatting of the CMake files.

This reverts commit cdd4cfe.

Stop setting `_WIN32_WINNT` (we need it only in Bazel). Include `absl_str_cat_quiet.h`

teo-tsirpanis · 2024-03-28T18:30:53Z

I installed cmake-format and formatted the files checkers complained about. checkers showed six files in its diff, and my latest commit (919fa1e) changed these six files.

coryan

LGTM. Let's wait for the CI robots before declaring success.

coryan · 2024-03-28T18:36:49Z

/gcbrun

coryan · 2024-03-28T19:34:32Z

I believe the build failures in https://github.com/googleapis/google-cloud-cpp/actions/runs/8472009569/job/23214103847?pr=13785 are fixed in #13836 . @teo-tsirpanis have you had a chance to rebase since #13836 got merged?

coryan · 2024-03-28T19:41:06Z

/gcbrun

coryan

Woo hoo! Thanks for this change. Not only did we remove the dependency on OpenSSL, I think the code is cleaner.

I will be merging the code once the last build succeeds.

coryan · 2024-03-28T22:25:17Z

Thanks again for the large contribution. We do appreciate it.

* Remove OpenSSL dependency on Windows. See googleapis/google-cloud-cpp#13785. * MNT: Re-rendered with conda-build 3.27.0, conda-smithy 3.34.1, and conda-forge-pinning 2024.04.22.06.37.48

teo-tsirpanis added 9 commits March 16, 2024 17:29

move OpenSSL header to implementation file

394ff96

port google_cloud_cpp_common to use Windows crypto APIs

5e668e1

port google_cloud_cpp_rest_internal to use Windows crypto APIs

cb1ec7c

forward google::cloud::storage::internal::SignStringWithPem to `goo…

0af9bb6

…gle::cloud::internal::SignUsingSha256` These functions are doing the same thing.

define WIN32_LEAN_AND_MEAN from build system and add it in CMake as…

d9c3e96

… well

fix copy-paste typo

eedb3e5

port google_cloud_cpp_storage to use Windows crypto APIs

900660a

explicitly list OpenSSL as a non-Windows dependency in vcpkg.json

6a14b35

fix calls to find_dependency(OpenSSL)

aa21ced

teo-tsirpanis requested review from a team as code owners March 16, 2024 17:56

teo-tsirpanis temporarily deployed to external March 16, 2024 17:56 — with GitHub Actions Inactive

teo-tsirpanis commented Mar 16, 2024

View reviewed changes

google/cloud/BUILD.bazel Show resolved Hide resolved

google/cloud/google_cloud_cpp_common.cmake Outdated Show resolved Hide resolved

google/cloud/storage/internal/openssl_util.cc Outdated Show resolved Hide resolved

teo-tsirpanis had a problem deploying to external March 16, 2024 19:14 — with GitHub Actions Error

teo-tsirpanis added 2 commits March 16, 2024 21:43

fix typo

0e9634b

format Bazel files

1f05827

teo-tsirpanis force-pushed the win32-crypto branch from d9f60fb to 1f05827 Compare March 16, 2024 19:43

teo-tsirpanis temporarily deployed to external March 16, 2024 19:43 — with GitHub Actions Inactive

coryan suggested changes Mar 16, 2024

View reviewed changes

teo-tsirpanis added 3 commits March 16, 2024 23:52

fix typos

bb6d7ff

remove unnecessary references to BoringSSL on Bazel

d6ba802

We depend on it only when we need to include a header. Bazel takes care of linking.

change the scope of some conditionals

966a85a

coryan mentioned this pull request Mar 17, 2024

cleanup: always put system headers at the bottom #13788

Merged

This was referenced Mar 22, 2024

refactor(storage): split MD5Hash() internal function #13826

Merged

refactor(storage): de-duplicate SignWithPem() #13825

Merged

Merge branch 'main' into win32-crypto

f451ca2

coryan reviewed Mar 28, 2024

View reviewed changes

google/cloud/BUILD.bazel Outdated Show resolved Hide resolved

google/cloud/storage/BUILD.bazel Show resolved Hide resolved

teo-tsirpanis added 3 commits March 28, 2024 19:30

add new directories with installed headers

e2554fb

define _WIN32_WINNT to Windows 10

aeeb6f6

add comment on casting the size of p12 files to 32-bit unsigned integer

0ad1dc9

teo-tsirpanis temporarily deployed to external March 28, 2024 17:31 — with GitHub Actions Inactive

clang-format

cdd4cfe

teo-tsirpanis had a problem deploying to external March 28, 2024 18:00 — with GitHub Actions Error

teo-tsirpanis commented Mar 28, 2024

View reviewed changes

coryan reviewed Mar 28, 2024

View reviewed changes

teo-tsirpanis added 2 commits March 28, 2024 20:15

Revert "clang-format"

9b928f7

This reverts commit cdd4cfe.

format and cleanup

919fa1e

Stop setting `_WIN32_WINNT` (we need it only in Bazel). Include `absl_str_cat_quiet.h`

teo-tsirpanis temporarily deployed to external March 28, 2024 18:29 — with GitHub Actions Inactive

teo-tsirpanis requested a review from coryan March 28, 2024 18:31

coryan reviewed Mar 28, 2024

View reviewed changes

Merge branch 'main' into win32-crypto

d2444bd

teo-tsirpanis temporarily deployed to external March 28, 2024 19:39 — with GitHub Actions Inactive

coryan approved these changes Mar 28, 2024

View reviewed changes

coryan merged commit 8b7fd79 into googleapis:main Mar 28, 2024
75 checks passed

teo-tsirpanis deleted the win32-crypto branch March 28, 2024 22:26

teo-tsirpanis mentioned this pull request Apr 22, 2024

Remove OpenSSL dependency on Windows. conda-forge/google-cloud-cpp-core-feedstock#16

Merged

5 tasks

teo-tsirpanis mentioned this pull request Apr 22, 2024

[google-cloud-cpp] update to the latest release (v2.23) microsoft/vcpkg#38204

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl(common+storage): remove OpenSSL dependency on Windows #13785

impl(common+storage): remove OpenSSL dependency on Windows #13785

teo-tsirpanis commented Mar 16, 2024 •

edited by coryan

Loading

devbww commented Mar 16, 2024

codecov bot commented Mar 16, 2024 •

edited

Loading

coryan left a comment

coryan commented Mar 22, 2024

coryan commented Mar 22, 2024

coryan commented Mar 28, 2024

teo-tsirpanis commented Mar 28, 2024

coryan commented Mar 28, 2024

teo-tsirpanis Mar 28, 2024

coryan Mar 28, 2024

coryan Mar 28, 2024

teo-tsirpanis commented Mar 28, 2024

coryan left a comment

coryan commented Mar 28, 2024

coryan commented Mar 28, 2024

coryan commented Mar 28, 2024

coryan left a comment

coryan commented Mar 28, 2024

impl(common+storage): remove OpenSSL dependency on Windows #13785

impl(common+storage): remove OpenSSL dependency on Windows #13785

Conversation

teo-tsirpanis commented Mar 16, 2024 • edited by coryan Loading

Footnotes

devbww commented Mar 16, 2024

codecov bot commented Mar 16, 2024 • edited Loading

Codecov Report

coryan left a comment

Choose a reason for hiding this comment

coryan commented Mar 22, 2024

coryan commented Mar 22, 2024

coryan commented Mar 28, 2024

teo-tsirpanis commented Mar 28, 2024

coryan commented Mar 28, 2024

teo-tsirpanis Mar 28, 2024

Choose a reason for hiding this comment

coryan Mar 28, 2024

Choose a reason for hiding this comment

coryan Mar 28, 2024

Choose a reason for hiding this comment

teo-tsirpanis commented Mar 28, 2024

coryan left a comment

Choose a reason for hiding this comment

coryan commented Mar 28, 2024

coryan commented Mar 28, 2024

coryan commented Mar 28, 2024

coryan left a comment

Choose a reason for hiding this comment

coryan commented Mar 28, 2024

teo-tsirpanis commented Mar 16, 2024 •

edited by coryan

Loading

codecov bot commented Mar 16, 2024 •

edited

Loading