-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
feat: add experimental cookie encryption support #27524
Conversation
Cool stuff :) |
3810333
to
0a48789
Compare
e601fc3
to
72f630b
Compare
patches/chromium/make_keychainpassword_service_name_and_service_name_mutable_strings.patch
Outdated
Show resolved
Hide resolved
// Only use a persistent prefs store when cookie encryption is enabled as that | ||
// is the only key that needs it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the cookie encryption code use this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On windows the encryption secret is protected with DPAPI
and the "protected" encryption key is stored in plain text in Local State
on disk.
d8c0db3
to
d2ae996
Compare
5651f9f
to
d89a0f9
Compare
scoped_refptr<JsonPrefStore> user_pref_store = | ||
base::MakeRefCounted<JsonPrefStore>(prefs_path); | ||
user_pref_store->ReadPrefs(); | ||
prefs_factory.set_user_prefs(user_pref_store); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that when cookie encryption is enabled we might store other, non-cookie-encryption-related prefs on disk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so yeah, I would have made it JSON prefs for all cases but that felt like a bit of an overreach for this PR. It would align better with Chromium to do that though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What preferences are in that group? What would we start storing on disk that previously wasn't persisted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is the only other pref we register into this store is the kProxy
pref which is overridden / ignored by the per-browser-context proxy settings later down the line. That pref is never read / set in our code so being in memory vs stored to disk wouldn't appear to have any affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we use an in-memory pref for the proxy config is because we reuse proxy config code from //chrome
https://github.com/electron/electron/blob/master/patches/chromium/proxy_config_monitor.patch to interact with the proxy service and that relies on reading the configs from a pref store https://source.chromium.org/chromium/chromium/src/+/main:components/proxy_config/pref_proxy_config_tracker_impl.cc, I didn't find the need the flush these values to disk at that time as they are mainly required only to communicate the data between the browser process and the proxy service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the pref store is change to a persistent one when cookie encryption is enabled we should also make relevant changes here https://github.com/electron/electron/blob/master/shell/browser/api/electron_api_session.cc#L536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepak1556 The BrowserProcess prefs store and the BrowserContext prefs store are different and don't need to be aligned afaik. They store things in different locations and having one in memory and one not seems fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, they don't need to be in sync.
Proxy config in BrowserProcess pref store are read by the SystemNetworkContextManager whereas the BrowserContext pref store are read by per-session network contexts.
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anywhere we can document this feature? It doesn't seem that this feature is very discoverable as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API LGTM
This is by design, Slack will take the load of testing it and once confidence is higher the plan is to document and flip then later fuse default 👍 Currently this is all very untested and critical to most applications so we're taking it very slowly. |
Release Notes Persisted
|
* feat: add experimental cookie encryption support on macOS * chore: fix TODO * update patches * feat: make cookie encryption work on windows * chore: update cookie encryption support comments * fix: only call OSCrypt::Init on windows * chore: make cookie encryption work on linux * Update shell/browser/net/system_network_context_manager.cc Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: fix lint * chore: update patches * chore: update patches to upstreamed variants * chore: use chrome ::switches constants * chore: remove bad patch * build: disable cookie encryption by default * chore: update patches * fix: provide std::string to NoDestructor * chore: fix macos, nodestructor syntax * build: fix macOS build due to mismatch in DEFINE Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>
* feat: add experimental cookie encryption support on macOS * chore: fix TODO * update patches * feat: make cookie encryption work on windows * chore: update cookie encryption support comments * fix: only call OSCrypt::Init on windows * chore: make cookie encryption work on linux * Update shell/browser/net/system_network_context_manager.cc Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: fix lint * chore: update patches * chore: update patches to upstreamed variants * chore: use chrome ::switches constants * chore: remove bad patch * build: disable cookie encryption by default * chore: update patches * fix: provide std::string to NoDestructor * chore: fix macos, nodestructor syntax * build: fix macOS build due to mismatch in DEFINE Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@MarshallOfSound has manually backported this PR to "14-x-y", please check out #29492 |
@MarshallOfSound has manually backported this PR to "13-x-y", please check out #29493 |
* feat: add experimental cookie encryption support on macOS * chore: fix TODO * update patches * feat: make cookie encryption work on windows * chore: update cookie encryption support comments * fix: only call OSCrypt::Init on windows * chore: make cookie encryption work on linux * Update shell/browser/net/system_network_context_manager.cc Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: fix lint * chore: update patches * chore: update patches to upstreamed variants * chore: use chrome ::switches constants * chore: remove bad patch * build: disable cookie encryption by default * chore: update patches * fix: provide std::string to NoDestructor * chore: fix macos, nodestructor syntax * build: fix macOS build due to mismatch in DEFINE Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org> Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>
* feat: add experimental cookie encryption support (#27524) * feat: add experimental cookie encryption support on macOS * chore: fix TODO * update patches * feat: make cookie encryption work on windows * chore: update cookie encryption support comments * fix: only call OSCrypt::Init on windows * chore: make cookie encryption work on linux * Update shell/browser/net/system_network_context_manager.cc Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: fix lint * chore: update patches * chore: update patches to upstreamed variants * chore: use chrome ::switches constants * chore: remove bad patch * build: disable cookie encryption by default * chore: update patches * fix: provide std::string to NoDestructor * chore: fix macos, nodestructor syntax * build: fix macOS build due to mismatch in DEFINE Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: update patches Co-authored-by: Electron Bot <electron@github.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>
It's unclear to me what the behavior is when you have support enabled and the user denies the prompt for keychain access in MacOS. Do the cookies get stored unencrypted then? Is there a way for us to know that the user has denied this access? |
This is mostly exploratory but the intention is to provide an Electron Fuse to enable cookie encryption for Electron apps. This has several technical challenges (mostly coming down to hard coded assumptions in Chromium) and the intention is for this WIP PR to remain open while I figure out how to make it work on all three platforms in a generic and sustainable way.
Refs #7073
Platform support as of the moment you're reading this:
Migration paths from Un-Encrypted to Encrypted is completely handled by Chromium, it will continue to read un-encrypted cookies but will encrypt-on-write so over time even existing cookies will either expire or be written-over with an encrypted value.
Going the other way (encryp -> unencrypt) is not supported and will be a lossy action (all cookies will be considered invalid or be garbled in HTTP requests). This "revert" story should be documented before landing.
By default Chromium uses values like
Chromium Safe Storage
to store the encryption keys, Electron will use${app.getName()} Safe Storage
. This results inElectron Safe Storage
being the default and packaged apps will haveMy App Safe Storage
.Notes: Added experimental cookie encryption support behind an Electron Fuse