857627 - don't expose the NSS certificate nickname API in PSM interfaces

Status: RESOLVED FIXED

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

Description

[Mike Kaply [:mkaply]]

The API for addCertFromBase64 takes a parameter aName - name of the cert for display purposes.

If you look at the code:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1559

You'll see it is completely ignored.

The nickname is generated from the cert.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Use nickname if it is there

Comment 2

[Mike Kaply [:mkaply]]

bsmith: any chance you could take a look at this?

Comment 3

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

Comment on attachment 739180 [details] [diff] [review]
Use nickname if it is there

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

A couple of things:
1. You'll probably need 8 lines of context in your patch before Brian will review it
2. We're working on a lot of other things that impact security in ways this patch doesn't, so I imagine this change isn't seen as a high priority.
3. I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.
4. Looks good otherwise

Comment 4

[Mike Kaply [:mkaply]]

> I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.

I hope not. It would actually be really useful because then you could easily delete a certificate that you added. Right now it's a pain in the butt.

In a managed environment with multiple users logging on to the same machine, there are use cases for adding a cert at startup and removing it at shutdown.

Comment 5

[Mike Kaply [:mkaply]]

Attachment: New patch with more context

Comment 6

[Mike Kaply [:mkaply]]

> 2. We're working on a lot of other things that impact security in ways this patch doesn't, so I imagine this change isn't seen as a high priority.

The more I think about this, the more I am bothered by this comment. What you are saying is that your work is more important than outside contributors to the project.

Mozilla is open source. As such, I would expect that everyone at Mozilla would respect the contributions of anyone.

This patch will take all of 1 minute to review.

As a sidenote, I'd like this for the next ESR.

Comment 7

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

You're right - that wasn't very helpful. I'll see if I can get Brian's attention on this, or if he'll delegate the review to me. Another option would be to ask another PSM peer for a review (https://wiki.mozilla.org/Modules/All#Security_-_Mozilla_PSM_Glue ). Honza would probably be your best bet. Normally a PSM patch requires review from two peers or review from the module owner, but in this case I imagine we can go with one reviewer.
Additionally (and I should have mentioned this earlier), it would be great to have an xpcshell test for this (obviously we don't have one now). Let me know if you need any pointers on that.

Comment 8

[Mike Kaply [:mkaply]]

> Additionally (and I should have mentioned this earlier), it would be great to have an xpcshell test for this (obviously we don't have one now). Let me know if you need any pointers on that.

Any thoughts on what to use for a sample CA?

I see various certs in security/nss/cmd/samples/, but there is no documentation around them as to what they are.

I'm assuming the test will basically be - addcert, then call findcertbynickname and make sure I got a cert back.

Also, I assume this would go in security/manager/ssl/tests/unit, since that is the only directory that has xpcshell tests?

Comment 9

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

(In reply to Michael Kaply (mkaply) from comment #8)
> Any thoughts on what to use for a sample CA?

I think it would be simplest to generate a bogus sample cert using certutil or openssl, get its base64 representation and store that as a string in the test itself.

> I see various certs in security/nss/cmd/samples/, but there is no
> documentation around them as to what they are.

Yeah, I wouldn't rely on those - we probably want to keep the test distinct and self-contained.

> I'm assuming the test will basically be - addcert, then call
> findcertbynickname and make sure I got a cert back.

Sounds good. I would also recommend checking that you got the cert you were expecting.

> Also, I assume this would go in security/manager/ssl/tests/unit, since that
> is the only directory that has xpcshell tests?

Perfect.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Michael Kaply (mkaply) from comment #4)
> > I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.
> 
> I hope not. It would actually be really useful because then you could easily
> delete a certificate that you added. Right now it's a pain in the butt.
> 
> In a managed environment with multiple users logging on to the same machine,
> there are use cases for adding a cert at startup and removing it at shutdown.

You cannot remove a certificate you added at shutdown because shutdown code might never run. And, code that does disk I/O, like this, generally should not run at shutdown. Generally, the approach of modifying a permanent database and then un-modifying it at shutdown is poor for this reason--especially when that database a security-critical database like this. BTW, at least rsleevi agrees with me on this, based on the conversation on #nss I had with him where I tried to find an alternative solution for you.

My concern with this patch is that, for now, it will work. But, if/when we move to alternate root CA trust mechanisms (not trust bits in the NSS database), then code that depends on this will break. So, I don't want to give anybody the false impression that this mechanism will continue to work for any length of time. In other words, the nickname mechanism should be considered an implementation detail, not a feature. This is why I think we should WONTFIX this and file a bug for removing the parameter.

If your goal is to have "per-session" certificate trust, then why not just implement a custom cert override service that returns "override cert error" for untrusted certs for the issuer of the certificate that you want to trust? That would solve your problem and also prevent the fundamental problem with this approach.

(In reply to Michael Kaply (mkaply) from comment #6)
> The more I think about this, the more I am bothered by this comment. What
> you are saying is that your work is more important than outside contributors
> to the project.

I would say this is less the issue than, simply, I think this is a bad idea. I agree it would be better for us to be more responsive.

FWIW, pretty much all of the certificate-related APIs in PSM need to be revamped. There will be a lot of growing pains in the next few months while things get sorted.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 751216 [details] [diff] [review]
New patch with more context

I think this code seems to do what it purports to do, but I don't think it is a good idea. Please r?bsmith again if you still think it should go in.

If we decide to do this, then we should have a test, so that we know right away when (not if) we break this in the future, so we can notify addon authors that are relying on this.

Comment 12

[Mike Kaply [:mkaply]]

> I think this code seems to do what it purports to do, but I don't think it is a good idea.

Can you explain why?

Your earlier explanation was about the shutdown problem, and I'll accept that. But why is it wrong for this API to use a nickname? Other NSS APIs take nicknames for certs. What exactly is the downside here?

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Michael Kaply (mkaply) from comment #12)
> Your earlier explanation was about the shutdown problem, and I'll accept
> that. But why is it wrong for this API to use a nickname? Other NSS APIs
> take nicknames for certs. What exactly is the downside here?

"In other words, the nickname mechanism should be considered an implementation detail, not a feature. This is why I think we should WONTFIX this and file a bug for removing the parameter."

Basically, I am still open to adding APIs if they are needed and we realistically expect to be able to support them long-term. But, I don't think we can realistically support the nickname mechanism long-term. You are right that some NSS APIs use nicknames, but I'm not sure we'll be using NSS's certificate trust APIs/databases long-term. If we can avoid adding APIs with NSS-isms, I'd like to do that.

Comment 14

[Mike Kaply [:mkaply]]

Why do you think nicknames are NSS specific? Other platforms support nicknames for certificates simply because the regular certificate name is quite painful to type every time.

Honestly, I don't care one way or another. I saw a bug. I reported it.

It's your component. Do what you want with it.

Comment 15

[Ben Bucksch (:BenB)]

> I saw a bug. I reported it.

You did more. You contributed a patch. In true open-source spirit.
The reactions are just sad.

Comment 16

[bhavana bajaj [:bajaj]]

This in not an ESR specific regression or a recent feature regression,specific crash or a sec-crit bug hence does not fall in our tracking criteria.

Comment 17

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

We should probably just remove that parameter. If referring to certificates by nickname is an important use-case, we should improve the APIs so they're actually usable for that purpose (for instance, if you did add a certificate with a nickname, but there happened to be a preexisting certificate by that nickname, it's not clear what nsIX509CertDB.findCertByNickname will return).

Comment 18

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

Supporting the NSS certificate nickname API is hindering architectural improvements to PSM. To address this, I intend to have PSM not expose anything that can modify or make use of the nickname API. If my understanding of the use-cases for this API is correct, consumers should be able to use either the dbKey API (nsIX509Cert.dbKey / nsIX509CertDB.findCertByDBKey) for a way to uniquely identify a certificate or the (to be added) nsIX509Cert.displayName API for a friendly display name. Please let me know if I've overlooked a use-case.

Comment 19

[Mike Kaply [:mkaply]]

Isn't displayName effectively nickname?

Comment 20

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

It can be, but it isn't guaranteed to be. For instance, you could have two certificates that return the same value of displayName. nsIX509CertDB.findCertByNickname will only return one of those certificates, and it might not be the expected one. The aim of the changes in this bug is in part to reshape the nsIX509Cert/nsIX509CertDB APIs so that there isn't ambiguous behavior of this kind.

Comment 21

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

Attachment: bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

This patch makes the following changes:
1. renames nsIX509Cert.windowTitle to displayName
2. removes (most of*) displayName's use of the NSS certificate nickname API
3. adds additional fields that displayName will attempt to use to find a good
name to return (namely, organizationalUnitName and organizationName)

*except for built-in roots, where we have a good hard-coded value for the name

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

Comment 22

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

Attachment: bug 857627 - 2/4: remove nsIX509Cert.nickname

In general, any code that was using nsIX509Cert.nickname should be able to use
the attribute displayName (if using nickname for display purposes) or the
attribute dbKey (if using nickname as a unique identifier for a certificate).

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

Comment 23

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

Attachment: bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

This obviates some uses of nsIX509CertDB.findCertByNickname. Other uses are
replaced by equivalent functionality.

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

Comment 24

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

Attachment: bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

This removes findCertByNickname, findEmailEncryptionCert, and
findEmailSigningCert.

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

Comment 25

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review94950

::: security/manager/ssl/nsNSSCertificate.cpp:411
(Diff revision 1)
> +    nsAutoCString fullNickname(mCert->nickname);
> +    nsCString::const_iterator start;
> +    fullNickname.BeginReading(start);
> +    nsCString::const_iterator matchEnd;
> +    fullNickname.EndReading(matchEnd);
> +    nsCString::const_iterator delimiter;

Unused variable

Comment 26

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review94952

::: security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js
(Diff revision 1)
>      "v9jIf3twECyxx/RVzONVcob7nPePESHiKoG4FbtcuUh0wSHvCmTwRIQqPDCIuHcF" +
>      "StSt3Jr9iXcbXEhe4mSccOZ8N+r7Rv3ncKcevlRl7uFfDKDTyd43SZeRS/7J8KRf" +
>      "hD/h2nawrCFwc5gJW10aLJGFL/mcS7ViAIT9HCVk23j4TuBjsVmnZ0VKxB5edux+" +
>      "LIEqtU428UVHZWU/I5ngLw==");
>  
> -  equal(cert.nickname, "(no nickname)",

No test for the displayName?

Comment 27

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review94958

Comment 28

[:Cykesiopka]

Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review96778

Looks good!

::: security/manager/pki/resources/content/certViewer.js:52
(Diff revision 1)
>    var child = document.getElementById(node);
>    var currCert;
>    var displayVal;
>    for (let i = chain.length - 1; i >= 0; i--) {
>      currCert = chain.queryElementAt(i, nsIX509Cert);
> -    if (currCert.commonName) {
> +    displayVal = currCert.displayName;

Nit: Let's make this `let displayValue = ...` instead.
Same for `currCert` above.

::: security/manager/ssl/nsNSSCertHelper.cpp:1976
(Diff revision 1)
> -  nsresult rv = GetWindowTitle(title);
> +  nsresult rv = GetDisplayName(displayName);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
> -  sequence->SetDisplayName(title);
> +  sequence->SetDisplayName(displayName);

Wouldn't hurt to add a retval check while we're here, but not a big deal.

::: security/manager/ssl/nsNSSCertificate.cpp:378
(Diff revision 1)
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> -  aWindowTitle.Truncate();
> +  aDisplayName.Truncate();
>  
> +  MOZ_ASSERT(mCert, "mCert should not be null in GetDisplayName");

This should have a corresponding `#include "mozilla/Assertions.h"`.

::: security/manager/ssl/nsNSSCertificate.cpp:407
(Diff revision 1)
> +    nsCString::const_iterator start;
> +    fullNickname.BeginReading(start);
> +    nsCString::const_iterator matchEnd;
> +    fullNickname.EndReading(matchEnd);
> +    nsCString::const_iterator delimiter;
> +    if (FindInReadable(NS_LITERAL_CSTRING(":"), start, matchEnd)) {
> +      // matchEnd now points to just after the ':'.
> +      nsCString::const_iterator end;
> +      fullNickname.EndReading(end);
> +      builtInRootNickname = Substring(matchEnd, end);
> +    }

I think we can replace this code with something simpler actually:
```cpp
int32_t index = fullNickname.Find(":");
if (index != kNotFound) {
  // Substring() safely handles an out of range starting position.
  builtInRootNickname = Substring(fullNickname,
                                  AssertedCast<uint32_t>(index + 1));
}
```

::: security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js:60
(Diff revision 1)
>          "Actual and expected commonName should match");
>    equal(cert.organization, "",
>          "Actual and expected organization should match");
>    equal(cert.organizationalUnit, "",
>          "Actual and expected organizationalUnit should match");
> -  equal(cert.windowTitle, "Luděk Rašek",
> +  equal(cert.displayName, "Luděk Rašek",

We should probably add more test cases for `displayName` somewhere.
Follow-up part or bug I guess.

Comment 29

[:Cykesiopka]

Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review97138

LGTM.

::: security/manager/ssl/tests/unit/test_local_cert.js:52
(Diff revision 1)
>  add_task(function* () {
>    // No master password, so no prompt required here
>    ok(!certService.loginPromptRequired);
>  
>    let certA = yield getOrCreateCert(gNickname);
> -  equal(certA.nickname, gNickname);
> +  equal(certA.displayName, gNickname);

Maybe add a comment here that nsIX509Cert no longer provides a way to get at the nicknames of certs, which is why `displayName` is being used instead?

Comment 30

[:Cykesiopka]

Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review97140

Looks good, but the following files still call `addCert` or `addCertFromBase64` with three arguments, which would be nice to fix:
```
netwerk/test/unit/test_altsvc.js
netwerk/test/unit/test_http2.js
netwerk/test/unit/test_immutable.js
security/manager/ssl/tests/mochitest/browser/head.js
security/manager/ssl/tests/unit/test_cert_signatures.js
```

::: security/manager/ssl/nsIX509CertDB.idl:345
(Diff revision 1)
>     *                indicating SSL, Email, and Obj signing trust
> -   * @param aName name of the cert for display purposes.
> +   * @return nsIX509Cert the resulting certificate
> -   *              TODO(bug 857627): aName is currently ignored. It should either
> -   *              not be ignored, or be removed.
>     */
> -  void addCert(in ACString certDER, in ACString aTrust, in AUTF8String aName);
> +  nsIX509Cert addCert(in ACString certDER, in ACString aTrust);

Nit: Let's rename `aTrust` to `trust` instead while we're here.
Same below.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1337
(Diff revision 1)
>  NS_IMETHODIMP
>  nsNSSCertificateDB::AddCertFromBase64(const nsACString& aBase64,
>                                        const nsACString& aTrust,
> -                                      const nsACString& /*aName*/)
> +                                      nsIX509Cert** addedCertificate)
>  {
> +  MOZ_ASSERT(addedCertificate);

Needs a corresponding `#include "mozilla/Assertions.h"`.

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js:41
(Diff revision 1)
> -  // certificate already exists.
> -  let int_cert = certDB.findCertByNickname("int-limited-depth");
>    notEqual(int_cert, null, "Intermediate cert should be in the cert DB");
>    let base64_cert = btoa(getDERString(int_cert));
> -  certDB.addCertFromBase64(base64_cert, "p,p,p", "ignored_argument");
> +  let returnedEE = certDB.addCertFromBase64(base64_cert, "p,p,p",
> +                                            "ignored_argument");

We can get rid of this argument now.

::: security/manager/ssl/tests/unit/test_cert_trust.js:186
(Diff revision 1)
> +    'ca',
> +    'int',
> +    'ee',

Nit: Let's use double quotes instead here and below.

Comment 31

[:Cykesiopka]

Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97142

Ripping out code -> :)

::: security/manager/ssl/LocalCertService.cpp:30
(Diff revision 1)
> +// Given a name, searches the internal certificate/key database for a
> +// self-signed certificate with subject and issuer distinguished name equal to
> +// "CN={name}". This assumes that the user has already authenticated to the
> +// internal DB if necessary.
> +static nsresult
> +FindLocalCertByName(const nsACString& aName, UniqueCERTCertificate& result)

Nit: We should probably be consistent here with use of the `a` prefix.

Comment 32

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97148

LGTM.

Comment 33

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

Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review96778

Thanks for the reviews!

> Wouldn't hurt to add a retval check while we're here, but not a big deal.

Sure.

> This should have a corresponding `#include "mozilla/Assertions.h"`.

Added.

> I think we can replace this code with something simpler actually:
> ```cpp
> int32_t index = fullNickname.Find(":");
> if (index != kNotFound) {
>   // Substring() safely handles an out of range starting position.
>   builtInRootNickname = Substring(fullNickname,
>                                   AssertedCast<uint32_t>(index + 1));
> }
> ```

Good call - that's much simpler.

> We should probably add more test cases for `displayName` somewhere.
> Follow-up part or bug I guess.

Good idea. I think at this point I'd prefer this as a follow-up bug (filed as bug 1321608).

Comment 34

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

Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review94950

> Unused variable

Turns out there was a much easier way of doing this, anyway.

Comment 35

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

Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review94952

> No test for the displayName?

It's on line 58 :)

Comment 36

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

Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review97138

> Maybe add a comment here that nsIX509Cert no longer provides a way to get at the nicknames of certs, which is why `displayName` is being used instead?

Sounds good - I added an explainer of why it works and what's going on behind the scenes.

Comment 37

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

Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review97140

Whoops - good catch.

> Nit: Let's rename `aTrust` to `trust` instead while we're here.
> Same below.

Sounds good. I also updated the comments.

> Needs a corresponding `#include "mozilla/Assertions.h"`.

Added.

> We can get rid of this argument now.

Good catch.

> Nit: Let's use double quotes instead here and below.

Fixed (I actually updated all single-quotes to double-quotes in that file).

Comment 38

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

Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97142

:D

> Nit: We should probably be consistent here with use of the `a` prefix.

Good call. I also annotated it as /*out*/.

Comment 39

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

Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

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

Comment 40

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

Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

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

Comment 41

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

Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

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

Comment 42

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

Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

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

Comment 43

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

Thanks for all the reviews!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=372593f6c097f93b1605b7be3d32601bba22b8e8

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d20e7d8bb510
1/4: improve nsIX509Cert.windowTitle (and rename to displayName) r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/16f6abade0e7
2/4: remove nsIX509Cert.nickname r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/eda8fdc7c21a
3/4: have nsIX509CertDB.addCert* functions return the added certificate r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/0932c3f7208b
4/4: remove nickname-related APIs from nsIX509CertDB r=Cykesiopka,jcj

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d20e7d8bb510
https://hg.mozilla.org/mozilla-central/rev/16f6abade0e7
https://hg.mozilla.org/mozilla-central/rev/eda8fdc7c21a
https://hg.mozilla.org/mozilla-central/rev/0932c3f7208b

Comment 46

[sharapanoff]

just for information - this change has broke the signTextJs (by Dana Keeler)- see https://github.com/mozkeeler/signTextJS/issues/49

and made the UI of signTextJs Plus to look "strange"