1063226 - Interfaces to add pins and query for pin status

Status: RESOLVED DUPLICATE of bug 787133

(Core :: Security: PSM, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1063226.patch

Simple query interface, implemented by splitting out the part of CheckPinsForHostname that looks up pins for the hostname, and returning the boolean value of whether that search found something.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8484617 [details] [diff] [review]
bug-1063226.patch

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

I'm technically not a peer or owner of this module, so you might want to ask keeler for review.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +192,5 @@
> +static bool
> +CheckPinsForHostname(const CERTCertList *certList, const char *hostname,
> +                     bool enforceTestMode)
> +{
> +  if (!certList) {

since you're cleaning this up, might as well do

if (!certList || !hostname || !hostname[0]) {
  return false;
}

@@ +199,5 @@
> +  if (!hostname || hostname[0] == 0) {
> +    return false;
> +  }
> +
> +  TransportSecurityPreload *foundEntry = FindPinsForHostname(hostname);

this can be const (not that it matters much)

::: security/manager/boot/src/PublicKeyPinningService.h
@@ +32,5 @@
> +
> +  /**
> +   * Returns true if pins exist for the given host.
> +   */
> +  static bool PinsExistForHostname(const char* hostname);

What's the advantage to exposing this rather than FindPinsForHostname (other than also having to expose the preload struct, of course)? What action are the eventual callers supposed to take if pins do or do not exist?

I know you told me off thread, but it's better to have these decisions documented publicly.

Comment 3

[Richard Barnes [:rbarnes]]

Attachment: bug-1063226.1.patch

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> Comment on attachment 8484617 [details] [diff] [review]
> bug-1063226.patch
> 
> Review of attachment 8484617 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: security/manager/boot/src/PublicKeyPinningService.h
> @@ +32,5 @@
> > +
> > +  /**
> > +   * Returns true if pins exist for the given host.
> > +   */
> > +  static bool PinsExistForHostname(const char* hostname);
> 
> What's the advantage to exposing this rather than FindPinsForHostname (other
> than also having to expose the preload struct, of course)? What action are
> the eventual callers supposed to take if pins do or do not exist?

As I understand it, the use case

Comment 4

[Richard Barnes [:rbarnes]]

Comment on attachment 8485045 [details] [diff] [review]
bug-1063226.1.patch

Clearing review request because it was sent in too much haste.  Sorry, keeler!

Comment 5

[Richard Barnes [:rbarnes]]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> Review of attachment 8484617 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: security/manager/boot/src/PublicKeyPinningService.h
> @@ +32,5 @@
> > +
> > +  /**
> > +   * Returns true if pins exist for the given host.
> > +   */
> > +  static bool PinsExistForHostname(const char* hostname);
> 
> What's the advantage to exposing this rather than FindPinsForHostname (other
> than also having to expose the preload struct, of course)? What action are
> the eventual callers supposed to take if pins do or do not exist?
> 
> I know you told me off thread, but it's better to have these decisions
> documented publicly.

Bugzilla sent this before I was finished!

As I understand it, the use case is for code that's making an HTTPS request to query this interface to see if the request will be subject to pins.  If it is, it knows that the request has been subject to whatever pins are in place; if not, it might choose not to make the request.  There is an implicit assumption that the calling code is OK with whatever pins are there, i.e., that no unauthorized pins have been added.

Plus, if we return bool, we don't have to expose the TransportSecurityPreload struct to the rest of the world :)

Mattias: Does this interface look OK to you guys, or would you want an interface that tells you what keys exist?

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Better not to expose the information, I agree. Thanks for documenting in the bug.

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: bug-1063226.2.patch

This patch shifts the interface over to nsISiteSecurityService, to make it easier to query from JS.  The actual implementations still reside in PublicKeyPinning service, but we are thinking about combining these two anyway.

Comment 8

[Zoran Jovanovic]

(In reply to Richard Barnes [:rbarnes] from comment #5)
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> > Review of attachment 8484617 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: security/manager/boot/src/PublicKeyPinningService.h
> > @@ +32,5 @@
> > > +
> > > +  /**
> > > +   * Returns true if pins exist for the given host.
> > > +   */
> > > +  static bool PinsExistForHostname(const char* hostname);
> > 
> > What's the advantage to exposing this rather than FindPinsForHostname (other
> > than also having to expose the preload struct, of course)? What action are
> > the eventual callers supposed to take if pins do or do not exist?
> > 
> > I know you told me off thread, but it's better to have these decisions
> > documented publicly.
> 
> Bugzilla sent this before I was finished!
> 
> As I understand it, the use case is for code that's making an HTTPS request
> to query this interface to see if the request will be subject to pins.  If
> it is, it knows that the request has been subject to whatever pins are in
> place; if not, it might choose not to make the request.  There is an
> implicit assumption that the calling code is OK with whatever pins are
> there, i.e., that no unauthorized pins have been added.
> 
> Plus, if we return bool, we don't have to expose the
> TransportSecurityPreload struct to the rest of the world :)
> 
> Mattias: Does this interface look OK to you guys, or would you want an
> interface that tells you what keys exist?

Looks good. I intended to upload a WIP patch with a similar interface to nsIX509CertDB in bug 1059202, but this is good. There may be a need for 'query all pins' type of interface, but not int the context of bug 1059202.

Comment 9

[Richard Barnes [:rbarnes]]

(In reply to Zoran Jovanovic from comment #8)
> > Mattias: Does this interface look OK to you guys, or would you want an
> > interface that tells you what keys exist?
> 
> Looks good. I intended to upload a WIP patch with a similar interface to
> nsIX509CertDB in bug 1059202, but this is good. There may be a need for
> 'query all pins' type of interface, but not int the context of bug 1059202.

There's pretty strong consensus among the Mozilla PKI folks that nsIX509CertDB is the wrong place for these interfaces.  Let's keep the focus on nsISiteSecurityService.

What do you mean by "query all pins"?  As in, "Give me all the pins for this domain"?

Comment 10

[Zoran Jovanovic]

(In reply to Richard Barnes [:rbarnes] from comment #9)
> (In reply to Zoran Jovanovic from comment #8)
> > > Mattias: Does this interface look OK to you guys, or would you want an
> > > interface that tells you what keys exist?
> > 
> > Looks good. I intended to upload a WIP patch with a similar interface to
> > nsIX509CertDB in bug 1059202, but this is good. There may be a need for
> > 'query all pins' type of interface, but not int the context of bug 1059202.
> 
> There's pretty strong consensus among the Mozilla PKI folks that
> nsIX509CertDB is the wrong place for these interfaces.  Let's keep the focus
> on nsISiteSecurityService.
I didn't disagree. :-) It's just that my WIP patch used similar interface change as in that bug (for speed and testing). Of course, as soon as I can use nsISiteSecurityService, I will.

> 
> What do you mean by "query all pins"?  As in, "Give me all the pins for this
> domain"?
Yes. But as I said, it's not important for this use case. 'PinsExist' is enough.