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.

Sign up for GitHub

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

Jump to bottom

fix: default to NTLM v2 in the network service for POSIX platforms #23846

Merged
merged 2 commits into from Jun 2, 2020
Merged

fix: default to NTLM v2 in the network service for POSIX platforms #23846

merged 2 commits into from Jun 2, 2020

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented May 29, 2020 •
edited

Description of Change

Fixes #22951

This has been the default for a while at the //net layer https://bugs.chromium.org/p/chromium/issues/detail?id=22532 , the network service configures the default incorrectly. I have reported it in upstream, will update here once I have a response.

Checklist

Release Notes

Notes: enable NTLM v2 for POSIX platforms and added --disable-ntlm-v2 switch to disable it.

@deepak1556 deepak1556 requested a review from a team as a code owner May 29, 2020 09:29
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 29, 2020
@deepak1556 deepak1556 requested a review from a team May 29, 2020 09:29
@deepak1556
Copy link
Member Author

deepak1556 commented May 29, 2020 •
edited

Marking WIP pending upstream fix https://chromium-review.googlesource.com/c/chromium/src/+/2222116

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 30, 2020
@deepak1556 deepak1556 removed the wip ⚒ label Jun 1, 2020
@deepak1556
Copy link
Member Author

Upstream PR has been merged.

@deepak1556
Copy link
Member Author

deepak1556 commented Jun 1, 2020 •
edited

Since this is a breaking change, I will change the PR to keep the default as NTLM v1 for the current release lines, will add a flag --enable-ntlm-v2 for apps and since 11-x-y is a quiet release, will do the same for master. After 11-x-y is branched out will change the default to NTLM v2 and flip the flag to --disable-ntlm-v2

@deepak1556
Copy link
Member Author

Looking into this a bit more, NTLM v2 was made default in 68.0.3416.0 which went out with our Electron 4 release. This broke accidentally after network service codepath was used to initialize the http auth preferences since Electron 6. Hence this should be treated as a bug fix rather than breaking change,

Also NTLM v2 is more widely used authentication protocol than v1 if at all NTLM is used. Hence enabling this as default with a flag to disable is a good change.

@deepak1556 deepak1556 removed the wip ⚒ label Jun 2, 2020
@deepak1556 deepak1556 requested review from a team and removed request for a team June 2, 2020 17:34
@deepak1556
Copy link
Member Author

PR is ready for review. Thanks!

@deepak1556 deepak1556 merged commit 512e154 into master Jun 2, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 2, 2020

Release Notes Persisted

enable NTLM v2 for POSIX platforms and added --disable-ntlm-v2 switch to disable it.

@deepak1556 deepak1556 deleted the robo/ntlm_v2 branch June 2, 2020 19:58
@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

I was unable to backport this PR to "7-3-x" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/7-3-x label Jun 2, 2020
@trop trop bot mentioned this pull request Jun 2, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

I have automatically backported this PR to "10-x-y", please check out #23916

@deepak1556 deepak1556 mentioned this pull request Jun 3, 2020
Merged
7 tasks
@trop
Copy link
Contributor

trop bot commented Jun 3, 2020

@deepak1556 has manually backported this PR to "8-x-y", please check out #23933

@deepak1556 deepak1556 mentioned this pull request Jun 3, 2020
Merged
7 tasks
@trop
Copy link
Contributor

trop bot commented Jun 3, 2020

@deepak1556 has manually backported this PR to "9-x-y", please check out #23934

@deepak1556 deepak1556 mentioned this pull request Jun 3, 2020
Merged
7 tasks
@trop
Copy link
Contributor

trop bot commented Jun 3, 2020

@deepak1556 has manually backported this PR to "7-3-x", please check out #23935

deepak1556 added a commit that referenced this pull request Jun 3, 2020
@deepak1556
fix: default to NTLM v2 in the network service for POSIX platforms (#…
778d21d 
…23846)
deepak1556 added a commit that referenced this pull request Jun 7, 2020
@deepak1556 @alexeykuzmin @MarshallOfSound
fix: default to NTLM v2 in the network service for POSIX platforms (#…
fc9fb0b 
…23934)

* build: fix for "enable_desktop_capturer = false" (#23864)

* build: fix filenames autogen with new BUILDFLAG syntax (#23952)

* fix: default to NTLM v2 in the network service for POSIX platforms (#23846)

* chore: update patch

Co-authored-by: Alexey Kuzmin <alkuzmin@microsoft.com>
Co-authored-by: Samuel Attard <sattard@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mac OS - HTTP authentication - NTLMv2 authentication not working
3 participants
@deepak1556 @nornagon @jkleinsc