envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.14k stars 4.83k forks source link

Update v8 version #28336

Open rahulchaphalkar opened 1 year ago

rahulchaphalkar commented 1 year ago

Title: Updating v8 requires uploading static snapshot at https://storage.googleapis.com/envoyproxy-wee8/v8-{version}.tar.gz ?

Description: The only way to update v8 is to have the relevant version's snapshot at the above bucket, is that right? I was hoping to update to v8's main branch until the next release is available, as discussed here, but looks like that is not possible.

alyssawilk commented 1 year ago

cc @mpwarres @lizan @phlax

phlax commented 1 year ago

The only way to update v8 is to have the relevant version's snapshot at the above bucket, is that right?

i think yes - its not an ideal situation - but i think that is how it is currently setup

phlax commented 1 year ago

cc @PiotrSikora

mpwarres commented 1 year ago

I can upload a new static snapshot, but would like to get clarification: is the proposal that we have Envoy use a version of v8 that is not an official release? How long would it be until the upstream v8 change is available in a standard release?

PiotrSikora commented 1 year ago

I can upload a new static snapshot, but would like to get clarification: is the proposal that we have Envoy use a version of v8 that is not an official release? How long would it be until the upstream v8 change is available in a standard release?

The reason for snapshots is that Gitiles used on *.googlesource.com properties doesn't create deterministic tarballs (see: https://github.com/google/gitiles/issues/84), so we need to store downloaded artifacts somewhere to avoid checksum mismatch.

There are no changes between those tarballs and the upstream (see: https://storage.googleapis.com/envoyproxy-wee8/wee8-fetch-deps.sh).

mpwarres commented 1 year ago

@rahulchaphalkar what is the specific version tag of v8 that you need the dependency updated to?

rahulchaphalkar commented 1 year ago

Thanks for the details. @mpwarres I was hoping to use v8 commit cc49dce9f5cb14daee8cc0946e996ba999f97d29 till a newer release of v8 is available, at which point we can switch to that release. Looking at repository_locations.bzl and wee8-fetch-deps.sh, it seems perhaps other dependencies like base_trace_event, icu, zlib etc. might need to be updated in repository_locations.bzl if v8 is updated? I can do that if required.

PiotrSikora commented 1 year ago

Using a random commit from the mainline branch of V8 in a production software in order to add support for a niche and bleeding edge feature is definitely something that I'd object to in principle (but that's @mpwarres's role now).

Even more so, because V8 is not safe / stable on their main branch, which is used for active development with a lot reverts and no security updates... Even the commit you referenced was originally reverted and only relanded last week.

Having said that, it looks that it's a tiny change, so if there are no other code dependencies, then I'd recommend cherry-picking that change and carrying it in bazel/v8.patch until it ships in a stable release of V8.

Also, it looks that V8 wasn't updated since October 2022, and there was plenty of CVEs and 0-days in V8 since then, so it should be updated to the latest stable release... Note that those CVEs are published with cpe:2.3:a:google:chrome:* and not cpe:2.3:a:google:v8:*, so perhaps it's worth updating the CVE scanner to account for that, since it would need to map Chrome versions to V8 versions (see: https://omahaproxy.appspot.com/ or the existing instructions about extracting BoringSSL version from Chromium's DEPS file). cc @phlax

phlax commented 1 year ago

thanks @PiotrSikora - ill take a look at that presently

phlax commented 1 year ago

@mpwarres as a first step can we update v8 to the latest stable - given above info we possibly also want to backport the update to the other stable branches

rahulchaphalkar commented 1 year ago

Thanks @PiotrSikora for the feedback. I did start with bazel/v8.patch, but it was recommended to upstream that to v8 and use the merged (main, LKGR) commit in the interim till a new release is available. (Reference) It was my understanding that reverts/instability mostly affects tip of main, but rarely LKGR since all ci tests need to be completed and passed for LKGR unlike main.

Seems like there are 3 options -

Happy to implement any of these based on the feedback here. FYI @htuch

htuch commented 1 year ago

@phlax do we have a systematic way to reason about which dependencies have stable main branches and which not for when we pin to non-release?

I agree we can use bazel/v8.patch for now given it is upstreamed. My main point (as always) is that we do not carry indefinitely patches in Envoy that belong upstream. That pattern is not sustainable and hurts velocity in various ways.

mpwarres commented 1 year ago

Using bazel/v8.patch for now SGTM. Separately I will work on updating proxy-wasm-cpp-host to the latest stable v8 release, but won't be able to start on that until next week.

phlax commented 1 year ago

do we have a systematic way to reason about which dependencies have stable main branches and which not for when we pin to non-release?

mostly i think trust and necessity - its different for different deps - but its generally either maintainers or codeowners that are responsible for these - or they have no release/some special requirement/etc

i think dep-sheps mostly push back on this if these are being added - i generally take a look to see if there are releases if im reviewing

the cve scanner is far from perfect and some deps are ~invisible to it at least how it currently tracks deps - but non-releases dont help with that - these are also missed by our release/update tracking

probs outside the scope of this quite important ticket tho - maybe we should raise it in a separate issue

phlax commented 1 year ago

would be good to get this update prior to next stable - not sure implications of doing that immediately before release

phlax commented 1 year ago

@mpwarres any progress on this ?

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

mpwarres commented 1 year ago

Sorry, got preempted by other items (and OOO) so not done yet. Working on it now.

phlax commented 7 months ago

cc @mpwarres this has a bit more urgency as we are receiving reports of multiple CVEs with current version

cc @nikita96

mpwarres commented 7 months ago

Ack, working on it now as my top priority.

nikita96 commented 4 months ago

Hi @phlax and @mpwarres any plans on upgrading v8 anytime soon? I see a lot of CVEs being reported on current version. If this is a lower priority item in your list, could you please share more details around how v8 is being used in envoy and how vulnerable is envoy if v8 is not upgraded?

phlax commented 4 months ago

i think we need to get this updated asap - ive just raised a PR (#35552) to set the correct CPE - which hopefully should break our CVE scanner

phlax commented 4 months ago

hmm - that broke the scanner in unexpected ways - but i dont think this can work

i just read @PiotrSikora 's comment properly about mapping deps - im not sure our scanner can handle that atm - im also not sure how it would be possible to do that without throwing lots of unrelated errors - there is not an obvious way to separate CVEs that are specifically v8 related