Tails

#7 Updated by segfault 2019-11-28 15:01:19

segfault wrote:
> Commit f629fceb9075b920eb4129b82618b80746b9f8a9 is ready for review

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

#11 Updated by segfault 2019-11-28 21:37:03

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 Feature #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 Feature #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.

#13 Updated by intrigeri 2019-11-29 07:44:11

> 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 Feature #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 commit: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 Feature #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 2019-11-30 02:54:41

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 2019-11-30 07:32:14

>> 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 2019-11-30 12:29:50

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.

#20 Updated by segfault 2019-11-30 14:47:35

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!