Closed Bug 1406856 Opened 7 years ago Closed 7 years ago

Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList

Categories

(Core :: Security: PSM, enhancement, P2)

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

(Whiteboard: [psm-backlog])

Attachments

(1 file, 1 obsolete file)

Bug 1406856 - Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList
59 bytes, text/x-review-board-request
keeler
: review+
jcj
: review+
Details 
You can get an nsIX509Cert from nsISSLStatus.idl which allows you to get the (end entity) server cert; it's be nice if we could get the whole cert chain. See bug 1406854 for context.
Priority: -- → P2
Whiteboard: [psm-backlog]
Attached patch Bug1406856.patch (obsolete) — — Splinter Review 
This, once the work for bug 731478 lands, is something like what we want.
Attachment #8920292 - Flags: feedback?(jjones)
Comment on attachment 8920292 [details] [diff] [review]
Bug1406856.patch

Keeler - can you take a look at this, too? Tanks.
Attachment #8920292 - Flags: feedback?(dkeeler)
Comment on attachment 8920292 [details] [diff] [review]
Bug1406856.patch

Review of attachment 8920292 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/nsSSLStatus.cpp
@@ +373,5 @@
>    mHasIsEVStatus = true;
>  }
>  
> +nsresult
> +nsSSLStatus::SetSucceededCertChain(UniqueCERTCertList certList)

You'll want to match the aVariableName argument naming convention in here.

@@ +395,5 @@
> +
> +NS_IMETHODIMP
> +nsSSLStatus::GetSucceededCertChain(nsIX509CertList** _result)
> +{
> +  MOZ_ASSERT(_result);

Seems like the rest of this file uses NS_ENSURE_ARG_POINTER()

@@ +409,5 @@
> +{
> +  MOZ_ASSERT(_result);
> +
> +  *_result = mFailedCertChain;
> +  NS_IF_ADDREF(*_result);

FYI, I don't understand how the reference counting here works, so I'm hoping Keeler will weigh in on it.

::: security/manager/ssl/nsSSLStatus.h
@@ +28,5 @@
>  class nsSSLStatus final
>    : public nsISSLStatus
>    , public nsISerializable
>    , public nsIClassInfo
> +  , public nsNSSShutDownObject

I'd be curious if Keeler knows a way to avoid having to do this.
Attachment #8920292 - Flags: feedback?(jjones) → feedback+
Comment on attachment 8920292 [details] [diff] [review]
Bug1406856.patch

Review of attachment 8920292 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, generally the right idea. There's a couple gotchas we'll need to address (the serialization is the main one).

::: security/manager/ssl/nsISSLStatus.idl
@@ +11,2 @@
>  
> +[scriptable, uuid(65e59a57-89b4-48f0-b79c-6e02562d731a)]

I think we actually don't want to change the uuid... see bug 1248628. We also have to update the serialization here: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/security/manager/ssl/nsSSLStatus.cpp#170 (basically, bump the version, and only read the additional attributes if the version is high enough. Also write out the new data, too.).

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1252,5 @@
>              ("HandshakeCallback using NEW cert %p (is not EV)", nssc.get()));
>      sslStatus->SetServerCert(nssc, EVStatus::NotEV);
>    }
>  
> +  sslStatus->SetSucceededCertChain(Move(builtChain));

We probably only want to do this if rv == Success (otherwise, we might get unexpected results...)

::: security/manager/ssl/nsSSLStatus.cpp
@@ +23,2 @@
>  NS_IMETHODIMP
>  nsSSLStatus::GetServerCert(nsIX509Cert** aServerCert)

I wonder if the serverCert should still be stored separately from the other two lists after this work...

@@ +395,5 @@
> +
> +NS_IMETHODIMP
> +nsSSLStatus::GetSucceededCertChain(nsIX509CertList** _result)
> +{
> +  MOZ_ASSERT(_result);

Yeah, I think it's best to also have the non-debug handle-invalid-input case.

@@ +409,5 @@
> +{
> +  MOZ_ASSERT(_result);
> +
> +  *_result = mFailedCertChain;
> +  NS_IF_ADDREF(*_result);

I think the non-macro-magic way to do it would be something like this:

nsCOMPtr<nsIX509CertList> tmpList = mFailedCertChain;
tmpList.forget(_result);

::: security/manager/ssl/nsSSLStatus.h
@@ +28,5 @@
>  class nsSSLStatus final
>    : public nsISSLStatus
>    , public nsISerializable
>    , public nsIClassInfo
> +  , public nsNSSShutDownObject

Well, we could refactor nsNSSCertList to not require a nsNSSShutDownPreventionLock in its constructor.

@@ +43,5 @@
>  
>    void SetServerCert(nsNSSCertificate* aServerCert, EVStatus aEVStatus);
>  
> +  nsresult SetSucceededCertChain(mozilla::UniqueCERTCertList certList);
> +  void SetFailedCertChain(nsIX509CertList *x509CertList);

nit: * to the left
Also, from an API point of view, these seem a bit mismatched (one takes a UniqueCERTCertList, the other takes a nsIX509CertList). Probably not a big deal since there aren't many consumers of these functions, but maybe as part of this work we could take the failedCertChain off of nsITransportSecurityInfo, which I think would change how we set it on the nsSSLStatus and would maybe result in unifying these types a bit.
Attachment #8920292 - Flags: feedback?(dkeeler)
Attachment #8920292 - Flags: feedback?
Attachment #8920292 - Flags: feedback+
Attachment #8920292 - Attachment is obsolete: true
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> >  NS_IMETHODIMP
> >  nsSSLStatus::GetServerCert(nsIX509Cert** aServerCert)
> 
> I wonder if the serverCert should still be stored separately from the other
> two lists after this work...

Yeah, part one is making sure there's always a chain. Part two will be modifying all the call sites to Get/SetServerCert to use the chain (and cleanup)

> ::: security/manager/ssl/nsSSLStatus.h
> @@ +28,5 @@
> >  class nsSSLStatus final
> >    : public nsISSLStatus
> >    , public nsISerializable
> >    , public nsIClassInfo
> > +  , public nsNSSShutDownObject
> 
> Well, we could refactor nsNSSCertList to not require a
> nsNSSShutDownPreventionLock in its constructor.

That feels like a separate bug.

 > @@ +43,5 @@
> >  
> >    void SetServerCert(nsNSSCertificate* aServerCert, EVStatus aEVStatus);
> >  
> > +  nsresult SetSucceededCertChain(mozilla::UniqueCERTCertList certList);
> > +  void SetFailedCertChain(nsIX509CertList *x509CertList);
> 
> nit: * to the left
> Also, from an API point of view, these seem a bit mismatched (one takes a
> UniqueCERTCertList, the other takes a nsIX509CertList). Probably not a big
> deal since there aren't many consumers of these functions, but maybe as part
> of this work we could take the failedCertChain off of
> nsITransportSecurityInfo, which I think would change how we set it on the
> nsSSLStatus and would maybe result in unifying these types a bit.

Shall we do this as part of the cleanup described above?
Comment on attachment 8920574 [details]
Bug 1406856 - Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList

How's this looking? I guess we need some tests...
Attachment #8920574 - Flags: feedback?(dkeeler)
(In reply to Mark Goodwin [:mgoodwin] from comment #7)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> > >  NS_IMETHODIMP
> > >  nsSSLStatus::GetServerCert(nsIX509Cert** aServerCert)
> > 
> > I wonder if the serverCert should still be stored separately from the other
> > two lists after this work...
> 
> Yeah, part one is making sure there's always a chain. Part two will be
> modifying all the call sites to Get/SetServerCert to use the chain (and
> cleanup)

Ok - sounds good.

> > ::: security/manager/ssl/nsSSLStatus.h
> > @@ +28,5 @@
> > >  class nsSSLStatus final
> > >    : public nsISSLStatus
> > >    , public nsISerializable
> > >    , public nsIClassInfo
> > > +  , public nsNSSShutDownObject
> > 
> > Well, we could refactor nsNSSCertList to not require a
> > nsNSSShutDownPreventionLock in its constructor.
> 
> That feels like a separate bug.

Sure.

>  > @@ +43,5 @@
> > >  
> > >    void SetServerCert(nsNSSCertificate* aServerCert, EVStatus aEVStatus);
> > >  
> > > +  nsresult SetSucceededCertChain(mozilla::UniqueCERTCertList certList);
> > > +  void SetFailedCertChain(nsIX509CertList *x509CertList);
> > 
> > nit: * to the left
> > Also, from an API point of view, these seem a bit mismatched (one takes a
> > UniqueCERTCertList, the other takes a nsIX509CertList). Probably not a big
> > deal since there aren't many consumers of these functions, but maybe as part
> > of this work we could take the failedCertChain off of
> > nsITransportSecurityInfo, which I think would change how we set it on the
> > nsSSLStatus and would maybe result in unifying these types a bit.
> 
> Shall we do this as part of the cleanup described above?

Sounds good.
Comment on attachment 8920574 [details]
Bug 1406856 - Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList

https://reviewboard.mozilla.org/r/191590/#review196858

Yep - this is looking good.

::: security/manager/ssl/SSLServerCertVerification.cpp:1471
(Diff revision 1)
>      if (!status) {
>        status = new nsSSLStatus();
>        infoObject->SetSSLStatus(status);
>      }
>  
>      if (!status->HasServerCert()) {

Now that I think about this, we should probably always set the server cert and succeeded cert chain here (otherwise, this code is suggesting we could have a different certificate on the status than the one we just verified). (In reality, I suspect we can never already have a server cert here, but just in case...)

::: security/manager/ssl/nsSSLStatus.h:57
(Diff revision 1)
>  
>    void SetCertificateTransparencyInfo(
>      const mozilla::psm::CertificateTransparencyInfo& info);
>  
> +  virtual void virtualDestroyNSSReference() override;
> +  void destructorSafeDestroyNSSReference();

Since we're not actually releasing anything, we don't have to declare a `destructorSafeDestroyNSSReference`.

::: security/manager/ssl/nsSSLStatus.cpp:19
(Diff revision 1)
> +#include "nsNSSShutDown.h"
>  #include "ssl.h"
>  
> +
> +void
> +nsSSLStatus::virtualDestroyNSSReference()

We still have to implement the destructor:
```
nsSSLStatus::~nsSSLStatus()
{
  nsNSSShutDownPreventionLock locker;
  if (isAlreadyShutDown()) {
    return;
  }
  shutdown(ShutdownCalledFrom::Object);
}
```
Attachment #8920574 - Flags: review+
Attachment #8920574 - Flags: feedback?(dkeeler) → feedback+
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51eaba841505
Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList r=keeler
Backed out for failing eslint at security/manager/ssl/tests/unit/head_psm.js:732:53 | Multiple spaces found before '=':

https://hg.mozilla.org/integration/autoland/rev/eb7b673e0390b2d97b25e15546c696a077a49327

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eb7b673e0390b2d97b25e15546c696a077a49327&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140417515&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/security/manager/ssl/tests/unit/head_psm.js:732:53 | Multiple spaces found before '='. (no-multi-spaces)
Flags: needinfo?(mgoodwin)
Comment on attachment 8920574 [details]
Bug 1406856 - Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList

https://reviewboard.mozilla.org/r/191590/#review199222
Attachment #8920574 - Flags: review+
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fb53febbfd2
Re-plumb nsISSLStatus.idl to carry with it the whole nsIX509CertList r=jcj,keeler
Flags: needinfo?(mgoodwin)
https://hg.mozilla.org/mozilla-central/rev/2fb53febbfd2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee: nobody → mgoodwin
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: