Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2350483002:
Improve error pages on client certificate failures. (Closed)
DescriptionImprove error pages on client certificate failures.
This makes the following changes:
- Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both
invalid and missing client certs. net_error_list.h actually already
documents it for this case, but the user-visible error does not.
- Since TLS lacks a dedicated alert for missing client certificates,
servers often send a generic handshake_failure alert. Detect this case
and map to ERR_BAD_SSL_CLIENT_AUTH_CERT.
- One of the many bad client cert alerts is access_denied.
Unfortunately, that was badly named so firewalls sometimes send it,
giving users a confusing error page. Detect this case too and map it
to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected
alert.
Add a bunch of tests for these cases.
Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT:
https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing
BUG=630883, 646567
Committed: https://crrev.com/fe132d9d1a03966649118030e50715d1a063f73c
Cr-Commit-Position: refs/heads/master@{#421265}
Patch Set 1 #Patch Set 2 #
Total comments: 4
Patch Set 3 : tweak text #Patch Set 4 : revise per meeting #
Total comments: 2
Patch Set 5 : curly quotes #
Messages
Total messages: 35 (22 generated)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's error be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Add a reload button and a suggestion to "Reload with a valid login certificate" on ERR_BAD_SSL_CLIENT_AUTH_CERT. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these caes. BUG=630883,646567 ========== to ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Add a reload button and a suggestion to "Reload with a valid login certificate" on ERR_BAD_SSL_CLIENT_AUTH_CERT. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ==========
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
davidben@chromium.org changed reviewers: + edwardjung@chromium.org, svaldez@chromium.org
edwardjung: How do you feel about this change UI-wise? Unfortunately, it's hard to be less obtuse than "reloading with a valid login certificate" since how you get those is different for each institution. :-( svaldez: For net/ changes.
https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... components/error_page_strings.grdp:258: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph> requested a login certificate. It may not have been provided or your login certificate may have expired or be invalid. Hrm. Probably should do "It" => "One"? https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... components/error_page_strings.grdp:336: <message name="IDS_ERRORPAGES_SUGGESTION_CLIENT_CERTIFICATE" desc="The message displayed in a list of suggestions following a network error suggesting to the user to reload with a valid client certificate. The suggestions list is prefixed with 'Try:'."> I used CLIENT instead of LOGIN in the code to be consistent elsewhere. We seem to always say CLIENT in code and the 'desc' field elsewhere also says "client certificate" with only the messages themselves saying "login certificate".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
net/ lgtm https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... components/error_page_strings.grdp:258: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph> requested a login certificate. It may not have been provided or your login certificate may have expired or be invalid. On 2016/09/16 22:06:13, davidben wrote: > Hrm. Probably should do "It" => "One"? Maybe "One may not have been provided or your login certificate may be expired or invalid."
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2350483002/diff/20001/components/error_page_s... components/error_page_strings.grdp:258: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph> requested a login certificate. It may not have been provided or your login certificate may have expired or be invalid. On 2016/09/19 20:43:15, svaldez wrote: > On 2016/09/16 22:06:13, davidben wrote: > > Hrm. Probably should do "It" => "One"? > > Maybe "One may not have been provided or your login certificate may be expired > or invalid." Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Apologies for the slow response David, just waiting on a response from rachelis. The wording feels a little cumbersome so was wondering if there was a more succinct way of putting it. rachelis has better wordsmithing skills than myself. On 2016/09/16 22:04:36, davidben wrote: > edwardjung: How do you feel about this change UI-wise? Unfortunately, it's hard > to be less obtuse than "reloading with a valid login certificate" since how you > get those is different for each institution. :-( > > svaldez: For net/ changes.
On 2016/09/20 09:19:31, edwardjung wrote: > Apologies for the slow response David, just waiting on a response from rachelis. > The wording feels a little cumbersome so was wondering if there was a more > succinct way of putting it. rachelis has better wordsmithing skills than myself. Sure. No rush. If it makes it easier to wordsmith, we can split the "no certificate was provided" and "certificate was provided, but server didn't like it" cases into two separate errors without difficulties on the //net side.
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Add a reload button and a suggestion to "Reload with a valid login certificate" on ERR_BAD_SSL_CLIENT_AUTH_CERT. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ========== to ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ==========
edwardjung: Updated the CL per our discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM, with one typographical nit. https://codereview.chromium.org/2350483002/diff/60001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2350483002/diff/60001/components/error_page_s... components/error_page_strings.grdp:258: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph> didn't accept your login certificate, or one may not have been provided. Could we use the curly quote mark in place of the apostrophe in didn’t as used previously. For typographical presentation.
davidben@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for error_page_strings.grdp https://codereview.chromium.org/2350483002/diff/60001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2350483002/diff/60001/components/error_page_s... components/error_page_strings.grdp:258: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph> didn't accept your login certificate, or one may not have been provided. On 2016/09/27 08:54:51, edwardjung wrote: > Could we use the curly quote mark in place of the apostrophe in didn’t as used > previously. For typographical presentation. Done.
LGTM, deferring to Edward.
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org, edwardjung@chromium.org Link to the patchset: https://codereview.chromium.org/2350483002/#ps80001 (title: "curly quotes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ========== to ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 ========== to ========== Improve error pages on client certificate failures. This makes the following changes: - Tweak ERR_BAD_SSL_CLIENT_AUTH_CERT's text be suitable for both invalid and missing client certs. net_error_list.h actually already documents it for this case, but the user-visible error does not. - Since TLS lacks a dedicated alert for missing client certificates, servers often send a generic handshake_failure alert. Detect this case and map to ERR_BAD_SSL_CLIENT_AUTH_CERT. - One of the many bad client cert alerts is access_denied. Unfortunately, that was badly named so firewalls sometimes send it, giving users a confusing error page. Detect this case too and map it to ERR_SSL_PROTOCOL_ERROR, the same error we would use for an unexpected alert. Add a bunch of tests for these cases. Screenshot of the new ERR_BAD_SSL_CLIENT_AUTH_CERT: https://drive.google.com/file/d/0Bz14lOW5Hke4ZzRPLXZwUUlzOFk/view?usp=sharing BUG=630883,646567 Committed: https://crrev.com/fe132d9d1a03966649118030e50715d1a063f73c Cr-Commit-Position: refs/heads/master@{#421265} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fe132d9d1a03966649118030e50715d1a063f73c Cr-Commit-Position: refs/heads/master@{#421265} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
