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

Pick up curl 7.72.0 current #13063

Merged
merged 5 commits into from
Sep 16, 2020
Merged

Pick up curl 7.72.0 current #13063

merged 5 commits into from
Sep 16, 2020

Conversation

wrowe
Copy link
Contributor

@wrowe wrowe commented Sep 11, 2020

Commit Message: Pick up curl 7.72.0 current

Additional Description:

  • Picks up fix of CVE-2020-8231: libcurl: wrong connect-only connection
    (Envoy does NOT use the affected CURLOPT_CONNECT_ONLY toggle,
    this change simply satisfies overly-simplistic audits, and this further
    reinforces issue Remove curl as an Envoy dependency #11816 )

  • Resolve premature force-AF_UNIX toggle in our working branch and
    disables UNIX_SOCKETS on windows, until afunix.h is used for sys/un.h
    (Breakage is between curl 7.69 and curl 7.72, which doesn't pick up afunix.h
    on windows; will address and resolve upstream before revisiting at envoy.)

  • Work around misplaced -MD cflag on Windows build
    See Windows flaw when building with bazel in modern CMake due to erroneous policy CMP0091 bazel-contrib/rules_foreign_cc#426
    for comprehensive discussion of the defect. This workaround
    could be dropped if rules_foreign_cc works around it for us.

  • Preparing for specific corrections to get curl building curl.lib
    with cmake using clang-cl on Windows, starting at envoy and pushing
    the fix(es) to CMakeLists.txt upstream.

Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a

- Picks up fix of CVE-2020-8231: libcurl: wrong connect-only connection
  (Envoy doesn't use the affected CURLOPT_CONNECT_ONLY toggle)

- Resolves small patch correction and premature force-AF_UNIX toggle
  in our dev branch (curl 7.72 doesn't pick up afunix.h on windows,
  will first address this upstream.)

- Preparing for specific corrections to get curl building curl.lib
  with cmake using clang-cl on Windows, starting at envoy and pushing
  the fix(es) to CMakeLists.txt upstream.

Signed-off-by: William A Rowe Jr <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13063 was opened by wrowe.

see: more, trace.

@htuch
Copy link
Member

htuch commented Sep 11, 2020

@wrowe this is failing curl_test.

@htuch htuch self-assigned this Sep 11, 2020
@wrowe
Copy link
Contributor Author

wrowe commented Sep 11, 2020

Thanks @htuch - that was a problem, we've had to disable UNIX_SOCKETS till we repair curl itself. Fixed the test short-term to not expect that feature on Windows builds, should be ready to merge once passing CI.

Signed-off-by: William A Rowe Jr <[email protected]>
+cmake_minimum_required(VERSION 3.0 FATAL_ERROR)
+cmake_minimum_required(VERSION 3.2 FATAL_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a Windows specific patch? If so wondering if a comment should be added to https://github.com/envoyproxy/envoy/blob/master/bazel/repositories.bzl#L677-L678

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to understand why this is needed.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo comment from @moderation

+cmake_minimum_required(VERSION 3.0 FATAL_ERROR)
+cmake_minimum_required(VERSION 3.2 FATAL_ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to understand why this is needed.

@sunjayBhatia
Copy link
Member

/wait

reworking the patch

- See bazel-contrib/rules_foreign_cc#426
  for comprehensive discussion of the defect. This workaround
  could be dropped if rules_foreign_cc works around it for us.

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@wrowe
Copy link
Contributor Author

wrowe commented Sep 15, 2020

We were able to drop the patch altogether by working around the CMake project's breakage. (This wasn't actually on the curl project's logic.)

@wrowe
Copy link
Contributor Author

wrowe commented Sep 15, 2020

@htuch This should be much easier to follow now, are ready to review/merge

@moderation
Copy link
Contributor

Really good to remove the patch @wrowe. Thanks. LGTM

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for sorting out the patch situation!

@htuch htuch merged commit 27591c2 into envoyproxy:master Sep 16, 2020
@sunjayBhatia sunjayBhatia deleted the bump-curl-7_72_0 branch September 16, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants