Closed Bug 1409259 Opened 2 years ago Closed 2 years ago

Implement a console warning for Symantec CAs affected by the distrust plan

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
relnote-firefox --- 58+
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug, )

Details

(Keywords: site-compat, Whiteboard: [psm-assigned])

Attachments

(5 files, 2 obsolete files)

Add logic to identify the Symantec CAs that are to be affected by the plan for the graduated distrust of Symantec roots to PSM, and then use that logic to emit a warning to the developer console.

This warning will be best-effort; architectural limitations will keep us from evaluating this code for resumed sessions. As there won't be resumed sessions once the distrust is in place, I'm not intending to depend on a re-architecture of session resumption for this bug. (Though it sure would be nice...)
Whiteboard: [psm-assigned]
Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

Mark, can you take a brief look at this and see if it's generally going the direction of your patch, and doing sort of what you suggested I do?

Thanks!
Attachment #8920009 - Flags: feedback?(mgoodwin)
Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

https://reviewboard.mozilla.org/r/190896/#review196500

Cool - lgtm with the template change.

::: security/certverifier/NSSCertDBTrustDomain.cpp:37
(Diff revision 1)
>  #include "pkix/pkix.h"
>  #include "pkix/pkixnss.h"
>  #include "prerror.h"
>  #include "secerr.h"
>  
> -#include "StartComAndWoSignData.inc"
> +#include "TrustOverrides.h"

Hmmm - maybe TrustOverrideUtils.h?

::: security/certverifier/TrustOverrides.h:19
(Diff revision 1)
> +  uint32_t len;
> +};
> +
> +static bool
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)

If we make this function a template and pass a reference to an array of a certain (i.e. a template parameter) size, we can make some simplifications (and it'll be safer).

::: security/certverifier/TrustOverrides.h:21
(Diff revision 1)
> +
> +static bool
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)
> +{
> +  MOZ_ASSERT(aCert && aDnList);

Let's add the non-debug code to handle getting a null pointer as well.

::: security/certverifier/TrustOverrides.h:23
(Diff revision 1)
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)
> +{
> +  MOZ_ASSERT(aCert && aDnList);
> +
> +  for (size_t i = 0; i < aListSz; i++) {

I think this can be `for (auto& dn : aDnList) {` with the above change.
Attachment #8919950 - Flags: review?(dkeeler) → review+
Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

https://reviewboard.mozilla.org/r/190896/#review196500

Thanks! Good ideas.
Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review196850

Seems reasonable but I think we should include the generation script.

::: security/certverifier/TrustOverride-SymantecData.inc:1
(Diff revision 4)
> +// Script downloaded from https://gist.github.com/ed5d869cd8da3951e393f4a2d8a3391f

Maybe just include the script in security/manager/tools?

::: security/certverifier/TrustOverride-SymantecData.inc:4
(Diff revision 4)
> +// Script downloaded from https://gist.github.com/ed5d869cd8da3951e393f4a2d8a3391f
> +// Invocation: crtshToDNStruct.py 17 3381895 847444 4350 4174851 4175126 12729019 8983600 12726040 8983601 30 3382830 254193 8984570 68409 26682 2771491 93 1039083
> +
> +// /C=US/O=GeoTrust Inc./CN=GeoTrust Global CA

If I'm reading the script correctly, this is only part of the subject DN (and not necessarily in the same order as in the certificate). Maybe it would be best to print a representation of the DN as-is?
Attachment #8919951 - Flags: review?(dkeeler)
Comment on attachment 8920423 [details]
Bug 1409259 - Add the Apple and Google sub-CA data.

https://reviewboard.mozilla.org/r/191392/#review196904

Same idea as the previous commit.
Attachment #8920423 - Flags: review?(dkeeler)
Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review196852

Looks good, modulo some minor issues (well, and tests).

::: browser/base/content/browser.js:7507
(Diff revision 4)
> +    if (this._isCertDistrustImminent) {
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +      let windowId = gBrowser.selectedBrowser.innerWindowID;
> +      // Use uri.prePath instead of initWithSourceURI() so that these can be
> +      // de-duplicated on the scheme+host+port combination.
> +      consoleMsg.initWithWindowID("The security certificate in use for this web site " +

This needs to be a localized message.
Also I guess we need a test for this :/

::: browser/base/content/browser.js:7508
(Diff revision 4)
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +      let windowId = gBrowser.selectedBrowser.innerWindowID;
> +      // Use uri.prePath instead of initWithSourceURI() so that these can be
> +      // de-duplicated on the scheme+host+port combination.
> +      consoleMsg.initWithWindowID("The security certificate in use for this web site " +
> +                      "will be invalid  in a future release of Firefox. For more " +

nit: extra space after 'invalid'

::: security/manager/ssl/nsNSSCallbacks.cpp:1443
(Diff revision 4)
> +  // TODO: Update to the real source of the certificate, once Bug 1406856 lands
> +  nsCOMPtr<nsIX509CertList> succeededCertChain = new nsNSSCertList();
> +  bool distrustImminent;
> +  if (NS_SUCCEEDED(IsCertificateDistrustImminent(succeededCertChain,
> +                                                 &distrustImminent))) {
> +    if (distrustImminent) {

I guess technically this is a "lonely if" (i.e. this could be just one compound statement, but then it might look bad due to wrapping long lines, so... ¯\_(ツ)_/¯ )
Attachment #8920009 - Flags: review?(dkeeler) → review+
Comment on attachment 8920582 [details]
Bug 1409259 - Implement the Symantec distrust algorithm WIP

https://reviewboard.mozilla.org/r/191596/#review196854

This seems like a reasonable approach. Have we verified that the affected certificate hierarchies will really only involve chains of length 3? (Like, are there no undisclosed constrained subCAs involved?)
Also, you know, tests.

::: security/manager/ssl/nsNSSCallbacks.cpp:1267
(Diff revision 1)
>      sslStatus->SetCertificateTransparencyInfo(certificateTransparencyInfo);
>    }
>  }
>  
>  static nsresult
> +SplitTripleCertificateChain(nsIX509CertList* aCertList,

Let's document this - what it expects, what it returns, etc.

::: security/manager/ssl/nsNSSCallbacks.cpp:1269
(Diff revision 1)
>  }
>  
>  static nsresult
> +SplitTripleCertificateChain(nsIX509CertList* aCertList,
> +                            /* out */ nsCOMPtr<nsIX509Cert>& aRoot,
> +                            /* out */ nsCOMPtr<nsIX509Cert>& aIntermediate,

Depending, we may want an nsIX509CertList here (and plural "intermediates").

::: security/manager/ssl/nsNSSCallbacks.cpp:1278
(Diff revision 1)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsISimpleEnumerator> chainElt;
> +  nsresult rv = aCertList->GetEnumerator(getter_AddRefs(chainElt));
> +  NS_ENSURE_SUCCESS(rv, rv);

I think we should avoid NS_ENSURE_SUCCESS and similar macros for new code.

::: security/manager/ssl/nsNSSCallbacks.cpp:1308
(Diff revision 1)
> +  if (!hasMoreCerts) {
> +    return NS_ERROR_FAILURE; // Insufficient chain length
> +  }
> +
> +  {
> +    nsCOMPtr<nsISupports> certSupports;

Factor this code pattern out?

::: security/manager/ssl/nsNSSCallbacks.cpp:1340
(Diff revision 1)
> +
> +  if (!hasMoreCerts) {
> +    return NS_OK; // standard length 3 chain
> +  }
> +
> +  return NS_ERROR_FAILURE; // Unexpectedly long chain, not relevant

Are we sure there are no chains of length other than 3 that we'll want to apply this to?

::: security/manager/ssl/nsNSSCallbacks.cpp:1352
(Diff revision 1)
>    MOZ_ASSERT(aResult);
>    if (!aCertList || !aResult) {
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> -  // TODO: Implement algorithm below (and add tests)
> +  nsCOMPtr<nsIX509Cert> rootCert, intCert, eeCert;

nit: one declaration per line

::: security/manager/ssl/nsNSSCallbacks.cpp:1371
(Diff revision 1)
> +    return NS_OK;
> +  }
>  
>    // If the end entity's notBefore date is not before 2016-06-01, exit false
> +  nsCOMPtr<nsIX509CertValidity> validity;
> +  eeCert->GetValidity(getter_AddRefs(validity));

This will probably never fail, but we should still be checking these return values.

::: security/manager/ssl/nsNSSCallbacks.cpp:1374
(Diff revision 1)
>    // If the end entity's notBefore date is not before 2016-06-01, exit false
> +  nsCOMPtr<nsIX509CertValidity> validity;
> +  eeCert->GetValidity(getter_AddRefs(validity));
>  
> -  // If the intermediate is one of the whitelisted intermediates, exit false
> +  PRTime notBefore;
> +  validity->GetNotBefore(&notBefore);

Same here.

::: security/manager/ssl/nsNSSCallbacks.cpp:1380
(Diff revision 1)
>  
> -  // Otherwise, this certificate will imminently be distrusted, exit true
> +  // PRTime is microseconds since the epoch, whereas JS time is milliseconds.
> +  // (new Date("2016-06-01T00:00:00Z")).getTime() * 1000
> +  static const PRTime JUNE_1_2016 = 1464739200000000;
> +
> +  if (notBefore > JUNE_1_2016) {

`*aResult = notBefore <= JUNE_1_2016;`?
Depends on: 1411683
Gerv:

I've authored a temporary string that says:

> The security certificate in use for this web site will be invalid  in a future release of Firefox. For more information, visit https://FILL_ME_IN/

I need to get that into the localization queue, but regarding the FILL_ME_IN website: Can you take on writing something up for this circumstance? Since I figure we might want to use this code in the future, I'm generically referring to the flags here as indicating a certificate that will be "imminently distrusted," so I am kind of imagining a wiki or SUMO page for "imminent PKI distrust actions" of which there could be a heading here for the upcoming work with the Symantec roots.
Flags: needinfo?(gerv)
I would say:

The security certificate in use on this web site will no longer be trusted in a future release of Firefox. For more information, visit https://FILL_ME_IN/

Here's a start for the FILL_ME_IN:

https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

Gerv
Flags: needinfo?(gerv)
Thanks, Gerv!
Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review196850

> If I'm reading the script correctly, this is only part of the subject DN (and not necessarily in the same order as in the certificate). Maybe it would be best to print a representation of the DN as-is?

Good idea.
Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review196852

> This needs to be a localized message.
> Also I guess we need a test for this :/

I don't think we can test this until the next patch, but I'm writing one using DN matching against the completed distrust algorithm.
Attachment #8920423 - Attachment is obsolete: true
Attachment #8920582 - Attachment is obsolete: true
Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review199176

Cool - lgtm with the python nits addressed. The naming the RootDNs thing isn't as important.

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:1
(Diff revision 5)
> +#!/usr/bin/env python

Looks like `./mach lint` notes some style issues with this file.

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:84
(Diff revision 5)
> +    r.raise_for_status()
> +
> +    cert = x509.load_pem_x509_certificate(r.content, default_backend())
> +    blocks.append(print_block(cert))
> +
> +  print("static const DataAndLength RootDNs[]= {")

Maybe add an argument so RootDNs can be what it'll end up being? (e.g. RootSymantecDNs for the Symantec certs)
Attachment #8919951 - Flags: review?(dkeeler) → review+
Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review199176

Thanks!

> Looks like `./mach lint` notes some style issues with this file.

Aha!

> Maybe add an argument so RootDNs can be what it'll end up being? (e.g. RootSymantecDNs for the Symantec certs)

It also needs deduplication logic, but how often are we going to run this thing? I figure I can make it nicer next time.
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review199794

Great - well decomposed.

::: security/manager/ssl/nsNSSCallbacks.cpp:1269
(Diff revision 1)
>    }
>  }
>  
>  static nsresult
>  IsCertificateDistrustImminent(nsIX509CertList* aCertList,
>                                /* out */ bool* aResult) {

I guess this goes in the previous commit, but I think we should use a reference here.

::: security/manager/ssl/nsNSSCallbacks.cpp:1327
(Diff revision 1)
> +
> +  // PRTime is microseconds since the epoch, whereas JS time is milliseconds.
> +  // (new Date("2016-06-01T00:00:00Z")).getTime() * 1000
> +  static const PRTime JUNE_1_2016 = 1464739200000000;
> +
> +  // If the end entity's notBefore date is not before 2016-06-01, exit false

Maybe flip this comment around? (i.e. if the ee's notBefore is before the cutoff, return true) I found it a bit difficult to reconcile the description of what was happening with the logical representation.

::: security/manager/ssl/nsNSSCallbacks.cpp:1484
(Diff revision 1)
>    } else {
>      DetermineEVAndCTStatusAndSetNewCert(status, fd, infoObject);
>    }
>  
> -  // TODO: Update to the real source of the certificate, once Bug 1406856 lands
> -  nsCOMPtr<nsIX509CertList> succeededCertChain = new nsNSSCertList();
> +  nsCOMPtr<nsIX509CertList> succeededCertChain;
> +  // This always returns NS_OK

Although it can return a null list... maybe make a note of this?
Attachment #8923613 - Flags: review?(dkeeler) → review+
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review199794

Thanks!

> I guess this goes in the previous commit, but I think we should use a reference here.

This must have been a rebase issue, because I'm sure we already did this somewhere. Sorry.

> Maybe flip this comment around? (i.e. if the ee's notBefore is before the cutoff, return true) I found it a bit difficult to reconcile the description of what was happening with the logical representation.

Will do, and this also should happen first, as it's the least expensive check.
Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review200394

LGTM.

::: security/manager/ssl/nsNSSCallbacks.cpp:1441
(Diff revision 7)
> +  bool distrustImminent;
> +  if (NS_SUCCEEDED(IsCertificateDistrustImminent(succeededCertChain,
> +                                                 &distrustImminent))) {
> +    if (distrustImminent) {

As David already noted, maybe change this to:

```
rv = IsCertificateDistrustImminent(succeededCertChain, &distrustImminent);
if (NS_SUCCEEDED(rv) && distrustImminent) {
  // ...
}
```

?
Attachment #8920009 - Flags: review?(ttaubert) → review+
Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review200394

Thanks, Tim!

> As David already noted, maybe change this to:
> 
> ```
> rv = IsCertificateDistrustImminent(succeededCertChain, &distrustImminent);
> if (NS_SUCCEEDED(rv) && distrustImminent) {
>   // ...
> }
> ```
> 
> ?

Okay, okay :D
Comment on attachment 8924032 [details]
Bug 1409259 - Add xpcshell tests for the Symantec distrust

https://reviewboard.mozilla.org/r/195244/#review200562

LGTM - just a few comments. In particular, we probably don't actually need the test_symantec_apple_google_unaffected directory at all.

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:11
(Diff revision 1)
> +// Tests handling of certificates issued by Symantec. If such
> +// certificates have a notBefore before 1 June 2016, and are not
> +// issued by an Apple or Google intermediate, they should emit a
> +// warning to the console.
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;

We should probably use Ci.nsIWebProgressListener.STATE_CERT_DISTRUST_IMMINENT

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:32
(Diff revision 1)
> +
> +add_tls_server_setup("SymantecSanctionsServer", "test_symantec_apple_google");
> +
> +// Whitelisted certs aren't to be distrusted
> +add_connection_test("symantec-whitelist-after-cutoff.example.com",
> +                    PRErrorCodeSuccess, noOp, shouldNotBeImminentlyDistrusted);

I think we should be safe passing null instead of noOp.

::: security/manager/ssl/tests/unit/test_symantec_apple_google/ee-from-whitelist-after-cutoff.pem.certspec:3
(Diff revision 1)
> +issuer:printableString/C=US/O=Google Inc/CN=Google Internet Authority G2
> +subject:ee-from-whitelist-after-cutoff
> +validity:20160601-20500101

Keep in mind if we ever implement validity period limits we would have to update/regenerate these.

::: security/manager/ssl/tests/unit/test_symantec_apple_google_unaffected.js:11
(Diff revision 1)
> +// Tests handling of certificates issued by Symantec. If such
> +// certificates have a notBefore before 1 June 2016, and are not
> +// issued by an Apple or Google intermediate, they should emit a
> +// warning to the console.
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;

Same as the other test.

::: security/manager/ssl/tests/unit/test_symantec_apple_google_unaffected.js:15
(Diff revision 1)
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;
> +
> +function shouldNotBeImminentlyDistrusted(aTransportSecurityInfo) {
> +  let isDistrust = aTransportSecurityInfo.securityState & STATE_CERT_DISTRUST_IMMINENT;
> +  Assert.ok(!isDistrust, "This host should not be imminently distrusted");

I feel like we can piggyback this test onto pretty much any other successful connection test and avoid adding these certificates files/this extra directory. Alternatively, if it's worth it to have a separate test for this case (to logically separate test cases and not have everything spaghettify), maybe just re-use a preexisting hostname/certificate from the BadCertServer or OCSPStaplingServer.
Attachment #8924032 - Flags: review?(dkeeler) → review+
Comment on attachment 8924032 [details]
Bug 1409259 - Add xpcshell tests for the Symantec distrust

https://reviewboard.mozilla.org/r/195244/#review200562

Thanks, good observations!
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

David - I don't know what I did to this (increasingly large) patchset to make Reviewboard think you've already reviewed this patch but this (the browser console test) is new tonight.
Attachment #8923613 - Flags: review+ → review?(dkeeler)
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201148

::: devtools/client/webconsole/test/browser_console_certificate_imminent_distrust.js:40
(Diff revision 4)
> +// Finally, copy in that key as a .ca file:
> +// (NOTE: files ended in .ca are added as trusted roots by the mochitest harness)
> +//   cp ../../../security/manager/ssl/tests/unit/test_symantec_apple_google/test-ca.pem symantecRoot.ca

Do we need to add `symantecRoot.p12` then? Seems like that's just transient.

::: devtools/client/webconsole/test/browser_console_certificate_imminent_distrust.js:77
(Diff revision 4)
> +    let opened = waitForBrowserConsole();
> +
> +    let hud = HUDService.getBrowserConsole();
> +    ok(!hud, "browser console is not open");
> +    info("wait for the browser console to open with ctrl-shift-j");
> +    EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window);

Should we maybe use `await HUDService.toggleBrowserConsole()` here? I remember EventUtils being a little fickle with regard to focus, leading to intermittent issues sometimes. We might not even need to wait for it to open then?
Attachment #8923613 - Flags: review?(ttaubert)
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201190

LGTM otherwise.
Attachment #8923613 - Flags: review?(ttaubert) → review+
Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201148

Thanks!
Try runs are good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=163e941eab23ae2780f859145e290d842ea04440
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08dceb6dd8dea622e18314676204dd9e070afe7

So let's do this thing.

Note to those coming after: This will cause some amount of performance hit for sites using Symantec roots, which we'll see in telemetry. It should be small, and is unfortunately unavoidable for this policy, though if it's bad we could try re-doing this as a hash map implementation.

Thanks to David and Tim for all the reviews!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/79d1ac7232f3
Refactor "TrustOverrides" header for existing trust overrides r=keeler
https://hg.mozilla.org/integration/autoland/rev/d3acb68f73c4
Add Symantec root and Apple/Google intermediate lists r=keeler
https://hg.mozilla.org/integration/autoland/rev/595e27212723
Add a console warning for soon-to-be-distrusted roots r=keeler,ttaubert
https://hg.mozilla.org/integration/autoland/rev/862c080c2913
Add xpcshell tests for the Symantec distrust r=keeler
https://hg.mozilla.org/integration/autoland/rev/98b1272e170c
Add browser console test for the distrust console message r=keeler,ttaubert
Keywords: checkin-needed
(In reply to J.C. Jones [:jcj] from comment #57)
> Note to those coming after: This will cause some amount of performance hit
> for sites using Symantec roots, which we'll see in telemetry. It should be
> small, and is unfortunately unavoidable for this policy 

Matt: You'll see this as a performance hit in the Canary for affected Symantec sites.
QA Contact: mwobensmith
Blocks: 1414358
(In reply to J.C. Jones [:jcj] from comment #21)
> Gerv:
> 
> I've authored a temporary string that says:
> 
> > The security certificate in use for this web site will be invalid  in a future release of Firefox. For more information, visit https://FILL_ME_IN/
> 
> I need to get that into the localization queue, but regarding the FILL_ME_IN
> website: Can you take on writing something up for this circumstance? Since I
> figure we might want to use this code in the future, I'm generically
> referring to the flags here as indicating a certificate that will be
> "imminently distrusted," so I am kind of imagining a wiki or SUMO page for
> "imminent PKI distrust actions" of which there could be a heading here for
> the upcoming work with the Symantec roots.

Should it be a concern that this string will always say Firefox, since it’s hard coded instead of using a placeholder for the branding? Normally it would be, but I’m not sure in this context.

I guess it's OK to say "Firefox" in Nightly and Beta, not so sure in a rebranded build?
Flags: needinfo?(jjones)
(In reply to Francesco Lodolo [:flod] from comment #61)
> Should it be a concern that this string will always say Firefox, since it’s
> hard coded instead of using a placeholder for the branding? Normally it
> would be, but I’m not sure in this context.

This is my first time doing anything with strings that users see, so you'd know better than I. That said, this is specifically a Mozilla CA Certificate Program action, so it might be a little fuzzy?

I think we could definitely do a follow on to either remove the branding or add in a branding variable. We need to do it quick though before this merges to Beta. :)

Just as a try, what about just deleting "of Firefox":

> certImminentDistrust.message = The security certificate in use on this web site will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

?
Flags: needinfo?(francesco.lodolo)
(In reply to J.C. Jones [:jcj] from comment #62)
> I think we could definitely do a follow on to either remove the branding or
> add in a branding variable. We need to do it quick though before this merges
> to Beta. :)

Not only that. I haven't exposed that string to localization tools yet, if I do the procedure to change it is
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

If we can fix it quickly, i.e. in the next 2 or 3 days, you can fix just the text without changing the string ID.

> Just as a try, what about just deleting "of Firefox":
> 
> > certImminentDistrust.message = The security certificate in use on this web site will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

That would work for me, but not sure whose call it is to approve this change.
Flags: needinfo?(francesco.lodolo)
Blocks: 1414627
Flags: needinfo?(jjones)
Added to Fx58 release notes.
Blocks: 1436062
You need to log in before you can comment on or make changes to this bug.