1275714 - [FlyWeb] Review TLS-related changes for FlyWeb landing

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kannan Vijayan [:djvj]]

These are TSL/encryption related changes in FlyWeb.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: flyweb-security.patch

Comment 2

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

Comment on attachment 8756532 [details] [diff] [review]
flyweb-security.patch

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

Looks good. r=me with comments addressed.

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +418,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsCertOverrideService::RememberTemporaryValidityOverrideUsingFingerprint(
> +    const nsACString& aHostName,

nit: these should be indented just two spaces

@@ +424,5 @@
> +    const nsACString& aCertFingerprint,
> +    uint32_t aOverrideBits)
> +{
> +  NS_ENSURE_ARG(!aCertFingerprint.IsEmpty());
> +  NS_ENSURE_ARG(!aHostName.IsEmpty());

Let's just go with the more verbose version of these:

if (aCertFingerprint.IsEmpty()) {
  return NS_ERROR_INVALID_ARG;
}

etc.

(If you like, they can probably be combined into one or statement.)

@@ +425,5 @@
> +    uint32_t aOverrideBits)
> +{
> +  NS_ENSURE_ARG(!aCertFingerprint.IsEmpty());
> +  NS_ENSURE_ARG(!aHostName.IsEmpty());
> +  if (aPort < -1)

nit: braces around conditionals

@@ +428,5 @@
> +  NS_ENSURE_ARG(!aHostName.IsEmpty());
> +  if (aPort < -1)
> +    return NS_ERROR_INVALID_ARG;
> +
> +  {

No need to add extra scope for the lock.

::: security/manager/ssl/nsICertOverrideService.idl
@@ +58,5 @@
>    /**
> +   *  Certs with the given fingerprint should always be accepted for the
> +   *  given hostname:port, regardless of errors verifying the cert.
> +   *  Host:Port is a primary key, only one entry per host:port can exist.
> +   *  The implementation will decide which fingerprint alg is used.

We should specify that we're using SHA-256 (so that the caller knows what algorithm to use).

@@ +63,5 @@
> +   *
> +   *  @param aHostName The host (punycode) this mapping belongs to
> +   *  @param aPort The port this mapping belongs to, if it is -1 then it
> +   *          is internaly treated as 443
> +   *  @param aCertFingerprint The cert fingerprint that should be accepted

It's probably best to specify that this is of the form 'AA:BB:...' (i.e. colon-separated upper-case hex bytes)

Comment 3

[Kannan Vijayan [:djvj]]

http://hg.mozilla.org/projects/larch/rev/dc52f6553083

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/724b6e4ac4e4
Changes in preparation for FlyWeb landing.  Add ability to pin using a cert fingerprint, in addition to using a cert. r=dkeeler

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/724b6e4ac4e4

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: cert-test.patch

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 8759830 [details] [diff] [review]
cert-test.patch

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

Never mind.  This was already reviewed and is in m-c in a different location.