Closed Bug 1091176 Opened 5 years ago Closed Last year

Implement report-uri directive for HPKP

Categories

(Core :: Security: PSM, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mmc, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Depends on: hpkp
So, some context for this request.

HPKP ("key pinning") lets us pin a given website to one or more intermediate SSL certificate issuers for a length of time, cached client side. This is similar to HSTS, insofar as once you ship it, it's on, and you have no choice but to make it work.

Unlike HSTS, however, if you ship a bad header, you break your site for that length of time *without possibility of recovery*. So it's extremely dangerous to ship HPKP headers without testing.

To make it possible to use HPKP in a reasonable and safe manner, the spec implements a "reporting" URI endpoint to which the client can post - in certain cases - the results of its HPKP evaluation.

This permits enabling HPKP for a site in "report only" mode, so that you can try out your new HPKP setup, and validate that it *actually works*, before enabled HPKP in enforce mode.

We have been asked to work towards enabling HPKP on certain critical Mozilla sites (for instance, BMO), but the lack of HPKP reporting in Firefox, combined with its HPKP enforcement, introduces a severe risk of undetectable issues for all Firefox users when we do so.

We can do limited testing with other browsers in the meantime, but it would be greatly beneficial to Mozilla (and everyone) for us to complete this piece of our HPKP implementation.
I am interested in this feature as well. One desire is to insure reliable delivery of HPKP reports. I can imagine a scenario where captive portal is used to restrict network access which would also prevent reliable reporting (eg Fx won't accept the cert on the captive portal then network access is denied thus the report isn't delivered). It would be nice to have the report delivery retried the next time access is restored.
Comment 2 is optional for WebOps concerns, but the relevant section of RFC 7469 section 2.1.4 says:

> In any case of report failure, the UA MAY attempt to re-send the report later.
>
> UAs SHOULD limit the rate at which they send reports.  For example, it is unnecessary to send the same report to the same report-uri more than once per distinct set of declared Pins.
There are some things in common between the reporting side of this and the existing TLS Error reporting functionality. Some of the things missing in the SecurityReporter (used for these TLS error reports) are things we already know we need (e.g. rate limiting of some kind).

One thing we need to watch out for (for my own benefit as much as anyone else): The spec says the UA is responsible for detecting and breaking loops: see https://tools.ietf.org/html/rfc7469#section-2.1.4
This would require a moderate (but not monumental) amount of work. Luckily we can probably reuse/repurpose existing reporting functionality Mark mentioned. Other work would include basically adding another header type to nsSiteSecurityService and handling it very similarly to HPKP, but not doing the enforcing. I estimate this would take someone about a quarter.

(In reply to Richard Soderberg [:atoll] from comment #1)
> We have been asked to work towards enabling HPKP on certain critical Mozilla
> sites (for instance, BMO), but the lack of HPKP reporting in Firefox,
> combined with its HPKP enforcement, introduces a severe risk of undetectable
> issues for all Firefox users when we do so.

If this is the goal, we have a much easier (and more effective) route: preloaded key pins. From the beginning we implemented the ability to have "test mode" pins and to collect telemetry on which Mozilla hosts were failing (since they represent critical infrastructure). This has the added benefit of not having to rely on the trust-on-first-use nature of HPKP.
How different is all that reporting from CSP reporting? We already have code for CSP reporting [1] and potentially it would make sense to factor our some parts, e.g. have a general SendReports function which can be called from CSP as well as other consumers? Just a thought!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#737
(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> If this is the goal, we have a much easier (and more effective) route:
> preloaded key pins. From the beginning we implemented the ability to have
> "test mode" pins and to collect telemetry on which Mozilla hosts were
> failing (since they represent critical infrastructure). This has the added
> benefit of not having to rely on the trust-on-first-use nature of HPKP.

When we deploy HPKP for all users on all browsers for any given site, we won't be able to exclude Firefox, and testing with a completely different channel first does little to assuage our concerns about Firefox specifically. We could absolutely test various intermediate pins in Firefox, but we can also test those with openssl(1).

The goal here is to bring Firefox's HPKP reporting into parity with WebKit and Chrome, since - as I understand it - we're the only HPKP-enforcing browser without reporting.

Note above, where I say Firefox, I mean "Firefox except on iOS, where we presumably use the underlying WebKit HPKP implementation", which does support reporting (and is, similarly, not representative of the Desktop HPKP implementation).
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> How different is all that reporting from CSP reporting?

Same theory, different JSON, I believe:

{
  "csp-report": {
    "document-uri": "https://example.com/foo/bar",
    "referrer": "https://www.google.com/",
    "violated-directive": "default-src self",
    "original-policy": "default-src self; report-uri /csp-hotline.php",
    "blocked-uri": "http://evilhackerscripts.com"
  }
}

vs.

{
  "date-time": date-time,
  "hostname": hostname,
  "port": port,
  "effective-expiration-date": expiration-date,
  "include-subdomains": include-subdomains,
  "noted-hostname": noted-hostname,
  "served-certificate-chain": [
    pem1, ... pemN
  ],
  "validated-certificate-chain": [
    pem1, ... pemN
  ],
  "known-pins": [
    known-pin1, ... known-pinN
  ]
}

(In reply to Richard Soderberg [:atoll] from comment #7)
> Note above, where I say Firefox, I mean "Firefox except on iOS, where we
> presumably use the underlying WebKit HPKP implementation", which does
> support reporting (and is, similarly, not representative of the Desktop HPKP
> implementation).

I was misled - WebKit does not implement HPKP at this time.
Attachment #8804049 - Flags: review?(mgoodwin)
Attachment #8804049 - Flags: review?(dkeeler)
Attachment #8804049 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review87140

::: security/manager/ssl/nsSiteSecurityService.cpp:179
(Diff revision 1)
>        valid = false;
>      }
>    }
> +
> +  if(valid && (0 < strnlen(collectedReportUri, MaxStateStringSize))) {
> +    //TODO: URI must have at least a http:// at the front so make min len 7?

That's not correct, the URI may be relative, so 0 is the correct min length
Here's a first pass at a part of this issue: parsing and storing the report-uri. (My first patch and first review request.)
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review87168

This seems like a good approach. I think there are a few minor bugs that might make this go more smoothly if we addressed them first (I noted them below). I also noted some style nits - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style should cover most situations if you have any questions (there are also some areas where PSM deviates from the style guidelines either intentionally or due to legacy code we haven't updated yet).

::: security/manager/ssl/nsISiteSecurityService.idl:85
(Diff revision 1)
>                         in string aHeader,
>                         in nsISSLStatus aSSLStatus,
>                         in uint32_t aFlags,
>                         [optional] out unsigned long long aMaxAge,
>                         [optional] out boolean aIncludeSubdomains,
> +                       [optional] out nsIURI aReportUri,

Since URI is an acronym, let's upper-case the whole thing when naming variables.

::: security/manager/ssl/nsISiteSecurityService.idl:195
(Diff revision 1)
>       *        default)
>       */
>       boolean setKeyPins(in string aHost, in boolean aIncludeSubdomains,
>                          in int64_t aExpires, in unsigned long aPinCount,
>                          [array, size_is(aPinCount)] in string aSha256Pins,
> +                        in nsIURI aReportUri,

If this is optional, it should have the [optional] property.

::: security/manager/ssl/nsSiteSecurityService.cpp:123
(Diff revision 1)
>  
>  SiteHPKPState::SiteHPKPState(nsCString& aStateString)
>    : mExpireTime(0)
>    , mState(SecurityPropertyUnset)
>    , mIncludeSubdomains(false)
> +  , mReportUri(nullptr) 

nit: watch out for trailing whitespace here and elsewhere

::: security/manager/ssl/nsSiteSecurityService.cpp:128
(Diff revision 1)
>    uint32_t hpkpIncludeSubdomains = 0; // PR_sscanf doesn't handle bools.
> -  const uint32_t MaxMergedHPKPPinSize = 1024;
> -  char mergedHPKPins[MaxMergedHPKPPinSize];
> -  memset(mergedHPKPins, 0, MaxMergedHPKPPinSize);
>  
> -  if (aStateString.Length() >= MaxMergedHPKPPinSize) {
> +  const uint32_t MaxStateStringSize = 1024;

We should really increase this maximum size (see bug 1190127).

::: security/manager/ssl/nsSiteSecurityService.cpp:130
(Diff revision 1)
> -  char mergedHPKPins[MaxMergedHPKPPinSize];
> -  memset(mergedHPKPins, 0, MaxMergedHPKPPinSize);
>  
> -  if (aStateString.Length() >= MaxMergedHPKPPinSize) {
> +  const uint32_t MaxStateStringSize = 1024;
> +
> +  //TODO: I've been lead to believe that uri->GetSpec returns an ASCII string 

Well, GetAsciiSpec probably does.

::: security/manager/ssl/nsSiteSecurityService.cpp:145
(Diff revision 1)
>      SSSLOG(("SSS: Cannot parse PKPState string, too large\n"));
>      return;
>    }
>  
> -  int32_t matches = PR_sscanf(aStateString.get(), "%lld,%lu,%lu,%s",
> +  //Report URI was added later and may not be present
> +  int32_t matches = PR_sscanf(aStateString.get(), "%lld,%lu,%lu,%[^,],%[^,]",

It would be really great to not use PR_sscanf here - see bug 1174555 (I'm thinking a tokenizer would be best - there's already platform support for them, I believe).

::: security/manager/ssl/nsSiteSecurityService.cpp:178
(Diff revision 1)
>      if (mSHA256keys.IsEmpty()) {
>        valid = false;
>      }
>    }
> +
> +  if(valid && (0 < strnlen(collectedReportUri, MaxStateStringSize))) {

style nits: space after 'if', avoid Yoda notation

::: security/manager/ssl/nsSiteSecurityService.cpp:194
(Diff revision 1)
>    if (valid) {
>      mState = (SecurityPropertyState)hpkpState;
>      mIncludeSubdomains = (hpkpIncludeSubdomains == 1);
> +    mReportUri = reportUri;
>    } else {
> -    SSSLOG(("%s is not a valid SiteHPKPState", aStateString.get()));
> +    SSSLOG(("%s (len=%i) is not a valid SiteHPKPState", aStateString.get(), aStateString.Length()));

style nit: watch out for lines >80 characters

::: security/manager/ssl/nsSiteSecurityService.cpp:232
(Diff revision 1)
>    for (unsigned int i = 0; i < mSHA256keys.Length(); i++) {
>      aString.Append(mSHA256keys[i]);
>    }
> +  
> +  aString.Append(',');
> +  if(mReportUri != nullptr) {

nit: 'if (mReportUri) {' is the preferred style

::: security/manager/ssl/nsSiteSecurityService.cpp:473
(Diff revision 1)
>                   aType == nsISiteSecurityService::HEADER_HPKP,
>                   NS_ERROR_NOT_IMPLEMENTED);
>  
>    NS_ENSURE_ARG(aSSLStatus);
>    return ProcessHeaderInternal(aType, aSourceURI, aHeader, aSSLStatus, aFlags,
> -                               aMaxAge, aIncludeSubdomains, aFailureResult);
> +                               aMaxAge, aIncludeSubdomains, aReportUri, aFailureResult);

Rather than passing around a nsIURI** (that may or may not be null), let's have a local nsCOMPtr<nsIURI> and pass a reference to it instead. Then, after calling ProcessHeaderInternal, we can just:

```
if (aReportUri) {
  *aReportUri = reportUri.forget();
}
```

::: security/manager/ssl/nsSiteSecurityService.cpp:695
(Diff revision 1)
>     } else if (aType == nsISiteSecurityService::HEADER_HPKP &&
>                directive->mName.Length() == report_uri_var.Length() &&
>                directive->mName.EqualsIgnoreCase(report_uri_var.get(),
>                                                  report_uri_var.Length())) {
> -       // We don't support the report-uri yet, but to avoid unrecognized
> -       // directive warnings, we still have to handle its presence
> +       // We don't support the report-uri yet, but we have begun parsing it
> +      if(reportUri == nullptr) 

style nit: 'if (!reportUri)' is sufficient. Also, always have braces around conditional bodies.

::: security/manager/ssl/nsSiteSecurityService.cpp:710
(Diff revision 1)
> +      if (directive->mValue.Length() == 0) {
> +        SSSLOG(("SSS: report-uri directive empty"));
> +        return nsISiteSecurityService::ERROR_EMPTY_REPORTURI;
> +      } 
> +
> +      nsresult rv = NS_NewURI(reportUri, directive->mValue, "", aSourceURI);

From my reading of the spec, it's unclear that report-uri may be relative (and, if it's not, does NS_NewURI do the correct thing here?)

::: security/manager/ssl/nsSiteSecurityService.cpp
(Diff revision 1)
>  {
>    if (aFailureResult) {
>      *aFailureResult = nsISiteSecurityService::ERROR_UNKNOWN;
>    }
>    SSSLOG(("SSS: processing HPKP header '%s'", aHeader));
> -  NS_ENSURE_ARG(aSSLStatus);

This is actually an essential part of the algorithm.  If this is making tests difficult, you should be able to make a FakeSSLStatus given an nsIX509Cert in xpcshell: https://dxr.mozilla.org/mozilla-central/rev/c845bfd0accb7e0c29b41713255963b08006e701/security/manager/ssl/tests/unit/head_psm.js#634

It looks like perhaps TestSTSParser.cpp should be converted to an xpcshell test (or, just don't modify that file and add a new (or modify an existing) xpcshell test).

::: security/manager/ssl/nsSiteSecurityService.cpp:905
(Diff revision 1)
>      *aIncludeSubdomains = foundIncludeSubdomains;
>    }
>  
> +  if (aReportUri != nullptr) {
> +    NS_IF_ADDREF(reportUri);
> +    *aReportUri = reportUri;

I think this should be '*aReportUri = reportUri.forget();' (NS_IF_ADDREF shouldn't be necessary)

::: security/manager/ssl/tests/compiled/TestSTSParser.cpp:141
(Diff revision 1)
>      rvs.AppendElement(TestSuccess("MAX-age  =100", false, 100, false, sss));
>      rvs.AppendElement(TestSuccess("max-AGE=100", false, 100, false, sss));
>      rvs.AppendElement(TestSuccess("Max-Age = 100 ", false, 100, false, sss));
>      rvs.AppendElement(TestSuccess("MAX-AGE = 100 ", false, 100, false, sss));
>  
> +    rvs.AppendElement(TestSuccess("maX-aGe=\"100\"", false, 100, false, sss));

These additional testcases seem unrelated to the changes in this bug - should this be a separate bug?

::: security/manager/ssl/tests/compiled/TestSTSParser.cpp:174
(Diff revision 1)
>      rvs.AppendElement(TestSuccess("max-age=100 ; includesubdomainsSomeStuff", true, 100, false, sss));
> -    rvs.AppendElement(TestSuccess("\r\n\t\t    \tcompletelyUnrelated = foobar; max-age= 34520103    \t \t; alsoUnrelated;asIsThis;\tincludeSubdomains\t\t \t", true, 34520103, true, sss));
> +    rvs.AppendElement(TestSuccess("\r\n\t\t    \tcompletelyUnrelated = foobar; max-age= 5184000    \t \t; alsoUnrelated;asIsThis;\tincludeSubdomains\t\t \t", true, 5184000, true, sss));
>      rvs.AppendElement(TestSuccess("max-age=100; unrelated=\"quoted \\\"thingy\\\"\"", true, 100, false, sss));
>  
> +    //Test capitalization
> +    rvs.AppendElement(TestSuccess("max-age=100;report-uri=\"https://example.com/pkp\"", false, 100, false, "https://example.com/pkp", sss));

I suspect TestSTSParser.cpp may not be the best place for these tests (if functionality can be tested in xpcshell, that will often be the best place. See e.g. https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_pinning_header_parsing.js ).

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:6
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

A description about what this test does would be helpful. Also, "HPKPTest" isn't too accurate or descriptive. Maybe "HPKPStorageRepresentationTest.cpp"? I guess that's a bit verbose.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:9
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsSiteSecurityService.h"
> +#include "nsIURI.h"
> +#include "OCSPCache.h"

I'm surprised you need many of these #includes.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:30
(Diff revision 1)
> +protected:
> +  psm_HPKPTest() : now(Now()) { }
> +
> +  static void SetUpTestCase()
> +  {
> +    NSS_NoDB_Init(nullptr);

I'm surprised NSS needs to be initialized for this test.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:75
(Diff revision 1)
> +  ASSERT_TRUE(c.mSHA256keys[1] == nsCString("pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ="));
> +  nsAutoCString spec; 
> +  c.mReportUri->GetSpec(spec);
> +  ASSERT_TRUE(spec == nsCString("https://example.com/pkp"));
> +}
> +

style nit: this last blank line seems unnecessary
Attachment #8804049 - Flags: review?(dkeeler)
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review87168

> From my reading of the spec, it's unclear that report-uri may be relative (and, if it's not, does NS_NewURI do the correct thing here?)

NS_NewURI _does_ do the correct thing here, at least according to the tests I added which gives me good confidence. I can't find any actual discussion of whether the report-uri was intended to be allowed to be relative or not. I believe Relative URIs are supported by Chrome, but am not 100% certain.
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review88140

This looks decent. It might be easier if we assumed report-uri has to absolute for now, but up to you.

I noted some other things in addition to what keeler already mentioned.

Remember to run `./mach xpcshell-test security/manager/ssl/tests/unit/` and ensure all tests pass even with your patch applied - looks like some tests currently fail after this patch is applied.

::: dom/locales/en-US/chrome/security/security.properties:42
(Diff revision 1)
>  PKPInvalidMaxAge=Public-Key-Pins: The site specified a header that included an invalid ‘max-age’ directive.
>  PKPMultipleIncludeSubdomains=Public-Key-Pins: The site specified a header that included multiple ‘includeSubDomains’ directives.
>  PKPInvalidIncludeSubdomains=Public-Key-Pins: The site specified a header that included an invalid ‘includeSubDomains’ directive.
>  PKPInvalidPin=Public-Key-Pins: The site specified a header that included an invalid pin.
>  PKPMultipleReportURIs=Public-Key-Pins: The site specified a header that included multiple ‘report-uri’ directives.
> +PKPEmptyReportURI=Public-Key-Pins: The site specified a header that included a single, empty ‘report-uri’ directive.

Hmm, I don't think the "single" part is necessary, but up to you.

::: netwerk/protocol/http/nsHttpChannel.cpp:1525
(Diff revision 1)
> +        case nsISiteSecurityService::ERROR_EMPTY_REPORTURI:
> +            consoleErrorTag = NS_LITERAL_STRING("PKPEmptyReportURI");
> +            break;
> +        case nsISiteSecurityService::ERROR_INVALID_REPORTURI:
> +            consoleErrorTag = NS_LITERAL_STRING("PKPInvalidReportURI");
> +            break;

devtools/client/webconsole/test/browser_webconsole_hpkp_invalid-headers.js should be expanded to cover these cases as well.
The changes will probably require review from a devtools peer.

(In fact, it might be easier to split out the console warning work into a separate commit, or a followup bug.)

::: security/manager/ssl/nsSiteSecurityService.cpp:144
(Diff revision 1)
> +  if (aStateString.Length() >= MaxStateStringSize) {
>      SSSLOG(("SSS: Cannot parse PKPState string, too large\n"));
>      return;
>    }
>  
> -  int32_t matches = PR_sscanf(aStateString.get(), "%lld,%lu,%lu,%s",
> +  //Report URI was added later and may not be present

Nit: Space between // and R, same sort of thing below.

::: security/manager/ssl/nsSiteSecurityService.cpp:194
(Diff revision 1)
>    if (valid) {
>      mState = (SecurityPropertyState)hpkpState;
>      mIncludeSubdomains = (hpkpIncludeSubdomains == 1);
> +    mReportUri = reportUri;
>    } else {
> -    SSSLOG(("%s is not a valid SiteHPKPState", aStateString.get()));
> +    SSSLOG(("%s (len=%i) is not a valid SiteHPKPState", aStateString.get(), aStateString.Length()));

%u? nsCString.Length() returns a uint32_t.

::: security/manager/ssl/nsSiteSecurityService.cpp:234
(Diff revision 1)
>    }
> +  
> +  aString.Append(',');
> +  if(mReportUri != nullptr) {
> +    nsAutoCString spec; 
> +    mReportUri->GetSpec(spec);

The return value should be checked or otherwise handled in some way.

::: security/manager/ssl/nsSiteSecurityService.cpp:705
(Diff revision 1)
>          return nsISiteSecurityService::ERROR_MULTIPLE_REPORT_URIS;
>        }
> -      SSSLOG(("SSS: found report-uri directive"));
> -      foundReportURI = true;
> +      SSSLOG(("SSS: found report-uri directive '%s'", directive->mValue.get()));
> +      foundReportUri = true;
> +
> +      if (directive->mValue.Length() == 0) {

Nit: `mValue.IsEmpty()` is probably clearer.

::: security/manager/ssl/nsSiteSecurityService.cpp:710
(Diff revision 1)
> +      if (directive->mValue.Length() == 0) {
> +        SSSLOG(("SSS: report-uri directive empty"));
> +        return nsISiteSecurityService::ERROR_EMPTY_REPORTURI;
> +      } 
> +
> +      nsresult rv = NS_NewURI(reportUri, directive->mValue, "", aSourceURI);

Maybe use nullptr for the |charset| param? AFAICT the HTTP(S) protocol handler treats them the same, but nullptr matches the default for NS_NewURI().

::: security/manager/ssl/nsSiteSecurityService.cpp:905
(Diff revision 1)
>      *aIncludeSubdomains = foundIncludeSubdomains;
>    }
>  
> +  if (aReportUri != nullptr) {
> +    NS_IF_ADDREF(reportUri);
> +    *aReportUri = reportUri;

I think what keeler meant was `reportUri.forget(aReportUri);`.

::: security/manager/ssl/tests/compiled/TestSTSParser.cpp:200
(Diff revision 1)
> +    TODO: These fail. But they may be allowed? Chrome seems to accept them?
> +    //Test single quotes

RFC7469 Section 2.1 says the syntax from RFC7230 Section 3.2.6 applies, and AFAICT, single quotes are not allowed.
Maybe it's a bug in Chrome if single quotes are accepted.

::: security/manager/ssl/tests/compiled/TestSTSParser.cpp:208
(Diff revision 1)
> +    TODO: Are these not allowed? The spec makes me think yes, but I think Chrome does not support them.
> +    //Test without quotes

IIUC what RFC7230 Section 3.2.6 is specifying, I think the answer is that they are not allowed - all of the following cases except the last contain one or more delimeters, so quotes are necessary.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:43
(Diff revision 1)
> +  //python -c 'import hashlib;import base64; import sys; sys.stdout.write(base64.b64encode(hashlib.sha256("whate3ver").digest()))'
> +
> +  //Test old-style with no comma at the end
> +  nsCString val1("123456,1,1,hXOPj5p/GwS1MpxZDry55CWSXG0JhAicQ6Ai3k8ZwoE=pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ=");
> +  SiteHPKPState a(val1);
> +  ASSERT_TRUE(a.mExpireTime == 123456);

Nit: This should probably be something like `EXPECT_EQ(123456, a.mExpireTime)` instead.
In other words, try to stick to EXPECT and use the expected value as the first argument.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:44
(Diff revision 1)
> +
> +  //Test old-style with no comma at the end
> +  nsCString val1("123456,1,1,hXOPj5p/GwS1MpxZDry55CWSXG0JhAicQ6Ai3k8ZwoE=pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ=");
> +  SiteHPKPState a(val1);
> +  ASSERT_TRUE(a.mExpireTime == 123456);
> +  ASSERT_TRUE(a.mState == 1);//State must be one for pins to be parsed

Nit: `EXPECT_EQ(SecurityPropertySet, a.mState)` seems more intuitive.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:45
(Diff revision 1)
> +  //Test old-style with no comma at the end
> +  nsCString val1("123456,1,1,hXOPj5p/GwS1MpxZDry55CWSXG0JhAicQ6Ai3k8ZwoE=pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ=");
> +  SiteHPKPState a(val1);
> +  ASSERT_TRUE(a.mExpireTime == 123456);
> +  ASSERT_TRUE(a.mState == 1);//State must be one for pins to be parsed
> +  ASSERT_TRUE(a.mIncludeSubdomains == true);

Nit: `EXPECT_TRUE(a.mIncludeSubdomains)`.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:72
(Diff revision 1)
> +  ASSERT_TRUE(c.mIncludeSubdomains == true);
> +  ASSERT_TRUE(c.mSHA256keys.Length() == 2);
> +  ASSERT_TRUE(c.mSHA256keys[0] == nsCString("hXOPj5p/GwS1MpxZDry55CWSXG0JhAicQ6Ai3k8ZwoE="));
> +  ASSERT_TRUE(c.mSHA256keys[1] == nsCString("pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ="));
> +  nsAutoCString spec; 
> +  c.mReportUri->GetSpec(spec);

Handle return value.
Attachment #8804049 - Flags: review?(cykesiopka.bmo)
Okay, I've addressed many but not all of the issues :Cykesiopka and :keeler raised (thank you!) and pushed a new review request with only :mgoodwin flagged. I'm going to keep working on this but #1190127 might be a tricky one to address so I'm going to start a discussion there.
Depends on: 1190127, 1174555
Assignee: nobody → tom
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [psm-backlog] → [psm-assigned]
Attachment #8804049 - Flags: review?(mgoodwin)
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review90410

I realise the changes here aren't final, but I just wanted to point out a few things.

::: security/manager/ssl/nsSiteSecurityService.cpp:230
(Diff revision 3)
> +    nsresult rv;
> +
> +    nsAutoCString spec; 
> +    rv = mReportURI->GetAsciiSpec(spec);

Nit: Please avoid unnecessarily splitting up variable declaration and initialisation.

::: security/manager/ssl/tests/compiled/TestSTSParser.cpp:143
(Diff revision 3)
> +    rvs.AppendElement(TestSuccess("maX-aGe=\"100\"", false, 100, false, sss));
> +    rvs.AppendElement(TestSuccess("MAX-age  =\"100\"", false, 100, false, sss));
> +    rvs.AppendElement(TestSuccess("max-AGE=\"100\"", false, 100, false, sss));
> +    rvs.AppendElement(TestSuccess("Max-Age = \"100\" ", false, 100, false, sss));
> +    rvs.AppendElement(TestSuccess("MAX-AGE = \"100\" ", false, 100, false, sss));

FYI, as part of the work for Bug 1313141, GeckoCppUnitTests are being converted to GTests, including this test, so that's another reason to not bother adding these test cases here.

::: security/manager/ssl/tests/gtest/HPKPTest.cpp:27
(Diff revision 3)
> +  nsCString val1("123456,1,1,hXOPj5p/GwS1MpxZDry55CWSXG0JhAicQ6Ai3k8ZwoE=pIg4hW3UeZefhBM+NU6KF1iXA6zPBcO3C8pGXy+DuJQ=");
> +  SiteHPKPState a(val1);
> +  EXPECT_EQ(123456, a.mExpireTime);
> +  EXPECT_EQ(SecurityPropertySet, a.mState);//State must be one for pins to be parsed
> +  EXPECT_TRUE(a.mIncludeSubdomains);
> +  EXPECT_TRUE(2 == a.mSHA256keys.Length());

This should probably be:
```cpp
ASSERT_EQ(2, a.mSHA256keys.Length());
```
instead, since we do want to stop further execution if the length is wrong.

I guess I wasn't clear the first time, sorry.
Comment on attachment 8804049 [details]
Bug 1091176 - Parsing and store report-uri for HPKP

https://reviewboard.mozilla.org/r/88188/#review102664

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:217
(Diff revision 3)
>    checkOK(certFromFile('b.preload.example.com-badca'), "b.preload.example.com");
>    // then we add a pin, and we should get a failure (ensuring the expiry is
>    // after the test timeout)
>    gSSService.setKeyPins("b.preload.example.com", false,
>                          new Date().getTime() + 1000000, 2,
> -                        [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], true);
> +                        [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], null, true);

There is one addon that uses this API such that changing the function signature would break it.
Which addon is that?
Assignee: tom → administration
Status: ASSIGNED → NEW
Whiteboard: [psm-assigned]
Assignee: administration → nobody
HPKP is looking like something we're not going to be investing much more in, so I think this is a wontfix.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WONTFIX
The limitation that this isn't implemented is documented at https://developer.mozilla.org/en-US/docs/Web/HTTP/Public_Key_Pinning and in the compat data.
You need to log in before you can comment on or make changes to this bug.