Open Bug 1532388 (clientid) Opened 8 months ago Updated 2 hours ago

Support for the 'CLIENTID' SMTP/IMAP command in Thunderbird

Categories

(MailNews Core :: Networking: IMAP, enhancement)

enhancement
Not set

Tracking

(Not tracked)

REOPENED

People

(Reporter: danielf, Assigned: danielf, NeedInfo)

References

Details

Attachments

(1 file, 44 obsolete files)

39.74 KB, patch
Details | Diff | Splinter Review

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

Steps to reproduce:

Background: With millions of bots actively engaged in brute force attempts, LinuxMagic has been spearheading adoption of CLIENTID for legacy protocols like IMAP and SMTP. Given the nature of attacks, simply rate limiting or blocking the attackers IP is no longer viable, given that thousands of devices may be behind a single public IP Address, such as in the case of a Carrier Grade Nat.

CLIENTID allows a device/software identifier to be presented before the authorization mechanism, this allows more granular control of which devices may present authentication credentials for a given account.

This is fully implemented on MagicMail servers and we are promoting it's use for all email platforms. We have filed RFC Drafts on the topic and produced patches for various open source platforms.

IMAP RFC Draft: https://tools.ietf.org/html/draft-yu-imap-client-id-01
SMTP RFC Draft: https://tools.ietf.org/html/draft-storey-smtp-client-id-05

LinuxMagic has a CCLA in place for the company.

We have attached the patch, and for the record this patched version is working in production environments.

For any questions, please feel to respond to this bug or contact us directly.

We would like to see this patch be reviewed and adopted.

Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core

Hi, thanks for attaching a patch here. We only take patches for current trunk, not TB 60. Can you please rebase the two patches and attach them as text/plain patch files. Then we need to find suitable reviewers, maybe myself, Gene Smith and BenC. At first glance, the patches are smallish and the changes appear to be straight forward.

Flags: needinfo?(danielf)

Magnus, any opinion on adopting CLIENTID?

Flags: needinfo?(mkmelin+mozilla)

Sounds interesting, sort of a poor man's oauth?
Can't find discussion or mention of this other than the proposed rfc. Never heard of anyone using "LinuxMagic" servers with tb but sure there's some.

If adopted by other servers, e.g., gmail, dovecot cyrus etc, would they accept the same string as is used in the patch, i.e.,
C: . clientid TB-UUID <toolkit.telemetry.cachedClientID>
S: . OK
?

While technically there is no prohibition in the DRAFT that the CLIENTID TYPE, in this case "TB-UUID" be used by multiple products, it is assumed that developers would use a CLIENTID TYPE that reflected the CLIENTID TOKEN presented reflects their implementation, eg others might use:

C: CLIENTID K9-UUID <K9 UUID Implementation/Key>
S: .OK

A commercial email client might choose to use..

C: CLIENTID MSO-PRODUCT-KEY XXXXX-XXXXX-XXXXX-XXXXX-XXXXX

Creating a semi-unique key name would help for someone looking at a history of CLIENTID's that accessed their account of course, rather than everyone choosing to use simply 'UUID' as the CLIENT TYPE.

Server implementations MIGHT perform some form of logic based on the CLIENTID TYPE, however the intent is that server implementations of IMAP accept any correctly formatted CLIENTID TYPE. It COULD be that in the future, a database of known CLIENTID TYPE's be registered, to ensure that different products don't step on each others usage.

PS, the brand of email servers that implements CLIENTID at the server level for POP/IMAP/SMTP is MagicMail heavily used throughout North America, although others (commercial and opensource) have indicated plans to support as well.

And maybe a 'poor mans' method, it simple to adopt, and extremely effective ;)
Even more important, requires no changes in people's existing email client settings.

Sounds pretty interesting.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #9048270 - Attachment is obsolete: true
Flags: needinfo?(danielf)

As per comments by Jorg K I have now rebased these patches against the latest trunk and uploaded as plaintext patch files, please let me know if there is anything else I can do to help move this patch forward.

Gene, could you please give these patches a workout.

Flags: needinfo?(gds)

I will probably need a temporary test IMAP account on a MagicMail imap server to actually test this. I don't think any other existing server supports CLIENTID capability yet.

Flags: needinfo?(gds)

(In reply to DevTeam from comment #4)

While technically there is no prohibition in the DRAFT that the CLIENTID TYPE,

That's true, but I think it is a flaw in the RFC.
The RFC first allows any string as client identity type, and then reserves the right to claim specific strings for special cases in the future.
In my opinion, it makes more sense to specify a fixed tag (eg UUID) first. This later avoids problems with incompatibilities.

in this case "TB-UUID" be used by multiple products, it is assumed that developers would use a CLIENTID TYPE that reflected the CLIENTID TOKEN presented reflects their implementation,

I think this is beyond the intention of the RFC.
But if it should be so, then the information should be better placed in the client identity token

Just my (1(1¢)

What also causes me stomachaches, is the fact that the server gets the opportunity to track the user across account boundaries.
That a server operator has an interest in it, I can understand. But I doubt that any user would readily agree with that.
I think we should at least make the function switchable. Best turned off by default.

I would find an account-specific ID better from a privacy point of view.

All comments welcome Alfred.. answering both in same comment:

  1. Using a TB-UUID vs just UUID, it allows the server to inform that the login attempt was from a Thunderbird client, for instance, since many clients may use UUID of the same format in the token. It also allows that since different clients MAY present UUID in different mannner, eg, one client presents it's UUID seperated by dashes, and another one doesnt' that if UUID token sanity checks are desired to be written, the TYPE can be a clue to the formatting used by that specific client.

  2. As a user, you WANT the server to validate that it is you trying to access your account with your AUTH credentials. This is not presenting information to anyone except the service that you want a trust relation with. This is NOT something that should be 'turned off', just like you can't turn off presenting your email address, IP Address or HELO; you want to empower your provider to do everything in it's power to validate it is YOU. And for the record, this is an account specific ID the way it is implemented. This isn't a privacy issue, this is a trust issue.

I agree it does sound better to have the uuid be per account, avoiding any privacy issue that may occur (say, connecting all your email accounts at that same provider to the same user)

Hello Magnus,

As pointed out in another comment the UUID being used is pulled from the account preferences file, which is unique per account.

I haven't yet tried the patch but I did post a request for some outside opinions on the imap mailing list (see thread that straddles March and April in links below with subject "CLIENTID capability for IMAP (and SMTP)"):

http://mailman13.u.washington.edu/pipermail/imap-protocol/2019-March/thread.html
http://mailman13.u.washington.edu/pipermail/imap-protocol/2019-April/thread.html

There are some comments pro and con and a link to a similar method used by "XMPP" (messaging protocol formally known as "Jabber").

I have links to the LM test account that I will try with the patch shortly.

Comment on attachment 9054526 [details] [diff] [review]
imap-patch-clientid-thunderbird-march-29-2019.diff

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

All comments responded to, CLIENTID server accounts provided to Gene, requesting review status
Assignee: nobody → danielf
Status: NEW → ASSIGNED
Comment on attachment 9054524 [details] [diff] [review]
smtp-patch-clientid-thunderbird-march-29-2019.diff

Gene, please let us know what you think.

Dan, thanks for the patches. can you fix a few white-space issues. We usually indent by two spaces, not four, but there is some old code that does four, in which case we follow that in the particular vicinity.

Also, we indent like this:
```
x = functionWithReallyLongNameToFillTheLine(xxx,
                                            yyy,
```
Attachment #9054524 - Flags: feedback?(gds)
Attachment #9054524 - Attachment is obsolete: true
Attachment #9054524 - Flags: feedback?(gds)
Attachment #9054526 - Attachment is obsolete: true
Attachment #9056641 - Attachment is obsolete: true

Hello Jorg,

Please excuse me if I am misunderstanding your request, I understood that the whitespace needs to be corrected but I may be misinterpreting the details of the necessary change.

I have adjusted the patches by:

  • Ensuring all indentation is 2 spaces where entirely new code (new functions) are added
  • Retained 4 space indentation in code that is surrounded by existing 4 space indented code
  • Shifted the second line of parameters for calls to nsExplainErrorDetails to line up with the column of the first parameters

You only marked the smtp patch as needing whitespace adjustments, however I noticed there was a few spots in the imap patch where 4 space indentation was being used (return statements inside ClientID function) and I have taken the liberty of correcting that indentation and re-uploading that patch as well.

Please review the updated patches and do not hesitate to let me know if there is any additional changes I can make.

Thanks in advance,

Daniel

Attachment #9056643 - Flags: review?(gds)
Attachment #9056643 - Flags: feedback?(gds)
Attachment #9056640 - Flags: review?(jorgk)
Attachment #9056640 - Flags: feedback?(jorgk)
Comment on attachment 9056640 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-8-2019.diff

Yes, this looks good now (apart from a few trailing spaces that crept is, but we can fix them in the next iteration).

I'll mark these patches f?Gene Smith since he's the one who will be testing them. If all good, I'll do the final review.
Attachment #9056640 - Flags: review?(jorgk)
Attachment #9056640 - Flags: feedback?(jorgk)
Attachment #9056640 - Flags: feedback?(gds)
Comment on attachment 9056643 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-8-2019.diff

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8817,5 @@
> +  {
> +    rv = ClientID();
> +    if (NS_FAILED(rv)) {
> +      // DF: Should we forcibly close the imap thread here?
> +      //    eg. TellThreadToDie()

Maybe Gene can try a few things here.

@@ +8818,5 @@
> +    rv = ClientID();
> +    if (NS_FAILED(rv)) {
> +      // DF: Should we forcibly close the imap thread here?
> +      //    eg. TellThreadToDie()
> +      // DF: Should we use the 'Log' function here?

I think so.
Attachment #9056643 - Flags: review?(gds)
Attachment #9056640 - Attachment is obsolete: true
Attachment #9056640 - Flags: feedback?(gds)

Hello Jorg,

Sorry about those trailing whitespaces I have fixed them quickly and re-uploaded the patch.

Regarding the second question in the comments about using the Log() function, there is already a MOZ_LOG call at that location -- should we perform an additional 'Log' call as well? And if so, should we log the same message as the MOZ_LOG call?

Applied the imap patch first and built, starting to test now...

Built ok but can't create the email account or login to webmail as described in the email from Michael Peddemors, President/CEO LinuxMagic Inc.

Will try some more.
Edit: Working now, thanks to help from Mr. Peddemors.

Confirmed a login (webmail) was successful. Additional emails sent privately to Gene to assist with his testing.

Not seeing a CLIENTID capability from server after login (SSL/TLS, Normal Password). Is this OK or do I need something else?

Hello Gene,

The CLIENTID command must be issued after STARTTLS/SSL is negotiated, but before the login/authentication has been performed.

The CLIENTID command will not be advertised both before SSL/TLS and after authentication.

Right now the server you are testing against is not configured to block authentications without first receiving a CLIENTID command, meaning your authentication attempts should always succeed regardless of whether you have presented a CLIENTID.

However it would be up to a server administrator to for example: lock down all authentication attempts to first require a successful clientid command, or to require an approved clientid type/key before authentication.

I hope this information has helped,

Daniel

Still confused. When is it advertised?

I logged in with openssl like this:

[gene@wally comm]$ openssl s_client -connect mail.cityemail.com:993 -crlf
CONNECTED(00000003)
depth=2 C = US, O = DigiCert Inc, OU = www.digicert.com, CN = DigiCert Global Root CA
verify return:1
depth=1 C = US, O = DigiCert Inc, OU = www.digicert.com, CN = Thawte RSA CA 2018
verify return:1
depth=0 C = CA, ST = British Columbia, L = Vancouver, O = Wizard IT Services, OU = IS, CN = *.cityemail.com
verify return:1
---
Certificate chain
 0 s:/C=CA/ST=British Columbia/L=Vancouver/O=Wizard IT Services/OU=IS/CN=*.cityemail.com
   i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=Thawte RSA CA 2018
:
:
   0090 - de 4c de c7 b6 e5 a4 4e-e1 f9 00 69 aa 66 46 ab   .L.....N...i.fF.

    Start Time: 1554765834
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: no
---
* OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE AUTH=PLAIN] MagicMail ready.
. CLIENTID TB-UUID 1234567890
. BAD Error in IMAP command received by server.
. login gene@cityemail.com ******************
. OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE SORT SORT=DISPLAY THREAD=REFERENCES THREAD=REFS THREAD=ORDEREDSUBJECT MULTIAPPEND URL-PARTIAL CATENATE UNSELECT CHILDREN NAMESPACE UIDPLUS LIST-EXTENDED I18NLEVEL=1 CONDSTORE QRESYNC ESEARCH ESORT SEARCHRES WITHIN CONTEXT=SEARCH LIST-STATUS SPECIAL-USE BINARY MOVE QUOTA] Logged in
. CLIENTID TB-UUID 12345
. BAD Error in IMAP command CLIENTID: Unknown command.

I tried to send CLIENTID before and after I logged in and get errors. Also, I never see CLIENTID in the capability strings returned by the server.

Sorry for wasting your time, issue was on our end..
Didn't check if support was enabled for that domain on that server when we gave you a production account.

openssl s_client -connect mail.cityemail.com:993 -crlf
CONNECTED(00000003)
depth=1 C = US, O = DigiCert Inc, OU = www.digicert.com, CN = Thawte RSA CA 2018
verify error:num=20:unable to get local issuer certificate
verify return:0

Certificate chain
0 s:/C=CA/ST=British Columbia/L=Vancouver/O=Wizard IT Services/OU=IS/CN=*.cityemail.com
i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=Thawte RSA CA 2018
1 s:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=Thawte RSA CA 2018
i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert Global Root CA

Server certificate
-----BEGIN CERTIFICATE-----
MIIGFjCCBP6gAwIBAgIQDYTCdd5vgUqeqMmchp3kHjANBgkqhkiG9w0BAQsFADBc
MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
d3cuZGlnaWNlcnQuY29tMRswGQYDVQQDExJUaGF3dGUgUlNBIENBIDIwMTgwHhcN
MTgxMDAzMDAwMDAwWhcNMTkxMjAyMTIwMDAwWjCBgDELMAkGA1UEBhMCQ0ExGTAX
BgNVBAgTEEJyaXRpc2ggQ29sdW1iaWExEjAQBgNVBAcTCVZhbmNvdXZlcjEbMBkG
A1UEChMSV2l6YXJkIElUIFNlcnZpY2VzMQswCQYDVQQLEwJJUzEYMBYGA1UEAwwP
Ki5jaXR5ZW1haWwuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA
1S8Zdh0vsgXmkMFRTMg2Usqn83faCs/10IsGwNLtRiRy/dOCYVn1wmgIiiaK6QiT
j3V7Qo2BR8+NpS8VCdB0vRy67V7HHamWk3tMzqJNFuA62y+a6t92A8TaKDD0Xj0s
1b8RtjqN7v/Ii5lsVJvyaI+h/lspHPqZHUvMQLODbFJN/QQQDdJmq3tLIF5m5SqN
8wQbovtQEIwajtwA2QZixpeweJ2W4UAX8djF91f4F68BcBg6H766eoWy8rvxxVwU
2KsYyQ1iSUxXyxDhbzfGFoa0R1lvgPA93BOrYRrxphGpYyLPBq1tvcF3iKsMLW4f
sobsYSEzDOryFIgYgR2l5wIDAQABo4ICrTCCAqkwHwYDVR0jBBgwFoAUo8heZVTl
MHjBBeoHCmpZzLn+3lowHQYDVR0OBBYEFKKSx5gaI/LmK7ozzX1jDbI4CcA+MCkG
A1UdEQQiMCCCDyouY2l0eWVtYWlsLmNvbYINY2l0eWVtYWlsLmNvbTAOBgNVHQ8B
Af8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMDoGA1UdHwQz
MDEwL6AtoCuGKWh0dHA6Ly9jZHAudGhhd3RlLmNvbS9UaGF3dGVSU0FDQTIwMTgu
Y3JsMEwGA1UdIARFMEMwNwYJYIZIAYb9bAEBMCowKAYIKwYBBQUHAgEWHGh0dHBz
Oi8vd3d3LmRpZ2ljZXJ0LmNvbS9DUFMwCAYGZ4EMAQICMG8GCCsGAQUFBwEBBGMw
YTAkBggrBgEFBQcwAYYYaHR0cDovL3N0YXR1cy50aGF3dGUuY29tMDkGCCsGAQUF
BzAChi1odHRwOi8vY2FjZXJ0cy50aGF3dGUuY29tL1RoYXd0ZVJTQUNBMjAxOC5j
cnQwCQYDVR0TBAIwADCCAQUGCisGAQQB1nkCBAIEgfYEgfMA8QB3ALvZ37wfinG1
k5Qjl6qSe0c4V5UKq1LoGpCWZDaOHtGFAAABZjvv1/sAAAQDAEgwRgIhAJ8gCH0i
rIfJ+WIi1Ghbmd/vlfjF9btzGGHwyD3XzBPGAiEA/YCT66FDqKooqzhp2NjZQbCn
GRaVcnboOkcrTTQS/mgAdgCHdb/nWXz4jEOZX73zbv9WjUdWNv9KtWDBtOr/XqCD
DwAAAWY779i1AAAEAwBHMEUCIQCW4cdcGCiI3Nv7nqA86NyNgKJWCwuAZsM/0zlu
0AYEowIgY+kfbpVoRw1ttzyikaXaKwTb2s7ZCg0uxXU7Zjrg/dgwDQYJKoZIhvcN
AQELBQADggEBAJ6Af3uxWlKj9C8naC6W29bCE1Hk48ucJG4I9LNcYe7fn1flweD9
aVFr4f5ppDG7aVUQJcTpiXb9iyW4gDpglLBnb3j9iBGKaqBfKIuV5RJJOh8LGS5T
yUs/e9DdIaQDxVT4m54VbxMk0Z2y+e/HQsgjtmBMfjzGUfolv8h0+qXSOi0ApfoR
sIQs3ZG4PG0lXTFsL1OvwoVQPvjncmbzuHdkogyESFbYNlvwjQA3C63/PjHjigEa
6pzBH07oCiqdLgRnw4KKOD9uPYHGzLV98mDcDkBD3Qj+Zrko8lDKaNZ7jkEcyrzM
SGQHE5a6YVr3F5eOPviz39AXwZXe1jgMvwc=
-----END CERTIFICATE-----
subject=/C=CA/ST=British Columbia/L=Vancouver/O=Wizard IT Services/OU=IS/CN=*.cityemail.com
issuer=/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=Thawte RSA CA 2018

No client certificate CA names sent

SSL handshake has read 3421 bytes and written 453 bytes

New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES128-GCM-SHA256
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
SSL-Session:
Protocol : TLSv1.2
Cipher : ECDHE-RSA-AES128-GCM-SHA256
Session-ID: E88A4395EE0665990EA2559399F6C5F54B8D060B764EA495C24756DC2191C1C2
Session-ID-ctx:
Master-Key: CB8A1D6E13D55EC8708026DB8F60E4D7A0E9354FA88C4944D7209EACA09CE6ED6B83B0CE79855233FA0683629C4CF2B3
Key-Arg : None
PSK identity: None
PSK identity hint: None
SRP username: None
TLS session ticket lifetime hint: 300 (seconds)
TLS session ticket:
0000 - 44 4d 16 1f 6f 37 43 13-6d 5e 1b 23 75 99 dd dc DM..o7C.m^.#u...
0010 - 8d de e5 f3 99 1d 48 da-dd 39 d0 a0 37 6b 13 da ......H..9..7k..
0020 - f1 ff 65 2a ee f1 ec 2f-59 64 fe d4 f3 04 40 2f ..e*.../Yd....@/
0030 - ef 6f 6d 58 2e db be f4-8b 66 01 1f 04 15 b6 6a .omX.....f.....j
0040 - bd 3e 2d 3f 8c 2d ef a1-2e de 2f 63 e9 27 d7 4d .>-?.-..../c.'.M
0050 - 21 86 87 11 29 24 9c 68-65 20 b2 b3 57 d9 67 b2 !...)$.he ..W.g.
0060 - 26 04 7c 3b a7 02 5d 1b-91 03 22 0b ec e9 0b 69 &.|;..]..."....i
0070 - 5c 10 14 7c b4 30 8b be-e7 5e cc 64 c7 ab ae b1 ..|.0...^.d....
0080 - 9a 4c d8 82 f7 27 3d fe-31 d6 15 a0 1c 93 31 ee .L...'=.1.....1.
0090 - 55 1c f8 1b 36 77 55 4a-f6 bb e0 2c a2 c7 bd f4 U...6wUJ...,....

Start Time: 1554770671
Timeout   : 300 (sec)
Verify return code: 20 (unable to get local issuer certificate)

  • OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE CLIENTID AUTH=PLAIN] MagicMail ready.

Yes, thank, now it's there, i.e., capability CLIENTID!! See it when I run tb or if I do openssl. I didn't see your comment before I noticed that it started working so thought maybe I did something to fix it accidentally.

However, I see another problem in the patch. Tb (actually, mozilla framework) doesn't allow prefs to be accessed except from the UI thread. The patch reads the ClientID string from the imap thread(s) so it MOZ_ASSERTs (crashes) at ServoUtils.h:34. I suspect that you built and tested without DEBUG defined since a release build leaves out MOZ_ASSERT() calls.

I think this can be fixed by making the local ClientID string a static variable, gClientId in nsImapProtocol.cpp and reading the pref and saving it one time in GlobalInitialization(). (The first imap thread where GlobalInitialization() is called is spawned by the UI so OK to access prefs there, where most are accessed) Then in new function ClientId() use the gClientId static var to complete the imap command string instead of reading the pref.

Thanks, can work on that in the morning. If you have any comments re:

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8817,5 @@

  • {
  • rv = ClientID();
  • if (NS_FAILED(rv)) {
  •  // DF: Should we forcibly close the imap thread here?
    
  •  //    eg. TellThreadToDie()
    

Maybe Gene can try a few things here.

@@ +8818,5 @@

  • rv = ClientID();
  • if (NS_FAILED(rv)) {
  •  // DF: Should we forcibly close the imap thread here?
    
  •  //    eg. TellThreadToDie()
    
  •  // DF: Should we use the 'Log' function here?
    

That was one area hard to get one's head around, which direction to go... Through an error manually, and tell the thread to die, or to return error conditions and let errors.. bubble through to native termination on error.

My uuid seems to be hardcoded to something about "c0ffee" like this:

toolkit.telemetry.cachedClientID  c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0 

Maybe not unique when you are running a personal/trunk/debug build? Looks truly random/unique on a release build.

Running OK without assert/crash with a static string read from prefs once in GlobalInitialization(). I see the CLIENTID TB-UUID command occurring (with ...c0ffee... string) after TLS connect but before authenticate.

This shows how I fixed nsImapProtocol.cpp so it uses a static string. I think this is OK but I'm no expert on mozilla strings. This also gets rid of the need for the added #include to handle the pref. The main item in here is static gClientId.

That was one area hard to get one's head around, which direction to go... Through an error manually, and tell the thread to die, or to return error conditions and let errors.. bubble through to native termination on error.

Before looking at this I probably need to understand what the IMAP clientid thing actually does or is supposed to do. From my observation with the cityemail.com account I don't see any effect. I first connected with the patched version of tb that sent the clientid (the "coffee" string in my case). I then connected with a release version and it worked OK even with no clientid sent. Then on the tb version having the patch that sends clientid, I modified the string sent so it was "coffee-gds" and that worked too, no complaints and CLIENTID command responded with OK.

So maybe the reason for the permissive behavior is because the server is set up permissive. Assuming that's what it is, and with a strict server, how would it behave with a bad or missing clientid command?

  1. Server remembers the 1st client ID string it receives for an account.
  2. Each time a client connects via tls it expects to see same client ID again.
    2.1 If server sees it, authentication occurs and all is OK for that mailbox
    2.2 If server sees a different client ID, what does it do? I would think it would fail the authentication? Or maybe it allows authentication but sends a email to one or more addresses warning that a different Id has been used for access.
    2.3 If it sees no client ID (CLIENTID not supported by client) does it still allow authentication? Or maybe also just sends an email warning(s)?

So if the ClientID() function fails and this results in no string sent or maybe the wrong string, it may not matter if this causes the server to fail authentication. Or it may not matter if it allows authentication but sends a warning email that someone accessed the account without a client ID.

I went back and re-read the draft imap clientid spec and it seem like it really doesn't specifically answer my questions. So I assume that the behavior would be server specific. Is or should there be a place where a user can configure the permissiveness or strictness of the server? I didn't see anything about clientid on your webmail.

(Note: I haven't yet applied the smtp patch or even looked at it yet.)

When I talk about the server sending warning emails I am referring to something like the message with this subject I received on the chile.com account:

Subject: [ALERT] New sign-in from unregistered device: Unknown

Not sure if this has anything to do with CLIENTID.

Hello Gene,

Lets see if I can answer some of your questions:

So maybe the reason for the permissive behavior is because the server is set up permissive.

Correct, the server you are testing on is configured to allow all authentications regardless of CLIENTID activity to allow for testing.

It would be up to a server administrator to configure their server to either: block authentications if the CLIENTID command is not presented, or block authentications if an approved CLIENTID type/id is not presented.

Assuming that's what it is, and with a strict server, how would it behave with a bad or missing clientid command?

If a user's account was locked down to only allow approved CLIENTID type/ids then you would expect:

  1. The CLIENTID command will succeed as long as syntax is correct and the CLIENTID type is allowed by the server settings.

  2. The AUTH command will fail if an approved CLIENTID type/id was not presented.

For example, I have locked down my testing account to only allow approved devices here:

01 CLIENTID TEST-UUID 465cf65c-5ae2-11e9-8702-5254000c130b
01 OK Recognized a valid CLIENTID command, used for authentication methods
01 LOGIN postmaster@linuxmagic.dev testing
01 NO [AUTHENTICATIONFAILED] Authentication failed.

Server remembers the 1st client ID string it receives for an account.

Not exactly, the server will remember all/any CLIENTIDs which occurred in any sessions with successful authentications, a failed authentication will not record the CLIENTID so as to prevent attack vectors.

For example one clientid may come from Thunderbird, and a separate clientid may come from the K9-Mail for Android.

So long as the account is not locked then any CLIENTID presented before a successful authentication will store that CLIENTID in the list of "devices" for that persons account.

Each CLIENTID is separate and can be separately approved in the users account preferences.

Once some chosen CLIENTIDs are approved the user may 'lock their mailbox' such that authentication will only succeed if one of the approved CLIENTIDs are first presented.

Each time a client connects via tls it expects to see same client ID again.

Yes, at least one of the approved CLIENTIDs.

2.1 If server sees it, authentication occurs and all is OK for that mailbox

Correct, if the server sees an approved CLIENTID then authentication will succeed (given the user has locked their mailbox).

2.2 If server sees a different client ID, what does it do? I would think it would fail the authentication? Or maybe it allows authentication but sends a email to one or more addresses warning that a different Id has been used for access.

Correct, if the account is locked and approved CLIENTID is not presented then the authentication will fail.

Delivery of a notification message is outside the scope of CLIENTID itself, but we do intend to tie notification messages into this system in MagicMail so that a user may be notified in the event of an unapproved device accessing their account, or an unknown device attempting to access their account.

2.3 If it sees no client ID (CLIENTID not supported by client) does it still allow authentication? Or maybe also just sends an email warning(s)?

Only if the account is not locked, if the account is locked then failing to provide a CLIENTID would mean authentication will fail.

Otherwise, if the account is not locked and the server administrator has not mandated that a CLIENTID command must always be presented then authentication will succeed without a CLIENTID command.

When I talk about the server sending warning emails I am referring to something like the message with this subject I received on the chile.com account:
Subject: [ALERT] New sign-in from unregistered device: Unknown
Not sure if this has anything to do with CLIENTID.

Yes, eventually this would be the end goal is to tie notifications into the system. The notifications on magicmail are still in the early testing phase. The intention is to tie CLIENTID and device identification into notification emails such that a user may turn on a setting: "Notify me if an unapproved device accesses my account" (given the account isn't locked) or "Notify me if an unapproved device tries to access my account" (whether the account is locked or not).

I hope I have been able to answer your questions, if you have any further questions please feel free to ask.

It would be up to a server administrator to configure their server to either: block authentications if the CLIENTID command is not presented, or block authentications if an approved CLIENTID type/id is not presented.

Apologies, the above sentence is slightly incorrect.

A server administrator may choose to mandate that a CLIENTID command MUST always be presented for all authentications on the server or specific domains on the server.

However, it is up to the owner of a mailbox (not the server administrator) to decide whether they want to lock down their mailbox to only allow approved devices to authenticate.

Dan, Thanks that really clarifies how it works!

Can the cityemail account be set so that clientid is mandatory so I can try it with different client id strings and do the "lock/unlock" of the mailbox. I assume the lock/unlock is done through the webmail?

Hello Gene,

We have manually approved two of three devices that were recorded in your device history.

We have approved the device:

TB-UUID c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0

And we have approved your webmail cookie so that you can still sign into the webmail.

Any attempts to authenticate to your account should now fail if you do not provide the above CLIENTID.

If you need anything else please feel free to let me know.

If you would like to setup an IRC channel or email thread so that we can maintain a faster back-and-forth correspondence for the duration of testing this patch we would be happy to oblige

Addendum: All of these comments are helpful Gene, as a planned update to the RFC Drafts to make it easier to understand various use cases seems in order. For the record, the server implementation MAY choose to respond to the issuance of a CLIENTID command, based on policies, but this is not required. Example use cases where a server MAY respond to a CLIENTID are as follows:

  • OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE CLIENTID AUTH=PLAIN]
    A CLIENTID TB-UUID cOOkies

The server MAY have reasons to respond with a A BAD for the following reasons

  • Server Side CLIENTID validation, illegal characters presented per RFC (in TYPE string or TOKEN string)
  • Server Side CLIENTID validation, CLIENTID token does not match CLIENTID type
  • Server Side CLIENTID validation, too many different tokens from an IP in a time window
  • Server Side CLIENTID validation, access temporarily deferred, system resource limits
  • Server Side CLIENTID validation, CLIENTID type not accepted by this server (eg operator restricts users to a specific client)
  • Server Side CLIENTID validation, CLIENTID type reported as malicious

However, frankly a mail server may choose not to do any tests or validation at the CLIENTID presentation, and simply take whatever is presented, and pass it on to internal system that will use that information, eg for recording a history of CLIENTID's used to access a person's resource, to check against permitted ACL's for a particular user/group, to compare against a history, and other aspects. But it is important to note, that their is no REQUIREMENT that the server itself do any CLIENTID validation at all in the RFC. (However, I would expect that any implementation would do some basic validation at that stage)

From that point on, errors or rules pertaining to things like authorization or permissions for individual accounts based on the earlier provided CLIENTID information won't usually be exposed later. It behooves the server to simply report back authentication failed, so as not to 'leak' information that might be helpful in attacks. All other reporting would be internal, eg alerts to system administrators of attacks, logging, alerting users to someone attempting to access their resources, and/or creating history records that can be used in the future by the user to 'lock' the account to only recognized entries in the history. Does that make it clearer than the RFC?

The RFC is very flexible, but yet allows for simplicity. Additional properties and systems that take advantage of the presented CLIENTID for personal or system security will be up to the server implementation, and the users personal preferences. Eg, the system implementation may CHOOSE to allow the user to have controls such as 'limit access' to 'only clients presenting a client id', 'only clients presenting a certain type of client id', or even 'only those historical client ids that I trust', however all that goes beyond this patch and it's behavior, and left for a system implementor to decide, invent, and implement based on the CLIENT ID information gained during the CLIENTID command.

Make sense?

We have manually approved two of three devices that were recorded in your device history.
We have approved the device:
TB-UUID c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0

What were the other 2? I also sent, I think:
TB-UUID c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0-gds

Don't remember a 3rd however at one point I think I may have just sent:
TB-UUID
with no clientid string.

And we have approved your webmail cookie so that you can still sign into the webmail.

I assume that at this time the lock/unlock feature is not available on webmail?

If you would like to setup an IRC channel or email thread so that we can maintain a faster back-and-forth correspondence for the duration of testing this patch we would be happy to oblige.

I will email you if questions are extreme but for now will try to keep relevant issue public on bugzilla. I'm not at a place I can do actual testing right now but will a bit later.

What were the other 2?

One was the original c0ffee uuid
One was the c0ffee-gds uuid
The third was passed via the webmail when you authenticated there.

I also sent, I think:
TB-UUID c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0-gds

We didn't approve this device, we can if you want.

Don't remember a 3rd

The webmail is implicitly providing a CLIENTID when it is authenticating over IMAP, the 3rd would be from the webmail and not really of any use to you. I probably didn't need to mention it :)

I assume that at this time the lock/unlock feature is not available on webmail?

Unfortunately no we have not exposed the feature to the webmail yet, too many people would shoot themself in the foot.

If you would like a more direct way to toggle the 'locked mailbox' configuration then let us know and we can figure something out.

I forced an error in the ClientID() function by sending only
. CLIENTID TB-UUID
without the UUID string. This causes the server to respond BAD . This
doesn't seem to cause any problems that I can see. Tb pops up an error
(containing the server's text from the BAD response) and no further
authentication is attempted AND the thread is killed after that by
existing code. So probably better not to kill immediately on ClientId()
failure.

I would also like to test what happens if the CLIENTID command times out
but haven't determined how to do that yet.

I would also like to test STARTTLS with imap but not sure which port to
use or if your server supports it. When I first connected to chile.com
account with tb it came up as STARTTLS on, I think, port 143 but it
never worked if I remember right until I tried TLS.

Also, I haven't applied or even looked at the smtp patch yet. So I need
to do that.

Client ID Works fine with STARTTLS on imap just like TLS. In tb, just selected STARTTLS and plain password. Switched to port 143 clicked OK to to go. (My original problem with starttls must have been some other issue.)

The smtp patch also seems to work fine. I still need to do a bit more testing with it. The smtp account defaults to starttls so I assume it will also work with tls, which I will try later.

Attached file smtp-clientid-suggested-changes.diff (obsolete) —

Didn't work with TLS. It tried to login without first sending CLIENTID and doesn't send. The attached diff has a possible fix for it (see "new gds") that works for me. I also adjusted the function that parses the clientid response success and error codes to maybe reduce some code duplications (also marked with "new gds"). The diff only includes the files I tweaked for smtp.

Also, I suggested some changes to the error strings. (I simulated the clientid errors and checked that the strings occur.) Didn't tweak the "suite" versions of the strings.

I have a few more question and comments that I will do tomorrow, nothing major.

I notice that the draft smtp clientid (or cid) rfc doesn't seem to mention using just tls but only talks about starttls. Your smtp server works OK now with pure TLS on port 465 with my proposed fix in the code and should still work with STARTTLS on port 587.

Hello Gene,

Thanks for the suggestions, I like your version of the switch case much better and I will incorporate that into the next patch I upload.

But I do have one question regarding the change you suggested for detecting SSL:

        // If have "clientid" in elho response, tls must be present     // new gds
        if (m_prefSocketType == nsMsgSocketType::SSL)                   // new gds
          m_tlsEnabled = true;                                          // new gds

First off thanks for finding this issue, I understand that if the server responds with CLIENTID that SSL should therefore be established (as per rfc).

Please correct me if I am mistaken here, the member m_prefSocketType refers to the SSL configuration chosen in the preferences, and not whether SSL has actually been established, right?

If so then I am wondering: could it be dangerous to assume that SSL is enabled just because CLIENTID was present in the capability response?

For example: A misconfigured server which is advertising CLIENTID at the wrong time could lead to Thunderbird revealing it's TB-UUID over a non-secure connection.

(In reply to Dan from comment #50)

Hello Gene,

Thanks for the suggestions, I like your version of the switch case much better and I will incorporate that into the next patch I upload.

But I do have one question regarding the change you suggested for detecting SSL:

        // If have "clientid" in elho response, tls must be present     // new gds
        if (m_prefSocketType == nsMsgSocketType::SSL)                   // new gds
          m_tlsEnabled = true;                                          // new gds

First off thanks for finding this issue, I understand that if the server responds with CLIENTID that SSL should therefore be established (as per rfc).

Please correct me if I am mistaken here, the member m_prefSocketType refers to the SSL configuration chosen in the preferences, and not whether SSL has actually been established, right?

You may be right, not sure. I didn't even mean to check the m_prefSocketType but just set m_tlsEnable so that might be worse. Maybe there is a better place to know definitely when pure SSL/TLS is established but it was late and I couldn't find it, so just tried this and it "worked".

If so then I am wondering: could it be dangerous to assume that SSL is enabled just because CLIENTID was present in the capability response?

For example: A misconfigured server which is advertising CLIENTID at the wrong time could lead to Thunderbird revealing it's TB-UUID over a non-secure connection.

If the tb user selects SSL/TLS, is it possible for an insecure connection to even occur regardless of how the server is configured?

Hi Gene, for the later comment, while it may NOT be possible currently to happen, it is safer to ensure in the code that could never happen. It is better to check what the condition IS rather than what it should be ;)

Let Dan work on finding the 'better' identifier than m_prefSocketType, but if anyone wants to weigh in, comments welcome.

PS, what does this mean? "This bug is not currently tracked. " Is there a flag we missed on this bug?

Dan, suggest it is time for a final patch, merging all the suggestions..

I've never seen "Tracked" used on any bug I've worked on. I think it may have something to do with making sure "milestones" are included in planned releases. But Wayne, Jorg of Magnus (cc'd on this bug) can answer authoritatively.

Sure I agree, Dan should find the optimum place to know when the pure TLS connection is definitely established, not just a place that implies it is probably established.

Hello Gene,

I believe after some thorough review of the code that the variable m_prefSocketType is our best option for determining the SSL connection status.

Just curious, you mentioned that we cannot call the Preferences from the ClientID function in the IMAP code, we are doing the same thing in the SMTP patch here:

+  if (!m_clientIDInitialized && m_tlsEnabled)
+  {
+    if (TestFlag(SMTP_EHLO_CLIENTID_ENABLED))
+    {
+      buffer = "CLIENTID TB-UUID ";
+      nsAutoCString clientID;
+      nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID);
+      if (NS_FAILED(rv) || clientID.IsEmpty()) {
+        // should we log or call ExplainErrorDetails here?
+        return NS_ERROR_FAILURE;
+      }
+      buffer += clientID;
+      buffer += CRLF;
+
+      status = SendData(buffer.get());

Should we be using a global ClientID string variable in the SMTP code as well? Or is it acceptable to make a call to Preferences::GetCString here?

Dan, It seems to be OK to call it where you have it since I think the smtp code is called directly from the UI thread. I have run your patch with DEBUG enabled and don't see the assert crash that I saw in the imap threads. I don't think there is an smtp thread that sticks around like the imap threads do so even if you read and stored in the constructor or initialization code for smtp it would still be called from the smtp code and not be any different really than just reading it each time we send an email. Also, I see one other read of a preference in the smtp code.

Attachment #9056695 - Attachment is obsolete: true
Attachment #9056643 - Attachment is obsolete: true
Attachment #9056643 - Flags: feedback?(gds)

(In reply to gene smith from comment #55)

Dan, It seems to be OK to call it where you have it since I think the smtp code is called directly from the UI thread. I have run your patch with DEBUG enabled and don't see the assert crash that I saw in the imap threads. I don't think there is an smtp thread that sticks around like the imap threads do so even if you read and stored in the constructor or initialization code for smtp it would still be called from the smtp code and not be any different really than just reading it each time we send an email. Also, I see one other read of a preference in the smtp code.

Ah wonderful, I was thinking that too but wanted to get your input on the matter to be safe.

I just finished merging all the changes and doing one last compile, I have uploaded the new patches for the next round of review.

If there is anything else I can do please let me know.

Comment on attachment 9058393 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-15-2019.diff

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

Looks good!

::: mailnews/imap/src/nsImapCore.h
@@ +142,4 @@
>  const eIMAPCapabilityFlag kHasSpecialUseCapability =      0x200000000LL;  /* RFC 6154: Sent, Draft etc. folders */
>  const eIMAPCapabilityFlag kGmailImapCapability =          0x400000000LL;  /* X-GM-EXT-1 capability extension for gmail */
>  const eIMAPCapabilityFlag kHasXOAuth2Capability =         0x800000000LL;  /* AUTH XOAUTH2 extension */
> +const eIMAPCapabilityFlag kHasClientIDCapability =        0x1000000000LL; /* ClientID capability */

Maybe the LL should line up with the ones above and the comment to the right also line up with the ones above?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8811,5 @@
> +          m_connectionType.EqualsLiteral("ssl")))
> +  {
> +    rv = ClientID();
> +    if (NS_FAILED(rv)) {
> +      // DF: Should we forcibly close the imap thread here?

I didn't see any problem with this as-is with my testing.

@@ +8814,5 @@
> +    if (NS_FAILED(rv)) {
> +      // DF: Should we forcibly close the imap thread here?
> +      //    eg. TellThreadToDie()
> +      // DF: Should we use the 'Log' function here?
> +      MOZ_LOG(IMAP, LogLevel::Error, ("Could not issue CLIENTID command"));

MOZ_LOG is correct here, I think. On reason is, Log() uses LogLevel::Info. However, the convention is to include the function name as prefix to the string, i.e.,
MOZ_LOG(IMAP, LogLevel::Error, ("TryToLogon: Could not issue CLIENTID command"));
Attachment #9058393 - Flags: review+
Attachment #9058393 - Flags: feedback+
Comment on attachment 9058392 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-15-2019.diff

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

Looks good too!

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +74,5 @@
>  ## LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response
>  smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded:  %s.
>  
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s

Possible alternative wording that I suggested:
smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s

@@ +77,5 @@
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s
> +
> +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
> +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s

Possible alternative:
smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail: Server responded: %s

Comment: With imap, the idea is for CLIENTID problems to just preclude authenication and not call attention to itself. Would the same apply to SMTP? These possible error string seem to call attention to CLIENTID.

::: mailnews/compose/src/nsComposeStrings.h
@@ +66,5 @@
>  
>  #define NS_ERROR_ILLEGAL_LOCALPART                  NS_MSG_GENERATE_FAILURE(12601)
>  
> +#define NS_ERROR_CLIENTID                           NS_MSG_GENERATE_FAILURE(12610)
> +#define NS_ERROR_CLIENTID_PERMISSION                NS_MSG_GENERATE_FAILURE(12611)

Had to lookup (again) how these evaluate. It appears they produce error codes 0x80553142 and 0x80553143. 
Is the gap from the last value (12601 or 0x3139) needed?

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1031,5 @@
> +{
> +  bool flagDefaultError = true;
> +  switch (m_responseCode / 100)
> +  {
> +  case 2: // 2xx code (250) -- success!

Can this be other than 250 and be a success?

@@ +1064,5 @@
> +      flagDefaultError = false;
> +      break;
> +    default:
> +      // Other 5xx CLIENTID error response - use default (this should never happen)
> +      MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,

Maybe include function name:
MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
          ("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get()));

@@ +1073,5 @@
> +  default: // CLIENTID error response something other than 2xx, 4xx or 5xx
> +    // unknwon smtp error - use default (this should never happen)
> +    m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT;
> +    MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
> +            ("Unexpected error occurred, server responded: %s\n", m_responseText.get()));

Add function name:
MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get()));

@@ +1250,5 @@
> +      buffer = "CLIENTID TB-UUID ";
> +      nsAutoCString clientID;
> +      nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID);
> +      if (NS_FAILED(rv) || clientID.IsEmpty()) {
> +        // should we log or call ExplainErrorDetails here?

Didn't notice this comment when testing before. I've never seen a pref fetch fail but who knows. Guess it couldn't hurt to add a MOZ_LOG here.

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +76,5 @@
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s
> +
> +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
> +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s

Same comments apply here as ::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
Attachment #9058392 - Flags: review+
Attachment #9058392 - Flags: feedback+

(In reply to gene smith from comment #59)

Comment on attachment 9058393 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-15-2019.diff

Review of attachment 9058393 [details] [diff] [review]:

Looks good!

::: mailnews/imap/src/nsImapCore.h
@@ +142,4 @@

const eIMAPCapabilityFlag kHasSpecialUseCapability = 0x200000000LL; /* RFC 6154: Sent, Draft etc. folders /
const eIMAPCapabilityFlag kGmailImapCapability = 0x400000000LL; /
X-GM-EXT-1 capability extension for gmail /
const eIMAPCapabilityFlag kHasXOAuth2Capability = 0x800000000LL; /
AUTH XOAUTH2 extension /
+const eIMAPCapabilityFlag kHasClientIDCapability = 0x1000000000LL; /
ClientID capability */

Maybe the LL should line up with the ones above and the comment to the right
also line up with the ones above?

Done.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8811,5 @@

  •      m_connectionType.EqualsLiteral("ssl")))
    
  • {
  • rv = ClientID();
  • if (NS_FAILED(rv)) {
  •  // DF: Should we forcibly close the imap thread here?
    

I didn't see any problem with this as-is with my testing.

Removed comment.

@@ +8814,5 @@

  • if (NS_FAILED(rv)) {
  •  // DF: Should we forcibly close the imap thread here?
    
  •  //    eg. TellThreadToDie()
    
  •  // DF: Should we use the 'Log' function here?
    
  •  MOZ_LOG(IMAP, LogLevel::Error, ("Could not issue CLIENTID command"));
    

MOZ_LOG is correct here, I think. On reason is, Log() uses LogLevel::Info.
However, the convention is to include the function name as prefix to the
string, i.e.,
MOZ_LOG(IMAP, LogLevel::Error, ("TryToLogon: Could not issue CLIENTID
command"));

Removed comment, added function name.

(In reply to gene smith from comment #60)

Comment on attachment 9058392 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-15-2019.diff

Review of attachment 9058392 [details] [diff] [review]:

Looks good too!

:::
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +74,5 @@

LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response

smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded: %s.

+## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
+smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s

Possible alternative wording that I suggested:
smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID
command. The message was not sent: Server responded: %s

No objection here, updated the wording.

@@ +77,5 @@

+## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
+smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s
+
+## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
+smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s

Possible alternative:
smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID
command indicates that your device is not permitted to send mail: Server
responded: %s

Looks good, updated the wording.

Comment: With imap, the idea is for CLIENTID problems to just preclude
authenication and not call attention to itself. Would the same apply to
SMTP? These possible error string seem to call attention to CLIENTID.

::: mailnews/compose/src/nsComposeStrings.h
@@ +66,5 @@

#define NS_ERROR_ILLEGAL_LOCALPART NS_MSG_GENERATE_FAILURE(12601)

+#define NS_ERROR_CLIENTID NS_MSG_GENERATE_FAILURE(12610)
+#define NS_ERROR_CLIENTID_PERMISSION NS_MSG_GENERATE_FAILURE(12611)

Had to lookup (again) how these evaluate. It appears they produce error
codes 0x80553142 and 0x80553143.
Is the gap from the last value (12601 or 0x3139) needed?

So the gap isn't necessary, but I observed that the other error codes seem to have gaps separating them from other non-related error codes:

#define NS_MSG_ERROR_READING_FILE                   NS_MSG_GENERATE_FAILURE(12563)

#define NS_MSG_ERROR_ATTACHING_FILE                 NS_MSG_GENERATE_FAILURE(12570)

#define NS_ERROR_SMTP_GREETING                      NS_MSG_GENERATE_FAILURE(12572)

#define NS_ERROR_SENDING_RCPT_COMMAND               NS_MSG_GENERATE_FAILURE(12575)

#define NS_ERROR_STARTTLS_FAILED_EHLO_STARTTLS      NS_MSG_GENERATE_FAILURE(12582)

...

#define NS_ERROR_SMTP_AUTH_MECH_NOT_SUPPORTED       NS_MSG_GENERATE_FAILURE(12599)

#define NS_ERROR_ILLEGAL_LOCALPART                  NS_MSG_GENERATE_FAILURE(12601)

I assume that this must be to leave room for other error codes?

And/or to create distinct separators between groups of related error codes?

So I actually introduced the gap on purpose as a means of "leaving room for other related error codes" but I'd be happy to remove the gap if you like.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1031,5 @@

+{

  • bool flagDefaultError = true;
  • switch (m_responseCode / 100)
  • {
  • case 2: // 2xx code (250) -- success!

Can this be other than 250 and be a success?

None that I am aware of.

@@ +1064,5 @@

  •  flagDefaultError = false;
    
  •  break;
    
  • default:
  •  // Other 5xx CLIENTID error response - use default (this should never happen)
    
  •  MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
    

Maybe include function name:
MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
("SendClientIDResponse: Unexpected error occurred, server
responded: %s\n", m_responseText.get()));

Added function name.

@@ +1073,5 @@

  • default: // CLIENTID error response something other than 2xx, 4xx or 5xx
  • // unknwon smtp error - use default (this should never happen)
  • m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT;
  • MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
  •        ("Unexpected error occurred, server responded: %s\n", m_responseText.get()));
    

Add function name:
MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
("SendClientIDResponse: Unexpected error occurred, server responded: %s\n",
m_responseText.get()));

Done.

@@ +1250,5 @@

  •  buffer = "CLIENTID TB-UUID ";
    
  •  nsAutoCString clientID;
    
  •  nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID);
    
  •  if (NS_FAILED(rv) || clientID.IsEmpty()) {
    
  •    // should we log or call ExplainErrorDetails here?
    

Didn't notice this comment when testing before. I've never seen a pref fetch
fail but who knows. Guess it couldn't hurt to add a MOZ_LOG here.

Added this call:

    MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error,
            ("ProcessAuth: failed to fetch clientid from preferences"));

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +76,5 @@

+## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
+smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s
+
+## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
+smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s

Same comments apply here as :::
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties

Updated these too.

So just need your comments on that gap in the constants Gene, if it should be removed then I have no objection.

I have combined all these updates into some new patches but I will hold off on uploading them till I hear back.

Attachment #9057803 - Attachment is obsolete: true
Attachment #9056794 - Attachment is obsolete: true
Attachment #9058392 - Flags: review+ → review?(jorgk)
Attachment #9058393 - Flags: review+ → review?(jorgk)

Dan, No problem with gap. Just curious as to the rationale.

Regarding the 250 code as success, should we explicitly check for the number 250 instead of just code/100==2 to indicate success? Looking at the various 2xx codes, they probably wouldn't happen anyhow with a CLIENTID command it would seem, except 250 of course. Guess that would depend on the smtp server implementation.

Did you flag Jorg to review your last patches? He's the official reviewer on these kind of things so before you submit more changes we should get his inputs. I went ahead and set a review flag on the patches so Jorg will see them.

Attachment #9058392 - Attachment is obsolete: true
Attachment #9058392 - Flags: review?(jorgk)
Attachment #9058733 - Flags: review?(jorgk)
Attachment #9058733 - Attachment is obsolete: true
Attachment #9058733 - Flags: review?(jorgk)
Attachment #9058735 - Flags: review?(jorgk)
Attachment #9058393 - Attachment is obsolete: true
Attachment #9058393 - Flags: review?(jorgk)
Attachment #9058736 - Flags: review?(jorgk)

(In reply to gene smith from comment #62)

Dan, No problem with gap. Just curious as to the rationale.

Ah okay I have left it then.

Regarding the 250 code as success, should we explicitly check for the number 250 instead of just code/100==2 to indicate success? Looking at the various 2xx codes, they probably wouldn't happen anyhow with a CLIENTID command it would seem, except 250 of course. Guess that would depend on the smtp server implementation.

Good idea, I have changed the code to this:

  case 2: // 2xx code (250) -- success!
    if (m_responseCode == 250)
    {
        m_clientIDInitialized = true;
        ClearFlag(SMTP_EHLO_CLIENTID_ENABLED);
        m_nextState = SMTP_AUTH_PROCESS_STATE;
        return NS_OK;
    }
    // If not 250 then use default error code
    // because that should never happen
    break;

Did you flag Jorg to review your last patches? He's the official reviewer on these kind of things so before you submit more changes we should get his inputs. I went ahead and set a review flag on the patches so Jorg will see them.

Ah I am a noob to this bugzilla workflow, I think I have correctly flagged Jorg for review on the latest patches I uploaded.

Please let me know if there is anything else I am missing.

(In reply to Dan from comment #66)

(In reply to gene smith from comment #62)

Regarding the 250 code as success, should we explicitly check for the number 250 instead of just code/100==2 to indicate success? Looking at the various 2xx codes, they probably wouldn't happen anyhow with a CLIENTID command it would seem, except 250 of course. Guess that would depend on the smtp server implementation.

Good idea, I have changed the code to this:

  case 2: // 2xx code (250) -- success!
    if (m_responseCode == 250)
    {
        m_clientIDInitialized = true;
        ClearFlag(SMTP_EHLO_CLIENTID_ENABLED);
        m_nextState = SMTP_AUTH_PROCESS_STATE;
        return NS_OK;
    }
    // If not 250 then use default error code
    // because that should never happen
    break;

Now I am second guessing myself... Would a non-250 be still be considered a success?

In which case the behaviour would be the same for all 2xx codes wouldn't it?

https://www.greenend.org.uk/rjk/tech/smtpreplies.html

Not sure if this is a definitive list (google: smtp response codes). But if any other than 250 happened in response to clientid it would be weird. However, not sure it would warrant a pop-up message. So whatever you think is fine with me.

(In reply to gene smith from comment #68)

https://www.greenend.org.uk/rjk/tech/smtpreplies.html

Not sure if this is a definitive list (google: smtp response codes). But if any other than 250 happened in response to clientid it would be weird. However, not sure it would warrant a pop-up message. So whatever you think is fine with me.

I think even if the response code is not 250, as long as it is a 2xx that indicates a successful command.

So I'm thinking we might want to revert that change and just treat all 2xx codes as success.

Comments are welcome.

Attachment #9058735 - Attachment is obsolete: true
Attachment #9058735 - Flags: review?(jorgk)
Attachment #9058764 - Flags: review?(jorgk)

(In reply to Dan from comment #69)

(In reply to gene smith from comment #68)

https://www.greenend.org.uk/rjk/tech/smtpreplies.html

Not sure if this is a definitive list (google: smtp response codes). But if any other than 250 happened in response to clientid it would be weird. However, not sure it would warrant a pop-up message. So whatever you think is fine with me.

I think even if the response code is not 250, as long as it is a 2xx that indicates a successful command.

So I'm thinking we might want to revert that change and just treat all 2xx codes as success.

Comments are welcome.

At this point I am unsure of any real benefits of differentiating the 2xx codes, beyond possibly debugging. So I have adjusted the code to use the old check for any 2xx code, if anybody has any objections or wishes to see checks for alternative success codes then I would be happy to adjust the patch to reflect that.

Comment on attachment 9058764 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-16-2019.diff

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

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +74,5 @@
>  ## LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response
>  smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded:  %s.
>  
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s

The message was not sent. The server responded: %s

@@ +77,5 @@
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s
> +
> +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
> +smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail: Server responded: %s

same here: the and period

I was curious and looked closer at the original smtp code. At a couple places in the original code it checks for 250 exactly, e.g.,:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/src/nsSmtpProtocol.cpp#704
But 2xx may have been forced to 250 here, not sure. See "fake to 250" string in the code.

Also, it seems to accept 250 to 259 at 3 places like this:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/src/nsSmtpProtocol.cpp#1710
This treats 200-249 and 260-299 as an error.

Considering this, I took another look at the SendClientIDResponse() and made another stab at restructuring it using the above concepts. The main change is that it eliminates the use the the switch constructs and eliminates a bit more code duplication. The MOZ_LOG for the unexpected errors is completely skipped unless SMTP logging is enabled. This also only allows 250-259 to indicate success (so 200-249,260-299 will fail and be in the "unexpected" category).
See attached file.

Attachment #9058852 - Flags: feedback?(danielf)
Attachment #9058852 - Attachment mime type: text/x-c++src → text/plain

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

Comment on attachment 9058764 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-16-2019.diff

Review of attachment 9058764 [details] [diff] [review]:

:::
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +74,5 @@

LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response

smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded: %s.

+## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
+smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s

The message was not sent. The server responded: %s

@@ +77,5 @@

+## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
+smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s
+
+## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
+smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail: Server responded: %s

same here: the and period

Done.

I will include these changes in the next attached patch, immediately after I get feedback from Gene on this next part.

(In reply to gene smith from comment #73)

Created attachment 9058852 [details]
SendClientIDResponse.cpp (proposed change to one function)

I was curious and looked closer at the original smtp code. At a couple places in the original code it checks for 250 exactly, e.g.,:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/src/nsSmtpProtocol.cpp#704
But 2xx may have been forced to 250 here, not sure. See "fake to 250" string in the code.

Also, it seems to accept 250 to 259 at 3 places like this:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/src/nsSmtpProtocol.cpp#1710
This treats 200-249 and 260-299 as an error.

Considering this, I took another look at the SendClientIDResponse() and made another stab at restructuring it using the above concepts. The main change is that it eliminates the use the the switch constructs and eliminates a bit more code duplication. The MOZ_LOG for the unexpected errors is completely skipped unless SMTP logging is enabled. This also only allows 250-259 to indicate success (so 200-249,260-299 will fail and be in the "unexpected" category).
See attached file.

That looks a lot better than what I wrote, there is a small enhancement that I see though. The nresult rv isn't necessary if you return immediately where you assign rv = NS_OK, that also eliminates the need for the else statement. See attached file and let me know if that looks any better.

Attachment #9059014 - Flags: review?(gds)
Attachment #9059014 - Attachment mime type: text/x-c++src → text/plain
Attachment #9058852 - Flags: feedback?(danielf)
Comment on attachment 9059014 [details]
SendClientIDResponse.cpp (proposed change to proposed change)

A project I worked on in the past had a coding rule that a function only has one return point. However, mozilla/tb doesn't seem to have that rule and in the past Jorg and Magnus have told me to change my code to more or less exactly like you did: multiple returns to eliminate the else. So looks fine to me!

To test the code I had to just set the value of m_responseCode at the beginning of the function. Dan, do you know if there is a way to have the server actually return real errors, e.g., 501, 550, 4xx, 2xx etc?
Attachment #9059014 - Flags: review?(gds) → review+

To test the code I had to just set the value of m_responseCode at the beginning of the function. Dan, do you know if there is a way to have the server actually return real errors, e.g., 501, 550, 4xx, 2xx etc?

That is a great idea for the long term and we will need to investigate setting up a service to allow testing future clientside implementations. However, in the short term would it be possible for you to get away with simulating the response codes?

Uploading the modified smtp patch with the new SendClientIDResponse() function and the modified SMTP error strings so that it can be reviewed.

Attachment #9058764 - Attachment is obsolete: true
Attachment #9058764 - Flags: review?(jorgk)
Attachment #9059052 - Flags: review?(mkmelin+mozilla)
Attachment #9059052 - Flags: review?(jorgk)
Attachment #9059052 - Flags: review?(gds)
Attachment #9059014 - Attachment is obsolete: true

(In reply to Dan from comment #77)

To test the code I had to just set the value of m_responseCode at the beginning of the function. Dan, do you know if there is a way to have the server actually return real errors, e.g., 501, 550, 4xx, 2xx etc?

That is a great idea for the long term and we will need to investigate setting up a service to allow testing future clientside implementations. However, in the short term would it be possible for you to get away with simulating the response codes?

I already tested the changed function (my version, not you current patch version) using simulated values. But even when I faked the response code, the error printed: "Server responded: OK". Anyhow, I will hold off on more testing until the official reviewers (Jorg and possibly Magnus) weigh in.

Also, I wonder if they may want this as a single combined smtp and imap patch rather than two separate patches?

Attachment #9058852 - Attachment is obsolete: true
Comment on attachment 9059052 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-17-2019.diff

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

(I'll let others review this)
Attachment #9059052 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9059052 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-17-2019.diff

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

Looks OK to me.
Attachment #9059052 - Flags: review?(gds) → review+
Comment on attachment 9059052 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-17-2019.diff

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

Looks OK to me. I mostly rely on Gene's judgement here. One nit and one question.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +915,5 @@
>          }
> +        else if (responseLine.LowerCaseEqualsLiteral("clientid"))
> +        {
> +            SetFlag(SMTP_EHLO_CLIENTID_ENABLED);
> +            // If have "clientid" in ehlo response, TLS must be present

Nit: English grammar derailed here, maybe add "we". Full stop at the end.

@@ +1227,5 @@
> +    if (TestFlag(SMTP_EHLO_CLIENTID_ENABLED))
> +    {
> +      buffer = "CLIENTID TB-UUID ";
> +      nsAutoCString clientID;
> +      nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID);

Sorry, I haven't read all the details here, can you explain this. Looking at my running instance, I see: bacaf80c-d65c-4ed8-9e29-4a7f6ff99063
Attachment #9059052 - Flags: review?(jorgk) → review+
Comment on attachment 9058736 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-16-2019.diff

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

This also looks OK. How about Gene put his seal of approval onto this one as well.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8804,5 @@
>  
> +  // do we have clientid support? And is tls/ssl running?
> +  // we are basing this check off whether m_connectionType
> +  // contains the strings "starttls" or "ssl" which indicate
> +  // that the connection is using SSL/TLS in some form

That comment block is ugly. We follow English grammar and use full sentences (when possible), so starting with an uppercase latter and ending with a full stop.
Attachment #9058736 - Flags: review?(gds)

(In reply to Jorg K (GMT+2) from comment #82)

Comment on attachment 9059052 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-17-2019.diff

Review of attachment 9059052 [details] [diff] [review]:

Looks OK to me. I mostly rely on Gene's judgement here. One nit and one
question.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +915,5 @@

     }
  •    else if (responseLine.LowerCaseEqualsLiteral("clientid"))
    
  •    {
    
  •        SetFlag(SMTP_EHLO_CLIENTID_ENABLED);
    
  •        // If have "clientid" in ehlo response, TLS must be present
    

Nit: English grammar derailed here, maybe add "we". Full stop at the end.

@@ +1227,5 @@

  • if (TestFlag(SMTP_EHLO_CLIENTID_ENABLED))
  • {
  •  buffer = "CLIENTID TB-UUID ";
    
  •  nsAutoCString clientID;
    
  •  nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID);
    

Sorry, I haven't read all the details here, can you explain this. Looking at
my running instance, I see: bacaf80c-d65c-4ed8-9e29-4a7f6ff99063

That string is what is presented as the CLIENTID token. Coincidence that Thunderbird already called it ClientID.

Attachment #9059052 - Attachment is obsolete: true
Attachment #9058736 - Attachment is obsolete: true
Attachment #9058736 - Flags: review?(jorgk)
Attachment #9058736 - Flags: review?(gds)

(In reply to Jorg K (GMT+2) from comment #82)

Comment on attachment 9059052 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-17-2019.diff

Review of attachment 9059052 [details] [diff] [review]:

Looks OK to me. I mostly rely on Gene's judgement here. One nit and one
question.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +915,5 @@

     }
  •    else if (responseLine.LowerCaseEqualsLiteral("clientid"))
    
  •    {
    
  •        SetFlag(SMTP_EHLO_CLIENTID_ENABLED);
    
  •        // If have "clientid" in ehlo response, TLS must be present
    

Nit: English grammar derailed here, maybe add "we". Full stop at the end.

Changed it to say:

// If we have "clientid" in the ehlo response, then TLS must be present.

(In reply to Jorg K (GMT+2) from comment #83)

Comment on attachment 9058736 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-16-2019.diff

Review of attachment 9058736 [details] [diff] [review]:

This also looks OK. How about Gene put his seal of approval onto this one as
well.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8804,5 @@

  • // do we have clientid support? And is tls/ssl running?
  • // we are basing this check off whether m_connectionType
  • // contains the strings "starttls" or "ssl" which indicate
  • // that the connection is using SSL/TLS in some form

That comment block is ugly. We follow English grammar and use full sentences
(when possible), so starting with an uppercase latter and ending with a full
stop.

Changed it to say:

// We check for clientid support and whether tls/ssl running
// by checking the server capability flags for CLIENTID and 
// whether m_connectionType contains "starttls" or "ssl".

If there's anything else to update let me know.

Comment on attachment 9059900 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-22-2019.diff

Thanks for fixing the comment.
Attachment #9059900 - Flags: review+
Comment on attachment 9059903 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-22-2019.diff

Thanks for updating the comment. Gene, can you please put your seal of approval onto this.

> That string is what is presented as the CLIENTID token.
> Coincidence that Thunderbird already called it ClientID.

I'm afraid you will have to educate the reviewer here and give him/me a short summary about this "client ID" stuff, unless you want me to study the RFCs linked in comment #0. All I can see is that you're using pref `toolkit.telemetry.cachedClientID` and that seems to uniquely identify the profile you're using.
Attachment #9059903 - Flags: review?(gds)
Comment on attachment 9059903 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-22-2019.diff

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

I diff'd this with the last patch I tested (-April-15-) and only difference is formatting and comments, so still looks good to me, except for minor items noted.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8802,5 @@
>      }
>    }
>  
> +  // We check for clientid support and whether tls/ssl running
> +  // by checking the server capability flags for CLIENTID and 

Do I detect a blank at the end of this line?

@@ +8809,5 @@
> +          (m_connectionType.EqualsLiteral("starttls") ||
> +          m_connectionType.EqualsLiteral("ssl")))
> +  {
> +    rv = ClientID();
> +    if (NS_FAILED(rv)) {

Maybe move the { to the next line to be consistent with the "if" above and most other "if"s in the file?
Attachment #9059903 - Flags: review?(gds) → review+

(In reply to Jorg K (GMT+2) from comment #89)

Comment on attachment 9059903 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-22-2019.diff

Thanks for updating the comment. Gene, can you please put your seal of
approval onto this.

That string is what is presented as the CLIENTID token.
Coincidence that Thunderbird already called it ClientID.

I'm afraid you will have to educate the reviewer here and give him/me a
short summary about this "client ID" stuff, unless you want me to study the
RFCs linked in comment #0. All I can see is that you're using pref
toolkit.telemetry.cachedClientID and that seems to uniquely identify the
profile you're using.

Jorg, Not sure if you are asking me or Dan but here's my understanding of clientid based on reading the imap spec and emails from Michael and Dan. I suspect you also want a summary of how the SMTP clientid works too. But I will wait to described that until after you understand the imap implementation which is actually simpler.

For imap, you have to set up with SSL/TLS or STARTTLS. Tb makes a tcp/ip connection to the appropriate server port (993 typically for TLS, 143 for STARTTLS) and tb and the server then create a secure connection using TLS. If STARTTLS used, tb sends a STARTTLS imap command to change over to TLS. The server then sends an unsolicited (untagged) CAPABILITY response with capability CLIENTID advertised.

* OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE CLIENTID AUTH=PLAIN] MagicMail ready.

When TB sees the CLIENTID capability and it verifies the connection type is tls or starttls, it then sends the clientid command via the now encrypted and secure channel:

tb: abc CLIENTID TB-UUID <toolkit.telemetry.cachedClientID>
sr: abc OK Recognized a valid CLIENTID command, used for authentication methods

Without clientid seen in capabilities, tb would just go ahead and do its normal authentication after the capability response is received. But if the server supports clientid, authentication will fail unless tb first sends the clientid string. So if clientid is not sent first, authentication will fail like this:

tb: 123 login gene@mail.com mycrypticpassword
sr: 123 NO [AUTHENTICATIONFAILED] Authentication failed.

So after clientid is sent, authentication can occur

tb: def login gene@mail.com mycrypticpassword
sr: def OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE SORT SORT=DISPLAY THREAD=REFERENCES THREAD=REFS THREAD=ORDEREDSUBJECT MULTIAPPEND URL-PARTIAL CATENATE UNSELECT CHILDREN NAMESPACE UIDPLUS LIST-EXTENDED I18NLEVEL=1 CONDSTORE QRESYNC ESEARCH ESORT SEARCHRES WITHIN CONTEXT=SEARCH LIST-STATUS SPECIAL-USE BINARY MOVE QUOTA] Logged in

The way I understand it is that if an unacceptable clientid value is sent, the server will still send an OK response. (It will send BAD only if the syntax is wrong due to tb coding error; clientid does not specify a NO response.) A failure will occur when the client tries to authenticate and the clientid sent was wrong, so the error will appear to be a bad user ID or password. There will be no indication that the problem is due to an unacceptable clientid string and this is by design.

An idea is that at the server's webmail the tb user can configure which CLIENTIDs are valid so if an unauthorized user gets your password, they won't know your clientid so their login will fail. You can also optionally set the server to send email notification to you of unauthorized access attempts from devices with unregistered clientIDs. But server policy for validating and accepting the clientid sent by the client (tb) is purely server dependent and does not involve tb.

Note: I haven't personally verified thattoolkit.telemetry.cachedClientID is really a UUID for every TB instance and profile, so the above discussion assumes it is.

Thanks Gene. I can remove the trailing space when landing, no new patch required. As for the brace style: I'll reformat the entire C++ code base soon, so it really doesn't matter now.

Thanks for the lengthy explanation: In a nutshell: The server will reject login attempts with an "incorrect" CLIENTID. So the user would have to configure the CLIENTIDs in the server somehow, like via a web interface, or the server may use TOFU (trust on first use), so if the user first connects with a TB client, the server will accept its UUID. Trouble only starts when the user switches to a new profile. Then they will have to extract the ID from the prefs. Or maybe the server has a "reset trust" function to restart TOFU.

This is a little like Fastmail's "app password", where each client has a special (long and cryptic) password. I think Gmail had such a concept years ago with an app-specific password at some stage, I can't remember now or find any information on it.

Why reuse the telemetry id? That's probably not advisable, and is a privacy concern.
Also in an earlier comment there was a statement that the id would be per account. The telemetry one would be global, right?

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

Why reuse the telemetry id? That's probably not advisable, and is a privacy concern.
Also in an earlier comment there was a statement that the id would be per account. The telemetry one would be global, right?

Yes, I think the telemetry clientid we are currently using is UUID to the profile, not the account.

(In reply to Dan from comment #15)

Hello Magnus,

As pointed out in another comment the UUID being used is pulled from the account preferences file, which is unique per account.

However, Dan's comment implies he is using an account specific UUID. I don't see an account specific UUID in themail.server.serverX.*prefs that could be used. Don't know what he means by "account preferences file".

(In reply to Jorg K (GMT+2) from comment #92)

Thanks Gene. I can remove the trailing space when landing, no new patch required. As for the brace style: I'll reformat the entire C++ code base soon, so it really doesn't matter now.

Hope this doesn't mean you are changing to the horrible RUST programming standard. It totally breaks finding the start of a function in VIM editor since the first {is no longer in column 1.

(In reply to Jorg K (GMT+2) from comment #92)

Thanks for the lengthy explanation: In a nutshell: The server will reject login attempts with an "incorrect" CLIENTID. So the user would have to configure the CLIENTIDs in the server somehow, like via a web interface, or the server may use TOFU (trust on first use), so if the user first connects with a TB client, the server will accept its UUID. Trouble only starts when the user switches to a new profile. Then they will have to extract the ID from the prefs. Or maybe the server has a "reset trust" function to restart TOFU.

The core of the clientid mechanism on a MagicMail server is the ability to toggle the "lock your mailbox" setting.

A user would start out with an unlocked mailbox and sign into their account while presenting a clientid, that clientid would be recorded into their clientid history file.

That user could then sign into the webmail and approve the recorded clientid in their history.

The user can now lock their mailbox and only authentications that were preceded by presenting the approved clientid will succeed.

(In reply to gene smith from comment #94)

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

Why reuse the telemetry id? That's probably not advisable, and is a privacy concern.
Also in an earlier comment there was a statement that the id would be per account. The telemetry one would be global, right?

Yes, I think the telemetry clientid we are currently using is UUID to the profile, not the account.

(In reply to Dan from comment #15)

Hello Magnus,

As pointed out in another comment the UUID being used is pulled from the account preferences file, which is unique per account.

However, Dan's comment implies he is using an account specific UUID. I don't see an account specific UUID in themail.server.serverX.*prefs that could be used. Don't know what he means by "account preferences file".

Inside ~/.thunderbird/profiles.ini there is a Path= field which describes a path to a profile directory.

Inside the respective profile directory is a prefs.js file which contains the user preferences with an entry for the telemetry clientid:

It looks something like this:

user_pref("toolkit.telemetry.cachedClientID", "55bc6efe-f3d6-4d8b-906e-f9d658f0a310");

I may be confusing account and profiles, I believe this telemetry ID is separate for each profile. But I am not sure how that relates to "accounts"?

(In reply to Dan from comment #95)

I may be confusing account and profiles, I believe this telemetry ID is separate for each profile. But I am not sure how that relates to "accounts"?

Within a "profile" you can have multiple accounts with various servers. You can create new profiles or select an existing profile at tb startup if you use, I think, the -Poption on the command line.

Under ~/.thunderbird you will see your various profiles with <random-stuff>.default being the original (default) profile you create and <random-stuff>.special-profile being an example new profile you might create.

So if we really need an account specific clientid then we need to generate and store (one time) a UUID for each separate account so you will see under each account in the profile something likemail.server.serverX.clientid. Right off, I'm not familiar at all as to how to produce a UUID. Maybe it would only have to be generated and stored only if CLIENTID capability is seen.

Since each profile is essentially a unique tb runtime instance, if you have the same account in 2 profiles, the clientid would be different for the same account in both profiles.

Comment on attachment 9059903 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-22-2019.diff

I'm happy with this now. I'll answer some of the other comments in a minute.
Attachment #9059903 - Flags: review+

(In reply to gene smith from comment #94)

Hope this doesn't mean you are changing to the horrible RUST programming standard.
It totally breaks finding the start of a function in VIM editor since the first {is no longer in column 1.

We'll be changing to Google style, see bug 1546364. All M-C code has been formatted to that standard. I'm afraid the opening brace for a function will go onto the same line as the function name and parameters, so no longer in column 1. Looks like [{ in vi still works, but not [[ :-(

(In reply to Dan from comment #95)

Gene already explained the concept of profiles and accounts. There are also identities which are a refinement of the account concept. The "toolkit.telemetry.cachedClientID" pref appears to identify the profile, not the account. If you have multiple profiles accessing the same account, you get different client IDs (not that I have tried). Generating a UUID per account would be possible, but obviously more work since it wouldn't use anything already available. The same problems occurs: The same account in two different profiles would have different client IDs unless manually tweaked. We could think up some UI to manage those client IDs.

For now, what we have seems fine and can be refined in another bug since we're already nearing the critical comment count of 100 ;-)

In reality, not many servers offer this, so if something goes wrong for the MagicMail customers, we know who will need to fix it ;-)

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

Why reuse the telemetry id? That's probably not advisable, and is a privacy concern.
Also in an earlier comment there was a statement that the id would be per account. The telemetry one would be global, right?

Is this good to go for now despite this concern? See my previous comment.

Flags: needinfo?(mkmelin+mozilla)

I don't think using the telemetry id is ok: those id's may be public or semi-public somewhere, which would open up the privacy can of worms (and potentially even legal issues). It's also not quite clear that the id is always there: you can disable telemetry at compile time, and some setups will do that. Furthermore, looking at the code I see there is functionality to reset that id, so the id can very well change over time, making it quite a non-starter.

So it's clear we need to generate one id ourselves, one we can trust is available and doesn't change. Since we have many different accounts it's also not that feasible to use a global id. Then it's very complicated to get a reasonable UI for it if that's ever needed (and sounds like it may be needed somewhere under advanced settings).

I'd suggest putting the id under nsIMsgIncomingServer.idl + nsISmtpServer.idl. It can then be accessed as needed. I think they should be stored as client_id and not as general UIDs so there aren't problems with collisions in case client_id is the same for multiple servers. use_condstore is one example: https://searchfox.org/comm-central/search?q=use_condstore&path=

Flags: needinfo?(mkmelin+mozilla)

To confirm Magnus, it is just the re-use of telemetry.uuid as CLIENT_ID that concerns you, would it be viable to store it as:
user_pref("mail.client_id", "XXXXXX-XXXXX-XXXX-XXXXX-XXXX");

And access it for all accounts for the single profile? Otherwise there is considerable more work, as the client_id has to be created ONLY when the user_pref("mail.smtpserver.smtpNN...") and user_pref("mail.server.serverNN...") get created, to avoid having two(2) CLIENTID's for a single person's account.

It (global mail.client_id) 'could' optionally be used to populate user_pref("mail.smptserver.smtpNN.client_id") during backend creation as an interim measure, to allow for later enhancements to allow per account client_id, however the nature of Thunderbird is that the outgoing smtp server can be "changed" for an identity, leading to two separate client_id's for the same email account.

(In reply to Jorg K (GMT+2) from comment #102)

I'd be OK with one client ID per profile. You can create one like so:
https://searchfox.org/comm-central/rev/f014ca0fb7127c585786bd2adb666f9074d5d088/mailnews/compose/src/nsMsgCompUtils.cpp#923

Hello Jorg,

I noticed the code you have linked is utilizing the function GenerateGlobalRandomBytes() which is a static function in nsMsgCompUtils.cpp.

If I am understanding the requested changes correctly... then for the IMAP patch I would be looking to swap out this line:

aPrefBranch->GetCharPref("toolkit.telemetry.cachedClientID", gClientId);

With something like this:

aPrefBranch->GetCharPref("mail.client_id", gClientId);
if (gClientId.IsEmpty()) {
  // Generate 128-bit UUID for the client id. 
  nsID uuid;
  GenerateGlobalRandomBytes((unsigned char*) &uuid, sizeof(nsID));
  char uuidString[NSID_LENGTH];
  uuid.ToProvidedString(uuidString);
  // Drop first and last characters (curly braces).
  uuidString[NSID_LENGTH - 2] = 0;
  gClientId = uuidString + 1;
  aPrefBranch->SetCharPref("mail.client_id", gClientId);
}

However in the above approach I wouldn't have access to the function GenerateGlobalRandomBytes() and would either need to duplicate it into the nsImapProtocol.cpp file or expose the function publicly (remove static + add header prototype?).

Obviously the SMTP patch would have a similar issue so I feel like I may be approaching the requested changes wrong?

Yes, please make GenerateGlobalRandomBytes() publicly visible.

I also noticed that GenerateGlobalRandomBytes() exists in the SMTP codebase, which means in my IMAP code I would be forced to make an include a header from the SMTP codebase and/or more properly move the function to a common location (which seems beyond the mandate of this ticket).

Do you have any alternate suggestions?

P.S. Another approach we investigated is the class in nsIUUIDGenerator.h which appears available to both IMAP and SMTP:

nsCOMPtr<nsIUUIDGenerator> uuidgen = mozilla::services::GetUUIDGenerator();
rv = uuidgen->GenerateUUIDInPlace(&aUUID);

Not sure if this is the recommended approach? Or should we look to moving GenerateGlobalRandomBytes()?

Oops, looks like I gave the wrong advice in comment #102, sorry :-( - Yes, GetUUIDGenerator() seems like the way to go.

Attached patch t.gds.diff (obsolete) — Splinter Review

Dan, This expands on your idea in comment 103. It directly calls the low level function instead of the static function.
This "diff" just shows the changes for clientid and not all the other changes you have made.

I was able to build and it still works with these changes and a fully random (I assume) client_id (a uuid) is produced. The client_id is still common to a given profile and not unique to each account.

I'm not sure I understood Magus point about condstore example in comment 100. I think he may want the pref to be called "mail.server.default.client_id" so it can later possibly be expanded to server specific, e.g. "mail.server.server3.client_id" in which case each imap server/account would have a different client_id.

I'm not sure how to keep the imap and corresponding smtp server tied together and having the same client_id if each account has its own personal client_id. Maybe it's not an absolute requirement?

(Anyhow, just saw the Jorg thinks you should use GetUUIDGenerator() so maybe that's a better way to go.)

Yes using the UUIDGenerator is the way to go.

It's unfortunate mail incoming/outgoing servers are separated, but not much we can do about that.

As kind of a compromise, and to allow setting per server client_ids, can we pre-generate two client_ids: one for incoming and one for outgoing (but making them the same), so

pref("mail.server.default.client_id", FOO);
pref("mail.smtpserver.default.client_id", FOO);

Compare to e.g. mail.smtpserver.default.try_ssl and the condstore example. Now that you add a "default", each server will use that unless specifically saving a per-server value. If someone is concerned, they can change them as they want, and it would be possible to add UI per server in the future if needed.

For the record, the approach we are taking, is that we are creating a default clientid for the profile, if it doesn't exist.

Then when loading either smtp or imap, if a account specific client does not exist for that service, it will use the default client id for the profile for now, and record that value in the local service specific preferences for that branch/server.

This will ensure that for the interim, the clientid is the same for incoming/outgoing for a single account. When creating a new account, this occurs from javascript UI, and not yet in per account preferences, but it will be needed to test connectivity, if the server has client id enforced.

In that case, it will also use the default client ID for the profile. On saving the new account, that default will be saved for the per account clientid as well. Thoughts for the future probably require input from higher ups, eg long term direction on when/how to tie incoming/outgoing to be considered 'one account'.

Most of that work completed today, however the new outgoing servers are created in a much different fashion, eg triggered via the UI js.. comm/mail/components/accountcreation/content/createInBackend.js .. outServer = MailServices.smtp.createServer() ..
comm/mailnews/compose/public/nsISmtpService.idl so requires a little more research.

Attachment #9060634 - Attachment is obsolete: true
Type: defect → enhancement

We're currently reformatting the entire C++ source in bug 1546364. Sadly that invalidates existing patches. So far, both patches should still apply, but be warned. I'll do mailnews/compose last, but mailnews/imap will be next invalidating the patch here. It's a small patch and not hard to rebase and you're working on it anyway.

Attachment #9059900 - Attachment is obsolete: true
Attachment #9061418 - Flags: review?(mkmelin+mozilla)
Attachment #9061418 - Flags: review?(jorgk)
Attachment #9061418 - Flags: review?(gds)
Attachment #9059903 - Attachment is obsolete: true
Attachment #9061419 - Flags: review?(mkmelin+mozilla)
Attachment #9061419 - Flags: review?(jorgk)
Attachment #9061419 - Flags: review?(gds)

I have uploaded new versions of the SMTP and IMAP patches which utilize a centralized default preference to share the clientid across smtp/imap servers, and then each server will implement a per-account clientid which can be overridden in the preferences.

If there is anything else I can do please let me know, and Jorg when the time comes I would be happy to perform the formatting changes to ensure these patches conform to the new style guide.

Realized that I can re-base the patch onto the latest trunk

Attachment #9061419 - Attachment is obsolete: true
Attachment #9061419 - Flags: review?(mkmelin+mozilla)
Attachment #9061419 - Flags: review?(jorgk)
Attachment #9061419 - Flags: review?(gds)
Attachment #9061433 - Flags: review?(mkmelin+mozilla)
Attachment #9061433 - Flags: review?(jorgk)
Attachment #9061433 - Flags: review?(gds)

Realized that I can rebase the patch onto the latest trunk

Attachment #9061418 - Attachment is obsolete: true
Attachment #9061418 - Flags: review?(mkmelin+mozilla)
Attachment #9061418 - Flags: review?(jorgk)
Attachment #9061418 - Flags: review?(gds)
Attachment #9061434 - Flags: review?(mkmelin+mozilla)
Attachment #9061434 - Flags: review?(jorgk)
Attachment #9061434 - Flags: review?(gds)

Dan, Are you saying this allows the user to enter a non-default clientid for an account in prefs? If so, what does the account specific pref name look like? I would assume it would look like "mail.server.serverX.clientid"? But it appears you are calling the "default" clientid "mail.default.clientid". I could be wrong but I think you need to call it "mail.server.default.clientid" like Magnus described in comment 100 for "mail.server.default.use_condstore" and comment 108 for clientid pref("mail.server.default.client_id").

Other than this, it looks good. I see you didn't have to get into any .js stuff. I will apply the patch and try to test it later today. Thanks!

EDIT: Comment 117 below answers my questions.

(In reply to gene smith from comment #116)

Dan, Are you saying this allows the user to enter a non-default clientid for an account in prefs? If so, what does the account specific pref name look like? I would assume it would look like "mail.server.serverX.clientid"? But it appears you are calling the "default" clientid "mail.default.clientid". I could be wrong but I think you need to call it "mail.server.default.clientid" like Magnus described in comment 100 for "mail.server.default.use_condstore" and comment 108 for clientid pref("mail.server.default.client_id").

Other than this, it looks good. I see you didn't have to get into any .js stuff. I will apply the patch and try to test it later today. Thanks!

You will see multiple entries.

The "default" will always end up as:

user_pref("mail.clientid.default", "261cf1b9-cd72-48f3-8532-6cb3551b1d15");

Then you will see the per server and per account entries as:

user_pref("mail.server.server1.clientid", "261cf1b9-cd72-48f3-8532-6cb3551b1d15");

and

user_pref("mail.smtpserver.smtp1.clientid", "261cf1b9-cd72-48f3-8532-6cb3551b1d15");

These are set by the pieces of code:

NS_IMETHODIMP
nsSmtpServer::SetClientid(const nsACString &aClientid)
{
  if (!aClientid.IsEmpty())
    return mPrefBranch->SetCharPref("clientid", aClientid);

Where the mPrefBranch refers to the respective server preference branch "mail.smtpserver.smtpN"

I hope this answers your question, if you have any other questions or I didn't clarify something enough please feel free to ask.

Hmm, the default mechanism for server specific preferences always uses "mail.server.default.XXX", you can see it when scrolling down from here:
https://searchfox.org/comm-central/rev/6c7c6cb06eea17eddd9f84ac554acc2bf1f53ab2/mailnews/mailnews.js#513

That's what Magnus suggested and that's what Gene also asked in comment #116. I don't really see why you need to deviate from the established scheme and I can't see that comment #117 explains it. You can have pref("mail.server.default.clientid", ""); and when you try to get the first client and find the pref empty, you can set the pref. Other than that, you're code will look like this stuff here:
https://searchfox.org/comm-central/rev/6c7c6cb06eea17eddd9f84ac554acc2bf1f53ab2/mailnews/imap/src/nsImapIncomingServer.cpp#267

We'd like to avoid a special new style for this pref. Or am I missing something?

EDIT: What was wrong with Gene's attachment 9060634 [details] [diff] [review]?

It seems a bit different since the "default" is shared between imap and smtp where the others are just shared within the protocol. So calling it mail.default.clientid for the shared default seems reasonable. Or maybe it could just be called mail.clientid_default ?

Well, we have a scheme in place for "server" defaults, why invent a new scheme here? I'm sure some defaults are shared between IMAP and POP, so I don't see why we can't share between SMTP and IMAP. That raises the unrelated question: Will there be POP3 support?

(In reply to Jorg K (GMT+2) from comment #118)

EDIT: What was wrong with Gene's attachment 9060634 [details] [diff] [review]?

This works but uses a different function and requires a free() call. However, it doesn't provide the possibility to have a server specific clientid entered via prefs editor like Dan's does. So what Dan has proposed seems like it covers all the bases; but I still need to try it.

I must be missing something. In one case you set "mail.server.default.clientid", in the other "mail.clientid.default". The result is the same, no? Only that if you're not using the standard scheme, you need to hand-roll your own.

(In reply to Jorg K (GMT+2) from comment #120)

Well, we have a scheme in place for "server" defaults, why invent a new scheme here? I'm sure some defaults are shared between IMAP and POP, so I don't see why we can't share between SMTP and IMAP. That raises the unrelated question: Will there be POP3 support?

Right off I don't know what is shared between imap and smtp but I see at least one that isn't:
mail.server.default.authMethod // imap
mail.smtpserver.default.authMethod // smtp

I don't see any pop prefs that are server specific like these. However, seem like pop3 could use clientid since, I think, it also supports connection via starttls and/or tls. They don't have a proposed rfc for it AFAIK.

But if you (Jorg) prefer to use mail.server.default.clientid for the "common" pref I don't see a problem since it's just a name and it can be accessed from imap and smtp threads OK like I did in my rejected diff attachment 9060634 [details] [diff] [review].

Personally, becuase the SMTP and IMAP use completely different methods, it can't be guaranteed that mail.server.default methods will always be accessible to IMAP (yes, currently it can be accessed) so we thought it safer to define it at the higher level.

This patch takes Magnus's comments into account, but because we have to address cases where legacy services, both IMAP and SMTP may not have a CLIENTID set yet, this allows them to be populated with the default, so that the same CLIENTID can be assured to be used for the accounts incoming and outgoing connections, making it easier for the end user to understand. (I am only 'approving' one device, not two). But we also had to address when new accounts are being setup, to make sure that a CLIENTID was available right away, and again hopefully the same for both incoming/outgoing.

It is conceivable that in the future, after consideration about the relationships between outgoing/incoming servers as a single account, that logic can be applied in the JS to create a new 'service' specific CLIENTID, without changing any of the backend code in this patch. Eg, when creating a new account, if it is using the same outgoing server, what logic will be decided on.. Should it create a matching outgoing server, with a new CLIENTID? Too many unknowns, so this approach solves all the current issues, while still allowing for future decisions on how to implement unique client ID's per 'account' if so desired.

I think Dan has done a good job in satisfying all current requirements, handling legacy and new account creation, while ensuring that future revisions to the way SMTP and IMAP code is handled won't require any further changes.

While for now, this means existing accounts will all use the same per 'profile' CLIENTID, it solves any potential confusing issues for the end user, with multiple incoming/outgoing servers, and still supports future enhancements.

PS, yes, there is no DRAFT for POP .. so no support added for now..

Applied patches and when building found 2 problem in nsImapProtocol.cpp:

  1. Need #include "nsIUUIDGenerator.h"
  2. In nsImapProtocol::ClientID()s/nresult/nsresult/

With these fixes, it seems to be working and I see in config editor:

mail.clientid.default 238194e3-777d-4976-9160-eb9019041d31
mail.server.server6.clientid          ditto
mail.smtpserver.smtp3.clientid        ditto

I haven't tried to create a new account to see that that code works. I will hold off on doing more until we make a final decision on the naming question.

Also, when patch updated, in nsMsgAccountManager.cpp there is a left-over comment that should be removed:
// Dan: Should we be checking if the clientid is already set?
It appears that the check was added.

Comment on attachment 9061433 [details] [diff] [review]
imap-patch-clientid-thunderbird-april-29-2019.diff

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

Probably needs some fixes and doesn't build. But after fixes, it seems to work. But haven't tried to add a new account. Will wait for next patch to try that.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +448,5 @@
>      // From when we first create the account until we have created some folders,
>      // we can change the store type.
>      (*_retval)->SetBoolValue("canChangeStoreType", true);
> +
> +    // create our default clientid

This occurs for every new account, right? Or is it only accounts that use IMAP? At this point we don't know if the server supports clientid, right?

@@ +450,5 @@
>      (*_retval)->SetBoolValue("canChangeStoreType", true);
> +
> +    // create our default clientid
> +    nsCString clientId;
> +    // Dan: Should we be checking if the clientid is already set?

Remove comment. It appears that check for already set is being done.

@@ +469,5 @@
> +        if (NS_FAILED(rv) || clientId.IsEmpty()) {
> +            return NS_ERROR_FAILURE;
> +        }
> +      }
> +      (*_retval)->SetCharValue("clientid", clientId);

Is this saving clientid to mail.server.serverX.clientid or to mail.smtpserver.smtpX.clientid. My guess is the former.

::: mailnews/imap/public/nsIImapIncomingServer.idl
@@ +52,5 @@
>  
> +  /**
> +   * clientid if required
> +   */
> +  attribute ACString clientid;

Is this used somewhere? Edit: Never mind, used to define Get/SetClientid().

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +2052,5 @@
> +  // If the pref value is already empty, ClearUserPref will return
> +  // NS_ERROR_UNEXPECTED, so don't check the rv here.
> +  mPrefBranch->ClearUserPref("clientid");
> +  return NS_OK;
> +}

Is this ever called with an empty aClientid string?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +942,1 @@
>        }

Maybe above needs 
#include "nsIUUIDGenerator.h"

@@ +6105,5 @@
> +  nsCString command(GetServerCommandTag());
> +  command += " CLIENTID TB-UUID ";
> +  command += m_clientId;
> +  command += CRLF;
> +  nresult rv = SendDataParseIMAPandCheckForNewMail(command.get(), nullptr);

nsresult rv = ...

@@ +8948,5 @@
>      }
>    }
>  
> +  // We check for clientid support and whether tls/ssl running
> +  // by checking the server capability flags for CLIENTID and 

blank at EOL
Attachment #9061433 - Flags: review?(gds) → review-
Comment on attachment 9061434 [details] [diff] [review]
smtp-patch-clientid-thunderbird-april-29-2019.diff

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

Seem OK pending name resolution.
Attachment #9061434 - Flags: review?(gds) → review+

(In reply to DevTeam from comment #124)

Personally, becuase the SMTP and IMAP use completely different methods, it
can't be guaranteed that mail.server.default methods will always be
accessible to IMAP (yes, currently it can be accessed)

It would seem quite unlikely we wouldn't have per server prefs also in the future, so I'd still prefer to go with the standard scheme we're using now.

Attachment #9061433 - Flags: review?(mkmelin+mozilla)
Attachment #9061434 - Flags: review?(mkmelin+mozilla)
Attachment #9061433 - Flags: review?(jorgk)
Attachment #9061434 - Flags: review?(jorgk)

Looks like the IMAP patch needs some more work. In the meantime I progressed with the reformatting. So please rebase to the latest trunk source when submitting your patch again. Sorry about the inconvenience, but you're just adding a few hunks, so it's not too bad to rebase.

Rebased on latest trunk, not much has changed here

Attachment #9061434 - Attachment is obsolete: true

Rebased on latest trunk and switch nsImapIncomingServer's Get/Set ClientId functions to use NS_IMPL_SERVERPREF_STR instead of direct definition.

Attachment #9061433 - Attachment is obsolete: true
Attachment #9061938 - Flags: review?(mkmelin+mozilla)
Attachment #9061938 - Flags: review?(jorgk)
Attachment #9061938 - Flags: review?(gds)

I have rebased the two patches onto the latest trunk, not much has changed besides that.

I am a little unsure with the issues surrounding mail.default.clientid, as I understand it there is a "standard" format for the default preferences and it would look something like: mail.server.default.clientid (for IMAP) and mail.smtpserver.default.clientid (for SMTP).

The part I am not understanding is how we would go about seeding these two "default" preferences with the same newly generated UUID. The only way I can conceptualize is they both check the same preference (mail.default.clientid or something similar) such that whichever one runs first (imap or smtp) will populate the default and then the other will pull from that default.

If the defaults were separate as mail.server.default.clientid and mail.smtpserver.default.clientid then wouldn't that result in generating different default clientids for smtp and imap respectively?

I missed a minor error in the last patch I uploaded, corrected it as fast as possible.

Attachment #9061938 - Attachment is obsolete: true
Attachment #9061938 - Flags: review?(mkmelin+mozilla)
Attachment #9061938 - Flags: review?(jorgk)
Attachment #9061938 - Flags: review?(gds)
Attachment #9061952 - Flags: review?(mkmelin+mozilla)
Attachment #9061952 - Flags: review?(jorgk)
Attachment #9061952 - Flags: review?(gds)

Sincere apologies for the spam, I made another mistake in the imap patch. Everything is compiling and working as intended now.

Attachment #9061952 - Attachment is obsolete: true
Attachment #9061952 - Flags: review?(mkmelin+mozilla)
Attachment #9061952 - Flags: review?(jorgk)
Attachment #9061952 - Flags: review?(gds)
Attachment #9061953 - Flags: review?(mkmelin+mozilla)
Attachment #9061953 - Flags: review?(jorgk)
Attachment #9061953 - Flags: review?(gds)

somehow I uploaded the old march-29 patch in the last post instead of the may-1 patch... Perhaps I haven't slept enough, sincere apologies again for more spam.

Attachment #9061953 - Attachment is obsolete: true
Attachment #9061953 - Flags: review?(mkmelin+mozilla)
Attachment #9061953 - Flags: review?(jorgk)
Attachment #9061953 - Flags: review?(gds)
Attachment #9061955 - Flags: review?(mkmelin+mozilla)
Attachment #9061955 - Flags: review?(jorgk)
Attachment #9061955 - Flags: review?(gds)

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

I'd suggest putting the id under nsIMsgIncomingServer.idl + nsISmtpServer.idl. It can then be accessed as needed.

I have been reviewing the previous patches where I have tried to place the id under nsIMsgIncomingServer.idl but after lots of minor issues I have come to the conclusion that nsIMsgIncomingServer may not be a suitable location, let me explain my issues:

When I call GetClientid from nsImapProtocop.cpp the imapServer object is of type nsIImapIncomingServer:

nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);

This means if I call imapServer->GetClientid I will receive:

nsImapProtocol.cpp:880:17: error: ‘class nsIImapIncomingServer’ has no member named ‘GetClientid’
       imapServer->GetClientid(m_clientId);
                   ^~~~~~~~~~~

This is because the interface nsIImapIncomingServer does not derive from the interface nsIIMsgIncomingServer (the non-interface class nsMsgIncomingServer does! but the interface does not!) which means that I would need to implement the 'attribute ACString clientid' into nsIImapIncomingServer and implement the actual definitions with NS_IMPL_SERVERPREF_STR in nsImapIncomingServer.cpp rather than nsMsgIncomingServer.cpp.

So this latest patch I have uploaded (sorry for the spam again) I have only implemented the clientid in nsImapIncomingServer and it appears to be working as intended.

I ran a full compile and ran tests against our local magicmail servers and it is generating and presenting the correct clientid credentials over imap and smtp.

Everything should be good this time around, apologies for any confusion I've caused with the previous posted patches.

Attachment #9061955 - Attachment is obsolete: true
Attachment #9061955 - Flags: review?(mkmelin+mozilla)
Attachment #9061955 - Flags: review?(jorgk)
Attachment #9061955 - Flags: review?(gds)
Attachment #9062041 - Flags: review?(mkmelin+mozilla)
Attachment #9062041 - Flags: review?(jorgk)
Attachment #9062041 - Flags: review?(gds)

Comment on attachment 9061935 [details]
smtp-patch-clientid-thunderbird-may-1-2019

This patch is empty.

Comment on attachment 9062041 [details] [diff] [review]
imap-patch-clientid-thunderbird-may-1-2019.diff

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

Thanks for the update. Looks like at I was on the wrong boat. Indeed, incoming and outgoing servers don't share the same preferences for their defaults, one is `mail.server.default.XX`, the other one `mail.smtpserver.default.XX`. So unless the first access to client ID wants to set both preferences, we need an independent single preference as you have done here. Since that preference is not part of the overall server preference scheme, it should possibly just be called `mail.clientid`.

I think the patch is incomplete, since I don't see any changes to mailnews.js which would still need to define `mail.server.default.clientid`. Maybe that's in the SMTP patch which is sadly empty.

Whether the client ID goes into nsIImapIncomingServer.idl or nsIMsgIncomingServer.idl somewhat depends on the question of whether a POP support is planned at some stage. I'm sure you can get the "msg" variant to compile as well, see comment below.

::: mailnews/imap/src/nsImapCore.h
@@ +143,4 @@
>  const eIMAPCapabilityFlag kHasSpecialUseCapability =      0x200000000LL;  /* RFC 6154: Sent, Draft etc. folders */
>  const eIMAPCapabilityFlag kGmailImapCapability =          0x400000000LL;  /* X-GM-EXT-1 capability extension for gmail */
>  const eIMAPCapabilityFlag kHasXOAuth2Capability =         0x800000000LL;  /* AUTH XOAUTH2 extension */
> +const eIMAPCapabilityFlag kHasClientIDCapability =       0x1000000000LL;  /* ClientID capability */

Not alinged :-( - after all that reformatting effort.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +876,5 @@
>          m_preferPlainText = preferPlainText;
>        }
>      }
> +    // We need to retrieve our clientid and initialized it if missing.
> +    imapServer->GetClientid(m_clientId);

If this were in nsIMsgIncomingServer.idl you could possibly use `server->GetClientid()`, although I haven't tried. There is a server variable further up in the function, `nsCOMPtr<nsIMsgIncomingServer> server`.
Attachment #9062041 - Flags: review?(jorgk) → feedback+

(In reply to Jorg K (GMT+2) from comment #138)

So unless the first access to client ID wants
to set both preferences, we need an independent single preference as you
have done here.

That shouldn't be much of a problem.
Or you set it up both as a one time generation through the migrator: https://searchfox.org/comm-central/rev/22360aa5a9fc717e0da454de4337f6612bd9f67d/mailnews/base/util/mailnewsMigrator.js#23

You mean: You add a piece of initialisation code in JS somewhere that populates one or two preferences with a client ID.

I think comment #139 is still unclear. You want one preference as in the patch or two?

Yes. One for ingoing and one for outgoing, but they can be set to be the same (by default).

Magnus, want to suggest that we more clearly explain first the basic locations and reasoning why of the defaults. Seems still confusion by everyone what your are suggesting wanting. Since in Thunderbird, there is no real connection between SMTP & IMAP, eg the sense of an 'account' which defines a pair of incoming/outgoing servers, and because it would be better for user experience to think of that 'pair' as the device/account if in the future you would like to have 'per account' clientid, the best effort stepping stone is to think of a profile specific CLIENTID, so that all existing 'pairs' will have the same clientid to start. Which means defining a 'profile wide' 'mail' 'default' 'clientid'. And per the first attempt and your comments, that should be separate from other UUID (eg the original use of telemetry id)

By all means tell us what naming convention you would like that to be.
We started with mail.clientid.default, but in keeping with other naming conventions, should that be mail.default.clientid

The idea of a default for smtp, and a default for imap goes counter to the idea that they should be the same for an 'account', and we suggest that there is more chance for future problems, where the defaults end up different.

Then of course, we have to address existing accounts, which makes the idea that on loading of either an SMPT server, or IMAP server, that the 'profile' default be used, so that existing 'pairs' will have the same clientid from the mail.default.clientid

Simply laoding the profile default, will ensure that it is used, and when the prefs are automatically stored back, the default profile clientid will be also written to the server specific prefs, whether it is IMAP or SMTP.

For NEW accounts being created, the logic for this initial rollout, is to ALSO load the profile default, so that we don't have to worry about matching the new IMAP account, with the existing outgoing server's clientid at this time. And again, the profile default clientid will be saved as the IMAP service specific clientid IF one doesn't exist. And having the logic separate for SMTP (outgoing) will ensure the same thing happens IF it is a new service.

We are saving it for the future, generating new CLIENTID as service is created, until we can work out the complexity, and the desired behaviour of account 'pairing', and the UI implications.

But proceeding in this way, allows for the actual implementation at a later date of per account CLIENTID's to be created via the JS account creation.

Having a SMTP specific CLIENTID, and/or IMAP specific CLIENTID doesn't bring any value, and can cause complications, and an account or profile based default is simpler, and more flexible. (The 'default' outgoing server (First one a client creates for instance), will much more likely be a candidate as the default to use for any new IMAP connections that also use that outgoing server. And the logic has to be decided for instance, when someone changes the outgoing server associated with an incoming server. And any future logic to 'reset' an account specific UUID if that concept is suggested etc..

If we all agree on this, we can then address how the 'code' should be implemented a lot easier.

PS, this approach also makes it so that no JS or UI code needs to be altered in this first patch release, simplifying code review and inclusion. Does this all make sense?

OK, reading through Magnus' comments from comment #108, #139 and #141, we should do two defaults:

pref("mail.server.default.client_id", FOO);
pref("mail.smtpserver.default.client_id", FOO);

or clientid. Without having to mess setting them both in IMAP or SMTP code, it would be easiest to set them up in
https://searchfox.org/comm-central/rev/22360aa5a9fc717e0da454de4337f6612bd9f67d/mailnews/base/util/mailnewsMigrator.js#23

Here's an example of how to get a UUID:
https://searchfox.org/comm-central/rev/22360aa5a9fc717e0da454de4337f6612bd9f67d/calendar/base/modules/calUtils.jsm#316
Or here:
https://searchfox.org/comm-central/rev/22360aa5a9fc717e0da454de4337f6612bd9f67d/chat/protocols/xmpp/xmpp.jsm#1204

C++ code can just rely on them being set and use imapServer->GetClientid(m_clientId); for retrieval.

Yes, what Jörg said.

I understand you want to consider imap+smtp an account, and it often is, but it's still counter productive to do so in code and then have to special case clientid for everything. If we generate the pair instead, e.g. UI could be possible - otherwise, difficult.

I have attached the latest iteration of the patch, we have combined both SMTP and IMAP into a single patch, and we have taken all comments into consideration.

We have removed the mail.default.clientid preference and we are simply generating a new default UUID and assigning it to both: mail.server.default.clientid and mail.smtpserver.default.clientid.

We had to avoid the macro NS_IMPL_SERVERPREF_STR because it expands to a function which calls nsMsgIncomingServer::SetCharValue(), the problem with SetCharValue() is it will clear the target preference if you attempt to assign the respective default value:

NS_IMETHODIMP            
nsMsgIncomingServer::SetCharValue(const char *prefname,            
                                  const nsACString& val)            
{                                
  if (!mPrefBranch)            
    return NS_ERROR_NOT_INITIALIZED;            
        
  if (val.IsEmpty()) {            
    mPrefBranch->ClearUserPref(prefname);            
    return NS_OK;                                     
  }                                            
                                    
  nsCString defaultVal;            
  nsresult rv = mDefPrefBranch->GetCharPref(prefname, defaultVal);            
               
  if (NS_SUCCEEDED(rv) && defaultVal.Equals(val))            
    mPrefBranch->ClearUserPref(prefname);            
  else            
    rv = mPrefBranch->SetCharPref(prefname, val);            
                    
                                           
  return rv;                                                                                                                                                                      
}

In the above function you can see defaultVal.Equals(val) will result in mPrefBranch->ClearUserPref() which means that we cannot populate the per-user IMAP preferences with the value stored in mail.server.default.clientid.

The same is not true for SMTP, we do not have NS_IMPL_SERVERPREF_STR in SMTP so the custom SetClientid function there will actually set the clientid regardless of whether it matches the mail.smtpserver.default.clientid or not.

Attachment #9061935 - Attachment is obsolete: true
Attachment #9062041 - Attachment is obsolete: true
Attachment #9062041 - Flags: review?(mkmelin+mozilla)
Attachment #9062041 - Flags: review?(gds)
Attachment #9062640 - Flags: review?(mkmelin+mozilla)
Attachment #9062640 - Flags: review?(jorgk)
Attachment #9062640 - Flags: review?(gds)
Comment on attachment 9062640 [details] [diff] [review]
clientid-thunderbird-full-may-3-2019.diff

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

Also, may need macro:
NS_IMPL_SERVERPREF_STR(nsImapIncomingServer, Clientid, "clientid")
put again in comm/mailnews/imap/src/nsImapIncomingServer.cpp if my suggestion for createInBackend.js is used.
Anyhow, maybe I just don't understand, but don't see were the UUID is generated.

::: mail/components/accountcreation/content/createInBackend.js
@@ +25,5 @@
>    inServer.port = config.incoming.port;
>    inServer.authMethod = config.incoming.auth;
>    inServer.password = config.incoming.password;
> +  // Set the incoming imap server clientid from the default.
> +  inServer.clientid = Services.prefs.getCharPref("mail.server.default.clientid");

Here's an alternate way to set the server clientid. This way we don't have to define special Set/GetClientid() functions and keep NS_IMPL_SERVERPREF_STR where other places Set/GetClientid() are called:

// Set the incoming imap server clientid from the default.
let def_cid = Services.prefs.getCharPref("mail.server.default.clientid");
let pB = "mail.server." + inServer.key + "." + "clientid";
Services.prefs.setCharPref(pB, def_cid);

@@ +83,5 @@
>      outServer.hostname = config.outgoing.hostname;
>      outServer.port = config.outgoing.port;
>      outServer.authMethod = config.outgoing.auth;
> +    // Set outgoing smtpserver clientid from the default.
> +    outServer.clientid = Services.prefs.getCharPref("mail.smtpserver.default.clientid");

For consistency with imap server set above, this can also be done as shown below; however, since the Set/GetClientid() function must remain explicitly defined for SMTP since no macro to defined them exists, this can remain as is in the patch:

// Set outgoing smtpserver clientid from the default.
def_cid = Services.prefs.getCharPref("mail.smtpserver.default.clientid");
pB = "mail.smtpserver." + outServer.key + "." + "clientid";
Services.prefs.setCharPref(pB, def_cid);

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +78,5 @@
>  
>    /**
> +   * clientid if required
> +   */
> +  attribute ACString clientid;

Don't understand the "if required". In general this is required since we set this for all servers even when the imap server doesn't support CLIENTID capability.

::: mailnews/base/util/mailnewsMigrator.js
@@ +60,5 @@
> +  }
> +  if (!Services.prefs.getCharPref("mail.smtpserver.default.clientid")) {
> +    Services.prefs.setCharPref("mail.smtpserver.default.clientid", default_clientid);
> +  }
> +}

Maybe I'm confused, but the production of the UUID seems to be missing here. The code I am testing with now is this:

/**
 * Creates mail.server.default.clientid if it is not yet set by producing a UUID. 
 * Then checks if mail.smtpserver.default.clientid is set and, if not, sets it to the value
 * of mail.server.default.clientid. This function occurs at each TB startup.
 */
function MigrateProfileClientid() {
  var default_clientid = Services.prefs.getCharPref("mail.server.default.clientid");
  if (!default_clientid.length) {
    // generate uuids without braces
    let uuidGen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
    default_clientid = uuidGen.generateUUID().toString().replace(/[{}]/g, "");
    //Services.prefs.setCharPref("mail.default.clientid", default_clientid);
    Services.prefs.setCharPref("mail.server.default.clientid", default_clientid);
  }
  // populate our default clientid preferences if they are missing
  if (!Services.prefs.getCharPref("mail.smtpserver.default.clientid").length) {
    Services.prefs.setCharPref("mail.smtpserver.default.clientid", default_clientid);
  }
}

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +2324,5 @@
> +  // If the pref value is already empty, ClearUserPref will return
> +  // NS_ERROR_UNEXPECTED, so don't check the rv here.
> +  mPrefBranch->ClearUserPref("clientid");
> +  return NS_OK;
> +}

Don't need these if different method used in createInBackend.js

::: mailnews/compose/src/nsSmtpServer.cpp
@@ +316,5 @@
> +  // If the pref value is already empty, ClearUserPref will return
> +  // NS_ERROR_UNEXPECTED, so don't check the rv here.
> +  mPrefBranch->ClearUserPref("clientid");
> +  return NS_OK;
> +}

Yes, need these since SMTP doesn't have the macro.

::: mailnews/mailnews.js
@@ +458,5 @@
> +// These two preferences are the defaults used by the
> +// clientid logic in normal operations if the user
> +// preference is missing.
> +pref("mail.server.default.clientid", "");
> +pref("mail.smtpserver.default.clientid", "");

Not sure about the comment.
Attachment #9062640 - Flags: review?(gds) → review-
Comment on attachment 9062640 [details] [diff] [review]
clientid-thunderbird-full-may-3-2019.diff

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

::: mail/components/accountcreation/content/createInBackend.js
@@ +83,5 @@
>      outServer.hostname = config.outgoing.hostname;
>      outServer.port = config.outgoing.port;
>      outServer.authMethod = config.outgoing.auth;
> +    // Set outgoing smtpserver clientid from the default.
> +    outServer.clientid = Services.prefs.getCharPref("mail.smtpserver.default.clientid");

Quick drive-by comment on the weekend: If there's a macro missing for SMTP, please create it, or move the existing one to a place where it's visible for both incoming and outgoing.

(In reply to Dan from comment #145)

We had to avoid the macro NS_IMPL_SERVERPREF_STR because it expands to a
function which calls nsMsgIncomingServer::SetCharValue(), the problem with
SetCharValue() is it will clear the target preference if you attempt to
assign the respective default value:

That's how it's supposed to work: only non-defaults should be stored. For default values the default would be obtained by the getter. To be clear: just have the default stored, and the per-server value will be obtained from there. You don't need (and should not) set it for every server.

Actually Magnus, in our opinion the CLIENTID 'should' be associated with the service, and not the default value, as soon as possible. While they might be 'derived' from the default, (to help ensure that the incoming/outgoing have the same value), once established, in the same way settings like hostname etc, storing it with the service ensures that it doesn't change, even if for some reason, the default was regenerated. The user should not have to reset all the services again, to the CLIENTID that they might have approved for use with their service provider. While it 'can' be looked as an optimization to use the default, there are reasons to keep it stored at the service level for persistence.

In the end you have the final say/recommendation, but we 'suggest' that once established from the default, it is stored in the service prefs for safety/stability, along with any other settings that shouldn't normally be changed for the lifetime of that service. IMHO, this is a preferred implementation.

Are you okay with that opinion/implementation? If you can comment back, we can bring this to a conclusion.

I understand you want it to never to change - and it wouldn't. Any UI for this would be per server, and a change for that server would be for that server only, not the default value. The default will of never be regenerated.

As the proposed code is now, a user could go into the config editor and "reset" mail.server.default.clientid and on next startup you would then be sending a different clientid for imap. It wouldn'd affect smtp if you did that I think. But it would if you reset mail.smptserver.default.clientid.

Edit: I stated this wrong. That is what would happen with Magnus's proposed way. With the code the way it is proposed by LinuxMagic, this would not happen. Resetting the "default" would not affect the individual server settings. Also, the individual server setting could be set to custom values (non-default) without the user having to define new prefs. It wouldn't preclude a future UI to set them either.

Which is exactly what you would not want to happen.. ".. you would then be sending a different clientid for imap ..". This could effectively lock you out of your service provider, or be detected as an attack if your IMAP started sending a different clientid... requiring a reset at each provider to allow the new key.. By storing the CLIENTID at the service level, whether it is the same or different from the default, if the user went in and used the config editor. (Or manually modified the prefs file) the existing 'server' definitions would still use the one that is in the per server prefs, eg the one they may have 'authorized' at their service provider.

Anyways, on the road this week, but when consensus is arrived on this topic, the patch can be finalized to conform with that decision. All other aspects appear to be resolved, agreed?

(In reply to gene smith from comment #151)

As the proposed code is now, a user could go into the config editor and "reset" mail.server.default.clientid ...

That voids the warranty ... and there will be dragons ;-)

Note: You can also edit certain Calendar/Lightning preferences and lose all your calendar entries in one hit.

(In reply to DevTeam from comment #152)
For any of this to be a problem the user would have to specify different clientid's for more than one server, and then go in to the config editor and change the defaults. This case is unlikely and clearly not supported. You can mess you your account badly by changing prefs there if you don't understand the consequences.

Comment on attachment 9062640 [details] [diff] [review]
clientid-thunderbird-full-may-3-2019.diff

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

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +77,5 @@
> +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response
> +smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent. Server responded: %s
> +
> +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response
> +smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail. Server responded: %s

to the CLIENTID
The server responded:

::: mailnews/base/util/mailnewsMigrator.js
@@ +52,5 @@
> + */
> +function MigrateProfileClientid() {
> +  // First try to fetch both the server and smtpserver default clientid prefs.
> +  var server_default = Services.prefs.getCharPref("mail.server.default.clientid");
> +  var smtpserver_default = Services.prefs.getCharPref("mail.smtpserver.default.clientid");

please use let instead of var

@@ +71,5 @@
>      // comma-separated list of all accounts.
>      var accounts = Services.prefs.getCharPref("mail.accountmanager.accounts")
>          .split(",");
> +    // grab the default server clientid
> +    var def_server_clientid = Services.prefs.getCharPref("mail.server.default.clientid");

please also: camelCase naming of vars
Attachment #9062640 - Flags: review?(mkmelin+mozilla)
Attachment #9062640 - Flags: review?(jorgk)

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

(In reply to DevTeam from comment #152)
For any of this to be a problem the user would have to specify different clientid's for more than one server, and then go in to the config editor and change the defaults. This case is unlikely and clearly not supported. You can mess you your account badly by changing prefs there if you don't understand the consequences.

If tl;dr, see last paragraph.

Say you have 3 account that require a clientid. One may accept the UUID that we generate. Another may require your birth date for the client id and another may want your name for the string. (The last two are probably not good examples but the draft RFC does not define what the clientid string has to be from what I understand.)

So with the LinuxMagic proposed code you can go into config editor and see the current values and set custom values:

mail.server.default.clientid   <UUID produced automatically by TB>
mail.server.server1.clientid   <UUID produced automatically by TB>  // leave this as is
mail.server.server2.clientid   my-birthdate-yyyy-mm-dd  // changed from original UUID value
mail.server.server3.clientid   gene.smith               // changed from original UUID value
mail.server.server4.clientid   <UUID produced automatically by TB> // this one may not even support clientid

A similar set of clientid's will exist and can be changed by the user for mail.smtpserver.*

If Magnus's method is used on account creation you will only see:

mail.server.default.clientid     <UUID produced automatically by TB>
mail.smtpserver.default.clientid <UUID produced automatically by TB>

But you can still add and set a custom value for a specific account with config editor using right-click new, e.g.,

mail.server.server2.clientid    my-birthdate-yyyy-mm-dd
mail.smtpserver.smtp3.clientid  my-birthdate-yyyy-mm-dd

These can also be set to the default when manually created. I have tested this and it does work as described.

If the user accidentally resets the default clientid, custom clientid's will continue to be used for those account(s) and not the new default that will be generated. However, if the user doesn't create the new account specific prefs, the new default will be sent by the accounts which will probably be rejected.

So I disagree with Magnus's statement that "this is clearly not supported" if he means the possible need to have custom clientid's for various accounts. I think I have shown how it is actually supported using both LinuxMagic's way and the somewhat less user friendly way that Magnus prefers. Either way the user has to determine which actual account corresponds with serverX and smtpX.

So ultimately if this clientid thing catches on, an advanced server setting UI should be added to do the setting of a special custom clientid. It may also require a similar setting for SMTP. (The currently hardcoded "TB-UUID" prefix should also be made configurable to the account via UI)

So in conclusion, I think it's probably OK to just set only the defaults as Magnus prefers and not automatically "populate" the server specific prefs with the default since the user can manually do this if it's wanted. If the user accidentally resets the default, as Jorg pointed out, it "voids the warranty".

(In reply to Jorg K (GMT+2) from comment #147)

Comment on attachment 9062640 [details] [diff] [review]
clientid-thunderbird-full-may-3-2019.diff

Review of attachment 9062640 [details] [diff] [review]:

::: mail/components/accountcreation/content/createInBackend.js
@@ +83,5 @@

 outServer.hostname = config.outgoing.hostname;
 outServer.port = config.outgoing.port;
 outServer.authMethod = config.outgoing.auth;
  • // Set outgoing smtpserver clientid from the default.
  • outServer.clientid = Services.prefs.getCharPref("mail.smtpserver.default.clientid");

Quick drive-by comment on the weekend: If there's a macro missing for SMTP,
please create it, or move the existing one to a place where it's visible for
both incoming and outgoing.

Hello Jorg,

I assume you mean the NS_IMPL_SERVERPREF_STR macro which is not available in SMTP?

The issue isn't that the macro isn't available, it's that the macro expands out to a function call to a base class function which isn't derived in the SMTP code.

To be specific the macro is in nsIMsgIncomingServer:

#define NS_IMPL_SERVERPREF_STR(_class, _postfix, _prefname) \
NS_IMETHODIMP                              \
_class::Get##_postfix(nsACString& retval)  \
{                                          \
  return GetCharValue(_prefname, retval);  \
}                                          \
NS_IMETHODIMP                              \
_class::Set##_postfix(const nsACString& chvalue) \
{                                          \
  return SetCharValue(_prefname, chvalue); \
}

The SetCharValue and GetCharValue functions exist in nsMsgIncomingServer which nsSmtpServer does not derive from.

So I can use the macro in SMTP but the code it expands to will produce compile errors.

I could cove the SetCharValue and GetCharValue functions over to the smtp server class... But that seems outside the scope of this task.

Let me know if you want me to do that, I will be posting another patch with all of the requested changes soon.

Flags: needinfo?(jorgk)

Sigh, looks like incoming and outgoing servers don't share any common interface, so "move the existing one to a place where it's visible for both incoming and outgoing" is impossible, see:
https://searchfox.org/comm-central/rev/31ebd78f17e1cd8d486323130d7d939a0046fedf/mailnews/compose/src/nsSmtpServer.h#16

So there are two options: Either copy the macros and what goes with them to nsISmtpServer.idl, or the smaller solution, looking at nsSmtpServer.cpp, there are already member variables for the "pref branches", so you're code in nsSmtpServer::GetClientid() is OK. Sorry, you and Gene already knew that.

Sorry about the confusion, I don't know this stuff by heart. Looking at your patch again, do we need nsSmtpServer::SetClientid() since a default pref is already set up?

Flags: needinfo?(jorgk)

(In reply to Jorg K (GMT+2) from comment #158)

Sigh, looks like incoming and outgoing servers don't share any common interface, so "move the existing one to a place where it's visible for both incoming and outgoing" is impossible, see:
https://searchfox.org/comm-central/rev/31ebd78f17e1cd8d486323130d7d939a0046fedf/mailnews/compose/src/nsSmtpServer.h#16

Hahaha, yeah I realized that too after some failed tests.

So there are two options: Either copy the macros and what goes with them to nsISmtpServer.idl, or the smaller solution, looking at nsSmtpServer.cpp, there are already member variables for the "pref branches", so you're code in nsSmtpServer::GetClientid() is OK. Sorry, you and Gene already knew that.

No worries, I will leave the smtp code as-is.

Sorry about the confusion, I don't know this stuff by heart. Looking at your patch again, do we need nsSmtpServer::SetClientid() since a default pref is already set up?

I think in the future we are leaving open the possibility for a UI to set the clientid, in which case I assume we will be doing something in javascript like:

outServer.clientid = clientid;

When assigning to the clientid member in Javascript the nsSmtpServer::SetClientid function is implicitly called.

At this point in time since we will be using the inherited default clientid the SetClientid function is not explicitly needed... However the GetClientid function is explicitly needed and I feel it would be wrong to only create a getter without a respective setter.

I will leave it up to others to decide whether the Setter should be removed or not, but it does seem like it would make sense to include it when creating a Getter for the same preference.

Sorry again, attribute ACString clientid; in nsISmtpServer.idl of course mandates both setter and getter. You cam make the attribute readonly then you don't need, or better, can't have, a setter.

Maybe that's the way to go for the incoming side, too. Or do you need to set a server specific client ID in JS code? I thought we just set the default.

(In reply to Jorg K (GMT+2) from comment #160)

Sorry again, attribute ACString clientid; in nsISmtpServer.idl of course mandates both setter and getter. You cam make the attribute readonly then you don't need, or better, can't have, a setter.

Maybe that's the way to go for the incoming side, too. Or do you need to set a server specific client ID in JS code? I thought we just set the default.

At the current point in time the setter isn't used so it could be readonly, this is true for both IMAP and SMTP.

However, IMAP uses NS_IMPL_SERVERPREF_STR which will define the setter no matter what, so at the very least I would be more interested in leaving the SMTP setter for the sake of consistency with IMAP.

If the consistency with IMAP isn't important, then one other point is: keeping the smtp clientid setter may help to minimize any future development confusion when adding the UI to modify the clientid (assuming that work will be done in the future).

Up to you though, should we keep the setter?

Just posting the patch as-is for now (Jorg: I haven't removed the smtp SetClientid, let me know if you want me to)

I believe we have taken all comments into consideration and the patch is working as intended for me.

The current behaviour is that only the mail.server.default.clientid and mail.smtpserver.default.clientid preferences will be filled and all of the per-user clientids will pull from those two. Since we do not have any UI built to modify the per-user preferences then I understand there cannot be any issues with the current approach unless somebody goes mucking around in their preferences.

If I understand correctly, this should leave the doors open for us to implement a UI around the per-user clientid at some point in the future.

If anybody has any questions or comments please post and I will be happy to answer.

Thanks again everybody

Attachment #9062640 - Attachment is obsolete: true
Attachment #9063094 - Flags: review?(mkmelin+mozilla)
Attachment #9063094 - Flags: review?(jorgk)
Attachment #9063094 - Flags: review?(gds)
Comment on attachment 9063094 [details] [diff] [review]
clientid-thunderbird-full-may-6-2019.diff

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

Looks fine to me except for "poopulate" :)
Didn't apply or test the patch yet. But I did test the last patch that I modified to be just like this and it worked fine.

Edit: At one point I noticed that smtp's SetClientId() wasn't called and I commented it out. It compiled OK but got a linker error "unknown identifier" so I put it back and the build finished. But maybe I did something wrong...

Edit2 Re: comment 164: Dan, I should have read yours and Jorg's comments above closer. However, I would vote for not using readonly since, like you say, incoming always produces a setter and both may be needed in the future. Also, the compiler probably optimized out any function that is never actually called.
Attachment #9063094 - Flags: review?(gds) → review+

(In reply to gene smith from comment #163)

Comment on attachment 9063094 [details] [diff] [review]
clientid-thunderbird-full-may-6-2019.diff

Review of attachment 9063094 [details] [diff] [review]:

Looks fine to me except for "poopulate" :)
Didn't apply or test the patch yet. But I did test the last patch that I
modified to be just like this and it worked fine.

Ahhhh! What a typo! Fixed that now

Edit: At one point I noticed that smtp's SetClientId() wasn't called and I
commented it out. It compiled OK but got a linker error "unknown identifier"
so I put it back and the build finished. But maybe I did something wrong...

This is because the IDL files define an interface contract, so by having 'attribute nsCString clientid' in the IDL file it mandates that a getter and setter both exist in the underlying C++ class, if not you will receive a linker error.

Jorg was saying we can make that into a 'readonly' attribute in the IDL file which would then mandate that only a getter exist, and the SetClientid function wouldn't be necessary.

Attachment #9063094 - Attachment is obsolete: true
Attachment #9063094 - Flags: review?(mkmelin+mozilla)
Attachment #9063094 - Flags: review?(jorgk)
Attachment #9063108 - Flags: review?(mkmelin+mozilla)
Attachment #9063108 - Flags: review?(jorgk)

As you know, we're reformatting our source tree. I completed mailnews/imap last night. Luckily the patch still applied. I've reformatted it a bit now.

Attachment #9063108 - Attachment is obsolete: true
Attachment #9063108 - Flags: review?(mkmelin+mozilla)
Attachment #9063108 - Flags: review?(jorgk)
Attachment #9063146 - Flags: review+
Comment on attachment 9063146 [details] [diff] [review]
clientid-thunderbird-full-may-6-2019.diff - reformatted a little

Here's the reformatting diff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9063108&action=interdiff&newid=9063146&headers=1
Note that interdiff is getting confused in nsImapServerResponseParser.cpp; that file has no change.
Attachment #9063146 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063146 [details] [diff] [review]
clientid-thunderbird-full-may-6-2019.diff - reformatted a little

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

Please add the bug number to the commit message like so:

Bug 1532388 - implement full support for STMP and IMAP CLIENTID. r=gds,jorgk,mkmelin

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +76,5 @@
>     */
>    attribute ACString type;
>  
>    /**
> +   * clientid if required

* The CLIENTID to use for this server.
* @see https://tools.ietf.org/html/draft-yu-imap-client-id-01

::: mailnews/compose/public/nsISmtpServer.idl
@@ +35,5 @@
>  
>    /// The username to access the server with (if required)
>    attribute ACString username;
>  
> +  /// The clientid to access the server with (if required)

CLIENTID upper cased I guess. And reference https://tools.ietf.org/html/draft-storey-smtp-client-id-05.

Or, I see now for SMTP it's CID?
Attachment #9063146 - Flags: review?(mkmelin+mozilla) → review+

I can fix those nits before checking this in, unless you want to upload a new patch. Note the typo in the suggested commit message.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e52fa180949c
implement full support for IMAP and SMTP CLIENTID. r=gds,jorgk,mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Yay, landed \o/.

Stuff like this isn't easy to do and you need to keep everyone happy ;-) - This is nothing compared to other bugs, especially bugs affecting UI/UX.

Target Milestone: --- → Thunderbird 68.0

It would be good to make sure we have some tests for this. The fake imap server is here: https://searchfox.org/comm-central/source/mailnews/test/fakeserver/imapd.js

Regressions: 1564096

This must also be mentioned in TB release notes for 68, regardless if it is opt-out (as now) or opt-in.

OS: Unspecified → All
Hardware: Unspecified → All
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2233190fdf4e
Backout the CLIENTID feature (leaving strings in place). a=backout DONTBUILD

I backed this out after much discussion in bug 1564096 about necessary improvements before this can ship in the product.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 68.0 → ---
Depends on: 1565379
Attached patch clientid-patch.patch (obsolete) — Splinter Review

This is what was backed out. It needs to be adjusted and relanded. Attachment 9077184 [details] [diff] from bug 1564096 doesn't appear to be very useful since users would have to set the client ID manually.

It would have allowed to keep the feature in for those that really wanted it, just inactive by default. Yes, clientID would need to be set manually, but that was intentional to fix the privacy problems and surely experimental new features do not need to be user-friendly. I haven't heard any better proposal yet.

Based on the comments during TB Release process, now working on a revised CLIENTID patch and discussions, to address some of the concerns, to simplify the landing process.

Alias: clientid

Hello Thunderbird Team,

I have revised the clientid patch and made a few major adjustments:

  1. I have modified the clientid provisioning algorithm to simply generate a new clientid for each service, with one catch: Services with matching usernames will receive the same clientid.

  2. I have added the clientidEnabled preference which can be toggled off on a per-service basis to control whether the clientid logic will run.

If I understand the issues that were raised, having different clientids across all services should solve the issue of being tracked across accounts, and the only situation where two services will have the same clientid would be when they already share a login username.

Then the ability to disable the system all together is a secondary addition that I don't believe is necessary when different clientids are provisioned on each service, but I'm sure some people may want to turn the system off all together.

Please let me know if there are any additional concerns with the patch, or if there is anything else I can do to prepare the patch for landing.

Thanks!

Attachment #9080752 - Flags: review?(mkmelin+mozilla)
Attachment #9080752 - Flags: review?(jorgk)
Comment on attachment 9080752 [details] [diff] [review]
clientid-thunderbird-full-jul-25-2019.patch

I think, Aceman can look at this first. Are the changes represented by the interdiff?
https://bugzilla.mozilla.org/attachment.cgi?oldid=9077607&action=interdiff&newid=9080752&headers=1

It said it didn't quite work and it seemed to have missed the changes in mailnews.js.
Attachment #9080752 - Flags: review?(jorgk) → review?(acelists)

Hello Jorg,

I'm not quite sure how I can solve the issue with interdiff -- would it be because the old patch was based on a much older version of comm-central?

I could upload a patch/diff indicating all of the changes I made to the original patch if you like?

You certainly don't need to solve the issue with the interdiff, it's old and doesn't work in, hmm, say, 30% of the cases, and 90% of the tricky cases. I've just uploaded the patch with 8 context lines per hunk as we are using, but it doesn't make a difference:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9077607&action=interdiff&newid=9080839&headers=1

The reviewer can compare the patches using some other tool if they like. Perhaps you can just point out any files we need to look at that the interdiff missed apart from mailnews.js.

mail/components/accountcreation/content/createInBackend.js

  • Added logic to populate clientid preference for newly created accounts

mailnews/base/public/nsIMsgIncomingServer.idl

  • Added attribute boolean clientidEnabled for imap

mailnews/base/util/mailnewsMigrator.js

  • Modified how clientids are provisioned to existing smtp + imap services

mailnews/base/util/nsMsgIncomingServer.cpp

  • Added NS_IMPL_SERVERPREF_BOOL for ClientidEnabled

mailnews/compose/public/nsISmtpServer.idl

  • Added attribute boolean clientidEnabled for smtp

mailnews/compose/src/nsSmtpProtocol.cpp

  • Added check around the call to smtpServer->GetClientid to first check whether clientid is enabled via smtpServer->GetClientidEnabled

mailnews/compose/src/nsSmtpServer.cpp

  • Added the nsSmtpServer get/set ClientidEnabled functions

mailnews/imap/src/nsImapProtocol.cpp

  • Added check around the call to server->GetClientid to first check whether clientid is enabled via server->GetClientidEnabled

mailnews/mailnews.js

  • Added default pref for clientidEnabled

The biggest changes are in the javascript in the way the clientid is provisioned for new/existing accounts, the logic in createInBackend.js never existed previously, and the logic in mailnewsMigrator.js was modified to give new clientids to each service and do a best-attempt at matching up services with the same login usernames in order to provision the same clientid for those services.

That should conclude all of the differences between the original patch and this patch, hopefully this helps! :)

(In reply to Dan from comment #180)

  1. I have modified the clientid provisioning algorithm to simply generate a new clientid for each service, with one catch: Services with matching usernames will receive the same clientid.

That's an inacceptable "catch" in my opinion.

He probably again only narrowly sees commercial services and ones where your username contains the full email address. In that case, it would be strange to have the same login (email) on multiple unrelated servers (only on the receiving and outgoing server than belong together). But this is not always the case (some servers do not need the hostname part in the username as it is superfluous). So I agree this can't be assumed and is a dangerous catch.

A username could be "anonymous", or just "john".

Yes, that's what I confirmed.
And you can have a username of 'john' on multiple unrelated servers, yielding different mailboxes (and email addresses).

I still think this feature should be disabled by default. Please require an opt-in.

If you want to enable it by default, there should be a very good justification why this is feature is necessary for the vast majority of users.
Being helpful for service providers shouldn't be a sufficient argument for enabling it, the privacy of users should be the decision factor. Please see the example I gave in the other bug, the client ID feature can be used to track user habits.

How would 'you' suggest that the issue be dealt with. An end user will expect only ONE (1) CLIENTID to represent their device, for both their SMTP and IMAP service. They would not want two alerts, or have to authorize two 'devices' for what they consider a single email account. So we do have to address a way to try to 'link' them... However, you do have a point in cases where the the user name is NOT a full email address. Eg, in theory I 'could' have a username associated (as ace mentioned) at two different providers.

Would using username+hostname, eg if they are BOTH the same, presume them to be the same account, would that be sufficient to ensure different CLIENTID per 'service' in your mind, while still servicing the needs of users to have only one CLIENTID per account for usability?

I will still leave what to 'some' users might think of as the same account having multiple CLIENTID(s).

Scenario:

One outgoing SMTP service, say 'john' at 'service1' defined by the hostname.

Two incoming IMAP services say 'john' at service1 and 'sales' at service1, but both accounts are setup to use the same outgoing service, where while most people think of this as a 'single service'.. there is no real way to 'link' them up together..

Remember, the idea is to make this simple for the average user, and simple for them to say, 'Yeah, I approve that device' like they can do with most other services.

(In reply to DevTeam from comment #191)

One outgoing SMTP service, say 'john' at 'service1' defined by the hostname.
Two incoming IMAP services say 'john' at service1 and 'sales' at service1, but both accounts are setup to use the same outgoing service, where while most people think of this as a 'single service'.. there is no real way to 'link' them up together..

Remember, the idea is to make this simple for the average user, and simple for them to say, 'Yeah, I approve that device' like they can do with most other services.

It must not be made too "simple" at the expense of privacy. Also, the SMTP service can even be at server2 (service2) unrelated to service1.

Also remember the different servers may need different clientID contents (the spec mentions MAC address, licence number, anything) so there is absolutely no general expectation that any common clientID has to be used for different servers, not even for the incoming and outgoing part of what you call 'service' (one username). Those can be 2 separate servers with different clientID requirement even with same provider.
This means, it is not as simple for the user as "I approve that device". You never explained how the user seeing some random string in the server UI can decide that "yes, this is my device" when he never saw that string. It is you trying to hide this clientID from the user making him unaware of it.

(In reply to Kai Engert (:kaie:) from comment #190)

I still think this feature should be disabled by default. Please require an opt-in.

If you want to enable it by default, there should be a very good justification why this is feature is necessary for the vast majority of users.
Being helpful for service providers shouldn't be a sufficient argument for enabling it, the privacy of users should be the decision factor. Please see the example I gave in the other bug, the client ID feature can be used to track user habits.

Side note, for conversation purposes, not technical implementation. We often see Mozilla pushing out technologies, (eg the DNS over HTTPS) where new technologies are 'You can opt out'. How would you say the decisions are made in once case.. (opt-out) vs (opt-in)? Is there any "standard" decision making models here that would help this discussion?

You mention the 'vast majority of users' I would posit that the vast majority of users DO want their provider to be able to determine that it was 'them' that accessed the service or tried to use their password.

And the vast majority DO want the outgoing/incoming services related to each other. And the vast majority or users do want the ability to detect when a new device connects to their account (if the provider supports that ability, they will want to take advantage of this) I STRONGLY believe this technology is in the best interest of the vast majority of users.

But they don't want to hunt and search on how to use the feature. They just want their email client to work out of the box with the latest security innovations.

It is the minority who will want to 'hide' from their legitimate providers, and it is those people who (we get it) you want to give a way to opt-out, just like you might want to give them the way to use a VPN or TOR node .. but given the real world threats out there, I believe it is our responsibility to help the majority of users protect themselves as easily as possible.

Ps... again off topic..
Thunderbird doesn't prevent someone from using the same password at two different services (hey, that might be a good feature request ;), and with 25% of ISP's users using webmail to access their accounts, there are many ways a provider can associate different mailboxes at the same porivder..

(In reply to :aceman from comment #192)

(In reply to DevTeam from comment #191)

One outgoing SMTP service, say 'john' at 'service1' defined by the hostname.
Two incoming IMAP services say 'john' at service1 and 'sales' at service1, but both accounts are setup to use the same outgoing service, where while most people think of this as a 'single service'.. there is no real way to 'link' them up together..

Remember, the idea is to make this simple for the average user, and simple for them to say, 'Yeah, I approve that device' like they can do with most other services.

It must not be made too "simple" at the expense of privacy. Also, the SMTP service can even be at server2 (service2) unrelated to service1.

Also remember the different servers may need different clientID contents (the spec mentions MAC address, licence number, anything) so there is absolutely no general expectation that any common clientID has to be used for different servers, not even for the incoming and outgoing part of what you call 'service' (one username). Those can be 2 separate servers with different clientID requirement even with same provider.
This means, it is not as simple for the user as "I approve that device". You never explained how the user seeing some random string in the server UI can decide that "yes, this is my device" when he never saw that string. It is you trying to hide this clientID from the user making him unaware of it.

Hi Ace, not sure where you get the idea that in any way we are trying to hide the use of CLIENTID from the user, on the contrary, we want CLIENTID to be as open as possible, and for users to go .. 'Woohoo! We now have CLIENTID support in my favourite email client'. But we do want to make it so that if the ISP and email account that they are using supports it, just like say using TLS, the email client will choose the most secure method the ISP offers by default.

Later, when the UI team gets around to further improving 'auto discovery' and expanding on what an 'account' is, that would be a perfect time to go even further with future options like 'Reset CLIENTID for this account', but we are doing thunderbird users an injustice if we wait on those future changes, to roll out an 'out of the box' CLIENTID experience, for those who have email accounts already supporting and protecting email accounts with CLIENTID.

(In reply to Kai Engert (:kaie:) from comment #190)

I still think this feature should be disabled by default. Please require an opt-in.

If you want to enable it by default, there should be a very good justification why this is feature is necessary for the vast majority of users.
Being helpful for service providers shouldn't be a sufficient argument for enabling it, the privacy of users should be the decision factor. Please see the example I gave in the other bug, the client ID feature can be used to track user habits.

Hello Kai,

The issue you described in the other ticket with regard to deriving user habits from the clientid (pasted below for ease)

You presented an alternative suggestion, to use a new random client ID for each new account created.

By introducing it, you will allow the server to track which devices that the user used over time, and this could be tied to derive user habits. For example, the server could learn which device is used during work hours, and which other device during weekends. With that, it could conclude that the user was at work at midnight when a specific email was sent. This sounds like information to me that the server shouldn't have, and it's more information than TB currently gives to the email server.

The habits you describe being which device is being used throughout the day, would that information not also be derivable in something like the HELO argument of the SMTP session?

I see in the SMTP code it attempts to use the hostname as the argument to the helo command, with a fallback on the local IPv6 for the connection, wouldn't the change in these items be trackable and therefore allow the server operator to derive that you were using a different device throughout the day?

Please excuse any ignorance I may exhibit in my questions above, and please correct me anywhere I am mistaken, I am just looking to establish a common ground of understanding and eliminate any misconceptions that we may have.

(In reply to DevTeam from comment #194)

Hi Ace, not sure where you get the idea that in any way we are trying to hide the use of CLIENTID from the user, on the contrary, we want CLIENTID to be as open as possible, and for users to go .. 'Woohoo! We now have CLIENTID support in my favourite email client'. But we do want to make it so that if the ISP and email account that they are using supports it, just like say using TLS, the email client will choose the most secure method the ISP offers by default.

I got the idea from the fact that there will be no indication in the UI that clientID is in use and the user will never see its actual value, unless he delves into prefs. But without seeing that string, the user can never decide in the server UI that that connection was from "his device".
So the user can't wow that TB now supports clientID as he does not even know it is in operation.
It seems it is only you (the company) that will have something to 'wow' when you can claim the clientID has any merit by advertising that Thunderbird is supporting it.

And still you are pushing a solution into Thunderbird that only covers one specific usecase of clientID (a random string pretending to by any device ID) for a specific server, while the spec allows for a much broader range of uses which will not be supported in Thunderbird, if the feature is hidden from the user as you want it.

(In reply to :aceman from comment #196)

(In reply to DevTeam from comment #194)

Hi Ace, not sure where you get the idea that in any way we are trying to hide the use of CLIENTID from the user, on the contrary, we want CLIENTID to be as open as possible, and for users to go .. 'Woohoo! We now have CLIENTID support in my favourite email client'. But we do want to make it so that if the ISP and email account that they are using supports it, just like say using TLS, the email client will choose the most secure method the ISP offers by default.

I got the idea from the fact that there will be no indication in the UI that clientID is in use and the user will never see its actual value, unless he delves into prefs. But without seeing that string, the user can never decide in the server UI that that connection was from "his device".
So the user can't wow that TB now supports clientID as he does not even know it is in operation.
It seems it is only you (the company) that will have something to 'wow' when you can claim the clientID has any merit by advertising that Thunderbird is supporting it.

And still you are pushing a solution into Thunderbird that only covers one specific usecase of clientID (a random string pretending to by any device ID) for a specific server, while the spec allows for a much broader range of uses which will not be supported in Thunderbird, if the feature is hidden from the user as you want it.

Ace, wasn't sure if I should respond, the last thing we want to see is a discussion of the patch turning into a 'flame' war, and a lot of your comments do seem 'accusing' and personal in tone. It does seem that you still have some misunderstandings on the CLIENTID (it is up to the client what type of CLIENTID they wish to use, servers by definition are designed to accept all types that conform to the 'spec', but if some other software vendor wanted to use a software license they can, UUID was the one that most of the TB involved in this discussion preferred)

If you feel you need more clarity on how CLIENTID can work, or that it is meant for every MTA and email client to use, or on the specs themselves, feel free to reach out and we can have an offline discussion. We would "like" TB to be a leader in helping make the world a more secure place, and this is why we are freely contributing our time and efforts.

But can we keep the conversation to the technical implementation details, and suggestions on how to make this work the best for all concerned. I am sure you have valuable contributions to add to the conversation, but if you want your contributions to be taken seriously, let's keep any personal attacks and slinging insults out of the conversation.

I trully hope "every" email platform will be able to take advantage of finally updating the way MTU->MTA communication occurs in legacy SMTP/IMAP protocols.

There are lots of improvements that can be made in the future (for the next version) but lets find a way to get this ito 68 so that the people who need it don't have to wait any longer..

(In reply to :aceman from comment #196)

And still you are pushing a solution into Thunderbird that only covers one specific use case of clientID (a random string pretending to be any device ID) for a specific server, while the spec allows for a much broader range of uses which will not be supported in Thunderbird, if the feature is hidden from the user as you want it.

With a imap server specific clientid configuration UI (maybe in Advanced server configuration) it must be disabled by default. When enabled, it would default to using the tb generated UUID as currently proposed. Another option would be a custom string to be entered by the user. If custom string chosen, the provider might send the user a custom clientID to enter, maybe via snail mail or SMS, and the user enters it manually. Maybe a way is possible to associated the imap clientID to a smtp account without needing the same "advanced" UI in the smtp server setup.

In any case, without a new UI setup features, the non-UUID clientID can be manually entered for imap and smtp using the config editor entries as I described in comment 156. The only thing currently lacking is a pref to enable/disable clientID. The currently proposed "config editor" only setup would definitely be more "hidden" from the user than a possible UI, but since disabled by default, probably not a problem.

(In reply to gene smith from comment #198)

(In reply to :aceman from comment #196)

And still you are pushing a solution into Thunderbird that only covers one specific use case of clientID (a random string pretending to be any device ID) for a specific server, while the spec allows for a much broader range of uses which will not be supported in Thunderbird, if the feature is hidden from the user as you want it.

With a imap server specific clientid configuration UI (maybe in Advanced server configuration) it must be disabled by default. When enabled, it would default to using the tb generated UUID as currently proposed. Another option would be a custom string to be entered by the user. If custom string chosen, the provider might send the user a custom clientID to enter, maybe via snail mail or SMS, and the user enters it manually. Maybe a way is possible to associated the imap clientID to a smtp account without needing the same "advanced" UI in the smtp server setup.

In any case, without a new UI setup features, the non-UUID clientID can be manually entered for imap and smtp using the config editor entries as I described in comment 156. The only thing currently lacking is a pref to enable/disable clientID. The currently proposed "config editor" only setup would definitely be more "hidden" from the user than a possible UI, but since disabled by default, probably not a problem.

Agreed Gene, and of course in the future when other enhancements are being done to the UI in client setup, more capabilities can be added, but for 68, we don't want to open up doing UI changes. For the record, the latest patch did include an option to enable/disable clientID, but Kai did have a point, that the best effort to make each 'account' have different UUID didn't take into account the case of a username only, instead of an email address, and for that case the check probably will have to be username+hostname the same, then you can treat them as the same account, otherwise use different UUID seems logical for updating legacy services, per Magnus original requests. I think Dan has almost polished off a patch with that correction.

Hello Thunderbird Team,

I have prepared the next iteration of the patch.

This iteration will always generate a new clientid for outgoing (smtp) services, and will only re-use outgoing clientids for incoming (imap) services if an outgoing service can be found with identical hostname and username.

I believe this should address the concerns raised earlier with usernames not being enough to tie together services.

If there are any issues or additional concerns please let me know.

Attachment #9082144 - Flags: review?(kaie)
Attachment #9082144 - Flags: review?(acelists)

(In reply to DevTeam from comment #197)

Ace, wasn't sure if I should respond, the last thing we want to see is a discussion of the patch turning into a 'flame' war, and a lot of your comments do seem 'accusing' and personal in tone. It does seem that you still have some misunderstandings on the CLIENTID (it is up to the client what type of CLIENTID they wish to use, servers by definition are designed to accept all types that conform to the 'spec', but if some other

Now that is something new, I don't see that in the spec. I always assumed it is the server which defined which clientID type it will accept.
Point me where in the spec your interpretation can be found please.

But can we keep the conversation to the technical implementation details, and suggestions on how to make this work the best for all concerned. I am sure you have valuable contributions to add to the conversation, but if you want your contributions to be taken seriously, let's keep any personal attacks and slinging insults out of the conversation.

Sorry about sounding personally insulting. But it insulting to me how you always take great care to actually not answer any questions, you just always repeat how great the clientID is and users are just waiting for this goodness. At most you propose to take discussion about its usefulness to other channels. But if we TB developers do not understand it, it appears logical to me to discuss it here first. Only after the benefit of clientID is clear, we can proceed to the actual implementation.

For me, you haven't proved any actual benefit of using clientID yet and manage to avoid to address most issues we raised. You just downplay the privacy problems and call the privacy-conscious people a minority blocking your great feature.

Many comments here, but let me just add a short one. Yes, username only is not enough, it needs to be the username + provider. Instead of exact hostname you could use the nsIEffectiveTLDService to match username + hostname tld.

I don't have any concern with the provider tracking the usage internally - that is pretty far from what I would consider privacy. The provider can basically do it already, they already typically also have access to all your email communications, and it's in the end a provider you yourself set up. Trying to somehow hide your usage from them is pretty futile and non-productive. There is also the angle that A) it's a paid provider (and they would thus have plenty of real info about you like your credit card), or B) it's a free provider - in this case, the alternative that they only would provide a web interface is privacy wise much worse in so many ways....

Attachment #9080752 - Attachment is obsolete: true
Attachment #9080752 - Flags: review?(mkmelin+mozilla)
Attachment #9080752 - Flags: review?(acelists)
Comment on attachment 9082144 [details] [diff] [review]
clientid-thunderbird-full-jul-31-2019.patch

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

Please see if the same issue applies more than once, didn't comment on every occurrence

::: mail/components/accountcreation/content/createInBackend.js
@@ +17,4 @@
>   * @return {nsIMsgAccount} - the newly created account
>   */
>  function createAccountInBackend(config) {
> +  // grab the uuid generator

please remove, it's pretty obvious that's what it does ;)

@@ +28,5 @@
>    inServer.authMethod = config.incoming.auth;
>    inServer.password = config.incoming.password;
> +  // this new clientid will be used for the outgoing server,
> +  // and will be applied to the incoming server only if the
> +  // incoming username and hostname match the outgoing

Please capitalize, and add period at the end of senteces. Make use of the 80ch. Maybe CLIENTID capitalizion too?

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +81,5 @@
> +   * @see https://tools.ietf.org/html/draft-yu-imap-client-id-01
> +   */
> +  attribute ACString clientid;
> +
> +  /* whether the clientid feature above is enabled */

Use /** */ style documentation comment so it would be included in generated documentation. Also, capitalization and ending .

::: mailnews/base/util/mailnewsMigrator.js
@@ +52,5 @@
> + */
> +function MigrateProfileClientid() {
> +  // grab the uuid generator
> +  let uuidGen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +  // comma-separated list of all accounts.

remove

@@ +54,5 @@
> +  // grab the uuid generator
> +  let uuidGen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +  // comma-separated list of all accounts.
> +  let accounts = Services.prefs.getCharPref("mail.accountmanager.accounts")
> +      .split(",");

more like accountIds perhaps?

@@ +64,5 @@
> +  // now walk all smtp servers again and generate and cache any
> +  // missing clientids, reusing cached clientids if possible
> +  for (let i = 0; i < smtpservers.length; i++) {
> +    if (!smtpservers[i])
> +      continue;

when would that happen?

@@ +67,5 @@
> +    if (!smtpservers[i])
> +      continue;
> +    let server = "mail.smtpserver." + smtpservers[i] + ".";
> +    if (!Services.prefs.prefHasUserValue(server + "clientid") ||
> +        !Services.prefs.getCharPref(server + "clientid").length) {

just check !Services.prefs.getCharPref(server + "clientid") - no need for the length since empty string is falsy

@@ +80,5 @@
> +    clientidCache[combinedKey] = Services.prefs.getCharPref(server + "clientid");
> +  }
> +  // now walk all imap accounts again and generate and cache any
> +  // missing clientids, reusing cached clientids if possible
> +  for (let i = 0; i < accounts.length; i++) {

you could just do
for (let accountKey of accountKeys)

::: mailnews/compose/public/nsISmtpServer.idl
@@ +40,5 @@
> +  /// @see https://tools.ietf.org/html/draft-storey-smtp-client-id-05
> +  attribute ACString clientid;
> +
> +  /* whether the clientid feature above is enabled */
> +  attribute boolean clientidEnabled;

for both of these comments, /** */ style please

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5574,5 @@
>  
> +nsresult nsImapProtocol::ClientID() {
> +  IncrementCommandTagNumber();
> +  nsCString command(GetServerCommandTag());
> +  command += " CLIENTID TB-UUID ";

shoudn't this be CLIENTID UUID
Attachment #9082144 - Flags: review?(kaie)
Attachment #9082144 - Flags: review?(acelists)

Hello Magnus,

Thank you for taking the time to review the patch.

I have tended to your comments and applied the same fixes across all occurrences of any issues that I could find.

You mentioned that we could use nsIEffectiveTLDService to match the hostname tld -- if it's not that important then we would prefer to just retain the simple string comparison for now.

But if you would really like to see this change then it would help significantly if you could provide some insight/examples on how we might use this class to achieve the comparison. I assume we would use one of the APIs getBaseDomain or getPublicSuffix, or a combination of them?

And lastly, with regard to your comment about switching TB-UUID to UUID, I believe this topic was brought up previously and we would prefer to retain the TB-UUID if possible.

Thanks again for taking the time to review this and If there are any additional questions or issues with this latest patch please let me know.

Attachment #9082144 - Attachment is obsolete: true
Attachment #9082328 - Flags: review?(mkmelin+mozilla)

(In reply to Dan from comment #204)

And lastly, with regard to your comment about switching TB-UUID to UUID, I believe this topic was brought up previously and we would prefer to retain the TB-UUID if possible.

I don't recall that? The spec only talks about UUID.

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

(In reply to Dan from comment #204)

And lastly, with regard to your comment about switching TB-UUID to UUID, I believe this topic was brought up previously and we would prefer to retain the TB-UUID if possible.

I don't recall that? The spec only talks about UUID.

For the record, the RFC only 'suggests' some ideas for what could be used for identifier types. It is up to the client to decide what it wants to use.
By using TB-UUID, it makes it clear that this is the UUID standard in use by Thunderbird. Server(s) would typically accept 'any' CLIENTID TYPE, as long as it follows the spec.. eg. from RFC..

  1. client identity type: A string identifying the identity type the
    client is providing. It MUST be between 1 and 16 characters and
    comprised of only alphanumeric and dash characters.

client-id-type = 1*16 ALPHA / DIGIT / "-"
;; alphanumeric with dash character

And while UUID is a fairly clear set of standards elsewhere, it IS possible for a client to have different ideas of the format they choose to call 'UUID' in their implementation, so this avoids confusion.

It also has the added benefit that a person in 'theory' could say, I only use thunderbird clients, so only accept connections that present the CLIENTID TYPE used by Thunderbird (If the MTA happens to support that ability). Or the added benefit of alerting the customer to 'An attempt was made to access your mailbox. (Claims ThunderBird)', eg differentiating from all other clients that might present a generic UUID as the label for the CLIENTID TYPE they use.

Does that make more sense now?

The CLIENTID spec is flexible enough to allow for any conceivable future concept of 'type'. But IF at some time in the future, if the IETF spec goes along a path that vendors can 'register' a new type string so two different vendors don't use the same string, this allows for such future directions from the start.

No, it does not make any sense. If the client can send anything, then TB can just send the "LICENSE" type with some fixed string of "free" for all profiles and all users and it would fulfill the spec.

Also, if each client sends any type it wants then the user will see a mess of clientID strings listed in the server UI (for those servers that present it) and is somehow supposed to know what device each one represents? Remember this is an experimental feature thus the server must explain to the user what those clientIDs are. And it will not know, if any client sends what it wants.
Also remember the user does not even know TB sends any clientID and what value it is, so he won't recognize any of the strings presented on the server.
Also read section 5.1 of the spec where the server may reject any clientID types that it does not know/recognize/support. It can be assumed that no server recognizes TB-UUID (and will not for a long time).

(In reply to :aceman from comment #207)

No, it does not make any sense. If the client can send anything, then TB can just send the "LICENSE" type with some fixed string of "free" for all profiles and all users and it would fulfill the spec.

No, the spec CLEARLY indicates that the CLIENTID SHOULD represent a uniqueness. Whether the TYPE they choose is 'MS-LICENSE' or 'HW-UUID' or 'SW-UUID', this is an indicator that the email client vender/developer chooses to indicate a sense of what they are using to define the 'unique string' of their preference. If the developer wants to, they can share more information about what a 'MS-LICENSE' format is, or a company that produces both an email client and a server, might want to do more based on the information presented.

Also, if each client sends any type it wants then the user will see a mess of clientID strings listed in the server UI (for those servers that present it) and is somehow supposed to know what device each one represents? Remember this is an experimental feature thus the server must explain to the user what those clientIDs are. And it will not know, if any client sends what it wants.

A server still has the ability to present to the user 'A new device attempted to connect' and they will see the string is different from ones previously permitted/seen. This is still valuable information. We HOPE that server implementations MIGHT take advantage of the 'TYPE' to give clearer messages on 'known' CLIENTID TYPES, eg TB-UUID wil quickly and easily be understood to be different than MS-LICENSE, and I expect a end user will also see the TYPE as a clue to it's origin.

Also remember the user does not even know TB sends any clientID and what value it is, so he won't recognize any of the strings presented on the server.
Also read section 5.1 of the spec where the server may reject any clientID types that it does not know/recognize/support. It can be assumed that no server recognizes TB-UUID (and will not for a long time).

IT will of course, be up to the MTA/Email Server on how they make this apparent to their users. But typically, I expect a user would see history of strings, that he can see as unique, eg last connect from device that identifies itself as '<string>', total times that device connected etc.

And in an IETF the "MAY" simply means it is technically possible, not that it would be expected to be used or recommended, but we can foresee a use case where a business might want to say, 'I only permit my staff to use these types of email clients', so the RFC spec allows for those use cases, even if it isn't typical. Remember, the spec says 'accept' anything presented in general, but allows for special cases. So, unless a person, or email operator has a very specific reason for affecting connection behavior based on a type of email client, they will be accepting any CLIENTID TYPE and CLIENTID, as long as it is formatted per the specification. (eg length and character set). But again, that will be an agreed apon behaviour between the user and his provider.

Still sounds like a bad idea not to just use UUID, we're all about standards after all - personally I think the spec should only support that, the other alternatives like license keys can easily be made to be UUIDs too and the rest will just have enough privacy problems that you won't get it implemented. Supporting cases like forcing users to use certain clients is against our philosophy. I can see that there would be some value in the server being able to say from which application a possibly fraudulent attempt was made, but having that be part of the command is just wrong. You need the spec to have another, optional proper field for that and not rely on heuristics and require each implementer to figure out tb- is somehow related to Thunderbird.

Comment on attachment 9082328 [details] [diff] [review]
clientid-thunderbird-full-aug-1-2019.patch

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

Re nsIEffectiveTLDService, if you don't use it most imap/smtp hosts would not get the same clientid (sure cases like mail.example.com with dual support, but the normal cases are imap.example.com + smtp.example.com). Looks like baseDomain is what you want. You just call Services.eTLD.getBaseDomainFromHost(hostname);

::: mailnews/base/util/mailnewsMigrator.js
@@ +53,5 @@
> +function MigrateProfileClientid() {
> +  let uuidGen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +  // Comma-separated list of all account ids.
> +  let accountKeys = Services.prefs.getCharPref("mail.accountmanager.accounts")
> +      .split(",");

you can return early if there are no accountKeys

@@ +55,5 @@
> +  // Comma-separated list of all account ids.
> +  let accountKeys = Services.prefs.getCharPref("mail.accountmanager.accounts")
> +      .split(",");
> +  // Comma-separated list of all smtp servers.
> +  let smtpServerKeys = Services.prefs.getCharPref("mail.smtpservers").split(",");

likewise

@@ +58,5 @@
> +  // Comma-separated list of all smtp servers.
> +  let smtpServerKeys = Services.prefs.getCharPref("mail.smtpservers").split(",");
> +  // A cache to allow CLIENTIDS to be stored and shared across services that
> +  // share a username and hostname.
> +  let clientidCache = {};

could just use new Map()

@@ +63,5 @@
> +  // Now walk all smtp servers and generate any missing CLIENTIDS, caching all
> +  // CLIENTIDS along the way to be reused for matching imap servers if possible.
> +  for (let smtpServerKey of smtpServerKeys) {
> +    if (!smtpServerKey)
> +      continue;

didn't answer the question, but maybe missing accounts was the reason for this?

@@ +82,5 @@
> +  // Now walk all imap accounts and generate any missing CLIENTIDS, reusing
> +  // cached CLIENTIDS if possible.
> +  for (let accountKey of accountKeys) {
> +    if (!accountKey)
> +      continue;

and this

@@ +86,5 @@
> +      continue;
> +    let serverKey = Services.prefs.getCharPref("mail.account." + accountKey +
> +       ".server");
> +    let server = "mail.server." + serverKey + ".";
> +    // Check if this server needs the CLIENTID prefernce to be populated.

typo
Attachment #9082328 - Flags: review?(mkmelin+mozilla)

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

Still sounds like a bad idea not to just use UUID, we're all about standards after all - personally I think the spec should only support that, the other alternatives like license keys can easily be made to be UUIDs too and the rest will just have enough privacy problems that you won't get it implemented. Supporting cases like forcing users to use certain clients is against our philosophy. I can see that there would be some value in the server being able to say from which application a possibly fraudulent attempt was made, but having that be part of the command is just wrong. You need the spec to have another, optional proper field for that and not rely on heuristics and require each implementer to figure out tb- is somehow related to Thunderbird.

At the end of the day, it should be the Thunderbird that chooses what the CLIENTID TYPE should be as long as it falls into the format for TYPE.

client-id-type = 1*16 ALPHA / DIGIT / "-";; alphanumeric with dash character

As Dan pointed out offline, it might even be 'technically' more correct if it was UUIDv2 or UUIDv3 as the case may be. However, I 'personally' see value in expressing that the TYPE is Thunderbird's use of UUID, eg that it was designed to be a UUID that works at the profile/service level, rather than say a UUID that was unique to a piece of hardware. Whatever decision your team makes on this, I am okay with, it is and should be a Thunderbird decision, just that I do see the long term usefulness of using a TYPE that carries more information from day 1. Nothing stops this from being reconsidered in the future.

Yes, there are probably otherways for it to be known what the 'UUID' definition is that TB uses. As to your suggestion that another option to the spec be added to allow sending more client specific options, an interesting thought, but probably better ways to accomplish that, however it was the intent of the spec to allow vendor's to use the TYPE to make a more explicit publication of the 'nature' of the string passed as CLIENTID

(In reply to DevTeam from comment #211)

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

Still sounds like a bad idea not to just use UUID, we're all about standards after all - personally I think the spec should only support that, the other alternatives like license keys can easily be made to be UUIDs too and the rest will just have enough privacy problems that you won't get it implemented. Supporting cases like forcing users to use certain clients is against our philosophy. I can see that there would be some value in the server being able to say from which application a possibly fraudulent attempt was made, but having that be part of the command is just wrong. You need the spec to have another, optional proper field for that and not rely on heuristics and require each implementer to figure out tb- is somehow related to Thunderbird.

At the end of the day, it should be the Thunderbird that chooses what the CLIENTID TYPE should be as long as it falls into the format for TYPE.

client-id-type = 1*16 ALPHA / DIGIT / "-";; alphanumeric with dash character

As Dan pointed out offline, it might even be 'technically' more correct if it was UUIDv2 or UUIDv3 as the case may be. However, I 'personally' see value in expressing that the TYPE is Thunderbird's use of UUID, eg that it was designed to be a UUID that works at the profile/service level, rather than say a UUID that was unique to a piece of hardware. Whatever decision your team makes on this, I am okay with, it is and should be a Thunderbird decision, just that I do see the long term usefulness of using a TYPE that carries more information from day 1. Nothing stops this from being reconsidered in the future.

Yes, there are probably otherways for it to be known what the 'UUID' definition is that TB uses. As to your suggestion that another option to the spec be added to allow sending more client specific options, an interesting thought, but probably better ways to accomplish that, however it was the intent of the spec to allow vendor's to use the TYPE to make a more explicit publication of the 'nature' of the string passed as CLIENTID

PS, Addendum .. a quick reread on section 6 Client Identity Types might also assist in this thread. I goes over more eloquently the recommended use of TYPE, eg..
"While there is no pre-defined list of client identity type defined by
this RFC, and all IMAP servers should be prepared to accept any form
suggested that IMAP client developers carefully consider the name of
the client identity type. For example, rather that using a
client identity type of UUID, consider the advantages of making it
more distinct, eg "<product_short_code>UUID"."

Hello Magnus

I am working to implement the changes you suggested, first off let me apologize for not answering your question regarding the comment.

I noticed the comment that you inquired about was actually outdated, the comment didn't really reflect what was happening in the code which I assumed was the confusion.

To answer your question:

Yes the purpose of the code is to find any accounts which do not have a clientid populated and attempt to either generate a new unique ID or attempt to pair IMAP services with SMTP services.

The model that was decided upon involved always generating a new clientid for SMTP servers, but to attempt to re-use SMTP clientids on IMAP services (but not vice versa) which had matching hostnames + usernames.

As I understand it, the reason for this is that it is likely for one SMTP server to correspond to many IMAP servers, but the inverse is unlikely.


On a separate note, I am implementing the call to Services.eTLD.getBaseDomainFromHost and I have come across situations where people may have assigned other addresses to the hostname which may not be a FQDN and result in getBaseDomainFromHost throwing exceptions.

For example, if the user has set an IP address as the hostname then getBaseDomainFromHost will throw the exception:

    [Exception... "Component returned failure code: 0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS) [nsIEffectiveTLDService.getBaseDomainFromHost]"  nsresult: "0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS)"  location: "JS frame :: resource:///modules/mailnewsMigrator.js :: MigrateProfileClientid :: line 102"  data: no]
    -- Exception object --
    + toString (function) 3 lines
    + name (string) 'NS_ERROR_HOST_IS_IP_ADDRESS'
    + message (string) 'Component returned failure code: 0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS) [nsIEffectiveTLDService.getBaseDomainFromHost]'
    + result (number) 2152398929
    + filename (string) 'resource:///modules/mailnewsMigrator.js'
    + lineNumber (number) 102
    + columnNumber (number) 0
    + data (object) null
    + stack (string) 257 chars
    + location (object) JS frame :: resource:///modules/mailnewsMigrator.js :: MigrateProfileClientid :: line 102
    *
    -- Stack Trace --
    MigrateProfileClientid@resource:///modules/mailnewsMigrator.js:102:50
    migrateMailnews@resource:///modules/mailnewsMigrator.js:25:27
    OnLoadMessenger@chrome://messenger/content/msgMail3PaneWindow.js:429:18
    onload@chrome://messenger/content/messenger.xul:1:16

And if the user has set the hostname to something like 'mail' (local dns entry?) then this exception is thrown:

    [Exception... "Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIEffectiveTLDService.getBaseDomainFromHost]"  nsresult: "0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS)"  location: "JS frame :: resource:///modules/mailnewsMigrator.js :: MigrateProfileClientid :: line 82"  data: no]
    -- Exception object --
    + toString (function) 3 lines
    + name (string) 'NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS'
    + message (string) 'Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIEffectiveTLDService.getBaseDomainFromHost]'
    + result (number) 2152398928
    + filename (string) 'resource:///modules/mailnewsMigrator.js'
    + lineNumber (number) 82
    + columnNumber (number) 0
    + data (object) null
    + stack (string) 256 chars
    + location (object) JS frame :: resource:///modules/mailnewsMigrator.js :: MigrateProfileClientid :: line 82
    *
    -- Stack Trace --
    MigrateProfileClientid@resource:///modules/mailnewsMigrator.js:82:48
    migrateMailnews@resource:///modules/mailnewsMigrator.js:25:27
    OnLoadMessenger@chrome://messenger/content/msgMail3PaneWindow.js:429:18
    onload@chrome://messenger/content/messenger.xul:1:16

How would you suggest handling this situations?

I assume there must be a function/class somewhere which would help me to determine whether the hostname is a viable argument to getBaseDomainFromHost? Or perhaps just a simple try/catch to handle the exception?

Any input would be appreciated before I post the next patch,

Thanks!

Flags: needinfo?(mkmelin+mozilla)

Hello Thunderbird Team,

I have attached the next iteration of the patch.

In this patch I have tended to all the comments that Magnus has made above, and I have opted to use a simple try/catch to handle the exceptions that getBaseDomainFromHost may throw.

I have also switched TB-UUID to simply UUID as requested.

If there are any additional concerns or issues with this iteration please let me know.

Thanks.

Attachment #9082328 - Attachment is obsolete: true
Attachment #9082784 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9082784 [details] [diff] [review]
clientid-thunderbird-full-aug-2-2019.patch

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

Can you add a test for the migration - around https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/test/unit/test_accountMigration.js#62

Also add logic to only do the migration once, see https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/util/mailnewsMigrator.js#23 for an example

::: mail/components/accountcreation/content/createInBackend.js
@@ +29,5 @@
> +  // This new CLIENTID is for the outgoing server, and will be applied to the
> +  // incoming only if the incoming username and hostname match the outgoing.
> +  let newOutgoingClientid = uuidGen.generateUUID().toString().replace(/[{}]/g, "");
> +  if (config.incoming.username == config.outgoing.username &&
> +      config.incoming.hostname == config.outgoing.hostname) {

I think you want to use gettBaseDomainFrom here too

::: mailnews/base/util/mailnewsMigrator.js
@@ +56,5 @@
> +  let accountKeys = Services.prefs.getCharPref("mail.accountmanager.accounts")
> +      .split(",");
> +  // Comma-separated list of all smtp servers.
> +  let smtpServerKeys = Services.prefs.getCharPref("mail.smtpservers").split(",");
> +  if (!accountKeys.length && !smtpServerKeys.length) {

nit: we prefer checking length == 0

@@ +66,5 @@
> +  // Now walk all smtp servers and generate any missing CLIENTIDS, caching all
> +  // CLIENTIDS along the way to be reused for matching imap servers if possible.
> +  for (let smtpServerKey of smtpServerKeys) {
> +    if (!smtpServerKey)
> +      continue;

I don't think this is ever the case now

@@ +88,5 @@
> +    clientidCache.set(combinedKey, Services.prefs.getCharPref(server + "clientid"));
> +  }
> +  if (!accountKeys.length) {
> +    return;
> +  }

this check can also go, we never get here when there's no accounts

@@ +93,5 @@
> +  // Now walk all imap accounts and generate any missing CLIENTIDS, reusing
> +  // cached CLIENTIDS if possible.
> +  for (let accountKey of accountKeys) {
> +    if (!accountKey)
> +      continue;

not needed
Attachment #9082784 - Flags: review?(mkmelin+mozilla) → feedback+
Flags: needinfo?(mkmelin+mozilla)
Attachment #9084884 - Flags: review?(mkmelin+mozilla)

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

Comment on attachment 9082784 [details] [diff] [review]
clientid-thunderbird-full-aug-2-2019.patch

Review of attachment 9082784 [details] [diff] [review]:

Can you add a test for the migration - around
https://searchfox.org/comm-central/rev/
99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/test/unit/
test_accountMigration.js#62

I tried to implement some form of test, but I'm not sure if it matches what you expected.

Let me know if there is anything else I can do to improve the test.

Also add logic to only do the migration once, see
https://searchfox.org/comm-central/rev/
99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/util/mailnewsMigrator.
js#23 for an example

Hmm.. I'm really not sure what you mean by this.

The migration already only runs once per startup of Thunderbird, and the clientid preferences are only populated if they are missing or empty.

I'm also not really sure how the piece of code you linked is related to ensuring the migration only runs once.

There must be something I am misunderstanding here?

::: mail/components/accountcreation/content/createInBackend.js
@@ +29,5 @@

  • // This new CLIENTID is for the outgoing server, and will be applied to the
  • // incoming only if the incoming username and hostname match the outgoing.
  • let newOutgoingClientid = uuidGen.generateUUID().toString().replace(/[{}]/g, "");
  • if (config.incoming.username == config.outgoing.username &&
  •  config.incoming.hostname == config.outgoing.hostname) {
    

I think you want to use gettBaseDomainFrom here too

Oops, fixed.

::: mailnews/base/util/mailnewsMigrator.js
@@ +56,5 @@

  • let accountKeys = Services.prefs.getCharPref("mail.accountmanager.accounts")
  •  .split(",");
    
  • // Comma-separated list of all smtp servers.
  • let smtpServerKeys = Services.prefs.getCharPref("mail.smtpservers").split(",");
  • if (!accountKeys.length && !smtpServerKeys.length) {

nit: we prefer checking length == 0

Done

@@ +66,5 @@

  • // Now walk all smtp servers and generate any missing CLIENTIDS, caching all
  • // CLIENTIDS along the way to be reused for matching imap servers if possible.
  • for (let smtpServerKey of smtpServerKeys) {
  • if (!smtpServerKey)
  •  continue;
    

I don't think this is ever the case now

@@ +88,5 @@

  • clientidCache.set(combinedKey, Services.prefs.getCharPref(server + "clientid"));
  • }
  • if (!accountKeys.length) {
  • return;
  • }

this check can also go, we never get here when there's no accounts

@@ +93,5 @@

  • // Now walk all imap accounts and generate any missing CLIENTIDS, reusing
  • // cached CLIENTIDS if possible.
  • for (let accountKey of accountKeys) {
  • if (!accountKey)
  •  continue;
    

not needed

I never realized this but it turns out that javascript split() will return an array with 1 element even when the input string is empty.

This meant that when the list of accounts/smtpservers was an empty string the smtpServerKeys array would still have 1 entry: an empty string (probably the reason the code I copied had this exact !smtpServerKey check, for example: https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/util/mailnewsMigrator.js#53).

I have removed the check for if (!smtpServerKey) in my code and I have modified the code slightly to check the server list string (the one that is passed to .split() ) length to prevent iterating when there are no servers in the list.

So the logic is something like this:

if !accounts AND !smtpServers
    return

if smtpServers.length > 0
    split smtpServers
        iterate smtpServers
            generate smtp clientids and cache for later
        
if accounts.length == 0
    return
    
split accounts
    iterate accounts
        generate new clientids or reuse clientids from cache

I think this should solve the issues you raised, but if there are any problems then let me know.

Thanks.

To enforce an "opt-in" policy, shouldn't these default to false?:

diff -r 57d0ff007e02 -r 3fdfa20d2434 mailnews/mailnews.js
--- a/mailnews/mailnews.js	Fri Aug 02 09:49:30 2019 +0200
+++ b/mailnews/mailnews.js	Fri Aug 02 15:21:56 2019 -0700
@@ -455,6 +455,12 @@
:
:
+pref("mail.server.default.clientidEnabled", true);
+pref("mail.smtpserver.default.clientidEnabled", true);`

But maybe default of opt-out has already been agreed to?

(In reply to gene smith from comment #218)

To enforce an "opt-in" policy, shouldn't these default to false?:

diff -r 57d0ff007e02 -r 3fdfa20d2434 mailnews/mailnews.js
--- a/mailnews/mailnews.js	Fri Aug 02 09:49:30 2019 +0200
+++ b/mailnews/mailnews.js	Fri Aug 02 15:21:56 2019 -0700
@@ -455,6 +455,12 @@
:
:
+pref("mail.server.default.clientidEnabled", true);
+pref("mail.smtpserver.default.clientidEnabled", true);`

But maybe default of opt-out has already been agreed to?

Please correct me if I am mistaken here... I think the agreement was that the feature could be enabled by default as long as different clientids were given to each account/service? Magnus, is this accurate?

Compiled and tested with the Aug 2 patch and it seems to work correctly. I see unique ClienIDs for each account with matching values between imap and smtp. (Don't have any with different user between them.) Also, see a generated UUID for "Local Folders"; is that OK? Don't remember, does this also work for POP (I don't think so).

I tested on cityemail test account which is set to permissive. Will try the Aug 12 patch after Magnus reviews it.

(In reply to gene smith from comment #220)

Compiled and tested with the Aug 2 patch and it seems to work correctly. I see unique ClienIDs for each account with matching values between imap and smtp. (Don't have any with different user between them.) Also, see a generated UUID for "Local Folders"; is that OK? Don't remember, does this also work for POP (I don't think so).

I tested on cityemail test account which is set to permissive. Will try the Aug 12 patch after Magnus reviews it.

I don't think the clientid on Local Folders would effect anything because it should never be used.

We didn't implement anything for POP, only IMAP and SMTP.

I could add a mechanism to prevent populating the clientid for the 'Local Folders' preference if that would be preferred?

(In reply to Dan from comment #221)

I don't think the clientid on Local Folders would effect anything because it should never be used.
We didn't implement anything for POP, only IMAP and SMTP.
I could add a mechanism to prevent populating the clientid for the 'Local Folders' preference if that would be preferred?

Then why do you not generate UUID only for IMAP and SMTP? Why is it created for other server types?

(In reply to Dan from comment #217)

I never realized this but it turns out that javascript split() will return an array with 1 element even when the input string is empty.
This meant that when the list of accounts/smtpservers was an empty string the smtpServerKeys array would still have 1 entry: an empty string (probably the reason the code I copied had this exact !smtpServerKey check, for example: https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/base/util/mailnewsMigrator.js#53).

Why do you access the prefs directly? You are not supposed to do that. Use the proper server methods to set the UUIDs and enumerate servers e.g. via MailServices.accounts .

(In reply to gene smith from comment #218)

To enforce an "opt-in" policy, shouldn't these default to false?:
But maybe default of opt-out has already been agreed to?

I think basically nobody who commented here wants this enabled by default and opt-out, except the author and Magnus. But he is appointed to make the decisions.

See also bug 1565379 and bug 1564096.

Hello Thunderbird team,

As per discussions that were happening over at https://bugzilla.mozilla.org/show_bug.cgi?id=1565379 I have prepared a new iteration of the clientid patch for thunderbird.

The effective changes since the last patch include:

  • Rebased the last patch (clientid-thunderbird-full-aug-12-2019.patch) onto the latest checkout of comm-central. This involved re-applying some failed thunks to the correct location, but effectively there was no changes to the logic of the patch.
    Files Changed:

    • mailnews/base/test/unit/test_accountMigration.js
    • mail/components/accountcreation/content/createInBackend.js
  • Turned clientidEnabled default setting to false
    Files Changed:

    • mailnews/mailnews.js
  • Added comment indicating clientidEnabled is not to be enabled until the prerequisite changes are completed with a link to the thread I linked directly above that contains the list of requirements to meet.
    Files Changed:

    • mailnews/mailnews.js
  • Added a contingency to clear the clientid if the user or host name is changed on an account (and adjusted a nearby comment to reflect the new action taking place)
    Files Changed:

    • mailnews/base/util/nsMsgIncomingServer.cpp

Please let me know if there are any issues with this iteration of the patch so I may correct them as soon as possible.

Thanks

Attachment #9082784 - Attachment is obsolete: true
Attachment #9084884 - Attachment is obsolete: true
Attachment #9084884 - Flags: review?(mkmelin+mozilla)
Attachment #9093690 - Flags: review?(mkmelin+mozilla)
Attachment #9093690 - Flags: review?(kaie)
Attachment #9093690 - Flags: review?(acelists)
Comment on attachment 9093690 [details] [diff] [review]
clientid-thunderbird-full-sep-18-2019.patch

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

I think this will need a clang-format pass, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format

Other than that, looks pretty good to me. Could you add CLIENTID support to our fake server https://searchfox.org/comm-central/source/mailnews/test/fakeserver/imapd.js and a test?

Would also be good to close bug 1565379 first before proceeding.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +1120,5 @@
>    rv = GetPrettyName(acctName);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // 5. Clear the clientid because the user or host have changed.
> +  SetClientid(nsCString());

EmptyCString() would be preferable
Attachment #9093690 - Flags: review?(mkmelin+mozilla)
Attachment #9093690 - Flags: review?(kaie)
Attachment #9093690 - Flags: review?(acelists)
Attachment #9093690 - Flags: feedback+

Don't worry about the clang-formatting. I'll do that before landing the patch. We also need to do a linting run.

OK, I treated this with eslint/prettier and clang-format. I've also changed the empty string thing and checked that it compiles. Here's a try run (which could be busted since right now, I don't know the current state of our tree after bug 1555497 and Daily results haven't arrived yet). Oh, and it needed some rebasing in mailnews/base/test/unit/test_accountMigration.js, you may want to check that hunk.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0ea0eee1021179360b085ca0e77197aa9b6819bd

Attachment #9063146 - Attachment is obsolete: true
Attachment #9063467 - Attachment is obsolete: true
Attachment #9077607 - Attachment is obsolete: true
Attachment #9080839 - Attachment is obsolete: true
Attachment #9093690 - Attachment is obsolete: true
Attachment #9094152 - Flags: review?(mkmelin+mozilla)
Attachment #9094152 - Flags: review?(kaie)
Attachment #9094152 - Flags: review?(acelists)
Comment on attachment 9094152 [details] [diff] [review]
clientid-thunderbird-full-sep-18-2019.patch

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

bug 1565379 and tests first
Attachment #9094152 - Flags: review?(mkmelin+mozilla)
Attachment #9094152 - Flags: review?(kaie)
Attachment #9094152 - Flags: review?(acelists)

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

Comment on attachment 9093690 [details] [diff] [review]
clientid-thunderbird-full-sep-18-2019.patch

Review of attachment 9093690 [details] [diff] [review]:

Other than that, looks pretty good to me. Could you add CLIENTID support to
our fake server
https://searchfox.org/comm-central/source/mailnews/test/fakeserver/imapd.js
and a test?

I see in the imapd.js fake server that the only places available to advertise capabilities are labelled as IMAP_STATE_NOT_AUTHED, or IMAP_STATE_AUTHED.

Unfortunately the CLIENTID command is supposed to take place before authentication -- but after TLS has been negotiated.

I see that the imapd.js server explicitly states it does not support TLS, which leads me to believe that there is no way for us to implement a working test for the CLIENTID command with this server.

If we were to advertise CLIENTID in either of the two available locations (IMAP_STATE_NOT_AUTHED, or IMAP_STATE_AUTHED) then thunderbird would never present the CLIENTID command because our logic in the imap client code of thunderbird looks like this:

  if ((GetServerStateParser().GetCapabilityFlag() & kHasClientIDCapability) &&
      !m_clientId.IsEmpty() &&
      (m_connectionType.EqualsLiteral("starttls") ||
       m_connectionType.EqualsLiteral("ssl"))) {

       // present clientid
  }

The above logic only runs before authentication has taken place, meaning TLS must be active and the auth command must not have been run yet.

So unless there is a way to establish or fake the TLS connection with the imapd server prior to the authentication command then I don't see a way we could possible implement this test without severely enhancing the imapd server first -- or altering thunderbird to ignore the connection type when presenting the clientid (which would fail to meet the RFC spec for clientid usage).

I may be naively overlooking something here, so please correct me If I am missing anything.

Thanks.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)

(In reply to Jorg K (GMT+2) from comment #227)

Created attachment 9094152 [details] [diff] [review]
clientid-thunderbird-full-sep-18-2019.patch

Oh, and it needed some rebasing in mailnews/base/test/unit/test_accountMigration.js, you may want to check that hunk.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0ea0eee1021179360b085ca0e77197aa9b6819bd

It would appear that you resolved the conflict correctly.

I reviewed the hunk you applied, ran the test, and everything looks good.

The two failures in the try run in X4 and Z4 were were expected and unrelated (and fixed now).

As for your other question: I don't have any in-depth knowledge of the imapd.js fake server, sorry.

Flags: needinfo?(jorgk)

(In reply to Dan from comment #229)
Simulating TLS may indeed be harder.
Perhaps the check should be if it's a secure context instead? That way you could simulate it for localhost in tests and such. See nsIContentSecurityManager.isOriginPotentiallyTrustworthy

Flags: needinfo?(mkmelin+mozilla)

Hello Magnus,

Apologies for my naivety with the Mozilla codebase here, but I am having some trouble establishing the correct usage of isOriginPotentiallyTrustworthy.

I have spent the last few days researching the usages of the function across the codebase, and the main issue I am running into is where to retrieve the respective nsIPrincipal for the context of the imap connection.

I have read through these wiki pages, but they didn't seem very related to the context in which we are trying to acquire the principal.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrincipal
https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Script_security

If it's not too much trouble I would greatly appreciate some hints to send me in the right direction.

Thanks.

Forgot to tag Magnus for needinfo

Flags: needinfo?(mkmelin+mozilla)

I didn't try if it works with an imap url or not. If not then just using equivalent http uri I think should be fine. For cpp, something like https://searchfox.org/mozilla-central/rev/01d1011ca4a460f751da030d455d35c267c3e210/dom/security/test/gtest/TestSecureContext.cpp#68-74 no?

Flags: needinfo?(mkmelin+mozilla)

Thanks a lot Magnus, I think the key that solved my problems was the function: CreateContentPrincipalFromOrigin.

I have some research results that I figure I may share regarding the usage of the IMAP URI with the IsOriginPotentiallyTrustworthy function.

This is the implementation that I began working with with, lacking some safety checks and possibly retrieving the URI incorrectly.

nsCOMPtr<nsIPrincipal> prin;
nsCOMPtr<nsIURI> uri;
nsCString uriSpec;

nsCOMPtr<nsIContentSecurityManager> csManager = do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID);
m_mockChannel->GetURI(getter_AddRefs(uri));
uri->GetSpec(uriSpec);
nsScriptSecurityManager::GetScriptSecurityManager()->CreateContentPrincipalFromOrigin(uriSpec, getter_AddRefs(prin));
bool isPotentiallyTrustworthy = false;
csManager->IsOriginPotentiallyTrustworthy(prin, &isPotentiallyTrustworthy);

In all of my tests this method did not indicate that the TLS secured connection was trustworthy.

I checked the URI spec that I was getting to see if maybe I was getting the wrong information, the URI was something like:

imap://postmaster%40linuxmagic%2Edev@192.168.2.11:143/select%3E.INBOX

Which when un-encoded would translate to:

imap://postmaster@linuxmagic.dev@192.168.2.11:143/select>.INBOX

I am using a test server at local IP 192.168.2.11, and the connection is indeed secured with TLS as confirmed by dovecot logs.

So I figured that I would dig into the function IsOriginPotentiallyTrustworthy to determine whether what we're trying to do is even supported or perhaps I was just using it wrong.

My investigations revealed that I was misunderstanding the purpose of the function, that it wouldn't tell me whether a connection was actually secure but whether a remote host was potentially secure (ie. does it resolve to localhost? or other unrelated rules like .onion links).

So from all of my research into this function I now understand that it will only be useful to tell me whether the URI resolves to a localhost address, which makes sense if the imapd.js tests happen over localhost then this would allow clientid to function with the imapd.js server.

So we would still need to retain the check for whether the connection is secured with tls/starttls but we would need to include the results of IsOriginPotentiallyTrustworthy as well -- this is on par with our own Magicmail server clientid implementation which only looks at whether the connection is considered "secure", which includes localhost.

So with this new understanding of IsOriginPotentiallyTrustworthy I tried hardcoding an IMAP URI to 127.0.0.1:

uriSpec = "imap://postmaster%40linuxmagic%2Edev@127.0.0.1:143/select%3E.INBOX";

However, this too yielded a false.

So it can be concluded that the IMAP URI itself would not work... And of course you foresaw this and suggested to replace the "imap:" with "http:".

So I tried replacing the "imap:" with "http:" instead, this time it actually worked!

Using http instead of imap seems to allow the function to run it's logic on the remote server URL and determine whether it's potentially trustworthy.

So the final implementation I homed in on looks like this:

nsCOMPtr<nsIPrincipal> prin;
nsCOMPtr<nsIURI> uri;
nsCString uriSpec;

nsCOMPtr<nsIContentSecurityManager> csManager = do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID);
m_mockChannel->GetURI(getter_AddRefs(uri));
uri->GetSpec(uriSpec);
// Replace the 'imap:' portion of the URI spec with 'http:' to allow
// the function IsOriginPotentiallyTrustworthy to work as intended.
uriSpec->ReplaceSubstring("imap:", "http:");
nsScriptSecurityManager::GetScriptSecurityManager()->CreateContentPrincipalFromOrigin(uriSpec, getter_AddRefs(prin));
bool isPotentiallyTrustworthy = false;
csManager->IsOriginPotentiallyTrustworthy(prin, &isPotentiallyTrustworthy);

// Whether our connection can be considered 'secure' and whether
// we should allow the CLIENTID to be sent over this channel.
bool isSecuredConnection = (m_connectionType.EqualsLiteral("starttls") ||
                            m_connectionType.EqualsLiteral("ssl") ||
                            isPotentiallyTrustworthy);

And then I am using the boolean isSecuredConnection to determine whether the clientid should be sent over the channel or not.

I am now planning to start work on the imapd.js serverside clientid implementation, but I would greatly appreciate any feedback on the above.

Specifically, if there is anything wrong with my above approach then I may be able to fix it before I submit the next patch.

Thanks.

Flags: needinfo?(mkmelin+mozilla)

Looks alright to me, except for that it should only replace imap and http from start of the uriSpec, not if it's (for some mysterious reason) elsewhere

Flags: needinfo?(mkmelin+mozilla)

Hello Magnus and Thunderbird Team,

I have been working to get the next patch ready but unfortunately have been encountering several issues along the way.

The first issue that started popping up was a segfault inside IsOriginPotentiallyTrustworthy, I've pasted the backtrace below just in-case anybody is curious about the segfault. However, keep reading because I believe I solved it myself and wouldn't mind verification of my assessment.

mDelimiterThread 60 "thunderbird" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffcf9a9700 (LWP 8496)]
mozilla::StaticPrefs::dom_securecontext_whitelist_onions () at /root/thunderbird_trunk/source/obj-x86_64-pc-linux-gnu/dist/include/mozilla/StaticPrefList_dom.h:927
927 ALWAYS_PREF(
(gdb) bt
#0  mozilla::StaticPrefs::dom_securecontext_whitelist_onions () at /root/thunderbird_trunk/source/obj-x86_64-pc-linux-gnu/dist/include/mozilla/StaticPrefList_dom.h:927
#1  nsMixedContentBlocker::IsPotentiallyTrustworthyOnion (aURL=0x7fffd335b120) at /root/thunderbird_trunk/source/dom/security/nsMixedContentBlocker.cpp:414
#2  0x00007fffe909ece6 in nsMixedContentBlocker::IsPotentiallyTrustworthyOrigin (aURI=0x7fffd335b120) at /root/thunderbird_trunk/source/dom/security/nsMixedContentBlocker.cpp:490
#3  0x00007fffe909e926 in nsContentSecurityManager::IsOriginPotentiallyTrustworthy (this=<optimized out>, aPrincipal=0x7fffd0038ca0, aIsTrustWorthy=0x7fffcf9a8577)
    at /root/thunderbird_trunk/source/dom/security/nsContentSecurityManager.cpp:1072
#4  0x00007fffe68986f9 in nsImapProtocol::TryToLogon (this=0x7fffcf863000) at /root/thunderbird_trunk/source/comm/mailnews/imap/src/nsImapProtocol.cpp:8163
#5  0x00007fffe689803b in nsImapProtocol::ProcessCurrentURL (this=0x7fffcf863000) at /root/thunderbird_trunk/source/comm/mailnews/imap/src/nsImapProtocol.cpp:1742
#6  0x00007fffe6894e8c in nsImapProtocol::ImapThreadMainLoop (this=0x7fffcf863000) at /root/thunderbird_trunk/source/comm/mailnews/imap/src/nsImapProtocol.cpp:1423
#7  0x00007fffe68949bd in nsImapProtocol::Run (this=0x7fffcf863000) at /root/thunderbird_trunk/source/comm/mailnews/imap/src/nsImapProtocol.cpp:1105
#8  0x00007fffe689509d in non-virtual thunk to nsImapProtocol::Run() () from /root/thunderbird_trunk/source/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so
#9  0x00007fffe6ae0fd2 in nsThread::ProcessNextEvent (this=0x7fffd34dd580, aMayWait=<optimized out>, aResult=<optimized out>)
    at /root/thunderbird_trunk/source/xpcom/threads/nsThread.cpp:1225
#10 0x00007fffe6ae3098 in NS_ProcessNextEvent (aThread=0x7fffed0fddc8, aMayWait=<optimized out>) at /root/thunderbird_trunk/source/xpcom/threads/nsThreadUtils.cpp:486
#11 0x00007fffe71204b7 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x7fffd2b42a00, aDelegate=0x7fffd21f0e40) at /root/thunderbird_trunk/source/ipc/glue/MessagePump.cpp:303
#12 0x00007fffe70aa686 in MessageLoop::RunInternal (this=<optimized out>) at /root/thunderbird_trunk/source/ipc/chromium/src/base/message_loop.cc:315
#13 MessageLoop::RunHandler (this=<optimized out>) at /root/thunderbird_trunk/source/ipc/chromium/src/base/message_loop.cc:308
#14 MessageLoop::Run (this=0x7fffcf8f6990) at /root/thunderbird_trunk/source/ipc/chromium/src/base/message_loop.cc:290
#15 0x00007fffe6adeacf in nsThread::ThreadFunc (aArg=<optimized out>) at /root/thunderbird_trunk/source/xpcom/threads/nsThread.cpp:458
#16 0x00007ffff7f574a9 in _pt_root (arg=0x7fffd196d160) at /root/thunderbird_trunk/source/nsprpub/pr/src/pthreads/ptthread.c:201
#17 0x00007ffff7bc16ba in start_thread (arg=0x7fffcf9a9700) at pthread_create.c:333
#18 0x00007ffff6def41d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

I think I have determined the cause of this issue being that I am calling IsOriginPotentiallyTrustworthy from a thread which is not the main thread. I found this by diving into the macro ALWAYS_PREF, which I found expands out to a function which ASSERTs NS_IsMainThread().

A quick test showed that the IMAP thread where I was running this code was indeed not the main thread.

Truth is I'm not sure how this code ever worked... It definitely did work at one point in time (before I posted my last update) and then just stopped working suddenly, but the block of code has been in the same location for all of my tests. There must have been some oversights on my part.

This is where I am lost Thunderbird team, I am not sure where I should put this code such that it would not violate the threading agreements.
The few places I considered (ex. nsImapProtocol::Initialize) don't seem to have the URI available yet.

Is there anywhere in the main thread where the URI is available? I imagine there is and I'm just blind so any direction here would be greatly appreciated.


Also, just an update on the unit test so far, I believe I have added clientid functionality to the imapd.js test server. I just added a simple CLIENTID function that returns an OK message, and added CLIENTID to the capability list, nothing fancy. See below for the changeset on the imapd.js fakeserver, but keep in mind that I haven't been able to test this because I am having trouble implementing the actual test.

--- a/mailnews/test/fakeserver/imapd.js Wed Sep 18 12:57:48 2019 -0700
+++ b/mailnews/test/fakeserver/imapd.js Fri Oct 11 09:13:02 2019 -0700
@@ -727,6 +727,7 @@
   this.kAuthSchemes = []; // Added by RFC2195 extension. Test may modify as needed.
   this.kCapabilities = [
     /* "LOGINDISABLED", "STARTTLS", */
+    "CLIENTID"
   ]; // Test may modify as needed.
   this.kUidCommands = ["FETCH", "STORE", "SEARCH", "COPY"];

@@ -738,7 +739,15 @@

   this._enabledCommands = {
     // IMAP_STATE_NOT_AUTHED
-    0: ["CAPABILITY", "NOOP", "LOGOUT", "STARTTLS", "AUTHENTICATE", "LOGIN"],
+    0: [
+      "CAPABILITY",
+      "NOOP",
+      "LOGOUT",
+      "STARTTLS",
+      "CLIENTID",
+      "AUTHENTICATE",
+      "LOGIN"
+    ],
     // IMAP_STATE_AUTHED
     1: [
       "CAPABILITY",
@@ -798,6 +807,7 @@
     NOOP: [],
     LOGOUT: [],
     STARTTLS: [],
+    CLIENTID: ["string", "string"],
     AUTHENTICATE: ["atom", "..."],
     LOGIN: ["string", "string"],
     SELECT: ["mailbox"],
@@ -1038,6 +1048,9 @@
     capa += "\0OK CAPABILITY completed";
     return capa;
   },
+  CLIENTID(args) {
+    return "OK Recognized a valid CLIENTID command, used for authentication methods";
+  },
   LOGOUT(args) {
     this.closing = true;
     if (this._selectedMailbox) {

I have created the new unit test file in mailnews/imap/test/unit/ and I can run my test with the xpcshell-test command, but I am struggling with how I would go about invoking the clientid code from the unit test.

So far my test looks like this

/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */

"use strict";

var incomingServer, server;

var kUserName = "user";
var kValidPassword = "password";

add_task(async function() {

  let daemon = new imapDaemon();
  server = makeServer(daemon, "", {
    // Make username of server match the singons.txt file
    // (pw there is intentionally invalid)
    kUsername: kUserName,
    kPassword: kValidPassword,
  });
  incomingServer = createLocalIMAPServer(server.port);

  // connect
  incomingServer.performExpand(null);

  // I don't actually know what this is doing, I assume it's wrong
  server.performTest("CLIENTID");

  do_timeout(500, endTest);
});

function endTest() {
  incomingServer.closeCachedConnections();
  server.stop();

  var thread = gThreadManager.currentThread;
  while (thread.hasPendingEvents()) {
    thread.processNextEvent(true);
  }

  do_test_finished();
}

I'm mainly struggling with how I would go about testing the clientid functionality, I assume it might be something like adjusting the clientid preferences then running performExpand to trigger a login, then rinse and repeat?

Any tips or pointers to send me in the right direction would be greatly appreciated.

Thanks

I was too hasty with the response yet again, forgot to tag magnus and others.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.