S/MIME at-rest encryption support

[TaaviE]

"Small" PR to add S/MIME at-rest encryption. With both RSA and EcDSA support. Tests for most code paths (even a few for existing PGP impl), some also cross-reference the result with openssl. Available API for management is minimal, but should suffice. Encrypted mail can successfully be decrypted by Thunderbird, Apple Mail and Outlook (if configured to do so).

In terms of what needs improvement:

• Function encryptMessage(encryptionKey, raw, callback) that was already previously deprecated but exists for zonemta-wildduck is PGP-only and should be cleaned up.
• Trying it out in a full test-deployment to see if everything works throughout (POP3, webhooks and so on).

[NickOvt]

Hello!

There are numerous errors with the given PR:

1. Tests don't pass
2. Linting doesn't pass
3. I would highly suggest not adding raw crypto primitives into WD codebase if possible. Is there really no smime related npm packages already? (so that the need for smime.js is removed).
4. Any refactors of WD functions (message-handler addAsync in particular, but applies to other funcs as well) is preferred to be carried out via separate branches and thus PRs or at least a single separate PR only for refactors.
5. I would prefer not to have a encryptAndPrepareMessageAsync function, but rather still have two different functions. One that encrypts and the other that prepares. New functions are preferred to be lean and do one thing without side-effects.

[NickOvt]

If there is no smime npm package available, I would strongly suggest to create one, a general package, that can be used not only in WD.

[NickOvt]

1. Tests don't pass
2. Linting doesn't pass
3. I would highly suggest not adding raw crypto primitives into WD codebase if possible. Is there really no smime related npm packages already? (so that the need for smime.js is removed).
4. Any refactors of WD functions (message-handler addAsync in particular, but applies to other funcs as well) is preferred to be carried out via separate branches and thus PRs or at least a single separate PR only for refactors.
5. I would prefer not to have a encryptAndPrepareMessageAsync function, but rather still have two different functions. One that encrypts and the other that prepares. New functions are preferred to be lean and do one thing without side-effects.

[TaaviE]

Tests don't pass
Linting doesn't pass

They previously didn't because the linter aborted the CI, which I've now resolved. Looking at the latest CI run it seems like user creation failed, but I don't think the current changes affect that? If you could check why those tests fail, that would be great.

I would highly suggest not adding raw crypto primitives into WD codebase if possible. Is there really no smime related npm packages already? (so that the need for smime.js is removed).
If there is no smime npm package available, I would strongly suggest to create one, a general package, that can be used not only in WD.

In general there are no good CMS libraries for Node. They're either outdated, unmaintained, fundamentally incorrect or all three at the same time. I even left a note about one of the (rather ridiculous) things I stumbled upon, if the upstream bug is fixed then it can maybe be used.

In terms of making a separate package, I don't think anyone is interested in providing this as a library (especially if other people might start relying on it).

I would prefer not to have a encryptAndPrepareMessageAsync function, but rather still have two different functions. One that encrypts and the other that prepares. New functions are preferred to be lean and do one thing without side-effects.

Preparing a message for encryption is a very connected process due to header manipulation. This is especially so if RFC 9788 (Header Protection) is to be implemented. While they could be split, it wouldn't provide any meaningful separation and could be a source of bugs if changed or tested separately.

Any refactors of WD functions (message-handler addAsync in particular, but applies to other funcs as well) is preferred to be carried out via separate branches and thus PRs or at least a single separate PR only for refactors.

Because of the previous point, only very tiny changes to addAsync and similar would stand on their own. Although if you see sections that could be pulled in separate, we can do that. Overall it was written with only PGP in mind and to add S/MIME support it had to be cleaned up a bit.

[NickOvt]

It is strongly encouraged not to force push to git branches, to have a clearer timeline of changes and diffs.

[TaaviE]

That applies to general-use branches, PR/MR branches should not contain fixup or merge commits.

[NickOvt]

Wildduck PRs are Squashed and Merged, there is no reason to follow the rule of force pushing to PR. Keep a clean commit history here, when PR is ready to be merged, the commits will be squashed anyway. Now it makes it difficult to follow, what exactly was changed between different commits. Last force push shows for example that nothing was changed...

[TaaviE]

Squash merge has a bad habit of breaking commit signatures.

[TaaviE]

There were a few tests that were dependent on each other using a PGP key that was too small, now fixed. Caused by applying same requirements on PGP as I did with S/MIME.

[TaaviE]

I've fixed the last test, the old implementation exposed (and relied on) all the message headers. It is much more selective now based on the guidance of relevant RFCs with the exception of Subject.

Subject should be replaced with [...] in the outer headers, but that not might be desirable for all MUAs and should probably be a separate user-configurable option. So I fixed the test by keeping it as it was. We can refine it in later PRs.

In terms of commit log, it should be neat enough to merge without squash (that'd break the signature). (Unless something new is found and should be fixed.)

[TaaviE]

I've removed the unnecessary dependency on PKI.js, it was too dangerous of a library as it caught and hid fatal exceptions. Plus a few more tests for the few things that couldn't be brought in from any other package.

I've also added a guard to warn against when OpenSSL has PKCS#1 v1.5 padding disabled, because it's vital for Apple Mail. This avoids starting WildDuck in a way that causes issues.

Everything works with Thunderbird and Apple Mail, testing with Outlook (New) would still be nice.

[NickOvt]

I've fixed the last test, the old implementation exposed (and relied on) all the message headers. It is much more selective now based on the guidance of relevant RFCs with the exception of Subject.

Subject should be replaced with [...] in the outer headers, but that not might be desirable for all MUAs and should probably be a separate user-configurable option. So I fixed the test by keeping it as it was. We can refine it in later PRs.

In terms of commit log, it should be neat enough to merge without squash (that'd break the signature). (Unless something new is found and should be fixed.)

Per WD repo policy, all PRs are squashed no matter the circumstances.

[NickOvt]

Overall the PR is starting to look good.
In smime.js combine the CBC and GCM functions. They are practically they same, the only major different that GCM provides AuthEnvelopedData while CBC naturally provides a EnvelopedData obj. Create a createEnvelopedData function that will take as param that it is either auth or not and based on it return the correct type, default to no auth EnvelopedData. Then you can easily combine two encryption functions and pass the encryption mode as a param. That should make the file bit smaller and the whole thing easier to follow

[TaaviE]

They are practically they same, the only major different that GCM provides AuthEnvelopedData while CBC naturally provides a EnvelopedData [...]

One is AEAD and the other isn't. That makes them very different in practice, nor are these the only (non-)AEAD algorithms that could be used. Mixing two different modes of encryption (or storage) is bad practice in cryptographic code.

[NickOvt]

They are practically they same, the only major different that GCM provides AuthEnvelopedData while CBC naturally provides a EnvelopedData [...]

One is AEAD and the other isn't. That makes them very different in practice, nor are these the only (non-)AEAD algorithms that could be used. Mixing two different modes of encryption (or storage) is bad practice in cryptographic code.

Indeed they are very different, but from code point of view, they are almost the same in output. What I mean is that, it would be great to have some sort of wrapper on top of these function or a more generic function that would be more easily extendable later on if some other algorithms are to be user or added, or KDFs changed, or other sha used, sha512 or whatever. Currently that makes it all sort of hardcoded

[NickOvt]

And really, at this point the SMIMEEncryptor could just as well be moved into a separate package :)

[TaaviE]

What I mean is that, it would be great to have some sort of wrapper on top of these function or a more generic function that would be more easily extendable later on if some other algorithms are to be user or added

That's basically what _encryptSmimeAsync does. The allowable combinations of algorithms is rather narrow, the practically usable set of combinations of algorithms is even smaller. For this reason clear separation within these separate sets is important. It prevents invalid combinations and makes the implementation itself easier to verify (or replace) looking at a single standard at a time. (Plus avoids mistakes like the ones made by PKI.js.)

Any mistakes in that logic could result in mail that can't be decrypted, so it's vital to make it as easy to follow as possible.

In terms of future perspective, if anything is to be added, it's going to be a whole new set of almost everything. Like ML-KEM+SHAKE + AES-256 for post-quantum support. That would need a reasonable implementation in STD.

And really, at this point the SMIMEEncryptor could just as well be moved into a separate package :)

True, but that will likely tempt others to use it considering the sorry state of the Node ecosystem and I don't think this is a burden we want. Maybe in the future, outside the scope of this PR/MR.

[NickOvt]

_encryptSmimeAsync relies on if statements to choose path, that is exactly what I was talking about in previous comment. This is fine, for now, but I would prefer a map approach or something less fixed, more dynamic. Less log code as well as you can just build the log based on cipher.

For reference:

Content Encryption

Algorithm	Status
AES-128-CBC	MUST support
AES-192-CBC	SHOULD
AES-256-CBC	SHOULD
AES-128-GCM	SHOULD (via AuthEnvelopedData)
AES-256-GCM	SHOULD (via AuthEnvelopedData)

Signature Algorithms

Algorithm	Status
RSA with SHA-256	MUST support
RSA with SHA-384	SHOULD
RSA with SHA-512	SHOULD
RSA-PSS with SHA-256	SHOULD
ECDSA with SHA-256	MUST support
ECDSA with SHA-384	SHOULD
ECDSA with SHA-512	SHOULD
RSA with SHA-1	legacy, should not use

The reference is also why I was asking to perhaps move the SMIME encryptor into a separate package, but for now indeed let's keep it in WD codebase, it can be moved out at any time.

[TaaviE]

The reference applies to MUAs and their algorithm support nor do we do any signing.

Supporting smaller AES key sizes would have no practical benefit. Any hash functions besides SHA256 for MGF or KDF is simply not supported by anything. It'd really just be an easy way to generate mail that "can't" be decrypted. The current options are the only remotely viable ones up until PQ support improves in MUAs.

[NickOvt]

After a more thorough review:

Critical Issues

1. Breaking change to encryptMessage() signature -- will break zonemta-wildduck (CRITICAL)

The callback-based encryptMessage(encryptionKey, raw, callback) now expects an encryptionKey object instead of a string pubKey. The shim at message-handler.js line ~1361 handles typeof encryptionKey === 'string' by wrapping it as { type: 'pgp', key: encryptionKey }. However, the return value has also changed: it now returns { type, raw } instead of just the raw Buffer. External consumers (zonemta-wildduck) that call encryptMessage(pubKey, raw, (err, encrypted) => { if (encrypted) raw = encrypted; }) will now receive an object, not a Buffer, and will store a stringified [object Object] or fail.

The PR description itself acknowledges: "Function encryptMessage(encryptionKey, raw, callback) that was already previously deprecated but exists for zonemta-wildduck is PGP-only and should be cleaned up." -- but the shim doesn't fully maintain backward compatibility.

Fix needed: The shim should unwrap the result for string callers: callback(null, res ? res.raw : res).

2. PGP behavior change: entire raw message now encrypted, not just body (CRITICAL REGRESSION)

The old PGP encryption separated headers from body, moved Content-Type and Content-Transfer-Encoding headers into the encrypted inner part, and kept all other headers on the outer envelope. The new _encryptPgpAsync encrypts the entire raw message (raw is passed to openpgp.createMessage({ binary: raw })), including all headers.

This means:

• The encrypted inner body now contains duplicate headers (From, To, Subject, Date, etc. appear in both the outer and inner encrypted part)
• The PGP-encrypted payload now includes headers that previously stayed outside -- this changes the format of encrypted messages and could confuse MUAs or break existing message recovery workflows
• The old behavior explicitly kept original Content-Type/Content-Transfer-Encoding as inner headers because they're body semantics. Now they're just part of the full raw blob.

While the outer headers are selected via OUTER_HEADER_NAMES (a good improvement), the inner content is the full original message. This is a significant behavioral change for PGP that needs explicit consideration.

3. raw.fill(0) zeroing original message buffer is dangerous (HIGH)

At message-handler.js, after successful encryption:

if (result && Buffer.isBuffer(raw)) {
    raw.fill(0);
}

This mutates the caller's buffer. In on-append.js, filter-handler.js, submit.js, etc., the callers receive raw from incoming connections or message compilation. If encryption succeeds but any subsequent step fails (e.g., messageHandler.add() throws), the original plaintext is irrecoverably destroyed. The caller has no way to retry or fall back.

In a high-volume system, this creates a data loss vector. If the database is temporarily unavailable and add fails after encryption, the message is lost.

Recommendation: Remove the raw.fill(0) from encryptMessageAsync. If plaintext zeroing is desired, it should be done by the caller after it has confirmed the message was fully stored.

4. on-copy.js now encrypts on both userData.encryptMessages AND targetData.encryptMessages (BEHAVIORAL CHANGE)

let targetEncrypted = !!(targetData.encryptMessages || userData.encryptMessages);

Previously, only targetData.encryptMessages was checked for COPY operations. Now if a user has encryptMessages: true globally, copying to ANY mailbox (including unencrypted ones) will re-encrypt. This is a policy change that might surprise users -- e.g., copying from Encrypted Mailbox A to non-encrypted Mailbox B would now encrypt in B too.

The same issue appears in message-handler.js MOVE logic.

5. openpgp 5.x to 6.x major version upgrade (HIGH RISK)

openpgp v6 is a major version with breaking API changes. The diff shows minRSABits changing from 1024 to 2048 in both the encryption path and the key verification function. This means:

• Existing users with RSA 1024 PGP keys will have their encryption silently fail -- encryptMessageAsync returns false and the message is stored unencrypted
• Key verification in getKeyInfo() now rejects 1024-bit keys, breaking user creation/update with existing small keys

There should be a migration path or at minimum a log warning. The test fixtures were updated to use 2048-bit keys, but existing production users aren't migrated.

Also: openpgp v6 dropped its asn1.js dependency (visible in package-lock.json), which means the bundle got smaller, but the API surface may differ in subtle ways that aren't covered by the existing tests.

---

High-Priority Issues

6. Startup-time RSA key generation (lib/smime.js lines ~1747-1752)

let { publicKey: probeKey } = crypto.generateKeyPairSync('rsa', { modulusLength: 2048 });
let probeCt = crypto.publicEncrypt(...)

This generates a 2048-bit RSA key pair synchronously on every import/require of lib/smime.js. On some systems this can take 100-500ms, and it blocks the event loop at startup. In a high-volume deployment where WildDuck processes restart frequently, this adds latency.

Recommendation: Either make this lazy (check on first use) or use a smaller key for the probe (the probe is just testing if the padding mode works, not actual encryption security).

7. crypto.webcrypto.subtle usage (lib/smime.js line 1740)

const subtle = crypto.webcrypto.subtle;

crypto.webcrypto.subtle.wrapKey/importKey are used for AES-KW in the ECDH path. While these are pure JS under the hood in Node.js (backed by OpenSSL), they're part of the WebCrypto API which has slightly different error semantics.
Just in case check if errors are correctly handled.

8. parseRecipients silently skips invalid certificates (lib/smime.js lines ~2006-2038)

The parseRecipients function catches all errors and continues, which means invalid or malformed certs silently disappear. Combined with _encryptSmimeAsync which also validates and filters certs, there's double validation with different behavior (one logs and skips, the other silently skips). A certificate that passes validateCertKey might fail parseRecipients for different reasons (e.g., ASN.1 parsing issues), and vice versa.

9. Missing S/MIME fields in user cache projections

In on-copy.js, userData is fetched from the user cache. The user cache projection must include the new smimeCerts, smimeCipher, smimeKeyTransport fields. Check that the user cache fetch in the COPY handler includes these fields.

---

Medium-Priority Issues

12. async IIFE pattern in callback-based code (on-append.js, submit.js)

Several locations wrap an async function in an immediately-invoked (async () => { ... })() to bridge callback-based code. This pattern is fine functionally, but any rejection from the async IIFE that isn't caught will produce an unhandled rejection crash:

• on-append.js ~line 558-648: The IIFE catches encryption errors but messageHandler.add() inside has its own error callback. If the IIFE itself throws (e.g., from the async code before messageHandler.add()), there's no .catch() on the IIFE.
• submit.js ~line 89-189: Same pattern.

Recommendation: Add .catch(err => callback(err)) to the IIFE invocations.

13. OUTER_HEADER_NAMES list may be too restrictive

The outer envelope headers are hardcoded:

const OUTER_HEADER_NAMES = [
    'date', 'subject', 'from', 'to', 'message-id', 'mime-version',
    'bimi-location', 'bimi-indicator', 'authentication-results'
];

Missing notable headers: reply-to, list-id, list-unsubscribe, references, in-reply-to. These are needed by MUAs for threading and list management. Without in-reply-to and references on the outer envelope, IMAP threading (THREAD command) and client-side threading will break for encrypted messages.

Also missing: cc, bcc -- cc is arguably needed for proper IMAP ENVELOPE response. The test explicitly checks that Cc is NOT on the outer envelope, which means the IMAP ENVELOPE command will return an incomplete envelope for encrypted messages. This is a significant behavioral difference from the old PGP path (which kept all headers except Content-Type/Content-Transfer-Encoding on the outer part).

14. S/MIME priority over PGP in getUserEncryptionKey

function getUserEncryptionKey(userData) {
    if (userData.smimeCerts && userData.smimeCerts.length) {
        return { type: 'smime', ... };
    }
    if (userData.pubKey) {
        return { type: 'pgp', key: userData.pubKey };
    }
    return false;
}

S/MIME always takes priority if both are configured. While the API enforces mutual exclusivity on creation/update, existing users with both pubKey and smimeCerts in their database document would silently switch from PGP to S/MIME without the user being aware.

15. No certificate expiry validation

validateCertKey checks key type and size but not whether the certificate has expired. An expired certificate will still be accepted, and encryption will proceed with a cert that should no longer be used. At a minimum, a warning log would be appropriate.

16. Max 16 S/MIME certificates per user

smimeCerts: Joi.array().items(...).max(16)

Each certificate results in a separate RecipientInfo in the CMS structure. For RSA-2048, each RecipientInfo is ~256 bytes of encrypted key + ASN.1 overhead. 16 recipients would add ~5-6KB to every encrypted message. For a system with hundreds of terabytes of stored email, this overhead per message could be significant if widely used. Consider whether 16 is the right limit or if a lower default (e.g., 5) would be more appropriate.

17. No Joi validation for smimeCipher and smimeKeyTransport in create endpoint

In the create user endpoint, smimeCipher uses Joi.string().default(...) without .valid(), while the update endpoint properly uses .valid(...SMIMEEncryptor.CIPHERS). This means a user can be created with an invalid cipher value like 'AES-XTS' which will fail at encryption time.

---

Low-Priority / Style Issues

18. Blank line left after removing encryptMessage import (lib/api/messages.js line 9)

The removal of the const encryptMessage = ... line leaves a double blank line.

19. Blank line left after removing promisify calls (lib/filter-handler.js lines ~441-442)

Same double-blank-line issue after removing this.prepareMessage and this.encryptMessage.

21. smime.js formatting inconsistency

The ASN.1 builder code in smime.js doesn't follow the project's Prettier formatting (4-space tabs, single quotes, 160 char width). Many lines are densely packed with multiple ASN.1 constructions on single lines. While the cryptographic code benefits from compactness for verification, it should still follow the project's formatting standard.

22. asn1js version pinning

"asn1js": "^3.0.7" uses caret range. Given this is cryptographic code, consider exact pinning to prevent unexpected behavioral changes from minor/patch updates.

---

Positive Aspects

1. Good test coverage: ~1400 lines of tests covering GCM, CBC, RSA/EC key types, multiple key sizes, mixed recipients, re-encryption prevention, key material zeroing, large messages, and edge cases. Cross-verification with OpenSSL CLI is excellent.

2. Pure JS implementation: Uses asn1js (pure JS) + Node.js built-in crypto module. No native C compiled dependencies, satisfying the cross-distro requirement.

3. Proper key material zeroing: CEK is zeroed in finally blocks, shared secrets are zeroed. The zeroing tests verify this works even when errors occur.

4. Encryption error handling vastly improved: The old code had catch (err) { // ignore } in multiple places. The new code logs structured GELF events for all encryption failures, making debugging in production much easier.

5. Cleaner code structure: The old MOVE/COPY encryption code with deeply nested callbacks and promise-within-promise patterns was brittle. The new encryptAndPrepareMessageAsync consolidation is much cleaner.

6. Proper "already encrypted" detection: The old code only detected multipart/encrypted (PGP). The new isMessageEncrypted() also detects S/MIME enveloped-data and authEnveloped-data content types.

---

Recommendations Before Merge

1. Fix the backward-compat shim for encryptMessage() to unwrap { raw } back to just Buffer for string-key callers
2. Remove raw.fill(0) from encryptMessageAsync -- it's a data-loss risk in the storage pipeline
3. Add in-reply-to, references, reply-to, cc to OUTER_HEADER_NAMES -- threading and ENVELOPE response depend on these
4. Reconsider the PGP behavioral change (encrypting full raw vs. body-only) -- this affects all existing PGP users
5. Add .catch() to IIFE patterns in on-append.js and submit.js
6. Fix Joi validation for smimeCipher in the create user endpoint
7. Consider the COPY/MOVE encryption policy change (user-level encryptMessages now triggers on all copies)
8. Add migration notes for the openpgp 5 to 6 upgrade and the RSA 1024 rejection
9. Make the RSA probe lazy to avoid blocking startup

[TaaviE]

1. Breaking change to encryptMessage() signature -- will break zonemta-wildduck (CRITICAL)

It needs an accompanying update and cleanup anyways for proper support. If it relies on a specific API, it should have (had) tests in the first place. The string key fallback in that sense is a mistake.

2. PGP behavior change: entire raw message now encrypted, not just body (CRITICAL REGRESSION)

That's just how headers are encrypted (based on relevant standards). All of them being exposed previously was a big flaw.

3. raw.fill(0) zeroing original message buffer is dangerous (HIGH)

Zeroing memory that stored sensitive data is intentional. If anything subsequent fails, it should be retried with the encrypted content anyways. If any failure handling actually would need re-encrypting it should be changed.

4. on-copy.js now encrypts on both userData.encryptMessages AND targetData.encryptMessages (BEHAVIORAL CHANGE)

When encryption is enabled, encryption is done, of course.

5. openpgp 5.x to 6.x major version upgrade (HIGH RISK)

If any users exist, they should do the migration themselves and enroll new keys that aren't obsolete. Enforcing different requirements on PGP and S/MIME is not a good idea.

This is just something that would have to mentioned in the changelog

6. Startup-time RSA key generation (lib/smime.js lines ~1747-1752)

Smaller keys might be forbidden by OSSL, thus the check can fail for unwanted reasons. The check is cheap enough in practice that it shouldn't be changed. One option is re-using snakeoil.key on Linux systems, but that might be more fragile in certain cases.

8. parseRecipients silently skips invalid certificates (lib/smime.js lines ~2006-2038)

Suddenly faulty certificates that shouldn't have made it to user certificate storage should not cause complete encryption failures. Failing for one (new) certificate is better than for all of them.

12. async IIFE pattern in callback-based code (on-append.js, submit.js)

Should be fixed now, I think this wasn't done previously either.

13. OUTER_HEADER_NAMES list may be too restrictive

Correct would be to hide basically everything, but for UX reasons a minimum viable set is exposed. More is not necessary for (unencrypted) email listing - it would significantly weaken privacy protections.

14. S/MIME priority over PGP in getUserEncryptionKey

S/MIME can't be enabled before disabling PGP.

15. No certificate expiry validation

Checking expiration would be a massive footgun, especially if people have a backup key in offline storage.

16. Max 16 S/MIME certificates per user

Generating a symmetric key for any recipient is extremely cheap, it might as well be a 100 certificates and there would be no issues.

17. No Joi validation for smimeCipher and smimeKeyTransport in create endpoint

Should be fixed.

[NickOvt]

Sorry, this wasn't a discussion, but requirements to get this PR accepted.

1. Breaking change to encryptMessage() signature -- will break zonemta-wildduck (CRITICAL)

It needs an accompanying update and cleanup anyways for proper support. If it relies on a specific API, it should have (had) tests in the first place. The string key fallback in that sense is a mistake.

2. PGP behavior change: entire raw message now encrypted, not just body (CRITICAL REGRESSION)

That's just how headers are encrypted (based on relevant standards). All of them being exposed previously was a big flaw.

3. raw.fill(0) zeroing original message buffer is dangerous (HIGH)

Zeroing memory that stored sensitive data is intentional. If anything subsequent fails, it should be retried with the encrypted content anyways. If any failure handling actually would need re-encrypting it should be changed.

4. on-copy.js now encrypts on both userData.encryptMessages AND targetData.encryptMessages (BEHAVIORAL CHANGE)

When encryption is enabled, encryption is done, of course.

5. openpgp 5.x to 6.x major version upgrade (HIGH RISK)

If any users exist, they should do the migration themselves and enroll new keys that aren't obsolete. Enforcing different requirements on PGP and S/MIME is not a good idea.

This is just something that would have to mentioned in the changelog

6. Startup-time RSA key generation (lib/smime.js lines ~1747-1752)

Smaller keys might be forbidden by OSSL, thus the check can fail for unwanted reasons. The check is cheap enough in practice that it shouldn't be changed. One option is re-using snakeoil.key on Linux systems, but that might be more fragile in certain cases.

8. parseRecipients silently skips invalid certificates (lib/smime.js lines ~2006-2038)

Suddenly faulty certificates that shouldn't have made it to user certificate storage should not cause complete encryption failures. Failing for one (new) certificate is better than for all of them.

12. async IIFE pattern in callback-based code (on-append.js, submit.js)

Should be fixed now, I think this wasn't done previously either.

13. OUTER_HEADER_NAMES list may be too restrictive

Correct would be to hide basically everything, but for UX reasons a minimum viable set is exposed. More is not necessary for (unencrypted) email listing - it would significantly weaken privacy protections.

14. S/MIME priority over PGP in getUserEncryptionKey

S/MIME can't be enabled before disabling PGP.

15. No certificate expiry validation

Checking expiration would be a massive footgun, especially if people have a backup key in offline storage.

16. Max 16 S/MIME certificates per user

Generating a symmetric key for any recipient is extremely cheap, it might as well be a 100 certificates and there would be no issues.

17. No Joi validation for smimeCipher and smimeKeyTransport in create endpoint

Should be fixed.

1: No. Keep the signature of the func the same, so that existing zonemta-wildduck version does not need to be updated to work with new code.

3: Recheck all code paths just in case. Overall it seems okay-ish. Consider extreme edge cases or cases that seem impossible (things happen).

5: Explain that to users :)

6: No, check once.

13: We base of off what provides highest interoperability.

14: Either I'm blind or you really can though. You only check input params, but never whether PGP already exists on user.

15: Create a config for that. That either we allow or disallow expired certs. For any meaningful Service Provider who wants to comply to best security standards it would make sense to (at least silently) fail encryption if using expired cert.

16: The point was about added size to the message considering the volume. I will allow 16 for now as SMIME adoption (and email encryption in general) is extremely little due to it's rather bad user experience (Providers such as Protonmail and Tutanota that try to improve the situation basically create a walled garden, other than that, it's tedious to setup encryption for average email user).

[TaaviE]

1: No. Keep the signature of the func the same, so that existing zonemta-wildduck version does not need to be updated to work with new code.

I guess, though zonemta-wildduck still needs an update. I assume you will create a PR yourself to add tests for these helper functions, if it's critical.

3: Recheck all code paths just in case. Overall it seems okay-ish. Consider extreme edge cases or cases that seem impossible (things happen).

I don't see any that would ever re-use raw.

5: Explain that to users :)

If anyone at all provides this as a service then the only migration path is a manual notification anyways. In theory the encryption path can (for now) tolerate <2048-bit keys, but it'd be a hack. It's much more reasonable to note this as a "breaking" change and move on, without creating more legacy to maintain.

6: No, check once.

It basically was, but I've moved it slighly higher up for clarity.

13: We base of off what provides highest interoperability.

That is the widest without unnecessarily compromising privacy and it works in practice. MUA that supports S/MIME or GPG is basically intended to work without external headers.

14: Either I'm blind or you really can though. You only check input params, but never whether PGP already exists on user.

If it already exists they can't enable S/MIME. Even if they somehow do (bypass the API), a precedence change at that point would be expected.

15: Create a config for that. That either we allow or disallow expired certs. For any meaningful Service Provider who wants to comply to best security standards it would make sense to (at least silently) fail encryption if using expired cert.

Absolutely not. If the trust of the certificates is not checked, there's absolutely no point in checking expiration. Except if course when you want to create a footgun at some random date encryption gets disabled.

16: The point was about added size to the message considering the volume. I will allow 16 for now as SMIME adoption (and email encryption in general) is extremely little due to it's terrible user experience (Providers such as Protonmail and Tutanota that try to improve the situation basically create a walled garden, other than that, it's tedious to setup encryption for average email user).

The certificates used aren't placed in the CMS, there's a key for each recipient identified by a SKI. Not being able to deduplicate attachments will dwarf it by a large margin.

[TaaviE]

Also tested with KMail, Claws, Evolution and Trojitá. Works well.

[NickOvt]

Findings

1. High: IMAP COPY now fails open on encryption/preparation errors and still
   inserts the copied message. In this branch, the catch only logs and execution
   continues into insertOne, so an encrypted destination can end up with
   plaintext mail if rebuild, prepare, or attachment-body storage fails. That is
   a regression from master, where those failures aborted the copy. See lib/
   handlers/on-copy.js:226.
2. High: the new S/MIME default is PKCS#1 v1.5, but the code already knows that
   transport may be unavailable on some OpenSSL 3 builds; when that or any other
   S/MIME encryption failure happens, several paths log and store the message
   unencrypted instead of failing closed. For users who explicitly enabled
   encryption, that is a bad default/failure combination. See lib/consts.js:149,
   server.js:47, lib/message-handler.js:273, lib/message-handler.js:2055.
3. Medium: the dependency bump to openpgp@6.1.1 breaks the repo’s declared
   runtime contract. The project still advertises node >=16, but the resolved
   openpgp package now requires >=18. That means installs on currently supported
   Node 16 environments will be invalid even before runtime behavior is
   considered. See package.json:96, package.json:118.
4. Medium: MOVE now does a full user lookup inside the per-message loop, which
   is an avoidable regression for large mailbox moves. master fetched the
   encryption-related user data once before iterating; this branch does one DB
   read per message. That adds latency and failure surface on exactly the long-
   running path you’re trying to simplify. See lib/message-handler.js:1156.

Point 3 can be skipped, I will bump the required node version in separate branch.

Open Questions / Improvements

• The user API descriptions say smimeCipher and smimeKeyTransport “can be
  overridden per-mailbox”, but I couldn’t find mailbox-level storage or override
  logic. If that is not implemented yet, the API text should be corrected now.
• Coverage is strong at the crypto-unit level, but there is still no integration
  coverage for the new S/MIME user API fields or for IMAP COPY/MOVE failure
  behavior. Those are the areas most likely to regress operationally.

[TaaviE]

High: IMAP COPY now fails open on encryption/preparation errors and still
inserts the copied message. In this branch, the catch only logs and execution
continues into insertOne, so an encrypted destination can end up with
plaintext mail if rebuild, prepare, or attachment-body storage fails. That is
a regression from master, where those failures aborted the copy. See lib/
handlers/on-copy.js:226.

High: the new S/MIME default is PKCS#1 v1.5, but the code already knows that
transport may be unavailable on some OpenSSL 3 builds; when that or any other
S/MIME encryption failure happens, several paths log and store the message
unencrypted instead of failing closed. For users who explicitly enabled
encryption, that is a bad default/failure combination. See lib/consts.js:149,
server.js:47, lib/message-handler.js:273, lib/message-handler.js:2055.

If the message was already plaintext and encryption fails there is no point in failing closed and to cause a service degradation. At this point in time it's not a risk that should be taken. In addition to that, anyone that desires "high-assurance" encryption should have their sender encrypt letters in the first place.

Medium: MOVE now does a full user lookup inside the per-message loop, which
is an avoidable regression for large mailbox moves. master fetched the
encryption-related user data once before iterating; this branch does one DB
read per message. That adds latency and failure surface on exactly the long-
running path you’re trying to simplify. See lib/message-handler.js:1156.

Should be better now.

The user API descriptions say smimeCipher and smimeKeyTransport “can be
overridden per-mailbox”, but I couldn’t find mailbox-level storage or override
logic. If that is not implemented yet, the API text should be corrected now.

Yeah, corrected.