1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom

Status: RESOLVED FIXED

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

Description

[Sid Stamm [:geekboy or :sstamm]]

After moving HSTS storage from nsIPermissionManager to DataStorage, the HSTS entries are no longer enumerable (so data about which sites have HSTS set is really hard to extract).  We should extend nsISiteSecurityService to allow enumeration of its data for extensions and possibly a built-in management UI for site security settings (if it is in the future of Firefox).

I propose we add an enumerate function to the IDL, that takes one parameter (the type of security setting to enumerate, e.g., HSTS):

> nsISimpleEnumerator enumerate(in uint32_t aType); 

And then we'll probably have to expose some simple XPCOM data structures for the enumerator that will pair the site settings to a hostnames.  Roughly, my initial thought is this:

> interface nsISiteHSTSState : nsISupports
> {
>   readonly attribute boolean isExpired;
>   attribute long expireTime;
>   attribute short securityPropertyState;
>   attribute boolean includeSubdomains;
> };
> 
> interface nsISSSEntry : nsISupports {
>   attribute uint32_t type;
>   attribute string hostname;
>   attribute nsISupports state;
> };

We can also add an nsISiteHPKPState structure for HPKP data or any future types of security state stored by this service.

To use the enumerator via JS, one could do something like:

>  var sss = Cc['@mozilla.org/ssservice;1'].getService(Ci.nsISiteSecurityService);
>   var enumerator = sss.enumerate(Ci.nsISiteSecurityService.HEADER_HSTS);
>   while (enumerator.hasMoreElements()) {
>     var entry = enumerator.getNext().QueryInterface(Ci.nsISSSEntry);
>     var host = entry.hostname;
>     var hsts = entry.state.QueryInterface(Ci.nsISiteHSTSState);
>     ...

David, what do you think of this general approach?  I'll whip together a prototype implementation.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I think this is a good approach. My one suggestion would be to get rid of the nsISSSEntry interface and move the hostname to the nsISiteHSTSState interface. My thinking is that since .enumerate takes what type of entry we're looking for anyway, having an abstract type nsISSSEntry that we then have to QI to nsISiteHSTSState is unnecessary.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Hi Sid, do you mind if I take this bug?  It blocks the first party isolation of HSTS and HPKP storage.

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: make DataStorage for HPKP and HSTS enumerable via xpcom. (WIP)

WIP patch.

Comment 4

[:Cykesiopka]

Comment on attachment 8825697 [details] [diff] [review]
make DataStorage for HPKP and HSTS enumerable via xpcom. (WIP)

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

I was skimming the changes and just wanted to make some minor comments.
FYI, I'm also touching nsISiteSecurityService stuff over in Bug 1329237.

::: security/manager/ssl/nsISiteSecurityService.idl
@@ +26,5 @@
>  
> +[scriptable, uuid(31313372-842c-4110-bdf1-6aea17c845ad)]
> +interface nsISiteSecurityState : nsISupports
> +{
> +  //attribute string hostname;

It would be nice to avoid `string` if possible - ACString would give us the safer nsACString& in C++ instead of raw `char*`.

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ +1402,5 @@
> +        }
> +        state = new SiteHPKPState(hostname, value);
> +        break;
> +      default:
> +        NS_ASSERTION(false, "SSS:Enumerate got invalid type");

Please use MOZ_ASSERT(false, ...) or MOZ_ASSERT_UNREACHABLE(...) instead.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review commit: https://reviewboard.mozilla.org/r/104154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104154/

Comment 6

[Jonathan Hao (inactive) [:jhao]]

I haven't written tests, but please take a look first.  Thanks.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review104984

In general, this looks like a good approach. I noted a few nits. The main question is what guarantees does this API provide? I already noted it doesn't include preloaded entries (either the hard-coded ones or the preloaded-delivered-by-kinto ones). Let's document the intended behavior of this API before going forward. Given that, I think tests will be easier to write, and then the implementation will be easier as well.

::: security/manager/ssl/nsISiteSecurityService.idl:30
(Diff revision 1)
>  [ref] native mozillaPkixTime(mozilla::pkix::Time);
>  
> +[scriptable, uuid(31313372-842c-4110-bdf1-6aea17c845ad)]
> +interface nsISiteSecurityState : nsISupports
> +{
> +  attribute ACString hostname;

These should be readonly too, right?
I imagine we can make them infallible as well.

::: security/manager/ssl/nsISiteSecurityService.idl:45
(Diff revision 1)
> +   * security property in question.
> +   */
> +  const short SECURITY_PROPERTY_UNSET = 0;
> +  const short SECURITY_PROPERTY_SET = 1;
> +  const short SECURITY_PROPERTY_KNOCKOUT = 2;
> +  const short SECURITY_PROPERTY_NEGATIVE = 3;

I would also document that ...NEGATIVE is used when we've gotten a negative result from HSTS priming.

::: security/manager/ssl/nsISiteSecurityService.idl:247
(Diff revision 1)
>       * @param aURI the nsIURI that this applies to
>       * @param aMaxAge lifetime (in seconds) of this negative cache
>       */
>      [noscript] void cacheNegativeHSTSResult(in nsIURI aURI, in unsigned long long aMaxAge);
> +
> +    nsISimpleEnumerator enumerate(in uint32_t aType);

This needs some documentation.

::: security/manager/ssl/nsSiteSecurityService.h:53
(Diff revision 1)
>   *  - An array of sha-256 hashed base 64 encoded fingerprints of required keys
>   */
> -class SiteHPKPState
> +class SiteHPKPState : public nsISiteHPKPState
>  {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

I don't think these classes are actually thread-safe (which is probably fine, since they should only live long enough to be used on a single thread).

::: security/manager/ssl/nsSiteSecurityService.h:58
(Diff revision 1)
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSISITEHPKPSTATE
> +  NS_DECL_NSISITESECURITYSTATE
> +
>    SiteHPKPState();
> -  explicit SiteHPKPState(nsCString& aStateString);
> +  explicit SiteHPKPState(const nsCString& aHost, const nsCString& aStateString);

nit: this isn't a one-argument constructor any longer, so it doesn't need to be marked explicit

::: security/manager/ssl/nsSiteSecurityService.h:92
(Diff revision 1)
>   * HSTS state consists of:
>   *  - Expiry time (PRTime (aka int64_t) in milliseconds)
>   *  - A state flag (SecurityPropertyState, default SecurityPropertyUnset)
>   *  - An include subdomains flag (bool, default false)
>   */
> -class SiteHSTSState
> +class SiteHSTSState : public nsISiteHSTSState

(same two comments with this class)

::: security/manager/ssl/nsSiteSecurityService.cpp:117
(Diff revision 1)
> +  aHostname = mHostname;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +SiteHSTSState::SetHostname(const nsACString & aHostname)

style nit: & or * to the left

::: security/manager/ssl/nsSiteSecurityService.cpp:347
(Diff revision 1)
> +NS_IMETHODIMP
> +SiteHPKPState::GetSHA256Keys(nsISimpleEnumerator** aSHA256Keys)
> +{
> +  nsCOMArray<nsIVariant> keys;
> +  for (const nsCString& key : mSHA256keys) {
> +    nsCOMPtr<nsIWritableVariant> variant = new nsVariant;

nit: I think it's best to always include the parentheses for operator new: "new nsVariant()"

::: security/manager/ssl/nsSiteSecurityService.cpp:1516
(Diff revision 1)
>  }
>  
> +NS_IMETHODIMP
> +nsSiteSecurityService::Enumerate(uint32_t aType,
> +                                 nsISimpleEnumerator** aEnumerator)
> +{

nit: NS_ENSURE_ARG(aEnumerator)
Also, we should check and return an invalid arg error if aType isn't one of the expected types.

::: security/manager/ssl/nsSiteSecurityService.cpp:1518
(Diff revision 1)
> +NS_IMETHODIMP
> +nsSiteSecurityService::Enumerate(uint32_t aType,
> +                                 nsISimpleEnumerator** aEnumerator)
> +{
> +  InfallibleTArray<mozilla::dom::DataStorageItem> items;
> +  mSiteStateStorage->GetAll(&items);

What about mPreloadStateStorage? Consumers of this API could get misleading or confusing results...

::: security/manager/ssl/nsSiteSecurityService.cpp:1525
(Diff revision 1)
> +  nsCOMArray<nsISupports> states;
> +  for (const mozilla::dom::DataStorageItem& item : items) {
> +    const nsCString& key = item.key();
> +    const nsCString& value = item.value();
> +
> +    if (key.Length() < 5 || key[key.Length() - 5] != ':') {

Instead of using the hard-coded value '5', let's do something like this:

* depending on aType, define the prefix we're looking for ("HSTS:" or "HPKP:")
* if the item's key doesn't start with the prefix, skip it
* set the hostname to the key with the prefix taken off (either by substring or replacement)

::: security/manager/ssl/nsSiteSecurityService.cpp:1553
(Diff revision 1)
> +        MOZ_ASSERT_UNREACHABLE("SSS:Enumerate got invalid type");
> +    }
> +
> +    states.AppendObject(state);
> +  }
> +  return NS_OK;

It doesn't look like aEnumerator gets assigned to anything?

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/1-2/

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review104984

For the use case in bug 1290529, we only need to enumerate and remove the non-preloaded ones.  I've documented this intended behavior.

> These should be readonly too, right?
> I imagine we can make them infallible as well.

Yes, they can be infallible except for hostname because only builtin types can.

> I don't think these classes are actually thread-safe (which is probably fine, since they should only live long enough to be used on a single thread).

I've changed them to NS_DECL_ISUPPORTS.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/2-3/

Comment 11

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105246

The general approach here is good, but this really needs a test - as it stands, enumerate() doesn't actually work properly.

::: security/manager/ssl/nsISiteSecurityService.idl:27
(Diff revision 3)
>  }
>  %}
>  [ref] native nsCStringTArrayRef(nsTArray<nsCString>);
>  [ref] native mozillaPkixTime(mozilla::pkix::Time);
>  
> +[scriptable, uuid(31313372-842c-4110-bdf1-6aea17c845ad), builtinclass]

Nit: We should probably have a comment explaining why this is a builtinclass - I for one am probably going to revisit this IDL in a few months and not understand why.

::: security/manager/ssl/nsISiteSecurityService.idl:59
(Diff revision 3)
> +};
> +
> +[scriptable, uuid(ae395078-c7d0-474d-b147-f4aa203a9b2c), builtinclass]
> +interface nsISiteHPKPState : nsISiteSecurityState
> +{
> +  readonly attribute nsISimpleEnumerator SHA256Keys;

Nit: This should be sha256Keys instead, per our style guide.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#IDL:
"When defining a method or attribute in IDL, the first letter should be lowercase"

::: security/manager/ssl/nsISiteSecurityService.idl:251
(Diff revision 3)
> +     * Enumerate the nsISiteSecurityState storage, but doesn't include preloaded
> +     * entries (either the hard-coded ones or the preloaded-delivered-by-kinto
> +     * ones)

Nit: This might read better as "Returns an enumerator of the nsISiteSecurityState storage. Doesn't include the preloaded [...]" or something similar.

::: security/manager/ssl/nsSiteSecurityService.h:44
(Diff revision 3)
>   * HPKP state consists of:
>   *  - Expiry time (PRTime (aka int64_t) in milliseconds)
>   *  - A state flag (SecurityPropertyState, default SecurityPropertyUnset)
>   *  - An include subdomains flag (bool, default false)
>   *  - An array of sha-256 hashed base 64 encoded fingerprints of required keys

Nit: Should probably update this.

::: security/manager/ssl/nsSiteSecurityService.h:87
(Diff revision 3)
>   * HSTS state consists of:
>   *  - Expiry time (PRTime (aka int64_t) in milliseconds)
>   *  - A state flag (SecurityPropertyState, default SecurityPropertyUnset)
>   *  - An include subdomains flag (bool, default false)

Nit: Should probably update this.

::: security/manager/ssl/nsSiteSecurityService.h:176
(Diff revision 3)
>  
>    uint64_t mMaxMaxAge;
>    bool mUsePreloadList;
>    int64_t mPreloadListTimeOffset;
>    bool mProcessPKPHeadersFromNonBuiltInRoots;
>    RefPtr<mozilla::DataStorage> mSiteStateStorage;

Nit: We should add an #include "mozilla/RefPtr.h" to this file for this use and the uses you're adding in nsSiteSecurityService.cpp.

::: security/manager/ssl/nsSiteSecurityService.cpp:58
(Diff revision 3)
> +const char kHSTSKeySuffix[] = ":HSTS";
> +const char kHPKPKeySuffix[] = ":HPKP";
> +
>  ////////////////////////////////////////////////////////////////////////////////
>  
> -SiteHSTSState::SiteHSTSState(nsCString& aStateString)
> +NS_IMPL_ISUPPORTS(SiteHSTSState, nsISiteHSTSState)

This needs to be
```
NS_IMPL_ISUPPORTS(SiteHSTSState, nsISiteSecurityState, nsISiteHSTSState)
```
instead, or `enumerate()` won't actually work.

Take this xpcshell test for example:
```js
"use strict";
const FAKE_HOST = "www.example.com";
do_get_profile();
function run_test() {
  let sss = Cc["@mozilla.org/ssservice;1"]
              .getService(Ci.nsISiteSecurityService);
  let uri = Services.io.newURI("https://" + FAKE_HOST);
  let sslStatus = new FakeSSLStatus();
  sss.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, uri,
                    "max-age=1234567890", sslStatus, 0);
  let entryEnumerator = sss.enumerate(Ci.nsISiteSecurityService.HEADER_HSTS);
  let entries = [];
  while (entryEnumerator.hasMoreElements()) {
    let entry = entryEnumerator.getNext();
    entries.push(entry.QueryInterface(Ci.nsISiteSecurityState));
  }
  greater(entries.length, 0, "Should get at least one HSTS entry");
  let foundFakeEntry = false;
  for (let entry of entries) {
    if (entry.hostname == FAKE_HOST) {
      foundFakeEntry = true;
      break;
    }
  }
  ok(foundFakeEntry, "Should find fake entry via enumerate()");
}
```

Here's what happens if we don't have nsISiteSecurityState in the list:
```
"\x07[5504] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file /moz/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h, line 414"
"#01: nsCOMPtr<nsISiteSecurityState>::Assert_NoQueryNeeded() (/moz/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:414 (discriminator 1))"
"#02: nsCOMPtr<nsISiteSecurityState>::operator=(nsISiteSecurityState*) (/moz/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:595)"
"#03: nsSiteSecurityService::Enumerate(unsigned int, nsISimpleEnumerator**) (/moz/mozilla-inbound/security/manager/ssl/nsSiteSecurityService.cpp:1489)"
"#04: NS_InvokeByIndex (/moz/mozilla-inbound/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:92)"
"[5504] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file /moz/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h, line 414"
"Hit MOZ_CRASH() at /moz/mozilla-inbound/memory/mozalloc/mozalloc_abort.cpp:33"
"ExceptionHandler::GenerateDump cloned child 5523"
"ExceptionHandler::SendContinueSignalToChild sent continue signal to child"
"ExceptionHandler::WaitForContinueSignal waiting for continue signal..."
```

::: security/manager/ssl/nsSiteSecurityService.cpp:120
(Diff revision 3)
> +  aHostname = mHostname;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +SiteHSTSState::GetExpireTime(int64_t* aExpireTime)

We should add checks like `NS_ENSURE_ARG(aExpireTime)` here and below.
XPConnect won't get this wrong, but we might if we happen to call the method in C++.

::: security/manager/ssl/nsSiteSecurityService.cpp:142
(Diff revision 3)
> +  return NS_OK;
> +}
> +
>  ////////////////////////////////////////////////////////////////////////////////
> +
> +NS_IMPL_ISUPPORTS(SiteHPKPState, nsISiteHPKPState)

Same NS_IMPL_ISUPPORTS() as the HSTS impl.

::: security/manager/ssl/nsSiteSecurityService.cpp:248
(Diff revision 3)
> +  aHostname = mHostname;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +SiteHPKPState::GetExpireTime(int64_t* aExpireTime)

Same NS_ENSURE_ARG() comment as the HSTS impl.

::: security/manager/ssl/nsSiteSecurityService.cpp:290
(Diff revision 3)
> +{
> +  NS_ENSURE_ARG(aSHA256Keys);
> +
> +  nsCOMArray<nsIVariant> keys;
> +  for (const nsCString& key : mSHA256keys) {
> +    nsCOMPtr<nsIWritableVariant> variant = new nsVariant;

Looks like you forgot to address keeler's comment here:
> nit: I think it's best to always include the parentheses for operator new: "new nsVariant()"

::: security/manager/ssl/nsSiteSecurityService.cpp:291
(Diff revision 3)
> +    variant->SetAsAUTF8String(key);
> +    keys.AppendObject(variant);

We should handle the return values for both of these calls.

::: security/manager/ssl/nsSiteSecurityService.cpp:1479
(Diff revision 3)
> +  InfallibleTArray<mozilla::dom::DataStorageItem> items;
> +  mSiteStateStorage->GetAll(&items);
> +
> +  nsCOMArray<nsISiteSecurityState> states;
> +  for (const mozilla::dom::DataStorageItem& item : items) {
> +    if (!StringEndsWith(item.key(), keySuffix)) {

Nit: This needs a corresponding #include "nsReadableUtils.h".

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105316

Just a few more minor comments...

::: security/manager/ssl/nsISiteSecurityService.idl:254
(Diff revisions 1 - 3)
>  
> +    /**
> +     * Enumerate the nsISiteSecurityState storage, but doesn't include preloaded
> +     * entries (either the hard-coded ones or the preloaded-delivered-by-kinto
> +     * ones)
> +     *

I would add that each item in the enumeration is a nsISiteSecurityState that can be QueryInterfaced to the appropriate nsISiteHSTSState or nsISiteHPKPState, depending on the provided type.

::: security/manager/ssl/nsSiteSecurityService.cpp:393
(Diff revisions 1 - 3)
>  SetStorageKey(nsAutoCString& storageKey, const nsACString& hostname, uint32_t aType)
>  {
>    storageKey = hostname;
>    switch (aType) {
>      case nsISiteSecurityService::HEADER_HSTS:
> -      storageKey.AppendLiteral(":HSTS");
> +      storageKey.AppendASCII(kHSTSKeySuffix);

Oh, they're suffixes. That makes much more sense.

::: security/manager/ssl/nsSiteSecurityService.cpp:1484
(Diff revisions 1 - 3)
> -      return NS_ERROR_FAILURE;
>      }
> -    nsCString hostname = key;
> -    hostname.Truncate(hostname.Length() - 5);
>  
> -    nsCOMPtr<nsISupports> state;
> +    nsCString hostname(StringHead(item.key(), item.key().Length() - 5));

Let's use keySuffix.Length() instead of 5

Comment 13

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105246

> Same NS_IMPL_ISUPPORTS() as the HSTS impl.

Oops, I meant this line needs to be fixed along with the equivalent line in the HSTS impl.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/3-4/

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Sorry for not having the tests ready in the first place.  I extended Cykesiopka's xpcshell test.  Please take another look.  Thanks.

Comment 16

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105686

This looks good, modulo some minor style issues.
However, it should be relatively straightforward to make the test cover all the functionality being added here, so I think we should do that.

::: security/manager/ssl/nsISiteSecurityService.idl:254
(Diff revision 4)
>       * @param aMaxAge lifetime (in seconds) of this negative cache
>       */
>      [noscript] void cacheNegativeHSTSResult(in nsIURI aURI, in unsigned long long aMaxAge);
> +
> +    /**
> +     * Returns an enumerator of the nsISiteSecurityState storage. Each item in

Nit: Whoops, sorry, I missed in my original review that this should probably be nsISiteSecurityService instead.

::: security/manager/ssl/nsSiteSecurityService.cpp:299
(Diff revision 4)
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    bool success = keys.AppendObject(variant);
> +    NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);

Nit: We generally prefer plain `if` statements over the NS_ENSURE macros.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:1
(Diff revision 4)
> +"use strict";

Nit: We generally prefer an explicit license block.
Public Domain should be appropriate.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:3
(Diff revision 4)
> +const NON_ISSUED_KEY_HASHES = ["AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
> +                               "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB="];

I personally think cuddle braces:
```js
const NON_ISSUED_KEY_HASHES = [
  "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
  "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB=",
];
```
looks nicer for lists like these, but not a big deal.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:7
(Diff revision 4)
> +const HOSTNAMES = ["bugzilla.mozilla.org",
> +                   "a.example.com",
> +                   "b.example.com",
> +                   "c.c.example.com",
> +                   "d.example.com"];

We should test the other attributes as well.
 - securityPropertyState should be straightforward.
 - includeSubdomains should be straightforward.
 - expireTime looks straightfoward for HPKP.
   For HSTS we can maybe do a fuzzy equals check.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:25
(Diff revision 4)
> +  equal(entries.length, HOSTNAMES.length,
> +        "Should get correct number of HSTS entries");
> +  return entries;
> +}
> +
> +do_get_profile();

Nit: We generally put this call before any function declarations.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:27
(Diff revision 4)
> +  return entries;
> +}
> +
> +do_get_profile();
> +function run_test() {
> +  sss.clearAll();

We should add some tests to ensure `enumerate()` doesn't return our test headers after this is called.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/4-5/

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105686

> Nit: We generally prefer plain `if` statements over the NS_ENSURE macros.

Thanks.  How about the NS_ENSURE_SUCCESS(rv, rv)?

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106128

Great! LGTM with comments addressed.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:17
(Diff revision 5)
> +  "a.example.com",
> +  "b.example.com",
> +  "c.c.example.com",
> +  "d.example.com"
> +];
> +const EXPIRE_TIME = Date.now() + 1234567890;

Let's have this value be in seconds (and multiply by 1000 as appropriate) to avoid the workaround on line 48.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:23
(Diff revision 5)
> +const HSTS_PREFIX = "hsts.";
> +const HPKP_PREFIX = "hpkp.";
> +
> +let sss = Cc["@mozilla.org/ssservice;1"].getService(Ci.nsISiteSecurityService);
> +
> +do_get_profile();

It would probably be best to call do_get_profile before instantiating nsISiteSecurityService (otherwise NSS will be initialized without a profile, which isn't representative of how this will work in the wild).

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:38
(Diff revision 5)
> +}
> +
> +function checkSiteSecurityStateAttrs(entries, typePrefix) {
> +  equal(entries.length, HOSTNAMES.length,
> +        "Should get correct number of entries");
> +  for (let entry of entries) {

This checks that every entry we get back is one we put in there, but it also seems important to check that we get back each entry we put in (we almost get this with the length equality check, but what if we just get back a bunch of entries that are all for "bugzilla.mozilla.org"?)

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:73
(Diff revision 5)
> +    let maxAge = Math.round((EXPIRE_TIME - Date.now()) / 1000);
> +    // Insert HSTS entry.
> +    sss.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, uri,
> +                      `max-age=${maxAge}; includeSubdomains`, sslStatus, 0);
> +    // Insert HPKP entry.
> +    sss.setKeyPins(HPKP_PREFIX + hostname, true, EXPIRE_TIME, 2,

It might be better to also use processHeader for HPKP since that's how users will encounter these settings.

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/5-6/

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106128

> Let's have this value be in seconds (and multiply by 1000 as appropriate) to avoid the workaround on line 48.

I can make this value be in seconds, but I don't think we can change `less` to `equal` on line 48.  By the time the SSService convert the max age to expire time, the PR_Now() it uses may differ from Date.now() even in seconds.

> This checks that every entry we get back is one we put in there, but it also seems important to check that we get back each entry we put in (we almost get this with the length equality check, but what if we just get back a bunch of entries that are all for "bugzilla.mozilla.org"?)

You're right.  I'll sort the arrays and compare them one by one.

> It might be better to also use processHeader for HPKP since that's how users will encounter these settings.

Sure, but in order to use processHeader for HPKP, I had to add some more codes to setup the certs and such.  Do you want to check again?

Comment 22

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review105686

> Thanks.  How about the NS_ENSURE_SUCCESS(rv, rv)?

Yeah, I should've been more clear, sorry.

In general, we avoid the NS_ENSURE macros for anything that's not argument checking at the top of a function.
However, we might still use the macro if most of the nearby code does as well for consistency.

For argument checks, both explicit `if` checks and NS_ENSURE_ARG etc are fine.

Comment 23

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106342

Looks good with comments addressed.
Thanks for doing this.

::: security/manager/ssl/nsSiteSecurityService.cpp:299
(Diff revision 6)
> +
> +  nsCOMArray<nsIVariant> keys;
> +  for (const nsCString& key : mSHA256keys) {
> +    nsCOMPtr<nsIWritableVariant> variant = new nsVariant();
> +    nsresult rv = variant->SetAsAUTF8String(key);
> +    NS_ENSURE_SUCCESS(rv, rv);

Nit: Just a reminder that we usually use explicit `if` checks in this situation.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:7
(Diff revision 6)
> + * 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/. */
> +"use strict";
> +
> +// This had better not be larger than the maximum maxAge for HPKP.
> +const GOOD_MAX_AGE_SECONDS = 69403;

I'm guessing you copied this from test_forget_about_site_security_headers.js?
I wonder if the value is significant... Anyways, just wondering out loud, nothing to fix here.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:17
(Diff revision 6)
> +const HSTS_HOSTNAMES = [
> +  "bugzilla.mozilla.org",
> +  "a.example.com",
> +  "b.example.com",
> +  "c.c.example.com",
> +  "d.example.com"

Nit: Please add a trailing comma.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:62
(Diff revision 6)
> +
> +  for (let entry of entries) {
> +    equal(entry.securityPropertyState,
> +          Ci.nsISiteSecurityState.SECURITY_PROPERTY_SET,
> +          "Entries should have security property set");
> +    equal(entry.includeSubdomains, true,

We should ideally test that `includeSubdomains` is false when we expect it to be as well. 
Maybe `HSTS_HOSTNAMES` and `HPKP_HOSTNAMES` can be modified to look something like this:
```js
const HSTS_TESTCASES = [
  { hostname: "bugzilla.mozilla.org", includeSubdomains: true },
  { hostname: "a.example.com", includeSubdomains: false },
];
```

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106466

LGTM - just the one comment.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:65
(Diff revisions 5 - 6)
>            Ci.nsISiteSecurityState.SECURITY_PROPERTY_SET,
>            "Entries should have security property set");
>      equal(entry.includeSubdomains, true,
>            "Entries should include subdomains");
> -    switch (typePrefix) {
> -      case HSTS_PREFIX:
> +    less(Math.abs(entry.expireTime - EXPIRE_TIME_SEC * 1000), 2000,
> +         "Entries should have expireTime within 2-second error");

Oh, I see. We should comment that there's a delay from our "now" and the "now" that the implementation uses. Also, 2 seconds might not be enough on slow platforms. Maybe 60 seconds? (Or even more?)

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/6-7/

Comment 26

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106466

> Oh, I see. We should comment that there's a delay from our "now" and the "now" that the implementation uses. Also, 2 seconds might not be enough on slow platforms. Maybe 60 seconds? (Or even more?)

I'll use 60 seconds.  I assume we don't need to have expireTime in seconds anymore, do we?

Comment 27

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106342

> We should ideally test that `includeSubdomains` is false when we expect it to be as well. 
> Maybe `HSTS_HOSTNAMES` and `HPKP_HOSTNAMES` can be modified to look something like this:
> ```js
> const HSTS_TESTCASES = [
>   { hostname: "bugzilla.mozilla.org", includeSubdomains: true },
>   { hostname: "a.example.com", includeSubdomains: false },
> ];
> ```

I've changed a fair amount of code to do this.  Even you've all r+'ed, please let me know if there's any problem.  Thanks, guys.

Comment 28

[:Cykesiopka]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106754

LGTM, just needs a minor wording update.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:80
(Diff revision 7)
> +    equal(entries[i].hostname, TESTCASES[i].hostname, "Hostnames should match");
> +    equal(entries[i].securityPropertyState,
> +          Ci.nsISiteSecurityState.SECURITY_PROPERTY_SET,
> +          "Entries should have security property set");
> +    equal(entries[i].includeSubdomains, TESTCASES[i].includeSubdomains,
> +          "Entries should include subdomains");

Looks like this needs updating.

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

https://reviewboard.mozilla.org/r/104154/#review106784

Looks good. I had an additional thought about the maxAge values, though.

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:14
(Diff revisions 6 - 7)
>  const KEY_HASHES = [ NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH ];
> -const HSTS_HOSTNAMES = [
> -  "bugzilla.mozilla.org",
> -  "a.example.com",
> -  "b.example.com",
> -  "c.c.example.com",
> +const TESTCASES = [
> +  {
> +    hostname: "a.pinning2.example.com",
> +    includeSubdomains: true,
> +    expireTime: Date.now() + 12345,

12 seconds seems too short, particularly since we have a 60 second error window. Maybe something more like 12 weeks?

::: security/manager/ssl/tests/unit/test_sss_enumerate.js:19
(Diff revisions 6 - 7)
> -  "c.c.example.com",
> -  "d.example.com"
> -].sort();
> -const HPKP_HOSTNAMES = [
> -  "a.pinning2.example.com",
> -  "b.pinning2.example.com",
> +    expireTime: Date.now() + 12345,
> +  },
> +  {
> +    hostname: "b.pinning2.example.com",
> +    includeSubdomains: false,
> +    expireTime: Date.now() + 67890,

Same idea.

Comment 30

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8826137 [details]
Bug 1115712 - make DataStorage for HPKP and HSTS enumerable via xpcom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104154/diff/7-8/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fd9ab0fdefa
make DataStorage for HPKP and HSTS enumerable via xpcom. r=Cykesiopka,keeler

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fd9ab0fdefa

Comment 33

[Ethan Tseng [:ethan]]

Jonathan, thanks for getting this done.  Good job!
Also thanks for the reviews, David and Cykesiopka.