1611624 - Does not use IDLE with Apple iCloud Mail Server

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Arno]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

Configure iCloud Mail Account with IMAP Server imap.mail.me.com

Actual results:

Mails will not push with the imap idle (allow immediate server notification when new messages arrive) protocol. Mails will only arrive by time (check for new massages every x minutes) or when you collect them manually (press get messages).

Expected results:

Mails should be pushed by IDLE function. Apple mail server gives capability: IDLE. IDLE function can positively be tested via Telnet and is working with other mail clients: e.g. Apple Mail on MacOS, Outlook on Windows, K9 Mail on Android

Comment 1

[gene smith]

It appears that tb assumes that when the imap connection is inside a SSL/TLS tunnel, that the capabilities returned in the initial greeting are definitive and tb doesn't ask again after authentication. You are right, tb never enables IDLE for icloud since icloud doesn't list IDLE in the initial capability greeting. (Icloud only mentions the proprietary XAPPLEPUSHSERVICE in the greeting which serves a similar purpose as IDLE but is not supported by tb.)

I did a connection to icloud using openssl and imap capability requests after authentication do list IDLE but currently tb never does this 2nd capability command to learn about it.

I checked one other imap servers (Courier) that connects using SSL/TLS and they list IDLE in the initial capability greeting so the problem identified here doesn't occur. I need to check a few more.

Anyhow, at this point I'm not sure if this is a bug or not...

You mention using telnet to test this. Does icloud allow totally unencrypted connections? Maybe you really mean openssl or something like it?

Comment 2

[gene smith]

I checked this with several other imap servers that support idle and they all report IDLE in the initial capability response before login/authentication occurs. The only time tb asks again for capability again after a mailbox connection is when STARTTLS is used. However, icloud doesn't support STARTTLS so that won't work-around the problem.
My reading of the imap rfc3501 is that tb is following the spec. The only time that a 2nd capability response should be obtained is after STARTTLS completes (due to transition from plaintext to encrypted that occurs). When the connection is encrypted from the start as with TLS/SSL, the server should provide the complete capability set and not leave out anything.
So the problem is that icloud should advertise IDLE in the initial capability response but it doesn't as shown below:

[gene@wally comm]$ openssl s_client -connect imap.mail.me.com:993 -crlf
CONNECTED(00000003)
depth=2 C = US, O = GeoTrust Inc., CN = GeoTrust Global CA
verify return:1
depth=1 CN = Apple IST CA 2 - G1, OU = Certification Authority, O = Apple Inc., C = US
verify return:1
depth=0 CN = imap.mail.me.com, OU = management:idms.group.859635, O = Apple Inc., ST = California, C = US
verify return:1
---
:
:
* OK [CAPABILITY XAPPLEPUSHSERVICE IMAP4 IMAP4rev1 SASL-IR AUTH=ATOKEN AUTH=PLAIN] (2006B33-db0005c7b77c) st43p00im-tygg09070901.me.com
. authenticate plain
+
AGQkLnNtdGhAaWabc3VkLmNvbQBvZmttenZzYmQtZHN4bS1qcWho
. OK user gd.smth authenticated
. capability
* CAPABILITY XAPPLEPUSHSERVICE IMAP4 IMAP4rev1 CONDSTORE ENABLE QRESYNC QUOTA NAMESPACE UIDPLUS CHILDREN BINARY UNSELECT SORT CATENATE URLAUTH LANGUAGE ESEARCH ESORT THREAD=ORDEREDSUBJECT THREAD=REFERENCES CONTEXT=SEARCH CONTEXT=SORT WITHIN SASL-IR SEARCHRES METADATA ID XMSEARCH X-SUN-SORT ANNOTATE-EXPERIMENT-1 X-UNAUTHENTICATE X-SUN-IMAP XUM1 MULTISEARCH IDLE
. OK Completed

With this server bug, many other imap capabilities other than IDLE are also not seen by tb.

Comment 3

[gene smith]

Attachment: icloud2.diff

I changed my mind. It does appear that there may be a fairly easy fix for this. The attached patch fixes the problem but I haven't tested it a lot yet.

Comment 4

[gene smith]

Attachment: icloud3.diff

I tested it again but made a small change and this version works fine with the fair large set of servers I have configured. Still, the only one I found that really needs this is icloud (Apple's server) so don't know if we want to implement this.

What this does is it expects to see an imap capability response getting parsed as part of the authentication or login imap command response, either as an untagged response or as a command code response (in brackets after the authenticate OK response). If the capability response is not detected, an explicit capability request is performed before doing anything else. This ensures that the full capability set is obtained, including IDLE and many others, when authenticated imap state is entered.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #4)

If the capability response is not detected, an explicit capability request is performed before doing anything else.
This ensures that the full capability set is obtained, including IDLE and many others, when authenticated imap state is entered.

I like the idea, so that we provide the user with a good experience and we might encounter systems other than icloud that are out of compliance. At the same time,

• is icloud aware they out of compliance with the RFC ?
• should detected instances of non-compliance trigger something like an assert, so these do not go entirely undetected?

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9135553 [details] [diff] [review]
icloud3.diff

Review of attachment 9135553 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good I think.
Needs a commit message, and please add there an explanation of why you're doing this.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2018,5 @@
>                 "response");  // we should always have this
> +  if (navCon) {
> +      navCon->CommitCapability();
> +      MOZ_LOG(IMAP, mozilla::LogLevel::Info,
> +              ("gds: caps parsed"));

remove

Comment 7

[gene smith]

(In reply to Wayne Mery (:wsmwk) from comment #5)

(In reply to gene smith from comment #4)

If the capability response is not detected, an explicit capability request is performed before doing anything else.
This ensures that the full capability set is obtained, including IDLE and many others, when authenticated imap state is entered.

I like the idea, so that we provide the user with a good experience and we might encounter systems other than icloud that are out of compliance. At the same time,

• is icloud aware they out of compliance with the RFC ?

Well, looking at this again and at rfc 3501, I'm don't really see that they are technically out of compliance. They just don't behave like all the other servers that connect via SSL/TLS. All the rest provide in the greeting or in the pre-login capability response at least the authentication methods (Icloud provides just auth methods). Then once you have logged in, if the capabilities are different, in the tagged OK response they provide the new capabilities. So servers may exclude IDLE from the pre-login CAPABILITY list, but once you login or authenticated, they tell you the new capabilities, like IDLE and all the others without being asked. Icloud doesn't do this but requires that you explicitly ask again for capabilities after authentication. And without the patch for this bug, tb never asks again so it never sees the IDLE capability (plus several others that may be helpful like CONDSTORE, UIDPLUS etc.)

• should detected instances of non-compliance trigger something like an assert, so these do not go entirely undetected?

I'm not sure this would help. Maybe a MOZ_LOG too could be added where I do the new Capability() call.

For some servers that provide the full capability set before login and then don't report them again as part of the login response, this change will cause a redundant capability request to occur. With all the other imap stuff going on, I don't see this as a big problem.

Comment 8

[gene smith]

Attachment: Bug1611624_iCloud-capabilities.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9137035 [details] [diff] [review]
Bug1611624_iCloud-capabilities.patch

Review of attachment 9137035 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have an account to test this. But looks reasonable, so rs=mkmelin

Comment 10

[Arno]

Hello,

thank you for your work. I'm not so familiar with these processes. As far as I understand, the behaviour is accepted as a bug. There was a patch written and the patch was reviewed. How long will it take until this patch finds it way in the next update of thunderbird. I would realy like to test and see if the problem is solved.

Comment 11

[gene smith]

Sorry, I think I should have set the keyword "checkin-needed-tb" to flag this as complete. But it's been a while so I need to re-look at the patch and test again that it merges and works with the most recent tb code. Reporter Arno, I can also provide you with a patched version of latest tb 68 that you can try if you want (localized in US-EN). Let me know if you do and the architecture you want, Linux, OSX or Windows. Thanks.

Comment 12

[gene smith]

Arno, thanks for the email request for a try build. The try server build process is occurring here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a389e4279d05d89525ba7ef233ee802d37c2fcc4. When the build finishes you will see a green "B" next to a "linux64 opt" label you can click on. When clicked you will see down below "Job Details". In the list you will find target.tar.bz2. You can download this and extract it and inside you will find the executable "thunderbird" that you can run from command line from its directory. It will appear as "Daily" but the version is shown as 68.10.1
I accidentally did the build for all platforms but for your purposes the "linux64 opt" items should be OK. (Not sure what "shippable" means but if you prefer it is OK too.)
I haven't re-tested with icloud so let me know how it goes. Thanks!

Comment 13

[Arno]

Hello Gene,

I tested and imap idle on apple icloud server works fine. I'm realy looking forward to find this patch in the regular version of thunderbird.

Thank you very much for your work!

Comment 14

[Magnus Melin [:mkmelin]]

So ready to land?

Comment 15

[gene smith]

Attachment: Bug1611624_iCloud-capabilities-v2-rebased.patch

So ready to land?

I updated the patch so it merges OK with current comm-central tip (no functional change). The original patch didn't merge. So, yes, this patch is ready.
Edit: Maybe I should mention that the original patch merges OK as-is with esr-68.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9164413 [details] [diff] [review]
Bug1611624_iCloud-capabilities-v2-rebased.patch

Review of attachment 9164413 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7cdeeeff5f4d
Ensure all imap capabilities are known after authentication. r=mkmelin

Comment 18

[Magnus Melin [:mkmelin]]

Looks like this is causing failures for comm/mailnews/imap/test/unit/test_imapClientid.js

Comment 19

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/f1905fddc9b00ae540e5c76053f88c609d0430d2

Comment 20

[gene smith]

Attachment: Bug1611624_iCloud-capabilities-v3.patch

The "fakeserver" used in the CLIENTID test doesn't provide capabilities with the authenticate response like iCloud. So I modified the test to expect the 2nd capability request sent by TB after authentication. This has no effect on the actual purpose of the CLIENTID test and prevents the failure.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9165261 [details] [diff] [review]
Bug1611624_iCloud-capabilities-v3.patch

Review of attachment 9165261 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 22

[Magnus Melin [:mkmelin]]

Needs a clang-format. I'll do that before landing

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b088d3ada770
Ensure all imap capabilities are known after authentication (support IDLE for iCloud). r=mkmelin DONTBUILD

Comment 24

[gene smith]

It looks like test_imapAuthMethods.js (https://searchfox.org/comm-central/rev/9f453dd597f0b985b8fe7689b239e3b70de7749d/mailnews/imap/test/unit/test_imapAuthMethods.js) may have the same problem. But when I try to manually run the test it's skipped.

Comment 25

[Magnus Melin [:mkmelin]]

See https://searchfox.org/comm-central/rev/9f453dd597f0b985b8fe7689b239e3b70de7749d/mailnews/imap/test/unit/xpcshell.ini#23-25 and remove the skip-if, to try it

Comment 26

[gene smith]

(In reply to Magnus Melin [:mkmelin] from comment #25)

See https://searchfox.org/comm-central/rev/9f453dd597f0b985b8fe7689b239e3b70de7749d/mailnews/imap/test/unit/xpcshell.ini#23-25 and remove the skip-if, to try it

That test has big problems as-is without this iCloud patch. E.g., on the first test tb sends an unsolicited imap "list" that the test is not expecting. Even if that's fixed, other problems occur as described in bug 553764 (not just on Mac).

Comment 27

[Rob Lemley [:rjl]]

Comment on attachment 9165261 [details] [diff] [review]
Bug1611624_iCloud-capabilities-v3.patch

[Approval Request Comment]
Looks like this could affect other providers besides iCloud.

Comment 28

[Wayne Mery (:wsmwk)]

Thanks for adding the request.
Being a protocol change - Feels like this deserves significant time on beta, so not taking for 78.1.1

Comment 29

[Wayne Mery (:wsmwk)]

Comment on attachment 9165261 [details] [diff] [review]
Bug1611624_iCloud-capabilities-v3.patch

[Triage Comment]
Approved for esr78

Comment 30

[Rob Lemley [:rjl]]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/0edeceaec7d1