1240173 - improve nsIX509Cert.dbKey api

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsIX509Cert.dbKey is not a great api:

  /**
   *  A unique identifier of this certificate within the local storage.
   */
  readonly attribute string dbKey;

a) Using string means C++ callers have to manually free the memory allocated.
b) It returns a base64-encoded string with CRLF every 64 characters. Consumers of this API do various things to deal with this (either filtering them out or replacing them with spaces), but they shouldn't have to.

Comment 1

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

Attachment: MozReview Request: bug 1240173 - improve nsIX509Cert.dbKey r=Cykesiopka

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

Comment 2

[:Cykesiopka]

Comment on attachment 8709677 [details]
MozReview Request: bug 1240173 - improve nsIX509Cert.dbKey r=Cykesiopka

https://reviewboard.mozilla.org/r/31499/#review28295

LGTM - just some questions and nits.

::: security/manager/ssl/nsCertOverrideService.cpp:592
(Diff revision 1)
>  matchesDBKey(nsIX509Cert *cert, const char *match_dbkey)

Hmm, looks like it's pretty straightforward to go one step further and use nsACString& for |match_dbkey|, which lets us replace all that looping stuff down below.

(Maybe follow up material though.)

::: security/manager/ssl/nsNSSCertificateDB.cpp:127
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByDBKey(const char *aDBkey, nsISupports *aToken,
> +nsNSSCertificateDB::FindCertByDBKey(const char* aDBKey, nsISupports* aToken,

Might be nice to change the IDL for this method to accept an ACString |aDBKey| for consistency with the nsIX509Cert IDL.

(Again, maybe follow up material.)

::: security/manager/ssl/nsNSSCertificateDB.cpp:127
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByDBKey(const char *aDBkey, nsISupports *aToken,
> +nsNSSCertificateDB::FindCertByDBKey(const char* aDBKey, nsISupports* aToken,

Nit: Probably wouldn't hurt to comment out |aToken| - it's unused.

::: security/manager/ssl/nsNSSCertificateDB.cpp:171
(Diff revision 1)
> -    return NS_ERROR_INVALID_ARG;
> +  if (decoded.Length() != 16ULL + serialNumberLen + issuerLen) {

Just to confirm: we're OK with not rejecting zero length serial numbers and issuer fields now?

::: security/manager/ssl/tests/unit/test_cert_dbKey.js:24
(Diff revision 1)
> +     "test assumption: common name can't be longer than 116 bytes (makes " +

Nit: Doesn't match |< 116| logic above.

(Also, I'm sadly unable to figure out why 116 is the limit (instead of say 127) - maybe I'm missing something obvious.)

::: security/manager/ssl/tests/unit/test_cert_dbKey.js:28
(Diff revision 1)
> +  for (let i  = 0; i < commonName.length; i++) {

Nit: Two spaces here.

Comment 3

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

https://reviewboard.mozilla.org/r/31499/#review28295

Thanks!

> Hmm, looks like it's pretty straightforward to go one step further and use nsACString& for |match_dbkey|, which lets us replace all that looping stuff down below.
> 
> (Maybe follow up material though.)

Good call.

> Nit: Probably wouldn't hurt to comment out |aToken| - it's unused.

There are a number of other unused token arguments in nsIX509CertDB, so perhaps this would be best as a follow-up.

> Might be nice to change the IDL for this method to accept an ACString |aDBKey| for consistency with the nsIX509Cert IDL.
> 
> (Again, maybe follow up material.)

const char* vs. ACString as input doesn't make a huge difference in ease-of-API-use here (particularly since we have to make a copy to take out the whitespace anyway).

> Just to confirm: we're OK with not rejecting zero length serial numbers and issuer fields now?

Yes - CERT_FindCertByIssuerAndSN handles this case (I added a testcase just the same - it just returns null).

> Nit: Doesn't match |< 116| logic above.
> 
> (Also, I'm sadly unable to figure out why 116 is the limit (instead of say 127) - maybe I'm missing something obvious.)

Ah, good catch. I added a comment explaining how I got at that number (basically, it's so every length field in the DER encoding will only be 1 byte).

Comment 4

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

Comment on attachment 8709677 [details]
MozReview Request: bug 1240173 - improve nsIX509Cert.dbKey r=Cykesiopka

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

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ab3256d851
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d26687859f4d

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9aad3c0e80e

Comment 7

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

I filed bug 1241646 for the unused token arguments cleanup.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9aad3c0e80e