Project

General

Profile

Feature #17220

Feature #17219: Replace TorBirdy

Set Thunderbird preferences previously set by TorBirdy

Added by segfault about 1 month ago. Updated 11 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
Due date:
% Done:

0%

Feature Branch:
feature/17219-replace-torbirdy
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Email Client

Description

TorBirdy sets a lot of Thunderbird preferences, see https://gitweb.torproject.org/torbirdy.git/tree/components/torbirdy.js#n535.

We should go through all of them, check whether they make sense in the context of Tails, and set them in our Thunderbird preferences.


Related issues

Related to Tails - Feature #17268: Revise prefs adopted from TorBirdy Confirmed
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

Associated revisions

Revision 87f737b8 (diff)
Added by segfault 13 days ago

Remove unused prefs (refs: #17220)

We adopted the pref "extensions.enigmail.protectHeaders" from TorBirdy, but it
is unused. The correct name is ".protectedHeaders".

Also, we don't want to enable this feature, see #13649.

Revision 222fc2e2 (diff)
Added by segfault 13 days ago

Set Enigmail preferences via AutoConfig (refs: #17220)

These preferences were previously set in /etc/thunderbird/pref/thunderbird.js,
but for some reason were not actually applied in Thunderbird.

Setting them via AutoConfig works.

Revision ca2cb3b9 (diff)
Added by segfault 13 days ago

Drop the custom Enigmail keyserver pref (refs: #17220)

We tested the default Enigmail keyservers work, so we don't have to set
a custom keyserver.

Revision 4dd91c82 (diff)
Added by segfault 11 days ago

Update names of prefs which force Thunderbird to use SSL (refs: #17220)

Revision e1e8daa4 (diff)
Added by segfault 11 days ago

Drop obsolete Thunderbird patch (refs: #17220)

This patch was already merged into Thunderbird.

History

#1 Updated by segfault about 1 month ago

  • Description updated (diff)

#2 Updated by segfault about 1 month ago

  • Description updated (diff)

#3 Updated by segfault 28 days ago

  • Feature Branch set to feature/17219-replace-torbirdy

I started with this ticket - which is the easiest one of the three subtasks of #17219, because we can probably just copy the settings from torbirdy.git/components/torbirdy.js to config/chroot_local-includes/etc/thunderbird/pref/thunderbird.js.

It's still a lot of work, because I want at least get a rough understanding of what those settings do, and whether they still exist (I already found two settings which were removed years ago). I do this by reading the descriptions on http://kb.mozillazine.org/About:config_entries.

I pushed a WIP commit, which adds the settings from the "Network" and "Security" sections. If anyone else wants to work on this, feel free to continue with the next sections.

#4 Updated by intrigeri 16 days ago

#5 Updated by segfault 14 days ago

  • Status changed from Confirmed to In Progress
  • Assignee set to segfault

#6 Updated by segfault 13 days ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)

Commit f629fceb9075b920eb4129b82618b80746b9f8a9 is ready for review

#7 Updated by segfault 13 days ago

segfault wrote:

Commit f629fceb9075b920eb4129b82618b80746b9f8a9 is ready for review

In fact all commits on the feature branch up to 07ebcd454ceefc044525981f1d3aefecdeddd240 are ready for review.

#8 Updated by intrigeri 13 days ago

  • Assignee set to intrigeri

#9 Updated by intrigeri 13 days ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to segfault
  • Reading the diff, it seems to me that the branch is re-enabling extensions.enigmail.protectHeaders. I did not verify this yet, though. We decided on #13649 to disable this feature; we also decided on a strategy to re-enable it, which I've pursued, but with little success so far (last time I checked, there was still no vaguely complete spec we could point MUA developers to, so it's hopeless in the current state of things to encourage them to add support for protected headers). I'm personally open to reconsider this and switch strategies, but IMO it needs a discussion that will not fit between now and 4.1; I don't think we have the legitimacy here to cancel/ignore a previous project-wide decision.
  • I did not check the Enigmail keyserver situation and we have no automated tests for it. Can you please confirm that the keyserver used by Enigmail has not changed compared to 4.0, and still works as well/poorly as usual?
  • I see a bunch of XXX in the imported prefs. I understand you added them, and just, wow, great work. I'm not sure what we should do about them. Do you have an idea about this? I'm fine with postponing this to a follow-up ticket and not blocking on this now. I'm also fine with just leaving them as-is.
  • Finally, I'm quite confused by the sslOnly and ssl_only situation:
    • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?
    • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

#10 Updated by segfault 13 days ago

#11 Updated by segfault 13 days ago

intrigeri wrote:

  • Reading the diff, it seems to me that the branch is re-enabling extensions.enigmail.protectHeaders. I did not verify this yet, though. We decided on #13649 to disable this feature; we also decided on a strategy to re-enable it, which I've pursued, but with little success so far (last time I checked, there was still no vaguely complete spec we could point MUA developers to, so it's hopeless in the current state of things to encourage them to add support for protected headers). I'm personally open to reconsider this and switch strategies, but IMO it needs a discussion that will not fit between now and 4.1; I don't think we have the legitimacy here to cancel/ignore a previous project-wide decision.

I did not mean to re-enable that feature, and actually I don't think I did, because the pref used by Enigmail is named protectedHeaders, not protectHeaders. The typo comes from TorBirdy. I now removed that pref.

  • I did not check the Enigmail keyserver situation and we have no automated tests for it. Can you please confirm that the keyserver used by Enigmail has not changed compared to 4.0, and still works as well/poorly as usual?

FTR: We discussed this via XMPP, intrigeri checked that the keyserver used by Enigmail works.

  • I see a bunch of XXX in the imported prefs. I understand you added them, and just, wow, great work. I'm not sure what we should do about them. Do you have an idea about this? I'm fine with postponing this to a follow-up ticket and not blocking on this now. I'm also fine with just leaving them as-is.

Because we have little time to get this ready for 4.1, I would like to postpone this to a follow-up ticket. I created #17268 for that.

  • Finally, I'm quite confused by the sslOnly and ssl_only situation:
    • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?
    • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

To clear this up, I will have to dive into the Thunderbird code again, which I don't have the time for today. So I'm postponing this to Saturday.

#12 Updated by intrigeri 12 days ago

  • Affected tool set to Email Client

#13 Updated by intrigeri 12 days ago

intrigeri wrote:

  • Reading the diff, it seems to me that the branch is re-enabling extensions.enigmail.protectHeaders. I did not verify this yet, though. We decided on #13649 to disable this feature; we also decided on a strategy to re-enable it, which I've pursued, but with little success so far (last time I checked, there was still no vaguely complete spec we could point MUA developers to, so it's hopeless in the current state of things to encourage them to add support for protected headers). I'm personally open to reconsider this and switch strategies, but IMO it needs a discussion that will not fit between now and 4.1; I don't think we have the legitimacy here to cancel/ignore a previous project-wide decision.

I did not mean to re-enable that feature, and actually I don't think I did, because the pref used by Enigmail is named protectedHeaders, not protectHeaders. The typo comes from TorBirdy. I now removed that pref.

Confirmed, and sorry for the noise!

  • I did not check the Enigmail keyserver situation and we have no automated tests for it. Can you please confirm that the keyserver used by Enigmail has not changed compared to 4.0, and still works as well/poorly as usual?

FTR: We discussed this via XMPP, intrigeri checked that the keyserver used by Enigmail works.

Confirmed in an image built from abc690987d33cba0d16575a4bb48e9a0726f5342.

  • I see a bunch of XXX in the imported prefs. I understand you added them, and just, wow, great work. I'm not sure what we should do about them. Do you have an idea about this? I'm fine with postponing this to a follow-up ticket and not blocking on this now. I'm also fine with just leaving them as-is.

Because we have little time to get this ready for 4.1, I would like to postpone this to a follow-up ticket. I created #17268 for that.

Perfect!

  • Finally, I'm quite confused by the sslOnly and ssl_only situation:
    • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?
    • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

To clear this up, I will have to dive into the Thunderbird code again, which I don't have the time for today. So I'm postponing this to Saturday.

Great. I'll do my best to help on the other remaining Thunderbird 68 / Torbirdy issues, in the hope it helps you focus on this one :)

#14 Updated by segfault 11 days ago

intrigeri wrote:

  • Finally, I'm quite confused by the sslOnly and ssl_only situation:
    • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?

Confirmed, this patch was merged into Thunderbird. I will remove it.

  • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

Also correct. A git grep sslOnly and git grep ssl_only in Debian's Thunderbird git repo (checked out at tag debian/1%68.2.2-1_deb10u1) shows that the ssl_only prefs are not used anymore, but the following prefs are being used:

mailnews.auto_config.fetchFromISP.sslOnly
mailnews.auto_config.sslOnly
mailnews.auto_config.guess.sslOnly

So I think it's safe to delete the ssl_only prefs and set the sslOnly prefs to true.

#15 Updated by intrigeri 11 days ago

  • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?

Confirmed, this patch was merged into Thunderbird. I will remove it.

Great. Thanks for looking into this! Note that this may impact Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch, which relies on a pref that won't have a default value set in the Thunderbird code anymore. I would hope that the fact we do set this pref ourselves in /etc/thunderbird/pref/thunderbird.js is sufficient to make this work, but I'd rather not rely on hope here, especially since I doubt we have means to easily test connections to Exchange servers :] See below.

  • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

Also correct. A git grep sslOnly and git grep ssl_only in Debian's Thunderbird git repo (checked out at tag debian/1%68.2.2-1_deb10u1) shows that the ssl_only prefs are not used anymore, but the following prefs are being used:

> mailnews.auto_config.fetchFromISP.sslOnly
> mailnews.auto_config.sslOnly
> mailnews.auto_config.guess.sslOnly
> 

So I think it's safe to delete the ssl_only prefs and set the sslOnly prefs to true.

Agreed, except one detail about mailnews.auto_config.sslOnly. I don't see it used in Debian's Thunderbird. I only see it in:

  • our obsolete Add-pref-for-whether-to-accept-plaintext-protocols-d.patch that you're going to remove, so let's ignore this one
  • Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch ← should this one be adjusted to use mailnews.auto_config.guess.sslOnly as well? This would avoid the potential regression I'm mentioning above. Ideally we would check the version of that patch that anonym last submitted upstream.

#16 Updated by segfault 11 days ago

intrigeri wrote:

  • We still have config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, while I thought anonym's patches that got merged upstream superseded that. Is this pref still honored anywhere?

Confirmed, this patch was merged into Thunderbird. I will remove it.

I have to take that back. The Thunderbird code I was looking at actually only had the patch applied because I applied it there earlier.

Looking at the bugzilla issue, I see that the result of the latest review of that patch was "I'm on the fence for these whether they are a good idea or should be dropped. And they need code changes, too.".

https://bugzilla.mozilla.org/user_profile?user_id=22599

Great. Thanks for looking into this! Note that this may impact Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch, which relies on a pref that won't have a default value set in the Thunderbird code anymore.

Didn't anonym introduce the pref used by that patch? At least on tag debian/1%60.4.0-1_deb8u1, I can't find any reference to mailnews.auto_config.ssl_only_mail_servers (which was the name of the pref used in the patch before I updated it).

  • Do we still need mailnews.auto_config.ssl_only_mail_servers and mailnews.auto_config.fetchFromISP.ssl_only or were they superseded by new prefs introduced upstream in v68 by anonym's patches?

Also correct. A git grep sslOnly and git grep ssl_only in Debian's Thunderbird git repo (checked out at tag debian/1%68.2.2-1_deb10u1) shows that the ssl_only prefs are not used anymore, but the following prefs are being used:

[...]

So I think it's safe to delete the ssl_only prefs and set the sslOnly prefs to true.

Agreed, except one detail about mailnews.auto_config.sslOnly. I don't see it used in Debian's Thunderbird. I only see it in:

  • our obsolete Add-pref-for-whether-to-accept-plaintext-protocols-d.patch that you're going to remove, so let's ignore this one
  • Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch ← should this one be adjusted to use mailnews.auto_config.guess.sslOnly as well? This would avoid the potential regression I'm mentioning above. Ideally we would check the version of that patch that anonym last submitted upstream.

If the pref used by that patch was also not set in Thunderbird 60, I don't see the potential for a regression, and then I don't think we should use mailnews.auto_config.guess.sslOnly. The documentation of that pref also clearly states that this is about using SSL when guessing, which is not what the Exchange patch does.

#17 Updated by segfault 11 days ago

  • Status changed from In Progress to Needs Validation
  • Assignee changed from segfault to intrigeri

#18 Updated by intrigeri 11 days ago

  • Status changed from Needs Validation to Resolved

Hi,

Looking at the bugzilla issue, I see that the result of the latest review of that patch was "I'm on the fence for these whether they are a good idea or should be dropped. And they need code changes, too.".

Indeed. And the corresponding Mozilla bug is closed as "RESOLVED FIXED".

https://bugzilla.mozilla.org/user_profile?user_id=22599

I guess you meant https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c222 :)

The rest of my previous comment, that you're disputing, only made sense under the assumption that you were going to remove config/chroot_local-includes/usr/share/tails/build/thunderbird-patches/Add-pref-for-whether-to-accept-plaintext-protocols-d.patch. Now that I've understood that in the end we can't do that, it's moot.

So I think we're good here, closing!

#19 Updated by intrigeri 11 days ago

  • Assignee deleted (intrigeri)

#20 Updated by segfault 11 days ago

intrigeri wrote:

https://bugzilla.mozilla.org/user_profile?user_id=22599

I guess you meant https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c222 :)

Indeed.

So I think we're good here, closing!

Yay!

Also available in: Atom PDF