Fix webui cert viewer showing wrong cert chain on NSS and no chain on OpenSSL.

Fix webui cert viewer showing wrong cert chain on NSS and no chain on OpenSSL.

The webui cert viewer was still using x509_certificate_model::GetCertChainFromCert instead of X509Certificate::GetIntermediateCertificates. This was fixed in the gtk cert viewer in r135231, but the webui cert viewer was missed. (x509_certificate_model::GetCertChainFromCert was not implemented on OpenSSL, and on NSS the chain it returned may differ from the the chain that was actually used.)

BUG=77757, 338887
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289994

[Ryan Sleevi, 2014-07-08 02:18:36]

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:29:
net::X509Certificate::OSCertHandles::iterator certs_end);
I'm a little confused by this.

I would think that ShowCertExportDialog(net::X509Certificate*) would be
sufficient, as that handles both single-cert and cert-chains. Have I missed
something?

It may be because I'm not familiar with the WebUI dialog here, but I would
think:

- If I click "View Certificate" from the Trust management dialog, only a single
certificate is exported
- If I click "View Certificate" from the status bubble, and click "Export
Certificate", then the entire chain is exported. Always. That is, it does not
depend on the current cert I've selected, it's always "the chain that I'm
viewing" that's exported.

Is that not the case today?

[mattm, 2014-07-08 02:39:48]

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:29:
net::X509Certificate::OSCertHandles::iterator certs_end);
On 2014/07/08 02:18:36, Ryan Sleevi wrote:
> I'm a little confused by this.
> 
> I would think that ShowCertExportDialog(net::X509Certificate*) would be
> sufficient, as that handles both single-cert and cert-chains. Have I missed
> something?
> 
> It may be because I'm not familiar with the WebUI dialog here, but I would
> think:
> 
> - If I click "View Certificate" from the Trust management dialog, only a
single
> certificate is exported
> - If I click "View Certificate" from the status bubble, and click "Export
> Certificate", then the entire chain is exported. Always. That is, it does not
> depend on the current cert I've selected, it's always "the chain that I'm
> viewing" that's exported.
> 
> Is that not the case today?

Yeah, if you select one of the other certs in the chain in the cert viewer, and
click export, it'll export from that one up.

[Ryan Sleevi, 2014-07-08 02:55:04]

On 2014/07/08 02:39:48, mattm wrote:
>
https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
> File chrome/browser/ui/certificate_dialogs.h (right):
> 
>
https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
> chrome/browser/ui/certificate_dialogs.h:29:
> net::X509Certificate::OSCertHandles::iterator certs_end);
> On 2014/07/08 02:18:36, Ryan Sleevi wrote:
> > I'm a little confused by this.
> > 
> > I would think that ShowCertExportDialog(net::X509Certificate*) would be
> > sufficient, as that handles both single-cert and cert-chains. Have I missed
> > something?
> > 
> > It may be because I'm not familiar with the WebUI dialog here, but I would
> > think:
> > 
> > - If I click "View Certificate" from the Trust management dialog, only a
> single
> > certificate is exported
> > - If I click "View Certificate" from the status bubble, and click "Export
> > Certificate", then the entire chain is exported. Always. That is, it does
not
> > depend on the current cert I've selected, it's always "the chain that I'm
> > viewing" that's exported.
> > 
> > Is that not the case today?
> 
> Yeah, if you select one of the other certs in the chain in the cert viewer,
and
> click export, it'll export from that one up.

Really? That's dumb. It doesn't do that on our other platforms.

Can we change that? Who would need to stamp?

[mattm, 2014-07-08 16:48:05]

On 2014/07/08 02:55:04, Ryan Sleevi wrote:
> On 2014/07/08 02:39:48, mattm wrote:
> >
>
https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
> > File chrome/browser/ui/certificate_dialogs.h (right):
> > 
> >
>
https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
> > chrome/browser/ui/certificate_dialogs.h:29:
> > net::X509Certificate::OSCertHandles::iterator certs_end);
> > On 2014/07/08 02:18:36, Ryan Sleevi wrote:
> > > I'm a little confused by this.
> > > 
> > > I would think that ShowCertExportDialog(net::X509Certificate*) would be
> > > sufficient, as that handles both single-cert and cert-chains. Have I
missed
> > > something?
> > > 
> > > It may be because I'm not familiar with the WebUI dialog here, but I would
> > > think:
> > > 
> > > - If I click "View Certificate" from the Trust management dialog, only a
> > single
> > > certificate is exported
> > > - If I click "View Certificate" from the status bubble, and click "Export
> > > Certificate", then the entire chain is exported. Always. That is, it does
> not
> > > depend on the current cert I've selected, it's always "the chain that I'm
> > > viewing" that's exported.
> > > 
> > > Is that not the case today?
> > 
> > Yeah, if you select one of the other certs in the chain in the cert viewer,
> and
> > click export, it'll export from that one up.
> 
> Really? That's dumb. It doesn't do that on our other platforms.
> 
> Can we change that? Who would need to stamp?

Had to refresh my memory on what the win cert viewer looks like.

The difference is that on win, the cert chain is a separate tab and has a "view
certificate" button so you can open another cert viewer window on other certs in
the chain if you wanted to export those. In our webui/old gtk one, it's all on
one page, and selecting a different cert in the chain changes what data is shown
as well as which one is exported. So I don't think we should change that unless
we change the design of the rest of the dialog too.

[Ryan Sleevi, 2014-07-10 02:50:18]

Sorry for the delay on this.

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.cc:100: std::string cert_title =
x509_certificate_model::GetTitle(*certs_begin);
If certs_begin == certs_end, this will crash.

STL pattern normally is that it's ok to supply where begin==end. Not sure if
that should change things.

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:23: net::X509Certificate* cert);
Since you're changing the type, it'd be "nicer" to pass a const-ref
scoped_refptr<>, since that's our signal "ownership may change here" (ala
scoped_ptr<>)

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:29:
net::X509Certificate::OSCertHandles::iterator certs_end);
On 2014/07/08 02:39:47, mattm wrote:
> Yeah, if you select one of the other certs in the chain in the cert viewer,
and
> click export, it'll export from that one up.

I'm not sure why we did that, nor am I convinced we want that, but OK.

Any chance we can change this behaviour? Or do we need to punt to UI (who have
largely said "IDK whatever")

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/certificate_viewer_webui.cc:254:
x509_certificate_model::GetCertChainFromCert(cert_, &cert_chain_);
Do we really need this method any more? (the x509_certificate_model). Seems like
we should nuke it entirely in favour of using the net::X509Certificate

Also, git-cl-format this, lines 251-253 seem style-wrong.

[mattm, 2014-07-10 22:58:00]

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.cc:100: std::string cert_title =
x509_certificate_model::GetTitle(*certs_begin);
On 2014/07/10 02:50:18, Ryan Sleevi wrote:
> If certs_begin == certs_end, this will crash.
> 
> STL pattern normally is that it's ok to supply where begin==end. Not sure if
> that should change things.

I added a dcheck, since it doesn't really make any sense to call this class with
an empty list.

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:23: net::X509Certificate* cert);
On 2014/07/10 02:50:18, Ryan Sleevi wrote:
> Since you're changing the type, it'd be "nicer" to pass a const-ref
> scoped_refptr<>, since that's our signal "ownership may change here" (ala
> scoped_ptr<>)

Done.

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:29:
net::X509Certificate::OSCertHandles::iterator certs_end);
On 2014/07/10 02:50:18, Ryan Sleevi wrote:
> On 2014/07/08 02:39:47, mattm wrote:
> > Yeah, if you select one of the other certs in the chain in the cert viewer,
> and
> > click export, it'll export from that one up.
> 
> I'm not sure why we did that, nor am I convinced we want that, but OK.
> 
> Any chance we can change this behaviour? Or do we need to punt to UI (who have
> largely said "IDK whatever")

Firefox cert viewer does the same, and we copied their design. I guess we could
change it if you propose what the new UI should look like. Like I was saying
above it does make some sort of sense to me since selecting a cert in the chain
changes both which cert data is shown and which cert will be exported.

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/376753002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/certificate_viewer_webui.cc:254:
x509_certificate_model::GetCertChainFromCert(cert_, &cert_chain_);
On 2014/07/10 02:50:18, Ryan Sleevi wrote:
> Do we really need this method any more? (the x509_certificate_model). Seems
like
> we should nuke it entirely in favour of using the net::X509Certificate
> 
> Also, git-cl-format this, lines 251-253 seem style-wrong.

Done.

[Ryan Sleevi, 2014-07-11 01:34:00]

I was just thinking that the export would always export either the entire chain
or the selected certificate, with a slight bias towards the first

That said, it's not an issue we need to solve right now, so LGTM

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.cc:92: DCHECK(certs_begin != certs_end);
DCHECK_NE() ?

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:25: void ShowCertExportDialog(
Perhaps document ;)

[mattm, 2014-07-11 01:47:52]

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.cc:92: DCHECK(certs_begin != certs_end);
On 2014/07/11 01:33:59, Ryan Sleevi wrote:
> DCHECK_NE() ?

It doesn't like iterators.

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/80001/chrome/browser/ui/certif...
chrome/browser/ui/certificate_dialogs.h:25: void ShowCertExportDialog(
On 2014/07/11 01:33:59, Ryan Sleevi wrote:
> Perhaps document ;)

Done.

[mattm, 2014-07-11 01:55:13]

+msw for chrome/browser/ui OWNERS

[msw, 2014-07-11 17:25:40]

Please add a more complete CL description, I have little idea of what was broken
and how this fixes it, it seems like it's mostly refactoring.

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.cc:70:
net::X509Certificate::OSCertHandles::iterator certs_begin,
nit: maybe use a typdef or using statement to make this file more concise?

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:21: // Show a save dialog allowing to
save |cert| alone or the cert + its chain.
nit: consider: "Show a dialog to save |cert|, potentially with its chain."

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:26: // Show a save dialog allowing to
save the certificate pointed to by
nit: consider: "Show a dialog to save the chain of certificates encompassed by
the iterators."

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:28: void ShowCertExportDialog(
Overloading function names is discouraged, can you differentiate these? Does it
make more sense to only have this iterator impl and update the callers?

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/certificate_viewer_webui.cc:105:
cert_chain.push_back(cert_->os_cert_handle());
Why can't you use cert_chain_ here?

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/certificate_viewer_webui.cc:258:
cert_chain_.push_back(cert_->os_cert_handle());
If this logic is commonly needed (3rd time I see it), does keeping a helper make
sense? You could unify the helper implementations to work like this.

[mattm, 2014-07-11 22:21:57]

Ryan, could you take another look too?

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.cc:70:
net::X509Certificate::OSCertHandles::iterator certs_begin,
On 2014/07/11 17:25:40, msw wrote:
> nit: maybe use a typdef or using statement to make this file more concise?

I'm not huge fan of that since it makes code harder to read. Since this isn't
long enough to cause weird wrapping issues I'd rather leave it.

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:21: // Show a save dialog allowing to
save |cert| alone or the cert + its chain.
On 2014/07/11 17:25:40, msw wrote:
> nit: consider: "Show a dialog to save |cert|, potentially with its chain."

removed

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:26: // Show a save dialog allowing to
save the certificate pointed to by
On 2014/07/11 17:25:40, msw wrote:
> nit: consider: "Show a dialog to save the chain of certificates encompassed by
> the iterators."

done(-ish)

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:28: void ShowCertExportDialog(
On 2014/07/11 17:25:40, msw wrote:
> Overloading function names is discouraged, can you differentiate these? Does
it
> make more sense to only have this iterator impl and update the callers?

Done.

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/certificate_viewer_webui.cc:105:
cert_chain.push_back(cert_->os_cert_handle());
On 2014/07/11 17:25:40, msw wrote:
> Why can't you use cert_chain_ here?

cert_chain_ is a member of CertificateViewerDialogHandler, this is
CertificateViewerModalDialog.

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/certificate_viewer_webui.cc:258:
cert_chain_.push_back(cert_->os_cert_handle());
On 2014/07/11 17:25:40, msw wrote:
> If this logic is commonly needed (3rd time I see it), does keeping a helper
make
> sense? You could unify the helper implementations to work like this.

Done.

[msw, 2014-07-11 22:42:45]

Nice, lgtm with a comment nit.

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/376753002/diff/100001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.cc:70:
net::X509Certificate::OSCertHandles::iterator certs_begin,
On 2014/07/11 22:21:57, mattm (OOO July 14-28) wrote:
> On 2014/07/11 17:25:40, msw wrote:
> > nit: maybe use a typdef or using statement to make this file more concise?
> 
> I'm not huge fan of that since it makes code harder to read. Since this isn't
> long enough to cause weird wrapping issues I'd rather leave it.

Acknowledged.

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:21: // Show a dialog to save the first
certificate or the whole chain encompassed by
nit: I think omitting "the first certificate or" would reduce confusion...
Otherwise, a caller might think they can somehow pass iterators encompassing 10
certs and somehow only have the first one saved. (or maybe I'm just confused)

[mattm, 2014-07-11 22:49:50]

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:21: // Show a dialog to save the first
certificate or the whole chain encompassed by
On 2014/07/11 22:42:45, msw wrote:
> nit: I think omitting "the first certificate or" would reduce confusion...
> Otherwise, a caller might think they can somehow pass iterators encompassing
10
> certs and somehow only have the first one saved. (or maybe I'm just confused)

The dialog lets the user choose between saving either the first cert or the
whole chain (both in a few different formats).

[Ryan Sleevi, 2014-07-11 22:55:50]

Unfortunately, I feel strong enough about this to NACK it, on the basis that I
think it puts as 'first class' dealing with handles in the API.

APIs that end up taking OSCertHandles, rather than X509Certificate*, now have to
worry if OSCertHandles.empty() is true, which is a valid fall-out from this API.
X509Certificate has a stronger guarantee, in that there will always be at least
one certificate.

I think we can follow-up on that clean-up separately, if MSW feels this is
critical, but don't think we should be modifying the //net interface in this CL.

https://codereview.chromium.org/376753002/diff/140001/net/cert/x509_certifica...
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/376753002/diff/140001/net/cert/x509_certifica...
net/cert/x509_certificate.h:256: void GetCertificateChain(OSCertHandles*
cert_chain) const;
I realize you added this based on msw's comments, but I'm not really a fan of
this.

X509Certificate exists as the canonical 'chain' type, and is meant to be what we
pass around for chains, because we rarely should be dealing in OSCertHandles.

[msw, 2014-07-11 23:01:15]

https://codereview.chromium.org/376753002/diff/140001/net/cert/x509_certifica...
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/376753002/diff/140001/net/cert/x509_certifica...
net/cert/x509_certificate.h:256: void GetCertificateChain(OSCertHandles*
cert_chain) const;
On 2014/07/11 22:55:49, Ryan Sleevi wrote:
> I realize you added this based on msw's comments, but I'm not really a fan of
> this.
> 
> X509Certificate exists as the canonical 'chain' type, and is meant to be what
we
> pass around for chains, because we rarely should be dealing in OSCertHandles.

I'll defer to you, my concern was duplicated code, which yields maintenance
burdens and inconsistencies (some copies get fixed, some don't, etc.)...

[msw, 2014-07-11 23:02:02]

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
File chrome/browser/ui/certificate_dialogs.h (right):

https://codereview.chromium.org/376753002/diff/140001/chrome/browser/ui/certi...
chrome/browser/ui/certificate_dialogs.h:21: // Show a dialog to save the first
certificate or the whole chain encompassed by
On 2014/07/11 22:49:50, mattm (OOO July 14-28) wrote:
> On 2014/07/11 22:42:45, msw wrote:
> > nit: I think omitting "the first certificate or" would reduce confusion...
> > Otherwise, a caller might think they can somehow pass iterators encompassing
> 10
> > certs and somehow only have the first one saved. (or maybe I'm just
confused)
> 
> The dialog lets the user choose between saving either the first cert or the
> whole chain (both in a few different formats).

Acknowledged, sorry for my confusion.

[mattm, 2014-08-15 02:14:02]

Ryan: reverted that part of the change, PTAL

[Ryan Sleevi, 2014-08-15 17:22:39]

lgtm

[mattm, 2014-08-15 18:23:55]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-08-15 18:27:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mattm@chromium.org/376753002/200001

[commit-bot: I haz the power, 2014-08-15 20:11:29]

Committed patchset #8 (200001) as 289994