Set SSL info when an HTTP auth dialog is triggered by direct navigation.

Set SSL info when an HTTP auth dialog is triggered by direct navigation.

This patch correctly sets the SSL information of a navigation entry when an
HTTP auth dialog is triggered. It only sets the information if the
navigation is initiated by the user (i.e. through the omnibox). It does not
set SSL information if the navigation ending up with the HTTP auth dialog
was triggered by a page redirect, as there is no SiteInstance to get the SSL
information from in that case.

BUG=395050

[palmer, 2015-09-25 01:33:26]

palmer@chromium.org changed reviewers:
+ creis@chromium.org, meacer@chromium.org, nasko@chromium.org

[palmer, 2015-09-25 01:33:26]

This is a rebased and hacked-to-build work in progress forked from meacer's CL
https://codereview.chromium.org/403933002/.

PTAL if you're feeling adventurous; I'll try to beat it into better shape soon.
Some interfaces have changed since meacer's CL.

[meacer, 2015-09-25 18:15:46]

https://codereview.chromium.org/1368863002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1368863002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.cc:590: const content::SSLStatus&
ssl_status = pending_entry->GetSSL();
Charlie and I had a chat about why this using the pending entry was safe here.
There were a couple of open questions still standing in the other CL such as
whether we always have a pending entry by the time we reach here. Unfortunately
I don't remember the discussion in detail :(

[palmer, 2015-09-29 00:07:41]

creis, meacer: I've added some TODO(palmer, creis, meacer) comments in 2 places.
I could use your help resolving them, because I don't understand navigation
basically at all. :)

[meacer, 2015-09-29 00:17:02]

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:135:
&signed_certificate_timestamp_ids);
Lines 132-135 aren't needed anymore now that this is using
GetSSLStatusForRequest, right? It seems like you could remove this method
altogether and use GetSSLStatusForRequest + SerializeSecurityInfo instead.

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:707: security_info =
StoreAndSerializeSecurityInfo(request_->url(), ssl_info,
nit: Braces since this code spans two lines?

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:165:
ssl_status.signed_certificate_timestamp_ids;
estark added a constructor for SSLStatus, this can now simply call it: 

entry->GetSSL() = SSLStatus(...);

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[palmer, 2015-09-29 00:46:36]

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:135:
&signed_certificate_timestamp_ids);
On 2015/09/29 00:17:02, Mustafa Emre Acer wrote:
> Lines 132-135 aren't needed anymore now that this is using
> GetSSLStatusForRequest, right? It seems like you could remove this method
> altogether and use GetSSLStatusForRequest + SerializeSecurityInfo instead.

Done.

https://codereview.chromium.org/1368863002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:707: security_info =
StoreAndSerializeSecurityInfo(request_->url(), ssl_info,
On 2015/09/29 00:17:02, Mustafa Emre Acer wrote:
> nit: Braces since this code spans two lines?

Done.

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:165:
ssl_status.signed_certificate_timestamp_ids;
How shall we get the SecurityStyle argument?

[meacer, 2015-09-29 00:48:51]

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:165:
ssl_status.signed_certificate_timestamp_ids;
On 2015/09/29 00:46:36, palmer wrote:
> How shall we get the SecurityStyle argument?

You can copy directly from ssl_status.security_style (which this code would miss
otherwise, hence the constructor :)

[palmer, 2015-09-29 00:56:46]

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1368863002/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:165:
ssl_status.signed_certificate_timestamp_ids;
On 2015/09/29 00:48:51, Mustafa Emre Acer wrote:
> On 2015/09/29 00:46:36, palmer wrote:
> > How shall we get the SecurityStyle argument?
> 
> You can copy directly from ssl_status.security_style (which this code would
miss
> otherwise, hence the constructor :)

Ah, right. But, now we are missing the SSLInfo... And the callers for this
function don't seem to have one handy.

I'm adding in SecurityStyle and the couple other missing fields in |entry|'s
SSLStatus, though.

[Charlie Reis, 2015-09-29 17:00:39]

On 2015/09/29 00:07:41, palmer wrote:
> creis, meacer: I've added some TODO(palmer, creis, meacer) comments in 2
places.
> I could use your help resolving them, because I don't understand navigation
> basically at all. :)

I'll try to make some time today to take a look.

[Charlie Reis, 2015-09-29 22:52:11]

Unfortunately, I have a lot of the same concerns, and I don't know of a good way
to resolve them the way the code currently works.

The problem is that we're trying to correlate an event in the network stack (on
the IO thread) with a particular NavigationEntry in the navigation stack (on the
UI thread).  There's no breadcrumb that ties these together (and unfortunately
there hasn't been since Chrome launched), and races exist that can cause the
current request to not match the current pending entry.

The same problem has come up in other scenarios as well, such as
DidFailProvisionalLoadWithError.  That causes https://crbug.com/513742 (see
comments 5-8).  There may be a way forward mentioned in that discussion, and I'm
interested in it, but it's a nontrivial change.

https://codereview.chromium.org/1368863002/diff/80001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_interstitial_delegate.cc (right):

https://codereview.chromium.org/1368863002/diff/80001/chrome/browser/ui/login...
chrome/browser/ui/login/login_interstitial_delegate.cc:55: void
LoginInterstitialDelegate::OverrideEntry(content::NavigationEntry* entry) {
I don't see a caller of this, and it looks like a dangerous method if used
incorrectly.  (It could easily lead to spoof attacks if the wrong entry is
passed in.)  Can we remove it?  If not, can we add some way to verify whether
the entry is the intended one, such as at least comparing against the
request_url?

https://codereview.chromium.org/1368863002/diff/80001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1368863002/diff/80001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:593: // gone by now, which might have
been why we couldn't use it.)
Following the trail in codesearch, it looks like LoginDialogCallback is a task
posted to the UI thread via a long chain starting in
ResourceLoader::OnAuthRequired.

That means it's definitely possible for the pending entry to have been discarded
or even replaced for a different navigation by the time we get here.  As a
result, I don't think it's safe to use GetPendingEntry below, at least in the
general case.  

(Some examples that come to mind: the user has just clicked Stop, or the
renderer process initiates a new same-process main frame navigation, or the
renderer process initiates a cross-process subframe navigation.)

Suppose we found a way to check whether the pending entry still matched this
particular request (though I'm not sure yet if that's possible).  How should the
code below behave if there is no pending entry or if it doesn't match?

https://codereview.chromium.org/1368863002/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1368863002/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:110: // one in the meantime.) Ensure this is
safe.
Same concerns.  I don't know how to make this safe.

[palmer, 2017-02-09 22:43:07]

I'm going to close this since the diff probably doesn't apply at all now, and I
don't know how to proceed on it. Also I'm swamped with other stuff at the
moment. Feel free to pick it up...