1137991 - Remove SSLv3 option from SSL panel in Privacy & Security preferences

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Edmund Wong (:ewong)]

Bug 1106470 will be dropping support for SSLv3.  Once that happens,
there's little point in having the SSLv3 option in Privacy & Security->SSL
panel.

Comment 1

[rsx11m]

Taking, but nothing has been checked in yet on the Core/PSM side (targeted for Gecko 39). This may need some migration code for people who still have SSL 3.0 enabled on purpose (if we remove the checkbox, no way for the user to uncheck it from the UI).

Comment 2

[Masatoshi Kimura [:emk]]

When bug 1106470 is fixed, security.tls.version.min=0 will be invalid and it will fallback to the default value (1).

Comment 3

[rsx11m]

Thanks, good to know. Indeed, that's more a UI problem as the boxes can get into a state where you can't modify either .min (if its value is less than the allowed minimum) or .max (if its value exceeds the allowed maximum). Thus, I should probably revisit the logic here to ensure that only valid input ranges are considered to initialize the checkboxes, thus to avoid locking up the selections.

Comment 4

[rsx11m]

On a side note: With the term "SSL" officially retiring soon, I'm wondering if we should rename that prefpane (which I'd file a separate bug for as it would likely require widespread adjustments to labels and Help content beyond the scope of what we are doing here). On the other hand, "SSL" is a reasonably established term, and other browsers/mail programs still use it routinely.

Something like "Transport Layer Security (SSL/TLS)" sounds more clear anyway (what's a "Socket" and why does it have to be "Secure"?), or maybe "Connection Security (SSL/TLS)" would be least ambiguous (with the short panel title in the prefpane tree being "SSL/TLS" or just "Connection Security").

Comment 5

[rsx11m]

Bug 1106470 has landed on mozilla-inbound.

Comment 6

[rsx11m]

Attachment: Proposed patch

This removes the allowSSL30 checkbox with respective changes to labels, logic, and Help content. The logic is extended to verify in UpdateSslBoxes() that the min/max values are in the valid range, and assumes valid min/max limits them if not; the preferences themselves aren't modified until the user actually touches any of the boxes.

Comment 7

[[e-mail]]

Comment on attachment 8574761 [details] [diff] [review]
Proposed patch

>+  gSslPrefElementIds = ["allowTLS10", "allowTLS11", "allowTLS12"];
>+  gSslPrefValues = [1, 2, 3];
Maybe use a Map to make the relation between pref values and the pref elements clearer?
gSslPrefElementIds = new Map([[1, "allowTLS10"], [2, "allowTLS11"], [3, "allowTLS12"]]);

Comment 8

[rsx11m]

Attachment: Use a Map instead (v2)

Yeah, that works nicely and simplifies things.

(From bug 1106470 attachment 8569820 [details] [diff] [review])
>   // convert min/maxFromPrefs to the internal representation
>   minFromPrefs += SSL_LIBRARY_VERSION_3_0;
>   maxFromPrefs += SSL_LIBRARY_VERSION_3_0;
>   // if min/maxFromPrefs are invalid, use defaults
>   if (minFromPrefs > maxFromPrefs ||
>-      minFromPrefs < range.min || maxFromPrefs > range.max) {
>+      minFromPrefs < supported.min || maxFromPrefs > supported.max ||
>+      minFromPrefs < SSL_LIBRARY_VERSION_TLS_1_0) {
>     return;
>   }

I've revisited that based on comment #2. Since indeed the defaults are used when either of the conditions is met, I've modified the minVersion/maxVersion definitions used for the checkbox initialization respectively and take the defaults for both even if just one of them is outside of the valid range (also get them from the default prefbranch as they may not match the minimum/maximum values of the allowed pref settings).

Comment 9

[[e-mail]]

Comment on attachment 8574850 [details] [diff] [review]
Use a Map instead (v2)

>+    minVersion = defaultBranch.getIntPref("security.tls.version.min");
>+    maxVersion = defaultBranch.getIntPref("security.tls.version.max");
document.getElementById("security.tls.version.min").defaultValue;
document.getElementById("security.tls.version.max").defaultValue;

>+    let currentBox = document.getElementById(name);
I wouldn't be averse to naming the id "id" ;-)

Comment 10

[rsx11m]

Attachment: Comment #9 addressed (v3)

Getting better...

Comment 11

[rsx11m]

(Quoting to Masatoshi Kimura [:emk] from bug 1141394)
> In bug 1106470, I intentionally left strings and idl constants
> in case we have to backout the change on beta or aurora.
> Once the SSLv3 removal reaches to release, we can cleanup them.

Shall we proceed in the same way "just in case" that the SSL 3.0 strings are needed if and when it should be decided not to let the Core changes proceed down the release train? It sure would be trivial to defer removal of those two entities for 3+ cycles after the changes have hit the releases.

Comment 12

[rsx11m]

Attachment: String changes deferred (v4)

Indeed, I think we should proceed in this way given that the core changes may depend on Google timing: 

(Quoting David Keeler [:keeler] from bug 1106470 comment #4)
> In general, looks good. I found more things to remove, however. Also, we
> should keep a very close eye on this. If it doesn't look like SSL 3 is going
> to be similarly removed from Chrome 44 or if that release date is not close
> enough to the release date for Firefox 39 (looks like 30 June 2015), we
> should hold this back at least one release.

Comment 13

[[e-mail]]

Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

Sorry for the delay, my computer has been acting strangely and I just noticed that it has 360 bad sectors. (But S.M.A.R.T. says that that's fine... go figure.)

Comment 14

[rsx11m]

Yeah, S.M.A.R.T. isn't always very smart, though it should report at least that those blocks on the disk have been reallocated to spares (or that it ran out of those). Thanks for the ui-review!

Comment 15

[Ian Neal]

Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

r=me
Could you link the relevant bug for the string removal.

Comment 16

[rsx11m]

Push for comm-central, please. I'll open a follow-up bug for the removal of the strings.

Comment 17

[rsx11m]

Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

[Approval Request Comment]
Regression caused by (bug #): Bug 1106470
User impact if declined: non-functional UI
Testing completed (on c-c, etc.): tested on c-c prior to the merge, thus c-a
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: Help only, see bug 1149581 for strings

Comment 18

[rsx11m]

Thanks, make that push please for comm-central /and/ comm-aurora.

Comment 19

[Philip Chee]

Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

http://hg.mozilla.org/comm-central/rev/2dfbfcd7d32c

Comment 20

[Philip Chee]

(In reply to Philip Chee from comment #19)
> http://hg.mozilla.org/comm-central/rev/2dfbfcd7d32c
http://hg.mozilla.org/releases/comm-aurora/rev/976e3ac4f9d9