Closed Bug 884449 Opened 6 years ago Closed 6 years ago

Update the SSL Preference Pane once TLS 1.2 is implemented and made the default

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.21

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #861471 +++

There is some progress in bug 480514 to implement TLS 1.2, and it appears from bug 733647 comment #15 that TLS 1.2 is already supported on trunk (though not clear to which extend, falling back to TLS 1.0 is bug 839310 and thus far unresolved).

While I can still remember what I did back in bug 861471 when implementing this, I'll go ahead an work out the necessary patch to add the TLS 1.2 checkbox and respective documentation updates. This should be straight-forward.

We'll need this when TLS 1.2 is made the default maximum version at the latest.
Attached patch Simple patch (obsolete) — Splinter Review
> We'll need this when TLS 1.2 is made the default maximum version

This may happen much sooner than initially thought, given that the respective patch in bug 733647 attachment 764166 [details] [diff] [review] has the required reviews already and thus can still land for the 24.0 cycle despite the feature not quite finished yet (in particular the TLS 1.0 fallback, which may be added during aurora later).

 -> Which means that this patch needs to go in before the next merge on *this* Monday, otherwise we'd either end up with a late-l10n situation or a broken UI for 2.20. Consequently, I'm requesting reviews at this time already.

The patch itself is fairly trivial = just adding the TLS 1.2 checkbox which I had in some patches for the design process in bug 861471 already anyway, plus the respective help updates.
Attachment #764833 - Flags: ui-review?(neil)
Attachment #764833 - Flags: review?(iann_bugzilla)
Neil, I don't know if this needs your ui-r+ at all given that the design itself was signed off in bug 861471 and this is just adding another value to it, but I'll leave the request up for now.

Also nominating for tracking-seamonkey2.21 as this is time sensitive, depending on when bug 733647 gets pushed on mozilla-central.
Comment on attachment 764833 [details] [diff] [review]
Simple patch

If they don't discontinue SSL 3.0 before they add TLS 1.3 then we'll have to change its access key to 0 so that 3 becomes free ;-)
Attachment #764833 - Flags: ui-review?(neil) → ui-review+
Comment on attachment 764833 [details] [diff] [review]
Simple patch

>+++ b/suite/security/prefs/pref-ssl.xul
>@@ -67,16 +67,21 @@
>                   label="&limit.tls10.label;"
>                   accesskey="&limit.tls10.accesskey;"
>                   oncommand="UpdateSslPrefs()"/>
>         <checkbox id="allowTLS11"
>                   class="nogray-disabled"
>                   label="&limit.tls11.label;"
>                   accesskey="&limit.tls11.accesskey;"
>                   oncommand="UpdateSslPrefs()"/>
>+        <checkbox id="allowTLS12"
>+                  class="nogray-disabled"
>+                  label="&limit.tls12.label;"
>+                  accesskey="&limit.tls12.accesskey;"
>+                  oncommand="UpdateSslPrefs()"/>
Should have noticed this before, but I prefer a ; at the end of the function call so:
oncommand="UpdateSslPrefs();"

r=me with that fixed.
Attachment #764833 - Flags: review?(iann_bugzilla) → review+
(In reply to Ian Neal from comment #4)
> >+                  oncommand="UpdateSslPrefs()"/>
> Should have noticed this before, but I prefer a ; at the end
Ooh, good catch!
Comment #4 addressed. I had the on...="...();" form in earlier versions of the patch in the other bug, but apparently they got lost somewhere in the process.

It should be safe to check this in even without the default change yet. If they figure that TLS 1.2 isn't mature enough yet to be the default we can still hide the checkbox in the beta phase, but at least all the strings are in now.
Attachment #764833 - Attachment is obsolete: true
Attachment #765022 - Flags: ui-review+
Attachment #765022 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: checkin-suite-needed
(In reply to neil@parkwaycc.co.uk from comment #3)
> If they don't discontinue SSL 3.0 before they add TLS 1.3 then we'll have to
> change its access key to 0 so that 3 becomes free ;-)

According to bug 839310 comment #14, 1.05% of some 170k tested servers didn't support TLS 1.x (and that test probably didn't cover mail servers), thus SSL 3.0 may still stick around for a while...
Callek, can this land on comm-central? It contains string changes, thus I don't want to miss Monday morning's train...
Flags: needinfo?(bugspam.Callek)
Comment on attachment 765022 [details] [diff] [review]
Patch for checkin [Check-in Comment 11]

a=me for CLOSED TREE checkin
Thanks, push for trunk please.
Flags: needinfo?(bugspam.Callek)
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: checkin-suite-needed
Target Milestone: --- → seamonkey2.21
Comment on attachment 765022 [details] [diff] [review]
Patch for checkin [Check-in Comment 11]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/4a17c2c1bb24
Attachment #765022 - Attachment description: Patch for checkin → Patch for checkin [Check-in Comment 11]
Bug 733647 hasn't landed on mozilla-central before the merge (currently in progress), thus the default for security.tls.version.max remains 1 representing TLS 1.0 for the maximum version on trunk and the aurora channel for now.

Consequently, if someone has concerns about allowing TLS 1.2 once SM 2.21 reaches the beta phase, please file a new bug for adding a hidden="true" to the "allowTLS12" checkbox for the beta/release channels. That would be trivial to add at this stage.
You need to log in before you can comment on or make changes to this bug.