docker-mailserver icon indicating copy to clipboard operation
docker-mailserver copied to clipboard

chore(encryption): support for passworded user email storage encryption keys

Open NorseGaud opened this issue 2 years ago • 45 comments

Description

  1. Allows users to ENABLE_PER_USER_STORAGE_ENCRYPTION in their docker-compose to enable the feature
  2. Within the various password impacting scripts:
    • Notice mail_crypt is enabled and, by default, generated a key for the user on creation.
    • Notice mail_crypt is enabled and, by default, update encryption password when email password is changed.
    • Notice mail_crypt is enabled and, by default, when updating the user's password, create user keys/encryption if there aren't any yet.
  3. Allow users to change the mail_crypt_curve with PER_USER_STORAGE_ENCRYPTION_CURVE and default to secp521r1
  4. Allow users to change the scheme with PER_USER_STORAGE_ENCRYPTION_SCHEME and default to CRYPT

Related issue: https://github.com/docker-mailserver/docker-mailserver/issues/2058

Related: https://github.com/docker-mailserver/docker-mailserver/pull/2134

Type of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Improvement (non-breaking change that does improve existing functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [x] This change requires a documentation update

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • [x] If necessary I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes

NorseGaud avatar Jul 09 '21 13:07 NorseGaud

Screen Shot 2021-07-09 at 11 34 31 AM (NO LONGER RELEVANT)

NorseGaud avatar Jul 09 '21 15:07 NorseGaud

@wernerfred

Also please make sure to mirror the changes you introduced to setup.sh to the corresponding docs site that it reflects the current state.

will do!

NorseGaud avatar Jul 09 '21 15:07 NorseGaud

Current state: I want to avoid writing tests for now until the overall changes I made are reviewed by a few people. That way I'm minimizing time spent on rewrites.

NorseGaud avatar Jul 09 '21 16:07 NorseGaud

Accidentally rebased again. I hope this is fine this time, otherwise excuse me again and force-push over it 🙈

georglauterbach avatar Jul 09 '21 17:07 georglauterbach

Bad habits never die 😅

wernerfred avatar Jul 09 '21 17:07 wernerfred

I don't mind reviewing, but will be busy until early next week. Unfortunately no time until then due to a deadline.

polarathene avatar Jul 09 '21 23:07 polarathene

There is no rush for this

NorseGaud avatar Jul 10 '21 05:07 NorseGaud

An opportunity for a PR if someone has time: check-for-changes.sh doesn't start on the very first start of the container and if no email is created. Where there is needed improvement is for the setup-stack.sh to loop at Unless using LDAP, you need at least 1 email account to start Dovecot instead of immediately return an error and proceed with the setup once the user creates the first email.

NorseGaud avatar Jul 12 '21 13:07 NorseGaud

Yes, that sounds good!

On Tue, Jul 13, 2021, 15:15 Nathan Pierce @.***> wrote:

@NorseGaud commented on this pull request.

In docs/content/config/security/mail_crypt.md https://github.com/docker-mailserver/docker-mailserver/pull/2080#discussion_r668752033 :

+1. Create 10-custom.conf and populate it with the following: +

  • mail_attribute_dict = file:%h/Maildir/dovecot-attributes
  • mail_plugins = $mail_plugins mail_crypt
  • mail_debug= yes
  • plugin {
  •  mail_crypt_curve = secp521r1
    
  •  mail_crypt_save_version = 2
    
  •  mail_crypt_require_encrypted_user_key = yes
    
  • }

+2. Create auth-passwdfile.inc and populate it with the following:

Thanks for clarifying. Give me a few days to consider how to do this and get back. I think the issue with that is now the user encryption keys/password stuff becomes an official feature -- which I'm totally fine with but maybe the maintainers don't agree! This would make sense since I added features in setup.sh for it, so it's already sort of a built-in feature in many ways. We can easily allow users to set an ENV to enable it and then even change the setup.sh to check for that ENV and automatically run the doveadm commands instead of requiring users to set -g or -c.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/docker-mailserver/docker-mailserver/pull/2080#discussion_r668752033, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABL3E24CA4UNJBP3CTMPSDTXQ37NANCNFSM5ACZ33OA .

DavyLandman avatar Jul 13 '21 16:07 DavyLandman

Docs need a bit more work and also need a few more tests. For now, it's in a good place to review.

NorseGaud avatar Aug 14 '21 21:08 NorseGaud

Just read the docs, how about flipping the order, so start with user keys? As the other one is more involved (and less secure)

DavyLandman avatar Aug 16 '21 17:08 DavyLandman

FInalizing this...

NorseGaud avatar Sep 07 '21 11:09 NorseGaud

Now tests are failing :(

NorseGaud avatar Sep 09 '21 03:09 NorseGaud

Turns out the failing test is also failing in master (I did checkout and make clean build):

❯ NAME=mailserver-testing:ci ./test/bats/bin/bats --filter "wait_for_empty_mail_queue_in_container fails when timeout reached" test/test_helper.bats
 ✗ wait_for_empty_mail_queue_in_container fails when timeout reached
   (from function `repeat_until_success_or_timeout' in file test/test_helper/common.bash, line 27,
    from function `repeat_in_container_until_success_or_timeout' in file test/test_helper/common.bash, line 70,
    from function `wait_for_empty_mail_queue_in_container' in file test/test_helper/common.bash, line 206,
    in test file test/test_helper.bats, line 225)
     `! TEST_TIMEOUT_IN_SECONDS=0 wait_for_empty_mail_queue_in_container "${CONTAINER_NAME}"' failed
   220 mail.my-domain.com ESMTP
   250 mail.my-domain.com
   250 2.1.0 Ok
   250 2.1.5 Ok
   354 End data with <CR><LF>.<CR><LF>
   250 2.0.0 Ok: queued as 8BDAE3C3D9B
   221 2.0.0 Bye
   d406f68a448c6d6748f31b4b5f683fb49ec2429c472021449c6503f60911d7b8

1 test, 1 failure

NorseGaud avatar Sep 09 '21 03:09 NorseGaud

Ok, using it in production without issue so far. CRYPT and userdb_mail_crypt_private_password=%{sha256:password} seem to work fine, but I can't get BLF-CRYPT figured out. If we support changing the SCHEME, and it turns out sha256 for the password doesn't work, then we might need to rethink this.

NorseGaud avatar Sep 09 '21 05:09 NorseGaud

Did you see if it works with BLF crypt when you pass in the plain password?

Otherwise, like is said, we are already passing in the sha256 before running crypt on it, that okayish. Just not sure at what point crypt is called. Maybe to derive the private key?

DavyLandman avatar Sep 09 '21 06:09 DavyLandman

Did you see if it works with BLF crypt when you pass in the plain password?

Otherwise, like is said, we are already passing in the sha256 before running crypt on it, that okayish. Just not sure at what point crypt is called. Maybe to derive the private key?

Yep, tried that and the same error

NorseGaud avatar Sep 09 '21 06:09 NorseGaud

Turns out the failing test is also failing in master

That is a recurring test that fails. I have advised we disable it until someone can warrant time to make it deterministic (which is difficult since we're not sure how to reproduce the failure reliably). It's not a useful test if it's not reliable, passing can't be taken seriously at that point.

polarathene avatar Sep 09 '21 06:09 polarathene

Turns out the failing test is also failing in master

That is a recurring test that fails. I have advised we disable it until someone can warrant time to make it deterministic (which is difficult since we're not sure how to reproduce the failure reliably). It's not a useful test if it's not reliable, passing can't be taken seriously at that point.

Thanks

NorseGaud avatar Sep 09 '21 06:09 NorseGaud

I too vote for disabling the tests that fail indeterministicslly :)

georglauterbach avatar Sep 09 '21 06:09 georglauterbach

CRYPT and userdb_mail_crypt_private_password=%{sha256:password} seem to work fine, but I can't get BLF-CRYPT figured out. If we support changing the SCHEME, and it turns out sha256 for the password doesn't work, then we might need to rethink this.

Just as a reminder about preferring CRYPT: https://github.com/docker-mailserver/docker-mailserver/pull/2080#discussion_r703970850

Traditional DES-crypted password in /etc/passwd (e.g. "pass" = vpvKh.SaNbR6s) The traditional DES-crypt scheme only uses the first 8 characters of the password, the rest are ignored.

password => 5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8 (SHA-256) => 5e884898 (input to DES).

That's 4 bytes, since every two char pairs are hexadecimal encoded. 32 bits of entropy? I could be mistaken but it'd seem passing in SHA256 hexadecimal as input would degrade security further in this case? (due to only 16 possible values per character)

An nvidia RTX 3080 can perform 2 billion descrypt hashes per second. That would take roughly 2 seconds to iterate through that keyspace (2^32).

I am not entirely familiar with this mailcrypt feature and implementation you've done to review that properly. But if I understand correctly, if an attacker has the encrypted email and these CRYPT passwords db, then they may not be able to recover the original plaintext password that gets preprocessed with SHA256, but they'd have an easy time getting the decryption key for the reading the mail of each user?


Looking into the PR logic I notice the two following lines:

# Creates a keypair with password value hashed (SHA256) to encrypt/decrypt mail with:
doveadm -o plugin/mail_crypt_private_password="$(echo -n "${PASSWD}" | sha256sum | awk '{print $1}')" mailbox cryptokey generate -u "${FULL_EMAIL}" -U

# Creates password, hashed via `-s` scheme:
HASH="$(doveadm pw -s SHA512-CRYPT -u "${FULL_EMAIL}" -p "${PASSWD}")"

That is, mailbox cryptokey generate is not hashing any password input is it? It's just SHA256 of whatever PASSWD is no? That's the password to the keypair generated?

While doveadm pw is creating passwords to store in the passwd db, but I'm not sure if the config file arg is a default or override scheme compared to the CLI -s that specifies a non-default scheme...? I assume that CLI is taking priority here and SHA512-CRYPT is the scheme used?


Dovecot docs:

  • Password schemes. Suggests {BLF-CRYPT}bcrypt-hash-here I think for storage.
  • Passwd-file. Note the 4 listed formats and default CRYPT. Other schemes must prefix the password when stored.
  • passdb settings, states that override_fields = userdb_mail_crypt_private_password=%{sha256:password} overrides the returned value (note it says after, not before lookup).
  • Config Variables implies that is taking a plaintext password input and using sha256 on it? (you are using sha256 twice? where the 1st time in the shell provides it as the "plaintext"?)
  • doveadm pw and doveadm mailbox cryptokey.

Other references:


Assumption of how it fits together (feel free to correct me :smile: ):

  • User password is set and uses SHA512-CRYPT for storage in db.
  • passdb handles auth requests with user pass hashed to (eg CRYPT), then compares to storage (SHA512-CRYPT)? (mismatch, failure?)
  • mailcrypt feature generates key with user password => SHA256.
  • mailcrypt unlocks key to decrypt/~~encrypt~~ mail with user password from passdb (if auth successful, returns value (lookup), but due to override returns SHA256 on user password (plaintext) instead?) - IIRC one of you said the encryption at rest is implicit, only decryption requires user password.

polarathene avatar Sep 09 '21 09:09 polarathene

As far as I understand it (I read the source last year, but some details might have been lost)

* `mailcrypt` feature generates key with user password => SHA256.

Mailcrypt generates a random private-key (and corresponding public key) and encrypts it with the user password. %{sha256:password} instead encrypts it with the sha256 hash of the password. I think this helps to disconnect the encryption of the private key from the password used for login. But more importantly, it prevents certain password characters getting interpreted as escapes.

* `mailcrypt` unlocks key to decrypt/~encrypt~ mail with user password from passdb (if auth successful, returns value (lookup), but due to override returns SHA256 on user password (plaintext) instead?) - IIRC one of you said the encryption at rest is implicit, only decryption requires user password.

It actually does not use passdb, (since it doesn't store the plain text password), it uses the password available during the login exchange. So it opens the private key when the client connects, and as long as the session is live, keeps the key "open".

I have to be honest, I don't understand the interaction with the passdb storage of the password. I would expect that all the normal password related stuff shouldn't be affected by this change. That is confusing me a bit about selecting a different crypt scheme.

DavyLandman avatar Sep 09 '21 09:09 DavyLandman

Thanks @polarathene and @DavyLandman. I appreciate your patience, time put into reviewing this, etc. We can figure out what's more secure and tweak as necessary once I get this at least working.

Ok, I had an issue last night where updating my user's password causes emails to show up blank, not be deletable, and throw:

deliver       | Sep  9 12:50:09 xkr dovecot: imap(nathan@x)<2640><lbnCbo/LXMVAcLJy>: Error: Mailbox INBOX: UID=121: read() failed: read(/var/mail/x/nathan/cur/1631191807.M296711P2944.xx,S=7223,W=7364:2,S) failed: Private key not available: Cannot decrypt key 18xa0d0c4x77391: Cannot decrypt key 4f4xx0f4d9f3816692b3: Password not available (FETCH BODY[])

The update seems to have worked:

[ec2-user@ip-x docker-mailserver]$ ./setup.sh email update nathan@x
Enter New Password:
Enter Old Password (to update encryption key):
result: Changed password for 1 key(s)
Successfully Updated nathan@x

Restarting the container with down and then up this morning shows that it still doesn't work...

However, I can down a second time before I up and it does start working. This indicates to me that we're not restarting something important when we've got the encryption enabled... Need some time to dig into this. A container restart shouldn't be necessary other than setting the ENV to enable this feature...

NorseGaud avatar Sep 09 '21 12:09 NorseGaud

did you close the connections to the server (from your mail client) while doing this change? To rule out that this is about an old active session that was keeping some old keys alive?

DavyLandman avatar Sep 09 '21 14:09 DavyLandman

did you close the connections to the server (from your mail client) while doing this change? To rule out that this is about an old active session that was keeping some old keys alive?

Yep, just retried that again. After a full down and up of docker-compose, it all works again. Before that, emails can't be read or deleted but email does show in the email client with a subject. It's just the contents and any write operations from what I can tell.

NorseGaud avatar Sep 09 '21 23:09 NorseGaud

I removed the sha256 password and still doesn't work unless I down and up. Same with adding a user... If anyone has any ideas, please let me know!

NorseGaud avatar Sep 11 '21 22:09 NorseGaud

If anyone has any ideas, please let me know!

I think I should have time this week. I have to look into the Debian 11 Bullseye PR failing test issue first, I'll chime in with any findings on my end if I try tackle this :)

polarathene avatar Sep 11 '21 23:09 polarathene

Ok, so ANY adding of a new key/signing of the key (like when I add a new email user) seems to trigger the bad state and requires a reboot. This indicates to me the problem is coming from a centralized location they all share, like the userdb maybe.

NorseGaud avatar Sep 13 '21 12:09 NorseGaud

Ok, found two problems. They didn't solve it though.

I confirmed that a second account, which I'm not updating the password for, can also not read email bodies when I update the password and encryption of others.

So it looks like something that's global is becoming corrupted.

NorseGaud avatar Sep 16 '21 23:09 NorseGaud

deliver       | Sep 16 23:20:02 xkr dovecot: imap([email protected])<4524><z9e4DSXMM67Ra7sA>: Error: Mailbox Trash: Saving mail: read() failed: read(/var/mail/xkr.email/mike/cur/1630301456.M318468P5503.ip-172-31-3-247.us-west-2.compute.internal,S=6014,W=6118:2,RS) failed: Decryption error: no private key available (read reason=)
deliver       | Sep 16 23:20:02 xkr dovecot: imap([email protected])<4524><z9e4DSXMM67Ra7sA>: Debug: Mailbox Trash: saving UID 0: Opened mail because: mail stream
deliver       | Sep 16 23:20:02 xkr dovecot: imap([email protected])<4524><z9e4DSXMM67Ra7sA>: Panic: file ostream-encrypt.c: line 631 (o_stream_encrypt_close): assertion failed: (estream->finalized || estream->ctx_sym == NULL || estream->ostream.ostream.stream_errno != 0)
deliver       | Sep 16 23:20:02 xkr dovecot: imap([email protected])<4524><z9e4DSXMM67Ra7sA>: Error: Raw backtrace: /usr/lib/dovecot/libdovecot.so.0(+0xdb73b) [0x7f8b2905173b] -> /usr/lib/dovecot/libdovecot.so.0(+0xdb7d1) [0x7f8b290517d1] -> /usr/lib/dovecot/libdovecot.so.0(+0x4a199) [0x7f8b28fc0199] -> /usr/lib/dovecot/libdovecot.so.0(+0x49276) [0x7f8b28fbf276] -> /usr/lib/dovecot/libdovecot.so.0(+0xffd5e) [0x7f8b29075d5e] -> /usr/lib/dovecot/libdovecot.so.0(o_stream_destroy+0x16) [0x7f8b29075d86] -> /usr/lib/dovecot/libdovecot-storage.so.0(maildir_save_finish+0x18d) [0x7f8b2918846d] -> /usr/lib/dovecot/libdovecot-storage.so.0(mailbox_save_cancel+0x4d) [0x7f8b2916067d] -> /usr/lib/dovecot/libdovecot-storage.so.0(mail_storage_copy+0x122) [0x7f8b29152f22] -> /usr/lib/dovecot/modules/lib10_quota_plugin.so(+0xf80c) [0x7f8b28d9180c] -> /usr/lib/dovecot/libdovecot-storage.so.0(+0x5badc) [0x7f8b29160adc] -> dovecot/imap(+0x1299e) [0x55c62928799e] -> dovecot/imap(command_exec+0x70) [0x55c629294dc0] -> dovecot/imap(+0x1e3f2) [0x55c6292933f2] -> dovecot/imap(+0x1e494) [0x55c629293494] -> dovecot/imap(client_handle_input+0x1b5) [0x55c629293845] -> dovecot/imap(client_input+0x7e) [0x55c629293d6e] -> /usr/lib/dovecot/libdovecot.so.0(io_loop_call_io+0x6f) [0x7f8b29067bef] -> /usr/lib/dovecot/libdovecot.so.0(io_loop_handler_run_internal+0x136) [0x7f8b290691e6] -> /usr/lib/dovecot/libdovecot.so.0(io_loop_handler_run+0x4c) [0x7f8b29067c8c] -> /usr/lib/dovecot/libdovecot.so.0(io_loop_run+0x40) [0x7f8b29067df0] -> /usr/lib/dovecot/libdovecot.so.0(master_service_run+0x13) [0x7f8b28fe8123] -> dovecot/imap(main+0x325) [0x55c629285bf5] -> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb) [0x7f8b28dd409b] -> dovecot/imap(_start+0x2a) [0x55c629285d8a]
deliver       | Sep 16 23:20:02 xkr dovecot: imap([email protected])<4524><z9e4DSXMM67Ra7sA>: Fatal: master: service(imap): child 4524 killed with signal 6 (core not dumped - https://dovecot.org/bugreport.html#coredumps - set /proc/sys/fs/suid_dumpable to 2)

NorseGaud avatar Sep 17 '21 00:09 NorseGaud