Add detection of configuration based on SRV records (RFC6186) #4719

livmackintosh · 2020-05-03T04:04:56Z

If "XML Discovery" yields no connection settings, query SRV records
on the domain portion of the email address. This will help increase
the likelihood of being able to set up accounts with only the email and
password.

Automated tests

NB: I attempted to merge the code that @daquexian had worked on but it
has drifted so far behind master that the amount of effort to merge that
in would be very large.

References:
#865
#2530

livmackintosh · 2020-05-03T04:06:18Z

I'm going to contribute some automated tests and fix the issues mentioned over the weekend but I thought it would be nice to get it out into the open to get eyes on it.

cketti · 2020-05-03T11:35:28Z

Thanks for working on this 👍

Actually using this method to discover server settings comes with the same problems as mentioned here: #3879 (review)
Sadly, I didn't actually create an issue outlining a plan to tackle this. I'll try to do this today.

For now I think it would be best to limit this pull request to the ConnectionSettingsDiscovery implementation. I'll add some comments in a bit.

app/autodiscovery/src/main/java/com/fsck/k9/autodiscovery/srvrecords/SrvServiceDiscovery.kt

livmackintosh · 2020-05-03T14:40:57Z

@cketti I won't reply individually to the comments as I'm generally in agreement with you with most of them, especially around "graceful failure".

Thanks for your comments :)

livmackintosh · 2020-05-04T01:58:56Z

@cketti Would there be an issue in me using JUnit-QuickCheck to write property-based tests? I feel the number of variables to account for could easily become quite high. I feel like there are many outcomes that depend only on one or two variables. It would be useful to generate data in the test that might account for situations where the SRV records have weird data.

Hopefully that makes at least a little sense. I posted this just before going to sleep :)

cketti · 2020-05-04T08:00:29Z

@cketti Would there be an issue in me using JUnit-QuickCheck to write property-based tests?

Without concrete example I currently don't see how it could be useful. Just go for it and I'll have a look at the result?

@daquexian

If "XML Discovery" yields no connection settings, query SRV records on the domain portion of the email address. This will help increase the likelihood of being able to set up accounts with only the email and password. This implementation prefers TLS incoming servers (IMAPs/POP3s) with the lowest priority value. If neither can be found, it will fall back to using IMAP/POP3 with STARTTLS security. It is then left to finishAutoSetup() to handle testing of the connection. Things this commit hasn't covered: * Domain validation ("the user should be alerted if the SRV target is not part of the email domain") * Setting SSL/TLS security for the outbound server (it assumes STARTTLS currently) NB: I attempted to merge the code that @daquexian had worked on but it has drifted so far behind master that the amount of effort to merge that in would be very large. References thunderbird#2530

Among a few minor style/convention changes, add an interface for the SRV resolver and remove POP3/POP3S support.

Allow SrvServiceDiscovery to be initialized with arbitrary SrvResolution interface implementation Add test for null happy-path case (no SRV records)

Previous commit was missing the required dependencies. This commit fixes that.

livmackintosh

I feel like this is close to completion @cketti with the following caveats worth considering:

It is probably advisable for us to check if the email domain is the parent of any servers returned in SRV records and check with the user (in case of DNS hijacking). This does feel outside the scope of this PR though and something that would be implemented higher up the stack before validating the settings and we maybe should see how this fits into Automatic account setup / server settings autodiscovery #4721
How can we determine the security setting for the SMTP server? There is no differentiation like IMAP/IMAPS in RFC6186. Should we assume STARTTLS? Or possibly attempt SSL/TLS depending on port number?

NB: I'm not a self-identified Kotlin/Java/Android developer so I may have done things in a weird/non-standard way - please let me know if this is the case. 😄

cketti · 2020-05-06T20:47:45Z

How can we determine the security setting for the SMTP server? There is no differentiation like IMAP/IMAPS in RFC6186. Should we assume STARTTLS? Or possibly attempt SSL/TLS depending on port number?

_submissions._tcp was added by RFC 8314, section 5.1

livmackintosh · 2020-05-06T20:49:08Z

_submissions._tcp was added by RFC 8314, section 5.1
Perfect! Thank you 👍

cketti

Thanks 👍

As you mentioned, this does have security implications. When implementing the plan outlined in #4721 we'll have to distinguish between trusted server settings and those we can only use after checking with the user. That's one of the reasons I don't want to use this in AccountSetupBasics just yet.

app/autodiscovery/src/main/java/com/fsck/k9/autodiscovery/srvrecords/SrvServiceDiscovery.kt

app/autodiscovery/src/test/java/com/fsck/k9/autodiscovery/srvrecords/SrvServiceDiscoveryTest.kt

app/ui/src/main/java/com/fsck/k9/activity/setup/AccountSetupBasics.java

Co-authored-by: cketti <ck@cketti.de>

It's not doing anything useful and was responsible for at least one bug.

* Rename SrvResolver and interface to use standard semantics * Add visual line separation between arrange, act, assert test blocks

* Reorder classes / functions by importance * Move SrvResolver interface to its own file * Use functional approach to retrieving mail services

Git history was manually cleaned up.

cketti · 2020-05-09T15:59:29Z

Thanks 👍 I manually cleaned up the Git history dfe2698 and merged the changes just now.

cketti · 2020-05-11T20:09:28Z

@livmackintosh: Do you want to continue working on this? Unfortunately, the plan outlined in #4721 is quite a lot of work. But if I know someone is willing to work on it, I'd be happy to break it down into more manageable tasks.

livmackintosh · 2020-05-12T06:29:39Z

@cketti Most definitely; I would love to see this over the line - it's crucial to getting K9 up to 2020+ standards. I can probably work with #4721 but equally, if you wish to break it down that's cool too.

cketti · 2020-05-12T20:26:39Z

Awesome. I created issue #4759 with a description of what I think is a reasonable next step.

monperrus · 2020-06-04T08:34:45Z

Really cool! Do we check the validity of the SRV record with DNSSEC?

cketti · 2020-06-04T10:49:46Z

@monperrus: Not yet, but the code isn't used yet. The plan is to trust host names under the email domain and records signed via DNSSEC.

livmackintosh · 2020-06-04T16:10:08Z

Might pick up on some of this this evening, just been crazy busy lately 😄

cketti reviewed May 3, 2020

View reviewed changes

app/autodiscovery/src/main/java/com/fsck/k9/autodiscovery/srvrecords/SrvServiceDiscovery.kt Outdated Show resolved Hide resolved

cketti reviewed May 3, 2020

View reviewed changes

cketti mentioned this pull request May 3, 2020

Automatic account setup / server settings autodiscovery #4721

Open

livmackintosh force-pushed the account-redux branch from 949032f to 7f8c7d7 Compare May 3, 2020 19:27

livmackintosh force-pushed the account-redux branch 2 times, most recently from bc9e978 to 113c045 Compare May 5, 2020 23:56

livmackintosh added 6 commits May 6, 2020 09:35

Fix ktlintCheck issues failing build

7933d6f

Remove POP3 SRV support and add resolver interface

d34a855

Among a few minor style/convention changes, add an interface for the SRV resolver and remove POP3/POP3S support.

Move SRV service selection out of the SrvResolver

4189f06

Add test class for SrvServiceDiscovery and separate SrvResolver

46975a0

Allow SrvServiceDiscovery to be initialized with arbitrary SrvResolution interface implementation Add test for null happy-path case (no SRV records)

Inject SRV Service Discovery dependencies with Koin

a3b109a

Previous commit was missing the required dependencies. This commit fixes that.

livmackintosh force-pushed the account-redux branch from be8641f to a3b109a Compare May 6, 2020 08:37

Add more SrvServiceDiscovery tests covering service priority

20edaed

livmackintosh marked this pull request as ready for review May 6, 2020 20:23

livmackintosh commented May 6, 2020

View reviewed changes

Add _submissions._tcp support (RFC8314 5.1)

09f5395

livmackintosh requested a review from cketti May 7, 2020 11:18

cketti requested changes May 8, 2020

View reviewed changes

livmackintosh and others added 4 commits May 8, 2020 22:58

Adjust code style in SrvServiceDiscovery test

97b5331

Co-authored-by: cketti <ck@cketti.de>

Change var -> val in SrvType and MailService

7cac7f8

Check if periodic mail sync is disabled in MailSyncWorker

48b70e5

Fix check to not sync a folder if it was checked recently

86ec27c

cketti and others added 19 commits May 9, 2020 00:05

Only reschedule sync job for the account whose settings were changed

25356b1

Save last sync time

ba247fc

Use last sync time to calculate initial delay for periodic syncs

2a61902

Retry failed periodic syncs with exponential backoff

785afd8

Update unread widgets from a background thread

2f2e2a2

Rewrite unread widget configurations created by old app versions

9dc4ed6

Fix displaying the folder name in unread widget title

86eaaaa

Update translations

b94ca48

Version 5.711

c4750a0

Prepare for version 5.712

06c9441

Remove usage of java.time API

9df2737

Version 5.712

2fd25b9

Prepare for version 5.713

fb183ec

Remove LocalFolder.close()

1c81168

It's not doing anything useful and was responsible for at least one bug.

Version 5.713

7150821

Prepare for version 5.714

c97162f

Make several semantic and style changes to SrvServiceDiscovery + test

755019b

* Rename SrvResolver and interface to use standard semantics * Add visual line separation between arrange, act, assert test blocks

Do not use SrvServiceDiscovery in AccountSetupBasics yet

638b572

Make more requested code changes

c99bdef

* Reorder classes / functions by importance * Move SrvResolver interface to its own file * Use functional approach to retrieving mail services

livmackintosh requested a review from cketti May 8, 2020 23:46

cketti added a commit that referenced this pull request May 9, 2020

Merge pull request #4719 from livmackintosh

259d0d0

Git history was manually cleaned up.

cketti closed this May 9, 2020

livmackintosh deleted the account-redux branch May 10, 2020 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detection of configuration based on SRV records (RFC6186) #4719

Add detection of configuration based on SRV records (RFC6186) #4719

livmackintosh commented May 3, 2020 •

edited

livmackintosh commented May 3, 2020

cketti commented May 3, 2020

livmackintosh commented May 3, 2020

livmackintosh commented May 4, 2020 •

edited

cketti commented May 4, 2020

livmackintosh left a comment •

edited

cketti commented May 6, 2020

livmackintosh commented May 6, 2020

cketti left a comment

cketti commented May 9, 2020

cketti commented May 11, 2020

livmackintosh commented May 12, 2020 •

edited

cketti commented May 12, 2020

monperrus commented Jun 4, 2020

cketti commented Jun 4, 2020

livmackintosh commented Jun 4, 2020

Add detection of configuration based on SRV records (RFC6186) #4719

Add detection of configuration based on SRV records (RFC6186) #4719

Conversation

livmackintosh commented May 3, 2020 • edited

livmackintosh commented May 3, 2020

cketti commented May 3, 2020

livmackintosh commented May 3, 2020

livmackintosh commented May 4, 2020 • edited

cketti commented May 4, 2020

livmackintosh left a comment • edited

Choose a reason for hiding this comment

cketti commented May 6, 2020

livmackintosh commented May 6, 2020

cketti left a comment

Choose a reason for hiding this comment

cketti commented May 9, 2020

cketti commented May 11, 2020

livmackintosh commented May 12, 2020 • edited

cketti commented May 12, 2020

monperrus commented Jun 4, 2020

cketti commented Jun 4, 2020

livmackintosh commented Jun 4, 2020

livmackintosh commented May 3, 2020 •

edited

livmackintosh commented May 4, 2020 •

edited

livmackintosh left a comment •

edited

livmackintosh commented May 12, 2020 •

edited