|
|
Created:
6 years, 3 months ago by Sorin Jianu Modified:
6 years, 3 months ago CC:
chromium-reviews, laforge, Ryan Sleevi, agl Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Project:
chromium Visibility:
Public. |
DescriptionComponent updater must fallback on using HTTP on Windows XPSP2 and below
BUG=411009
Committed: https://crrev.com/c512b7a845d12db2eb9d573f14aab6577c7509f7
Cr-Commit-Position: refs/heads/master@{#295633}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 1
Patch Set 3 : Refactor common version check code. #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 7
Patch Set 6 : Made Windows-specific name; changed the logic polarity. #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 39 (10 generated)
sorin@chromium.org changed reviewers: + cpu@chromium.org, waffles@chromium.org
lgtm but see comment https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) Do we need to do anything for VERSION_SERVER_2003 (includes 64-bit XP)?
https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:83: const base::win::OSInfo* os_info = base::win::OSInfo::GetInstance(); no need to cache with os_info. GetInstance has it cached. https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) use (version() < base::win::VERSION_VISTA)
https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) On 2014/09/17 21:26:45, waffles wrote: > Do we need to do anything for VERSION_SERVER_2003 (includes 64-bit XP)? We could, I hope cpu@ helps me clarify the condition.
lets not do anything for the server OS. They need to upgrade to get sha2 support. We are helping the xp users just because we know that many fear the WGA cop. This should be the true for people with server OS.
Thank you, no changes made, PTAL. I'll make code changes if needed. I just want to double check. https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:83: const base::win::OSInfo* os_info = base::win::OSInfo::GetInstance(); On 2014/09/17 21:28:18, cpu wrote: > no need to cache with os_info. GetInstance has it cached. Agreed on caching. The local variable is to make the call site prettier. Plus, it might save a function call to GetInstance, since that stuff is not inlined and might or might not get PGO. I could rewrite the code if you'd like me to. https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) On 2014/09/17 21:28:18, cpu wrote: > use (version() < base::win::VERSION_VISTA) > > Will it be possible that catches some other previous versions that we want to support but the condition rejects? There is also a test for the SP. The way the code is written makes the SP test apply to XP only. With this proposed change, the SP applies to anything less than VISTA. This might be a pedantic observation. I will make the change if needed.
Also, do we need to handle XP 64bit, as suggested by waffles@
Uploaded to reflect the last feedback from cpu@.
sorin@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/581803002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/20001/chrome/browser/component... chrome/browser/component_updater/chrome_component_updater_configurator.cc:81: bool CanUseAltUrlSource() { This code is also in SSLErrorClassification::IsWindowsVersionSP3OrLower and also in http://crrev.com/561613002 sha256_legacy_support_win.cc - consider moving to a central place and calling from all three locations. This would also allow us to upgrade the function later to perform an actual query to CAPI to get SHA-256 capabilities (for the Windows Server 2003 edge cases).
Will, I attempted to refactor the common code, it is somehow different that what we've agreed on over chat. I still want to review and test more tomorrow, but I'd like you to take a look and see if it is going in the right direction. Thank you!
lgtm w/nit feel free to raise a bug to make MaybeHasSHA256Support() less "maybe" and more "probably" and assign it to me. https://codereview.chromium.org/581803002/diff/60001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/60001/base/win/win_util.h#newc... base/win/win_util.h:140: // may be re-implemented i nthe future to return a reliable value, based on nit: typo
https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:474: MaybeWindowsHasSHA256Support()) { Both SSLErrorClassification::IsWindowsVersionSP3OrLower and SSLErrorClassification::MaybeWindowsHasSHA256Support return false if you call them not on Windows, but you introduced a "!" here. I believe this changes the behavior for non-Windows. In general, what does it mean to call "SSLErrorClassification::MaybeWindowsHasSHA256Support" if you're not running Windows? Maybe the function should just be "MaybeHasSHA256Support"?
sorin@chromium.org changed reviewers: + agl@chromium.org
Thank you! agl@, I need an owner approval for /chrome/browser/ssl/ changes. cpu@, I need an owner approval for /base/win/ changes. wfh@, please PTAL fot the overall sanity of the change. https://codereview.chromium.org/581803002/diff/60001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/60001/base/win/win_util.h#newc... base/win/win_util.h:140: // may be re-implemented i nthe future to return a reliable value, based on On 2014/09/18 17:35:22, Will Harris wrote: > nit: typo Done. https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:474: MaybeWindowsHasSHA256Support()) { On 2014/09/18 18:00:06, waffles wrote: > Both SSLErrorClassification::IsWindowsVersionSP3OrLower and > SSLErrorClassification::MaybeWindowsHasSHA256Support return false if you call > them not on Windows, but you introduced a "!" here. I believe this changes the > behavior for non-Windows. > > In general, what does it mean to call > "SSLErrorClassification::MaybeWindowsHasSHA256Support" if you're not running > Windows? Maybe the function should just be "MaybeHasSHA256Support"? Done.
lgtm
felt@chromium.org changed reviewers: + felt@chromium.org
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
c/b/ssl lgtm
cpu@chromium.org changed reviewers: - rsleevi@chromium.org
lgtm for base\win and CUS
sorin@chromium.org changed reviewers: - agl@chromium.org
lgtm
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( This check displays the string IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, which says: Your computer is running an old version of Microsoft Windows that cannot process this website's security certificate. Because of this problem, Chromium can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to a more recent version of Windows. Is that still correct, given the new semantics of MaybeOSHasSHA256Support()? https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:370: bool SSLErrorClassification::MaybeOSHasSHA256Support() { this is still specific to Windows, not any OS. (e.g., this doesn't check Android). can you therefore name it accordingly, with Windows in the name please?
Thank you. I did not make any code changes in response to the last batch of comments. I am looking for guidance from the /chrome/browser/ssl owner on how to handle that SSL error condition, if needed. Naming aside, the code behaves as before. Ideas? https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( On 2014/09/18 18:30:36, felt wrote: > This check displays the string IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, which > says: > > Your computer is running an old version of Microsoft Windows that cannot process > this website's security certificate. Because of this problem, Chromium can't > tell whether the certificate came from <ph > name="SITE">$1<ex>google.com</ex></ph> or from someone on your network > pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your > computer to a more recent version of Windows. > > Is that still correct, given the new semantics of MaybeOSHasSHA256Support()? The observable behavior of the code should be the same as before. The semantics of the call are unchanged. Since this it appears this is Windows specific, should I introduce a platform specific compile time check to only run this code on OS_WIN? https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:370: bool SSLErrorClassification::MaybeOSHasSHA256Support() { On 2014/09/18 18:30:36, felt wrote: > this is still specific to Windows, not any OS. (e.g., this doesn't check > Android). can you therefore name it accordingly, with Windows in the name > please? I believe that the assumption here is that the non-Windows OSes have likely SHA-256 support.
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( On 2014/09/18 18:41:07, Sorin Jianu wrote: > On 2014/09/18 18:30:36, felt wrote: > > This check displays the string IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, which > > says: > > > > Your computer is running an old version of Microsoft Windows that cannot > process > > this website's security certificate. Because of this problem, Chromium can't > > tell whether the certificate came from <ph > > name="SITE">$1<ex>google.com</ex></ph> or from someone on your network > > pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update > your > > computer to a more recent version of Windows. > > > > Is that still correct, given the new semantics of MaybeOSHasSHA256Support()? > > The observable behavior of the code should be the same as before. The semantics > of the call are unchanged. Since this it appears this is Windows specific, > should I introduce a platform specific compile time check to only run this code > on OS_WIN? No, if the behavior is the same, then that's fine. https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:370: bool SSLErrorClassification::MaybeOSHasSHA256Support() { On 2014/09/18 18:41:07, Sorin Jianu wrote: > On 2014/09/18 18:30:36, felt wrote: > > this is still specific to Windows, not any OS. (e.g., this doesn't check > > Android). can you therefore name it accordingly, with Windows in the name > > please? > > I believe that the assumption here is that the non-Windows OSes have likely > SHA-256 support. Except apparently old Android doesn't. It would help me remember what this method does if the name makes it clear that it's specifically about Windows.
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( On 2014/09/18 18:44:27, felt wrote: > On 2014/09/18 18:41:07, Sorin Jianu wrote: > > On 2014/09/18 18:30:36, felt wrote: > > > This check displays the string IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, > which > > > says: > > > > > > Your computer is running an old version of Microsoft Windows that cannot > > process > > > this website's security certificate. Because of this problem, Chromium can't > > > tell whether the certificate came from <ph > > > name="SITE">$1<ex>google.com</ex></ph> or from someone on your network > > > pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update > > your > > > computer to a more recent version of Windows. > > > > > > Is that still correct, given the new semantics of MaybeOSHasSHA256Support()? > > > > The observable behavior of the code should be the same as before. The > semantics > > of the call are unchanged. Since this it appears this is Windows specific, > > should I introduce a platform specific compile time check to only run this > code > > on OS_WIN? > > No, if the behavior is the same, then that's fine. (Please feel free to add the platform specific compile time check if you'd like though, I don't object to it.)
ok, so I've tried many things until I settled for this version of the code, which resembles a lot what had been there before my changes, modulo the function name change and moving the code out to a helper function in base::win felt, are you ok with this?
Thank you, I appreciate it! https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h#new... base/win/win_util.h:137: // Returns true is the current operating system has support for SHA-256 nit: "Returns true if"
lgtm
Thank you all! https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h#new... base/win/win_util.h:137: // Returns true is the current operating system has support for SHA-256 On 2014/09/18 21:42:44, felt wrote: > nit: "Returns true if" Done.
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581803002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581803002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 3366aad2feba9868f2a40529edbae88ddcf2e58b
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c512b7a845d12db2eb9d573f14aab6577c7509f7 Cr-Commit-Position: refs/heads/master@{#295633} |