861471 - Update the SSL Preference Pane after bug 733642 changed preference names and semantics

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Philip Chee]

Relevant Bugs:
FX Bug 733632 - Remove TLS version UI (Options > Advanced > Encryption > Protocols)
Core Bug 733642 - Allow the user to enable any version of TLS that libssl supports, maintaining our current defaults


Bug 733642
Removed:
> -pref("security.enable_ssl3",             true);
> -pref("security.enable_tls",		 true);

Added:
> +pref("security.tls.version.min", 0);
> +pref("security.tls.version.max", 1);

https://hg.mozilla.org/mozilla-central/rev/04dbe811e4a0#l3.26

> +  // 0 means SSL 3.0, 1 means TLS 1.0, 2 means TLS 1.1, etc.


From Bug 733632:

> tl;dr: Let's remove the "Protocols" option in Advanced encryption options 
> dialog box.
> 
> Right now, we have two checkboxes:
> 
>   [X] Use SSL 3.0   [X] Use TLS 1.0
> 
> We are adding support for TLS 1.1. When we add this support, we could add 
> another checkbox like this:
> 
>   [X] Use SSL 3.0   [X] Use TLS 1.0   [X] Use TLS 1.1
> 
> However, this would be confusing, because it is not possible to enable SSL 3.0 
> and TLS 1.1 without also enabling TLS 1.0; the range of enabled versions must 
> be contiguous, so only the following choices would be valid:
> 
>   [X] Use SSL 3.0   [X] Use TLS 1.0   [X] Use TLS 1.1
>   [X] Use SSL 3.0   [X] Use TLS 1.0   [ ] Use TLS 1.1
>   [ ] Use SSL 3.0   [X] Use TLS 1.0   [X] Use TLS 1.1
>   [X] Use SSL 3.0   [ ] Use TLS 1.0   [ ] Use TLS 1.1
>   [ ] Use SSL 3.0   [X] Use TLS 1.0   [ ] Use TLS 1.1
>   [ ] Use SSL 3.0   [ ] Use TLS 1.0   [X] Use TLS 1.1
> 
> Note also that the current UI lets us do this:
> 
>   [ ] Use SSL 3.0   [ ] Use TLS 1.0
> 
> which is nonsense, because at least one version must be enabled to do anything 
> sensible.
> 
> We will have compatibility features implemented so that basically will really 
> need to toggle these prefs, except experts. Therefore, I think about:config is 
> a sufficient UI for controlling this feature.

See Bug 733632 for other proposed UI.

Comment 1

[rsx11m]

I can look into this. Following Firefox's example to just remove the section from the pref pane obviously would be the easiest, but I assume that we may want to keep it (judging from Phil's opening post and his selection of the summary title).

The least intrusive way would be to add another checkbox to the current group:

  [x] Enable SSL 3.0
  [x] Enable TLS 1.0
  [ ] Enable TLS 1.1

and then have some JavaScript magic to only allow combinations which are possible, thus switching on or off other boxes as needed. In addition to the problem of unchecking the last remaining box, another ambiguity occurs going from

  [x] Enable SSL 3.0
  [x] Enable TLS 1.0
  [x] Enable TLS 1.1

to disable TLS 1.0

  [x] Enable SSL 3.0
  [ ] Enable TLS 1.0
  [x] Enable TLS 1.1

in which case it would be ambiguous whether to clear SSL 3.0 or TLS 1.0. This would either need some serious explanation in the Help content or even some pop-up warning if an impossible selection is made and thus refused.

(Quoting Zoltán Halassy from bug 733632 comment #15)
> Would two drop-down select boxes for minimum and maximum protocol versions
> confuse the user? It's in the "Advanced" tab anyway. The only constraint
> would be the MIN <= MAX.

This appears to be the more viable solution, a possible two-line implementation of this would be:

  You can choose which encryption protocols SeaMonkey uses for secure connections.
  Minimum required version:  [SSL 3.0]
  Minimum supported version: [TLS 1.0]

or

  You can restrict which encryption protocols to use for secure connections.
  Don't use versions older than: [SSL 3.0]
  Don't use versions newer than: [TLS 1.0]

Making those radiogroups would be difficult space-wise and may look too bulky. Changes to the implementation and related Help contents as well as the addition of some JavaScript logic to enforce MIN <= MAX should be reasonable here.

Discussion welcome.

Comment 2

[Philip Chee]

> Discussion welcome.
I'd normally suggest discussions should go to news.mozilla.org -> mozilla.dev.apps.seamonkey

Useful links:
<http://www.seamonkey-project.org/community#groups>

Comment 3

[rsx11m]

I'm not using newsgroups, thus bugzilla will have to do from my side.

(In reply to rsx11m from comment #1)
>   You can restrict which encryption protocols to use for secure connections.
>   Don't use versions older than: [SSL 3.0]
>   Don't use versions newer than: [TLS 1.0]

This is fairly straight-forward to implement, thus I'll just go ahead working out the patch to have something specific to discuss. It's either that (with these or some other labels) or removing it entirely as anything else proposed in bug 733632 would be rather complex in terms of logic or imply ridiculously long labels.

Comment 4

[rsx11m]

Attachment: Proposed patch (w/o Help)

Here the patch implementing the drop-down menu version with min<=max control.

This uses the "older/newer" variant of the labels as those seem more intuitive, details can be explained in the Help content. Also, the alternative longer text for the description wouldn't fit into a single line.

Comment 5

[rsx11m]

Attachment: Screenshot 2-listbox version

with the "max" menu opened. I'm using a grid to align the two drop-down menus, thus they aren't off by a few pixels. Setting the "min" box to "TLS 1.1" will force the "max" box to "TLS 1.1" as well; setting "max" to "SSL 3.0" will force "min" to go to "SSL 3.0" as well.

Comment 6

[[e-mail]]

Other ideas I came up with:

Radio buttons like Wikipedia diff screen. Probably takes up too much space though.

Multiselect listbox. Unfortunately the xul listbox is actually an extended select listbox. (A multiselect listbox only allows a single or contiguous selection, which is what we want.)

Although it could be done with checkboxes I agree the UI would be very awkward. A checkbox would be enabled if a) it was just before the first or just after the last checked checkbox b) it was the first or the last checked checkbox but not both.

Comment 7

[rsx11m]

(In reply to [e-mail] from comment #6)
> Radio buttons like Wikipedia diff screen. Probably takes up too much space
> though.

Yes, that option crossed my mind as well. This would imply that the left column has no labels and lines up with the right column providing the labels, and I'm not sure if this would line up as nicely as the two drop-down boxes (and on all platforms, that is). Making it two rows horizontally may need more space than is available, unless the labels to the left are really short. In either case, the logic should be similar to the menulist now.

> Multiselect listbox. Unfortunately the xul listbox is actually an extended
> select listbox. (A multiselect listbox only allows a single or contiguous
> selection, which is what we want.)

Ok, so I assume that this isn't an option.

> Although it could be done with checkboxes I agree the UI would be very
> awkward. A checkbox would be enabled if a) it was just before the first or
> just after the last checked checkbox b) it was the first or the last checked
> checkbox but not both.

Anything with checkboxes would complicate the JavaScript logic (and UX) quite a bit, thus I'd rather stick with menulists or the radiobuttons at best.

Comment 8

[rsx11m]

Attachment: Horizontal radio buttons

Here a quick mockup of the horizontal-radiogroup version, so this would fit but definitely would pose a problem with the accesskey assignment.

Comment 9

[rsx11m]

Attachment: Patch for radiogroups

Here the alternative patch with horizontal radiogroups and accesskeys defined. Minimum protocol selectors have characters (e.g, changing from S=SSL-3.0 to T=TLS-1.0 as a likely scenario) whereas the maximum selectors have numbers (e.g., from 0=TLS-1.0 to 1=TLS-1.1).

I had to change a couple of other accesskeys as well to accommodate those.

Comment 10

[rsx11m]

Having test-run both variants, my personal impression is that the radio buttons are easier to handle, both with the mouse (direct click rather than click+move) and the keyboard (direct accesskey for each option). Thus, from that point of view I'd prefer them over the drop-down menus. On the other hand, those may be easier to extend for additional protocols (e.g., TLS-1.2). There is space for a 4th radio button in each row, but not for a 5th if that should become necessary, where I'd expect older protocols to be retired once the newer ones become available.

Comment 11

[Ian Neal]

Comment on attachment 737193 [details] [diff] [review]
Patch for radiogroups

I prefer this version to the menu drop down version.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-ssl.dtd
> <!ENTITY warn.enteringsecure                "Loading a page that supports encryption">
>-<!ENTITY warn.enteringsecure.accesskey      "L">
>+<!ENTITY warn.enteringsecure.accesskey      "o">
> <!ENTITY warn.insecurepost                  "Sending form data from an unencrypted page to an unencrypted page">
>-<!ENTITY warn.insecurepost.accesskey        "S">
>+<!ENTITY warn.insecurepost.accesskey        "n">
For localisation purposes you might have to change the entity name to "warn.enteringsecure.accesskey2" and "warn.insecurepost.accesskey2" to make sure localisers check for accesskey conflicts.

>+++ b/suite/security/prefs/pref-ssl.js
>@@ -0,0 +1,26 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+function Startup()
>+{
>+  // as a sanity check, reset security.tls.version.min if it exceeds
>+  // security.tls.version.max which shouldn't be possible by UI logic
>+  SelectMaxVersion (document.getElementById("security.tls.version.max").value);
Nit: no space required between function name and (
>+}
>+
>+function SelectMinVersion(aValue)
I would prefer the function was called something like CheckMaxVersion, maybe have the argument being aMinValue
>+{
>+  // reset security.tls.version.max if it is lower than the selected minimum
>+  let maxVersion = document.getElementById("security.tls.version.max");
>+  if (maxVersion.value < aValue)
>+    maxVersion.value = aValue;
>+}
>+
>+function SelectMaxVersion(aValue)
I would prefer the function name to be something like CheckMinVersion, maybe have the argument being aMaxValue
>+{
>+  // reset security.tls.version.min if it is higher than the selected maximum
>+  let minVersion = document.getElementById("security.tls.version.min");
>+  if (minVersion.value > aValue)
>+    minVersion.value = aValue;
>+}

Don't forget to update the services.sync.prefs.sync.security.security.tls.version.min and services.sync.prefs.sync.security.security.tls.version.max prefs in http://mxr.mozilla.org/comm-central/source/suite/browser/browser-prefs.js in a similar way to bug 733642 (https://hg.mozilla.org/mozilla-central/diff/04dbe811e4a0/browser/app/profile/firefox.js)

f+ for the moment as I want to review updated patch with help.

Comment 12

[Ian Neal]

Either in this bug or a spin off, http://mxr.mozilla.org/comm-central/source/suite/profile/migration/src/nsThunderbirdProfileMigrator.cpp needs to be updated to.

Comment 13

[rsx11m]

(In reply to Ian Neal from comment #11)
> a similar way to bug 733642
> (https://hg.mozilla.org/mozilla-central/diff/04dbe811e4a0/browser/app/profile/firefox.js)

Thanks for the hint, and apparently a typo got into the Firefox patch (I'm not familiar with the sync stuff, but that looks wrong to me - filed bug 861633).

(In reply to Ian Neal from comment #12)
> Either in this bug or a spin off,
> http://mxr.mozilla.org/comm-central/source/suite/profile/migration/src/
> nsThunderbirdProfileMigrator.cpp needs to be updated to.

Well, I can change the MAKESAMETYPEPREFTRANSFORM entries to match name and type, that's easy; but if more migration code is needed to translate the old prefs into the new ones, that should go into a separate bug.

Comment 14

[Ian Neal]

(In reply to rsx11m from comment #13)
> (In reply to Ian Neal from comment #11)
> > a similar way to bug 733642
> > (https://hg.mozilla.org/mozilla-central/diff/04dbe811e4a0/browser/app/profile/firefox.js)
> 
> Thanks for the hint, and apparently a typo got into the Firefox patch (I'm
> not familiar with the sync stuff, but that looks wrong to me - filed bug
> 861633).
Yes, looks wrong to me too.

> (In reply to Ian Neal from comment #12)
> > Either in this bug or a spin off,
> > http://mxr.mozilla.org/comm-central/source/suite/profile/migration/src/
> > nsThunderbirdProfileMigrator.cpp needs to be updated to.
> 
> Well, I can change the MAKESAMETYPEPREFTRANSFORM entries to match name and
> type, that's easy; but if more migration code is needed to translate the old
> prefs into the new ones, that should go into a separate bug.
Yes, that is all I was after. The default prefs should be sufficient along with release notes (so I have added that keyword to the bug).

Comment 15

[rsx11m]

Attachment: Proposed patch (v2)

All items in comment #11 and comment #14 addressed. I'll get working on updating the Help file.

Comment 16

[rsx11m]

The current text in ssl_help.xhtml contains the following two paragraphs:

> <p><strong>Important note regarding TLS</strong>: Some servers that do not
>   implement SSL correctly cannot negotiate the SSL handshake with client
>   software (such as the browser) that supports TLS. Such servers are known as
>   <q>TLS intolerant</q>.</p>
> 
> <p>When the Enable TLS 1.0 option in the SSL preferences panel is selected, the
>   browser attempts to use the TLS protocol when making secure connections with
>   a server. If that connection fails because the server is TLS intolerant, the
>   browser will fall back to using SSL 3.0 (if enabled).</p>

The question aside to which extent the warning for TLS-intolerant servers is still applicable, it appears to me that this is a detail which can be simplified quite a bit. The main message IMO is that we try the highest (thus most secure) allowed version first and then fall back to lower (less secure) versions that we are still allowed to use. I'll try to generalize and shorten this statement.

Further up it reads:

> <p>The Secure Sockets Layer (SSL) protocol defines rules governing mutual
>  authentication between a website and browser software and the encryption of
>  information that flows between them. The Transport Layer Security (TLS)

This could use a link to glossary.xhtml#ssl which lists the protocols it is used for and complements that information. Assuming that the settings also apply to other communication like IMAP/POP/SMTP or LDAP, I'll add something that also mentions email services, thus not exclusively "website and browser software" as stated now.

Comment 17

[rsx11m]

Attachment: Proposed patch (v3)

This patch adds the Help update. I've extended the introduction as motivated in comment #16 and merged the paragraphs on TLS intolerance into the other paragraphs while generalizing and shortening their contents.

The only other change relative to attachment 737258 [details] [diff] [review] is the addition of aria-labelledby attributes to the radiogroups for screen-reader support.

(In reply to Ian Neal from comment #14)
> (https://hg.mozilla.org/mozilla-central/diff/04dbe811e4a0/browser/app/profile/firefox.js)
> Yes, looks wrong to me too.

Bug 861633 landed on mozilla-central already, so the code here should be correct.

Comment 18

[rsx11m]

(In reply to [e-mail] from comment #6)
> Although it could be done with checkboxes I agree the UI would be very
> awkward. A checkbox would be enabled if a) it was just before the first or
> just after the last checked checkbox b) it was the first or the last checked
> checkbox but not both.

After some feedback in the MozillaZine SM Builds forum (see link in the URL field) that the currently proposed radiobutton version may be too heavy in appearance and needs some figuring out what it does by the user, I'm working on a patch for an alternative checkbox version that's generic enough to be easily extended for future versions.

General idea:

1. Initialize checkboxes from prefs
   a. first, all boxes are unchecked and enabled
   b. check all boxes within {min,max} range (inclusive) 
   c. disable all boxes within {min+1,max-1} range (inclusive)
   d. if min==max, disable the single box checked (special case) 
2. Whenever a box is modified, identify the new range
   a. find the checked box with the lowest version
   b. find the checked box with the highest version
3. If that's a valid range, update {min,max} prefs
4. Update checked and disabled status based on new range (#1)

ETA tomorrow. My requests for the current patch remain for now; as said, this would be an alternative proposal.

Comment 19

[rsx11m]

Attachment: Alternative 3-checkbox version

This is the general layout of the alternative patch proposals coming up as motivated in the MZ forum thread. The description has been extended by a second row, making it clear that at least one box must be selected and that a range rather than arbitrary selection has to be made.

This is a straight-forward extension of the current layout, just changed from vertical to horizontal to accommodate the additional labels and the 4th box once bug 480514 and bug 861266 are established in the Core code.

Comment 20

[rsx11m]

Attachment: Alternative 3-checkbox patch

This is a direct implementation of the logic detailed in comment #18.
The Help content has been updated with a couple of examples in the Notes.

Comment 21

[rsx11m]

Attachment: Illustrative 4-checkbox function

This is for information only, adding the future TLS 1.2 checkbox to give a better impression how the logic functions. Shown here, top to bottom:

1. Default status (min=0, max=1)
2. Checking TLS 1.2 implies TLS 1.0 and TLS 1.1 (min=0, max=3)
3. Unchecking SSL 3.0 makes TLS 1.0 available again (min=1, max=3)
4. Unchecking TLS 1.2 makes TLS 1.1 available again (min=1, max=2)
5. Unchecking TLS 1.0 locks TLS 1.1 as the last box (min=2, max=2)
6. Checking SSL 3.0 implies TLS 1.0 and make TLS 1.1 available (min=0, max=2)

Comment 22

[rsx11m]

Attachment: Illustrative 4-checkbox patch

Patch implementing attachment 739582 [details] for you to use as a functional demo.

Comment 23

[rsx11m]

Attachment: Non-disabling 4-checkbox patch

I was thinking that disabling the boxes as done in the previous patches may be ambiguous. We usually gray out options in the pref panels which are currently not valid (i.e., the function is disabled, not just the option inaccessible). Thus, it may not be recognized as "you can't change this right now" but possibly as "this version won't work right now" (the other two guys in the MZ thread disagree with me on that, but I bring this up here anyway).

So, this version (4 checkboxes again for illustration) doesn't disable the boxes and just applies all the other logic. Thus, intermediate or single boxes stay enabled. When you try to uncheck them, you'll get a respective "pushed" appearance first, but when releasing the checkbox it remains checked (to be more specific, the JavaScript logic immediately switches it back as part of the oncommand processing). This may be a bit too weak as a user feedback.

Comment 24

[rsx11m]

Attachment: Non-graying label compromise

This is an intermediate solution between disabling the checkbox when it isn't accessible and leaving it unchanged even if unchecking it isn't possible.

In an extension of attachment 739577 [details], the checkboxes are still disabled, but the style of the label remains unchanged (i.e., it is neither grayed not gets the shadow when disabled). This should distinguish the "this is not functional right now" case from the "you can't touch this right now but it's still in effect" semantics needed here.

Comment 25

[rsx11m]

Attachment: Non-graying label patch

This is attachment 739580 [details] [diff] [review] extended with style definitions to prevent the label from graying out when the checkbox is disabled, thus conveying the desired status. Since I'm lazy I've defined the respective class="nogray-disabled" rules in the global communicator.css file. I'm not sure if those need to go into the themes instead, but this should do for now to illustrate the function.

That's my last patch in this series, there are hopefully enough alternatives on the table now to pick the best one from the UX perspective.

Comment 26

[rsx11m]

Attachment: Non-graying patch (using themes)

Moved checkbox.nogray-disabled[disabled="true"] rules from communicator.css to the prefpanels.css definitions of both themes where they should fit better.

If we go with a 3-checkbox version, I'd prefer this over the other options in attachment 739580 [details] [diff] [review] and attachment 739597 [details] [diff] [review].

Comment 27

[[e-mail]]

OK, so I got around to giving these patches a spin.

With both the menulist patch and the radiobutton patches I felt the behaviour when you increased the minimum above the maximum or decreased the maximum below the minimum felt awkward. I wonder what the effect would have been if those options had simply been disabled instead. The other issue with both patches is that they took up a lot of space, although as a workaround I think the menulists could have been put on the same line. The menulists also had the advantage of requiring the fewest accesskeys of any of the patches.

The checkbox patch worked quite well, however the non-graying effect didn't appear work for me in the Modern theme.

Comment 28

[rsx11m]

Attachment: Non-graying patch (v3)

(In reply to [e-mail] from comment #27)
> The checkbox patch worked quite well, however the non-graying effect didn't
> appear work for me in the Modern theme.

Oops, my bad. Yes, the modern theme needs a more specific selector, and also an !important; to overwrite the more general definition in checkbox.css (apparently overriding the toolkit definition?). Fixed in this patch.

> With both the menulist patch and the radiobutton patches I felt the
> behaviour when you increased the minimum above the maximum or decreased the
> maximum below the minimum felt awkward. I wonder what the effect would have
> been if those options had simply been disabled instead.

Well, I could use a similar approach as for the checkboxes, iterating over the elements of each list and disabling them according to the {min,max} values.

> The menulists also had the advantage of requiring the fewest accesskeys of any
> of the patches.

Yes, but as a drawback you are loosing the ability to change the selection with a single key stroke given that you first have to select the list, then modify it.

Comment 29

[rsx11m]

Attachment: Disabled radio buttons

Similar to attachment 737192 [details] but with disabling of invalid choices.

1. default (minimum TLS 1.1 is disabled due to maximum being TLS 1.0 only)
2. changing minimum to TLS 1.0 to exclude SSL 3.0 (maximum SSL 3.0 disabled)
3. changing maximum to TLS 1.1 (minimum TLS 1.1 becomes available)
4. changing minimum to TLS 1.1 (maximum TLS 1.0 becomes disabled)
5. changing minimum to SSL 3.0 again (all choices become available)

Comment 30

[rsx11m]

Attachment: Radio buttons patch (v4)

This is attachment 737347 [details] [diff] [review] (v3), modified to implement attachment 739906 [details].

I haven't updated the 2-menulist patch given that Ian f-'ed it in the beginning (and the difference should be minimal except for the 1-line layout changes).

Comment 31

[rsx11m]

(In reply to rsx11m from comment #28)
> (In reply to [e-mail] from comment #27)
> > The menulists also had the advantage of requiring the fewest accesskeys of any
> > of the patches.
> 
> Yes, but as a drawback you are loosing the ability to change the selection with
> a single key stroke given that you first have to select the list, then modify it.

BTW: The 3-checkbox patch in attachment 739900 [details] [diff] [review] leaves all existing accesskeys as they currently are, thus no need for the user to adjust to new ones. It only adds the "1" for the new TLS 1.1 checkbox.

Comment 32

[rsx11m]

Comment on attachment 739907 [details] [diff] [review]
Radio buttons patch (v4)

Based on feedback received in the MZ forum thread, this version seems to be clearer than attachment 737347 [details] [diff] [review]. The main issue appears to be that the user has to grasp the "from ... to ..." meaning of the two rows, thus to understand that checking SSL 3.0 in "Oldest" and TLS 1.1 in "Newest" implies that TLS 1.0 is also active despite not being checked anywhere.

If you want to revisit the menulist option, the labels can be more descriptive there to make it clearer what those are doing. For the 1-line version, e.g.,

  Support from [SSL 3.0 v] (oldest) to [TLS 1.1 v] (newest)

but that still requires some mental exercise to understand that TLS 1.0 is covered (and may be even less clear given that the radiobuttons illustrate that order of protocols which the menulists don't).

In contrast, the 3-checkbox version is absolutely unambiguous which protocols are permitted. Thus, it still may be the best solution after all.

Comment 33

[rsx11m]

Comment on attachment 739580 [details] [diff] [review]
Alternative 3-checkbox patch

Let me cancel the requests for this patch as attachment 739900 [details] [diff] [review] with non-graying labels is the preferred version for this solution.

Comment 34

[[e-mail]]

Comment on attachment 737347 [details] [diff] [review]
Proposed patch (v3)

Sorry, I don't like this version.

Comment 35

[[e-mail]]

Comment on attachment 739907 [details] [diff] [review]
Radio buttons patch (v4)

This version is OK, I guess.

Comment 36

[[e-mail]]

Comment on attachment 739900 [details] [diff] [review]
Non-graying patch (v3)

This one looks OK too. It has the advantage of using less space, but that tristate checkbox still bugs me, even though you've done it the best way.

Comment 37

[[e-mail]]

It occurs to me that none of the patches work properly with locked preferences.

My proposed "menulist with disabled items" patch would actually have worked best here, since the appropriate menulist(s) would have been disabled, and therefore the state of their items would have been irrelevant.

The non-disabling radiobutton patch has the problem that you can e.g. set a minimum version greater than the locked maximum version, which will then attempt to update the maximum.

The disabling radiobutton patch would almost work except that it would have to be tweaked not to accidentally enable a radiobutton when the radiogroup is disabled.

The checkbox version would need a bit more thought. Obviously if both versions are locked then all the checkboxes need to be disabled otherwise the checkboxes for the locked version and any unavailable versions need to be disabled. Note however that in this case the labels for those checkboxes need to be grayed too!

Comment 38

[rsx11m]

(In reply to [e-mail] from comment #37)
> It occurs to me that none of the patches work properly with locked preferences.

I'll have to look into this. The preferences have a read-only "locked" property, that's the easy part, but I'll have to figure out how to set this for testing.

I found a couple of references regarding creating a .cfg file pointing from
https://developer.mozilla.org/en-US/docs/XUL/School_tutorial/Handling_Preferences
but this needs some learning.

> The disabling radiobutton patch would almost work except that it would have
> to be tweaked not to accidentally enable a radiobutton when the radiogroup
> is disabled.

This could probably be done by just adding "&& !.locked" phrases to the .disabled terms, that might work out of the box.

> The checkbox version would need a bit more thought. Obviously if both
> versions are locked then all the checkboxes need to be disabled otherwise
> the checkboxes for the locked version and any unavailable versions need to
> be disabled. Note however that in this case the labels for those checkboxes
> need to be grayed too!

Yes, in addition to just the "disabled" attribute this version could have another attribute for the "nogray" status. If just one but not the other preference is locked, all boxes <= min will be disabled but !nogray if the .min pref is locked, and all boxes >= max will be disabled but !nogray if the .max pref is locked.

So, that's feasible as well but implies a bit more overhead.

Comment 39

[rsx11m]

Well, so for some reason I can't get the prefs locked. I've added the following

  pref("general.config.obscure_value", 0);
  pref("general.config.filename", "mozilla.cfg");

as instructed and it shows up in about:config. It also seems to access the file

  prefLock("security.tls.version.min", 1);

given that I get an error on startup if mozilla.cfg isn't present. However, neither is the value set nor the preference locked. I've tried with other preferences associated with a pref pane and they weren't locked either.

As a fallback, I've simply tested replacing "xxx.locked" with "true" which should hopefully have the same effect, but please double-check that it works.

Comment 40

[rsx11m]

Attachment: Radio buttons patch (v5)

(In reply to rsx11m from comment #38)
> (In reply to [e-mail] from comment #37)
> > The disabling radiobutton patch would almost work except that it would have
> > to be tweaked not to accidentally enable a radiobutton when the radiogroup
> > is disabled.

This is attachment 739907 [details] [diff] [review] with locking code added. Also, disabling the radiogroup will disable all radiobuttons below it. I've renamed the functions to Enable{Min,Max}Version() given that this is now their main function.

> This could probably be done by just adding "&& !.locked" phrases to the
> .disabled terms, that might work out of the box.

Make that "|| .locked" of course, as implemented here.

Comment 41

[rsx11m]

Attachment: Non-graying checkbox (v4)

(In reply to rsx11m from comment #38)
> (In reply to [e-mail] from comment #37)
> > The checkbox version would need a bit more thought. Obviously if both
> > versions are locked then all the checkboxes need to be disabled otherwise
> > the checkboxes for the locked version and any unavailable versions need to
> > be disabled. Note however that in this case the labels for those checkboxes
> > need to be grayed too!
> 
> Yes, in addition to just the "disabled" attribute this version could have
> another attribute for the "nogray" status. If just one but not the other
> preference is locked, all boxes <= min will be disabled but !nogray if the
> .min pref is locked, and all boxes >= max will be disabled but !nogray if
> the .max pref is locked.

This adds the "locked" logic to attachment 739900 [details] [diff] [review] along with a new "nogray" attribute. Since there isn't automatically a property added, I have to use setAttribute()/removeAttribute() instead.

Tested with both classic and modern themes.

Comment 42

[rsx11m]

Attachment: Non-graying checkbox (v5)

Nit corrected:

>+    if ((minLocked && maxLocked) || minLocked && index <= minVersion ||
>+                                    maxLocked && index >= maxVersion)

The "&&" binds stronger than the "||" as this works, but I've put those into explicit parentheses anyway to keep the code unambiguous (and consistent with other uses of ORed "&&" clauses). That's the only change.

Comment 43

[Jens Hatlak (:InvisibleSmiley)]

(In reply to rsx11m from comment #39)
>   prefLock("security.tls.version.min", 1);

Typo? According to [1] it's lockPref(), and this is also what I remembered ([2] refers to lock_pref(), but I think that's wrong). Reading [1] also suggests that you should have a double slash on the first line of the file.

[1] <http://kb.mozillazine.org/Locking_preferences>
[2] <https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences>

Comment 44

[rsx11m]

Thanks, that did it! I've tried a couple of permutations and thought that I had tried all, but apparently not. Using the correct identifier didn't fix it alone, those "//" at the beginning of the file are indeed required for some reason.

Comment 45

[[e-mail]]

Comment on attachment 740166 [details] [diff] [review]
Non-graying checkbox (v5)

>+      currentBox.disabled = (index > minVersion && index < maxVersion) ||
>+                            (index == minVersion && minVersion == maxVersion);
[Don't know if IanN will agree with me but I think this might be easier to read if it said index == minVersion && index == maxVersion]

Comment 46

[[e-mail]]

Comment on attachment 740155 [details] [diff] [review]
Radio buttons patch (v5)

>+  let minLocked  = document.getElementById("security.tls.version.min").locked ||
>+                   document.getElementById("limitMinVersion").disabled;
IIRC you don't have to perform both checks as the prefwindow disables the radiogroup if the pref is locked. In fact, if the radiogroup is disabled you might as well just return early, as the radiobuttons will already be disabled so disabling them again is a no-op.

Comment 47

[rsx11m]

Attachment: Non-graying checkbox (v6)

(In reply to [e-mail] from comment #45)
> >+      currentBox.disabled = (index > minVersion && index < maxVersion) ||
> >+                            (index == minVersion && minVersion == maxVersion);
> [Don't know if IanN will agree with me but I think this might be easier to
> read if it said index == minVersion && index == maxVersion]

I like that one better too, thus let's base it on index in all cases.

Comment 48

[rsx11m]

Attachment: Radio buttons patch (v6)

(In reply to [e-mail] from comment #46)
> >+  let minLocked  = document.getElementById("security.tls.version.min").locked ||
> >+                   document.getElementById("limitMinVersion").disabled;
> IIRC you don't have to perform both checks as the prefwindow disables the
> radiogroup if the pref is locked. In fact, if the radiogroup is disabled you
> might as well just return early, as the radiobuttons will already be
> disabled so disabling them again is a no-op.

Yes this works. I wasn't quite sure about the interaction between those elements and thus played it safe, but just checking on the radiogroup being disabled and only running the loop if it's not indeed does the job already.

Comment 49

[Ian Neal]

Comment on attachment 740576 [details] [diff] [review]
Non-graying checkbox (v6)

r=me as I prefer this one.

Comment 50

[Ian Neal]

Comment on attachment 740580 [details] [diff] [review]
Radio buttons patch (v6)

This is ok, but I prefer the checkbox version.

Comment 51

[rsx11m]

Comment on attachment 740580 [details] [diff] [review]
Radio buttons patch (v6)

Thanks Ian, I'm marking this one as obsolete then to avoid confusion.

Comment 52

[rsx11m]

I've obsoleted the additional patches but left the screenshots visible, thus the history of the decision is clear.

[It would be nice if one could (un)obsolete multiple attachments at once...]

Since there were no further requests for changes, push for comm-central please.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9c50ce882411

Comment 54

[Alexander L. Slovesnik]

I wonder why this patch shows TLS 1.1 as available option in SeaMonkey preferences? Bug 733647 is not fixed yet.

Comment 55

[rsx11m]

That specific bug is about making TLS 1.1 the default, isn't it?
Bug 565047 - Implement TLS 1.1 (RFC 4346) is resolved as fixed.

Comment 56

[rsx11m]

Note that bug 733642 provided the means to enable TLS 1.1 but not TLS 1.2 (for which bug 480514 is still pending), thus I don't see any indication that TLS 1.1 isn't supported yet at this time.

(Quoting Brian Smith (:bsmith) from bug 733642 comment #31)
> [...] We need the ".min" pref for some enterprise deployments that must
> disable SSLv3 due to government regulations. We need the ".max" pref so that
> people can test TLS 1.1 and TLS 1.2 support before we enable either of them
> by default, and to make it easier to rollback the enabling of TLS 1.1 or TLS
> 1.2 in case a last-minute regression is found.

Following this logic, we can still hide or disable the TLS 1.1 checkbox in comm-aurora or comm-beta, in case support for TLS 1.1 is indeed rolled back for some reason in Core.

Comment 57

[rsx11m]

(Quoting Wan-Teh Chang from bug 565047 comment #118)
> TLS 1.1 is now available in the new NSS 3.14 release.
> Marked this NSS bug fixed.
> 
> Bug 733647 is about using TLS 1.1 in Gecko (Firefox, Thunderbird),
> on by default. Almost all of the necessary workarounds are performed
> by NSS. The only workaround that must be performed by an application
> is the protocol version fallback when the handshake fails. (The
> workarounds in NSS do not require protocol version fallback.)

Which is Bug 839310 - Add insecure fallback from TLS 1.1 -> TLS 1.0.

Since TLS 1.1 isn't the default yet, lack of the fallback shouldn't impact the user unless he or she enables TLS 1.1 explicitly. In this case, the user should be aware of that selection, and we mention in the Help content that TLS 1.1 ("newer versions" in general, that is) may not be supported yet by websites. Thus, if the user runs into problems, the solution of disabling TLS 1.1 again should be straightforward to find.

Thus, unless anyone disagrees, I don't see an immediate need for any action. I've CCed myself to the relevant bugs and will follow them, in case anything comes up during the aurora or beta periods.