Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
465133004:
Remove DenyCertForHost from SSLHostStateDelegate API. (Closed)
|
Created:
6 years, 4 months ago by jww Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, markusheintz_, jam, felt Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRemove DenyCertForHost from SSLHostStateDelegate API.
The DENIED judgement set by DenyCertForHost is never actually used to
make any interstitial decisions (it is, in fact, explicitly treated as
unknown). To simplify the API and make it cleaner, this change removes
DenyCertForHost from ssl_policy_backend.h and ssl_host_state_delegate.h
and the related classes.
BUG=402764
Committed: https://crrev.com/6a55df7b58ee45c958df5ad3e18f679d2c464106
Cr-Commit-Position: refs/heads/master@{#293569}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Adressed comments from pkasting #
Total comments: 2
Patch Set 3 : Style nit #
Total comments: 6
Patch Set 4 : Removed UNKNOWN and CertPolicy #
Total comments: 2
Patch Set 5 : Removed references to UNKNOWN #Patch Set 6 : Rebase on ToT #
Total comments: 10
Patch Set 7 : Sleevi comments #
Total comments: 10
Patch Set 8 : More sleevi comments #Patch Set 9 : Rebase on ToT #Messages
Total messages: 34 (5 generated)
pkasting@, this is the API change to remove the unnecessary "Deny" methods.
https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:348: // Chrome we treats DENIED values as UNKNOWN. Can we get rid of the concept of a DENIED value? https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:24: // session restarts), based on command line flags. Nit: How about this comment: Tracks whether the user has allowed a certificate error exception for a specific site, SSL fingerprint, and error. Based on command-line flags, remembers this decision either until end-of-session or for a particular length of time. https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:46: // Revoke all user decisions for |host| in the given Profile. The Nit: While here: Revoke -> Revokes; consider changing names/comments to be less vague than "user decision" https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: // |host|, otherwise false. Nit: How about: Returns whether the user has allowed a certificate error exception for |host|. https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:88: // Query the content settings to retrieve a dictionary of certificate Nit: Same sorts of comments as above. https://codereview.chromium.org/465133004/diff/1/content/browser/ssl/ssl_poli... File content/browser/ssl/ssl_policy_backend.cc (right): https://codereview.chromium.org/465133004/diff/1/content/browser/ssl/ssl_poli... content/browser/ssl/ssl_policy_backend.cc:46: if (!ssl_host_state_delegate_) Nit: This whole function could just be: return ssl_host_state_delegate_ ? ssl_host_state_delegate_->QueryPolicy(host, cert, error, expired_previous_decision) : net::CertPolicy::UNKNOWN; I don't think the DCHECK is really worth the extra verbiage.
https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:348: // Chrome we treats DENIED values as UNKNOWN. On 2014/08/22 00:57:52, Peter Kasting wrote: > Can we get rid of the concept of a DENIED value? I don't think so; they're used by net/. We could make a new enum just for SSLHostStateDelegate purposes, but that adds an extra layer of translations to net::CertPolicy::Judgements that we'd have to add. I did modify this comment slightly to make it clearer that it will only be ALLOWED or UNKNOWN. https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:24: // session restarts), based on command line flags. On 2014/08/22 00:57:52, Peter Kasting wrote: > Nit: How about this comment: > > Tracks whether the user has allowed a certificate error exception for a specific > site, SSL fingerprint, and error. Based on command-line flags, remembers this > decision either until end-of-session or for a particular length of time. Done, with slight addition of "...and experimental group..." https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:46: // Revoke all user decisions for |host| in the given Profile. The On 2014/08/22 00:57:52, Peter Kasting wrote: > Nit: While here: Revoke -> Revokes; consider changing names/comments to be less > vague than "user decision" Revoke -> Revokes done. I took a whack at improving the comment; let me know what you think. https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: // |host|, otherwise false. On 2014/08/22 00:57:53, Peter Kasting wrote: > Nit: How about: > > Returns whether the user has allowed a certificate error exception for |host|. Done. https://codereview.chromium.org/465133004/diff/1/chrome/browser/ssl/chrome_ss... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:88: // Query the content settings to retrieve a dictionary of certificate On 2014/08/22 00:57:52, Peter Kasting wrote: > Nit: Same sorts of comments as above. Done. https://codereview.chromium.org/465133004/diff/1/content/browser/ssl/ssl_poli... File content/browser/ssl/ssl_policy_backend.cc (right): https://codereview.chromium.org/465133004/diff/1/content/browser/ssl/ssl_poli... content/browser/ssl/ssl_policy_backend.cc:46: if (!ssl_host_state_delegate_) On 2014/08/22 00:57:53, Peter Kasting wrote: > Nit: This whole function could just be: > > return ssl_host_state_delegate_ ? > ssl_host_state_delegate_->QueryPolicy(host, cert, error, > expired_previous_decision) : > net::CertPolicy::UNKNOWN; > > I don't think the DCHECK is really worth the extra verbiage. Done.
LGTM https://codereview.chromium.org/465133004/diff/20001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/465133004/diff/20001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:577: InRememberCertificateErrorDecisionsGroup(); Old indenting was correct.
https://codereview.chromium.org/465133004/diff/20001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/465133004/diff/20001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:577: InRememberCertificateErrorDecisionsGroup(); On 2014/08/22 04:45:29, Peter Kasting wrote: > Old indenting was correct. Done.
jam@, can you double check the content/ changes? rsleei@, can you double check the chrome/*ssl* changes?
https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (left): https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:187: backend_->DenyCertForHost(handler->ssl_info().cert.get(), You said this code isn't called, but this calls DenyCert, so it seems it was.
https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (left): https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:187: backend_->DenyCertForHost(handler->ssl_info().cert.get(), On 2014/08/23 01:33:20, Ryan Sleevi wrote: > You said this code isn't called, but this calls DenyCert, so it seems it was. In the description, I actually wrote that it's never "functionally used." A fine line, and I'll try to clarify the comment because it is, indeed, confusing. Yes, this calls DenyCert which, in the storage strategy, stores the decision as "DENIED". However, the only place QueryPolicy is ever called (which is the only way to retrieve this information) is right here in ssl_policy.cc through ssl_policy_backend, where there's an explicit comment that DENIED will be treated as UNKNOWN (see line 48 above). Hence the desire to get rid of this confusing portion of the SSLHostStateDelegate API that implies that DENIED is meaningful, but in practice, isn't.
lgtm
The great part about deleting code is you often get to delete even more. Holding back the LG in order for you to knock out some more code. https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (left): https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:187: backend_->DenyCertForHost(handler->ssl_info().cert.get(), On 2014/08/23 14:14:42, jww wrote: > On 2014/08/23 01:33:20, Ryan Sleevi wrote: > > You said this code isn't called, but this calls DenyCert, so it seems it was. > > In the description, I actually wrote that it's never "functionally used." A fine > line, and I'll try to clarify the comment because it is, indeed, confusing. > > Yes, this calls DenyCert which, in the storage strategy, stores the decision as > "DENIED". However, the only place QueryPolicy is ever called (which is the only > way to retrieve this information) is right here in ssl_policy.cc through > ssl_policy_backend, where there's an explicit comment that DENIED will be > treated as UNKNOWN (see line 48 above). Hence the desire to get rid of this > confusing portion of the SSLHostStateDelegate API that implies that DENIED is > meaningful, but in practice, isn't. So then shouldn't you also be removing that enum value from the CertPolicy? And if you do that, does it still make sense as an enum when it's just boolean (UNKNOWN, aka "!ALLOWED", and ALLOWED). I'm happy to leave it as an enum, but that means updating the comment.
https://codereview.chromium.org/465133004/diff/40001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:348: // we treats DENIED values as UNKNOWN. nit: "Chrome we treats" Also: can we get rid of UNKNOWN? Just have ALLOWED or DENIED, with DENIED as the default unless overridden by the user?
Perhaps it's crazy post-Burning Man talk, but I've had a vision. Based on feedback, this latest CL gets rid of UNKNOWN and makes DENIED the default. But wait, there's more. I've removed net::CertPolicy completely. The only consumer was ssl_host_state_delegate.h, and I realized that the only thing it used was the enum, so I've moved the enum to ssl_host_state_delegate.h and removed the CertPolicy code completely. One minor note: You'll notice that I switched the order of DENIED and ALLOWED. This is because we want the "0" in the enum, which had been UNKNOWN, to now be DENIED for those instances already out there in the experiment. That is, for any currently running Chrome, when UNKNOWN disappears, if it's stored in content settings and then loaded, it should become DENIED. https://codereview.chromium.org/465133004/diff/40001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:348: // we treats DENIED values as UNKNOWN. On 2014/08/25 17:13:44, felt wrote: > nit: "Chrome we treats" Done > > Also: can we get rid of UNKNOWN? Just have ALLOWED or DENIED, with DENIED as the > default unless overridden by the user? Yeah. pkasting suggested something similar, and now that I've thought about it more, it makes sense to me, so the latest CL does this. https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (left): https://codereview.chromium.org/465133004/diff/40001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:187: backend_->DenyCertForHost(handler->ssl_info().cert.get(), On 2014/08/25 06:17:24, Ryan Sleevi wrote: > On 2014/08/23 14:14:42, jww wrote: > > On 2014/08/23 01:33:20, Ryan Sleevi wrote: > > > You said this code isn't called, but this calls DenyCert, so it seems it > was. > > > > In the description, I actually wrote that it's never "functionally used." A > fine > > line, and I'll try to clarify the comment because it is, indeed, confusing. > > > > Yes, this calls DenyCert which, in the storage strategy, stores the decision > as > > "DENIED". However, the only place QueryPolicy is ever called (which is the > only > > way to retrieve this information) is right here in ssl_policy.cc through > > ssl_policy_backend, where there's an explicit comment that DENIED will be > > treated as UNKNOWN (see line 48 above). Hence the desire to get rid of this > > confusing portion of the SSLHostStateDelegate API that implies that DENIED is > > meaningful, but in practice, isn't. > > So then shouldn't you also be removing that enum value from the CertPolicy? > > And if you do that, does it still make sense as an enum when it's just boolean > (UNKNOWN, aka "!ALLOWED", and ALLOWED). I'm happy to leave it as an enum, but > that means updating the comment. I'll up you one. CertPolicy is dead. It's completely unused at this point. I still like having an enum to make it clear and explicit about what's going on, but I've move it to ssl_host_state_delegate.h, and I've removed net::CertPolicy completely.
https://codereview.chromium.org/465133004/diff/60001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/465133004/diff/60001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:48: // The judgment must be UNKNOWN because QueryPolicy guarantees that it will Update all comments that talk about UNKNOWN. (There are several across a number of files.)
https://codereview.chromium.org/465133004/diff/60001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/465133004/diff/60001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:48: // The judgment must be UNKNOWN because QueryPolicy guarantees that it will On 2014/09/03 23:02:55, Peter Kasting wrote: > Update all comments that talk about UNKNOWN. (There are several across a number > of files.) Whoops. Thought I had gotten them, but clearly not. Done.
LGTM
https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:54: // implementation for more information. These last two sentences are 'layering' violations, in that a good comment should try to not focus on how it's used/called, but how it should/can be used/called. I feel like they don't add much value, and you're better off documenting any concerns here in the ExceptionsHard call site. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:268: GURL url = GetSecureGURLForHost(host); how does this (or not) handle websockets? (wss:// vs https://) I believe the *intent* is that wss:// exceptions behave the same as https:// exceptions, but since you're synthesizing to a ContentSettingsPattern, I want to make sure that the scheme consideration in content settings takes this into consideration. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:43: // ChromeSSLHostStateDelegate implementation: Revokes all SSL certificate 1) Drop the "// ChromeSSLHostDate delegate implementation:" comment 2) Split the documentation for RevokeUser and RevokeUser...hard to the associated methods. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: virtual bool HasAllowed(const std::string& host); HasAllowed -> HasAllowException ? symmetry with your previous two methods. This documentation could use beefing up, particularly in respect to how it plays with QueryPolicy. Namely, HasAllowException doesn't mean any certs are allowed, you still have to call QueryPolicy. It's merely a sign that SOME error may be allowed. https://codereview.chromium.org/465133004/diff/100001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/465133004/diff/100001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:49: // user every time he comes back to the page. s/he comes/they come/
https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:54: // implementation for more information. On 2014/09/04 00:17:25, Ryan Sleevi wrote: > These last two sentences are 'layering' violations, in that a good comment > should try to not focus on how it's used/called, but how it should/can be > used/called. > > I feel like they don't add much value, and you're better off documenting any > concerns here in the ExceptionsHard call site. Done. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:268: GURL url = GetSecureGURLForHost(host); On 2014/09/04 00:17:25, Ryan Sleevi wrote: > how does this (or not) handle websockets? (wss:// vs https://) > > I believe the *intent* is that wss:// exceptions behave the same as https:// > exceptions, but since you're synthesizing to a ContentSettingsPattern, I want to > make sure that the scheme consideration in content settings takes this into > consideration. Great question. Yes, wss:// is taken care of by all of this. In fact, there was a bug earlier where wss:// was *not* taken care of, back when we passed around GURLs instead of just host names, and there was a unit test somewhere (I don't remember right now where) that caught that problem. Thus, now, we just query about hosts, which is why we make the GetSecureGURLForHost call to create a content setting friendly GURL. So in short, wss:// is most definitely taken care of. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:43: // ChromeSSLHostStateDelegate implementation: Revokes all SSL certificate On 2014/09/04 00:17:25, Ryan Sleevi wrote: > 1) Drop the "// ChromeSSLHostDate delegate implementation:" comment > > 2) Split the documentation for RevokeUser and RevokeUser...hard to the > associated methods. Done. https://codereview.chromium.org/465133004/diff/100001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: virtual bool HasAllowed(const std::string& host); On 2014/09/04 00:17:25, Ryan Sleevi wrote: > HasAllowed -> HasAllowException ? > > symmetry with your previous two methods. > > This documentation could use beefing up, particularly in respect to how it plays > with QueryPolicy. Namely, HasAllowException doesn't mean any certs are allowed, > you still have to call QueryPolicy. It's merely a sign that SOME error may be > allowed. Done. https://codereview.chromium.org/465133004/diff/100001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/465133004/diff/100001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:49: // user every time he comes back to the page. On 2014/09/04 00:17:25, Ryan Sleevi wrote: > s/he comes/they come/ Done.
lgtm https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:46: // RevokeUserAllowExceptionsHard is the same as RevokeUserAllowExceptions but linebreak between the two methods https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:47: // additionally may close idle connections in the process. This version should s/version// https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: // just that there exists an exception. To see if a particular certificate and s/allowed but just that/allowed, just that/ https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:55: // error combination exception is allowed, QueryPolicy must be used. s/QueryPolicy must be used/use QueryPolicy()/
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/465133004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:
While running git apply --index -p1;
error: patch failed: chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:262
error: chrome/browser/ssl/chrome_ssl_host_state_delegate.cc: patch does not
apply
Patch: chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
Index: chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
index
c306b22f0a9295b7f8e582a8e977fa859be4bc42..86ac4bbec0a79bf0fb1cfe9c3a1a7c29bf300a54
100644
--- a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
+++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
@@ -38,7 +38,7 @@ const char
kRememberCertificateErrorDecisionsFieldTrialDefaultGroup[] =
"Default";
const char kRememberCertificateErrorDecisionsFieldTrialLengthParam[] =
"length";
-// Keys for the per-site error + certificate finger to judgement content
+// Keys for the per-site error + certificate finger to judgment content
// settings map.
const char kSSLCertDecisionCertErrorMapKey[] = "cert_exceptions_map";
const char kSSLCertDecisionExpirationTimeKey[] = "decision_expiration_time";
@@ -46,12 +46,6 @@ const char kSSLCertDecisionVersionKey[] = "version";
const int kDefaultSSLCertDecisionVersion = 1;
-// Closes all idle network connections for the given URLRequestContext. This is
-// a big hammer and should be wielded with extreme caution as it can have a
big,
-// negative impact on network performance. In this case, it is used by
-// RevokeUserDecisionsHard, which should only be called by rare, user initiated
-// events. See the comment before RevokeUserDecisionsHard implementation for
-// more information.
void CloseIdleConnections(
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
url_request_context_getter->
@@ -262,16 +256,43 @@ ChromeSSLHostStateDelegate::~ChromeSSLHostStateDelegate()
{
Clear();
}
-void ChromeSSLHostStateDelegate::DenyCert(const std::string& host,
- net::X509Certificate* cert,
- net::CertStatus error) {
- ChangeCertPolicy(host, cert, error, net::CertPolicy::DENIED);
-}
-
void ChromeSSLHostStateDelegate::AllowCert(const std::string& host,
net::X509Certificate* cert,
net::CertStatus error) {
- ChangeCertPolicy(host, cert, error, net::CertPolicy::ALLOWED);
+ GURL url = GetSecureGURLForHost(host);
+ const ContentSettingsPattern pattern =
+ ContentSettingsPattern::FromURLNoWildcard(url);
+ HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+ scoped_ptr<base::Value> value(map->GetWebsiteSetting(
+ url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, std::string(),
NULL));
+
+ if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
+ value.reset(new base::DictionaryValue());
+
+ base::DictionaryValue* dict;
+ bool success = value->GetAsDictionary(&dict);
+ DCHECK(success);
+
+ bool expired_previous_decision; // unused value in this function
+ base::DictionaryValue* cert_dict = GetValidCertDecisionsDict(
+ dict, CreateDictionaryEntries, &expired_previous_decision);
+ // If a a valid certificate dictionary cannot be extracted from the content
+ // setting, that means it's in an unknown format. Unfortunately, there's
+ // nothing to be done in that case, so a silent fail is the only option.
+ if (!cert_dict)
+ return;
+
+ dict->SetIntegerWithoutPathExpansion(kSSLCertDecisionVersionKey,
+ kDefaultSSLCertDecisionVersion);
+ cert_dict->SetIntegerWithoutPathExpansion(GetKey(cert, error), ALLOWED);
+
+ // The map takes ownership of the value, so it is released in the call to
+ // SetWebsiteSetting.
+ map->SetWebsiteSetting(pattern,
+ pattern,
+ CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS,
+ std::string(),
+ value.release());
}
void ChromeSSLHostStateDelegate::Clear() {
@@ -279,11 +300,11 @@ void ChromeSSLHostStateDelegate::Clear() {
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS);
}
-net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
- const std::string& host,
- net::X509Certificate* cert,
- net::CertStatus error,
- bool* expired_previous_decision) {
+content::SSLHostStateDelegate::CertJudgment
+ChromeSSLHostStateDelegate::QueryPolicy(const std::string& host,
+ net::X509Certificate* cert,
+ net::CertStatus error,
+ bool* expired_previous_decision) {
HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
GURL url = GetSecureGURLForHost(host);
scoped_ptr<base::Value> value(map->GetWebsiteSetting(
@@ -293,7 +314,7 @@ net::CertPolicy::Judgment
ChromeSSLHostStateDelegate::QueryPolicy(
// full query.
*expired_previous_decision = false;
if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
- return net::CertPolicy::UNKNOWN;
+ return DENIED;
base::DictionaryValue* dict; // Owned by value
int policy_decision;
@@ -306,24 +327,23 @@ net::CertPolicy::Judgment
ChromeSSLHostStateDelegate::QueryPolicy(
if (!cert_error_dict) {
// This revoke is necessary to clear any old expired setting that may
// lingering in the case that an old decision expried.
- RevokeUserDecisions(host);
- return net::CertPolicy::UNKNOWN;
+ RevokeUserAllowExceptions(host);
+ return DENIED;
}
success = cert_error_dict->GetIntegerWithoutPathExpansion(GetKey(cert,
error),
&policy_decision);
// If a policy decision was successfully retrieved and it's a valid value of
- // ALLOWED or DENIED, return the valid value. Otherwise, return UNKNOWN.
- if (success && policy_decision == net::CertPolicy::Judgment::ALLOWED)
- return net::CertPolicy::Judgment::ALLOWED;
- else if (success && policy_decision == net::CertPolicy::Judgment::DENIED)
- return net::CertPolicy::Judgment::DENIED;
+ // ALLOWED, return the valid value. Otherwise, return DENIED.
+ if (success && policy_decision == ALLOWED)
+ return ALLOWED;
- return net::CertPolicy::Judgment::UNKNOWN;
+ return DENIED;
}
-void ChromeSSLHostStateDelegate::RevokeUserDecisions(const std::string& host) {
+void ChromeSSLHostStateDelegate::RevokeUserAllowExceptions(
+ const std::string& host) {
GURL url = GetSecureGURLForHost(host);
const ContentSettingsPattern pattern =
ContentSettingsPattern::FromURLNoWildcard(url);
@@ -348,19 +368,20 @@ void ChromeSSLHostStateDelegate::RevokeUserDecisions(const
std::string& host) {
// showing the interstitial. We probably need to introduce into the networking
// stack a way revoke SSLConfig's allowed_bad_certs lists per socket.
//
-// For now, RevokeUserDecisionsHard is our solution for the rare case where it
-// is necessary to revoke the preferences immediately. It does so by flushing
-// idle sockets.
-void ChromeSSLHostStateDelegate::RevokeUserDecisionsHard(
+// For now, RevokeUserAllowExceptionsHard is our solution for the rare case
+// where it is necessary to revoke the preferences immediately. It does so by
+// flushing idle sockets, thus it is a big hammer and should be wielded with
+// extreme caution as it can have a big, negative impact on network
performance.
+void ChromeSSLHostStateDelegate::RevokeUserAllowExceptionsHard(
const std::string& host) {
- RevokeUserDecisions(host);
+ RevokeUserAllowExceptions(host);
scoped_refptr<net::URLRequestContextGetter> getter(
profile_->GetRequestContext());
profile_->GetRequestContext()->GetNetworkTaskRunner()->PostTask(
FROM_HERE, base::Bind(&CloseIdleConnections, getter));
}
-bool ChromeSSLHostStateDelegate::HasUserDecision(const std::string& host) {
+bool ChromeSSLHostStateDelegate::HasAllowException(const std::string& host) {
GURL url = GetSecureGURLForHost(host);
const ContentSettingsPattern pattern =
ContentSettingsPattern::FromURLNoWildcard(url);
@@ -379,8 +400,7 @@ bool ChromeSSLHostStateDelegate::HasUserDecision(const
std::string& host) {
for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance())
{
int policy_decision; // Owned by dict
success = it.value().GetAsInteger(&policy_decision);
- if (success && (static_cast<net::CertPolicy::Judgment>(policy_decision) !=
- net::CertPolicy::UNKNOWN))
+ if (success && (static_cast<CertJudgment>(policy_decision) == ALLOWED))
return true;
}
@@ -400,44 +420,3 @@ bool ChromeSSLHostStateDelegate::DidHostRunInsecureContent(
void ChromeSSLHostStateDelegate::SetClock(scoped_ptr<base::Clock> clock) {
clock_.reset(clock.release());
}
-
-void ChromeSSLHostStateDelegate::ChangeCertPolicy(
- const std::string& host,
- net::X509Certificate* cert,
- net::CertStatus error,
- net::CertPolicy::Judgment judgment) {
- GURL url = GetSecureGURLForHost(host);
- const ContentSettingsPattern pattern =
- ContentSettingsPattern::FromURLNoWildcard(url);
- HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
- scoped_ptr<base::Value> value(map->GetWebsiteSetting(
- url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, std::string(),
NULL));
-
- if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
- value.reset(new base::DictionaryValue());
-
- base::DictionaryValue* dict;
- bool success = value->GetAsDictionary(&dict);
- DCHECK(success);
-
- bool expired_previous_decision; // unused value in this function
- base::DictionaryValue* cert_dict = GetValidCertDecisionsDict(
- dict, CreateDictionaryEntries, &expir…
(message too large)
Late to the party, but this is a nice change - thanks Joel. I notice however that this is being sent to the CQ without resolving Sleevi's last 4 comments.
lgtm https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:329: // lingering in the case that an old decision expried. nit: "setting that may be lingering"
On 2014/09/05 07:06:52, felt wrote: > Late to the party, but this is a nice change - thanks Joel. > > I notice however that this is being sent to the CQ without resolving Sleevi's > last 4 comments. Yeah, I think I ran git cl set_commit from the wrong workspace :-P Fortunately, there was no way it was committing given the large conflict it has with https://codereview.chromium.org/441043005/. Don't worry, I've got his comments addressed, and I'll address your nit as well. Sorry for the confusion!
https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:329: // lingering in the case that an old decision expried. On 2014/09/05 07:13:18, felt wrote: > nit: "setting that may be lingering" Done. https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:46: // RevokeUserAllowExceptionsHard is the same as RevokeUserAllowExceptions but On 2014/09/04 19:41:19, Ryan Sleevi wrote: > linebreak between the two methods Done. https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:47: // additionally may close idle connections in the process. This version should On 2014/09/04 19:41:19, Ryan Sleevi wrote: > s/version// Done. https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:54: // just that there exists an exception. To see if a particular certificate and On 2014/09/04 19:41:19, Ryan Sleevi wrote: > s/allowed but just that/allowed, just that/ Done. https://codereview.chromium.org/465133004/diff/120001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:55: // error combination exception is allowed, QueryPolicy must be used. On 2014/09/04 19:41:19, Ryan Sleevi wrote: > s/QueryPolicy must be used/use QueryPolicy()/ Done.
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/465133004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/465133004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as e10405ced9e93f64d0f9bf7c829f190e40097e3d
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6a55df7b58ee45c958df5ad3e18f679d2c464106 Cr-Commit-Position: refs/heads/master@{#293569} |
