feat: add DANE/TLSA support for outbound SMTP connections#22
feat: add DANE/TLSA support for outbound SMTP connections#22titanism wants to merge 5 commits intozone-eu:masterfrom
Conversation
This commit adds comprehensive DANE (DNS-based Authentication of Named Entities) support for verifying TLS certificates using TLSA records (RFC 6698). Features: - Automatic detection of native dns.resolveTlsa (Node.js v22.15.0+, v23.9.0+) - Custom resolver support for older Node.js versions (e.g., Tangerine) - DANE disabled by default when no resolver is available - Configurable verification mode (enforce or log-only) - Full RFC 6698 support: PKIX-TA, PKIX-EE, DANE-TA, DANE-EE usage types - Certificate verification against TLSA records during TLS upgrade - Parallel TLSA resolution for multiple MX hosts New configuration options: - dane.enabled: Enable/disable DANE verification - dane.resolveTlsa: Custom async function to resolve TLSA records - dane.logger: Logging function for DANE events - dane.verify: Enforce verification (true) or log-only (false) New connection properties: - daneEnabled: Whether DANE is active for the connection - daneVerifier: Certificate verification function for TLS upgrade - tlsaRecords: Array of TLSA records for the MX host - requireTls: Whether TLS is required (set when DANE records exist) For Node.js versions without native TLSA support, use Tangerine: https://github.com/forwardemail/tangerine Closes zone-eu/zone-mta#253
|
I bumped package.json to v1.6.0 since this is a minor release |
…support - Update DANE (RFC 6698, RFC 7671) from Partial to Supported - Full DANE verification for outbound SMTP connections - PKIX-TA, PKIX-EE, DANE-TA, DANE-EE usage types support - Reference: zone-eu/mx-connect#22 - Add CalDAV Scheduling Extensions (RFC 6638 ) as Supported - CalDAV scheduling with iMIP support - Update iTIP (RFC 5546) from Not Supported to Supported - REQUEST, REPLY, CANCEL, and VFREEBUSY methods - Update iMIP (RFC 6047) from Not Supported to Supported - Email-based calendar invitations with response links - Reference: c4d1629 - Update all 24 translated documentation files
|
I will review this later as it is very large change but immediate notes:
|
|
Tangerine is only a devDependency used for tests, I just reverted the package version change. |
|
I just removed |
Reference nodejs/node#57159 which tracks AD flag support needed for proper DNSSEC validation. Document recommendations for production use including DNSSEC-validating resolvers and DNS-over-HTTPS alternatives.
andris9
left a comment
There was a problem hiding this comment.
Note: As this PR was quite big I had Claude review it.
Code Review: DANE/TLSA Support PR #22
Summary
This PR adds DANE (DNS-based Authentication of Named Entities) support for verifying TLS certificates using TLSA records. I've focused on exception safety, memory leaks, and implementation completeness.
CRITICAL ISSUES (Must Fix Before Merge)
1. Uncaught Exception in extractSPKI() - lib/dane.js:103-112
function extractSPKI(cert) {
const publicKey = cert.publicKey;
if (!publicKey) {
return null;
}
// This line can throw if publicKey is invalid format (not null but corrupted)
return nodeCrypto.createPublicKey(publicKey).export({
type: 'spki',
format: 'der'
});
}Problem: nodeCrypto.createPublicKey() throws synchronously if the public key is malformed. This propagates through getCertData() -> verifyCertAgainstTlsa() -> createDaneVerifier() and will crash the process.
Impact: A malicious or misconfigured server sending a malformed certificate will crash the application.
Fix: Wrap in try/catch:
function extractSPKI(cert) {
try {
const publicKey = cert.publicKey;
if (!publicKey) {
return null;
}
return nodeCrypto.createPublicKey(publicKey).export({
type: 'spki',
format: 'der'
});
} catch (err) {
return null;
}
}2. Uncaught Exception in getCertData() - lib/dane.js:121-127
function getCertData(cert, selector) {
if (selector === DANE_SELECTOR.SPKI) {
return extractSPKI(cert);
}
// cert.raw can throw if cert object is malformed
return cert.raw;
}Problem: Accessing cert.raw on a malformed certificate object can throw. The function assumes cert is a valid X509 certificate.
Fix: Add null checks and try/catch protection.
3. DANE-TA/PKIX-TA Cannot Work - Certificate Chain Not Available - lib/dane.js:226-234
function createDaneVerifier(tlsaRecords, options) {
return function checkServerIdentity(hostname, cert) {
// ...
const result = verifyCertAgainstTlsa(cert, tlsaRecords); // No chain passed!Problem: The TLS checkServerIdentity callback only receives the peer certificate, not the full certificate chain. DANE-TA (usage=2) and PKIX-TA (usage=0) require verifying against the certificate chain.
Impact: If a user configures TLSA records with usage=2 (DANE-TA), verification will always fail even with valid certificates because the chain is never passed.
Options:
- Document this limitation clearly in README
- Log a warning when DANE-TA/PKIX-TA records are detected
- Implement chain retrieval via
socket.getPeerCertificate(true)(requires architectural change)
4. Missing Error Handling in verifyCertAgainstTlsa() - lib/dane.js:167-211
The loop over tlsaRecords doesn't catch exceptions from getCertData() or hashCertData():
for (const record of tlsaRecords) {
// ...
const certData = getCertData(cert, selector); // Can throw!
const hashedData = hashCertData(certData, matchingType); // Can throw!Fix: Wrap in try/catch and continue to next record on error.
HIGH-SEVERITY ISSUES
5. Redundant util.promisify() Call on Every Invocation - lib/dane.js:78-80
if (hasNativeResolveTlsa) {
const resolveTlsaAsync = util.promisify(dns.resolveTlsa); // Created every call!Problem: Creates a new promisified function on every TLSA resolution. While not a memory leak, it's inefficient.
Fix: Move outside the function or cache the promisified version:
const resolveTlsaAsync = hasNativeResolveTlsa ? util.promisify(dns.resolveTlsa) : null;6. PKIX Verification Not Complete - lib/dane.js:181-193
if (usage === DANE_USAGE.DANE_EE || usage === DANE_USAGE.PKIX_EE) {Problem: PKIX-EE (usage=1) and PKIX-TA (usage=0) are supposed to also validate the certificate against the system's trusted CA store (PKIX validation), not just match the TLSA record. The current implementation treats them identically to DANE-EE.
RFC 6698 Section 2.1: PKIX usage types require both PKIX path validation AND TLSA matching.
7. Silent DANE Failure Could Be Security Issue - lib/mx-connect.js:127-139
} catch (err) {
// Log error but don't fail the connection
if (delivery.dane.logger) {
delivery.dane.logger({ msg: 'TLSA lookup failed', ... });
}
mx.tlsaRecords = []; // Silently continue without DANE
}Problem: DNS errors (SERVFAIL, timeout) cause DANE to silently fall back to no verification. An attacker who can cause DNS failures could bypass DANE.
Recommendation: When dane.verify = true, DNS lookup failures (except NODATA/NXDOMAIN) should be treated as errors. Consider adding a strictDns option.
MEDIUM-SEVERITY ISSUES
8. Test Coverage Gaps
The tests don't cover:
- Malformed certificates
- Invalid TLSA record formats (missing
certfield, invalid usage values) - Race conditions in parallel TLSA resolution
- Memory pressure scenarios
- Real certificate verification (all tests use mocks)
9. Documentation Claims DANE-TA Support That Doesn't Work
README.md states:
Full RFC 6698 support: PKIX-TA, PKIX-EE, DANE-TA, DANE-EE usage types
But DANE-TA and PKIX-TA cannot work without the certificate chain (see issue #3).
LOW-SEVERITY ISSUES
10. Inconsistent Error Code Handling - lib/dane.js:69-73, 83-87
if (err.code === 'ENODATA' || err.code === 'ENOTFOUND' || err.code === 'ENOENT') {The same check is duplicated in two places. Should be extracted to a helper function like isNotFoundError() (which already exists in lib/tools.js).
11. hasNativeResolveTlsa Uses Callback API Check
const hasNativeResolveTlsa = typeof dns.resolveTlsa === 'function';This checks the callback-style API, but then promisifies it. Should probably also check dns.promises.resolveTlsa for native promise support.
12. No TTL Handling for Cached TLSA Records
Pre-resolved tlsaRecords in MX entries have no TTL enforcement. If users cache and reuse connection objects, stale TLSA records could be used.
Memory Leak Analysis
No memory leaks found. The code properly:
- Doesn't store references in module-level state
- Uses
Promise.all()correctly (array is temporary) - Socket handling defers to existing get-connection.js which properly destroys sockets
Recommendations
- Must fix #1-4 before merge - these can cause process crashes
- Fix #5 for efficiency
- Document #3 and #6 - clarify that only DANE-EE is fully supported
- Consider #7 - add strict DNS mode for high-security deployments
- Add integration tests with real TLS connections
Summary Table
| Issue | Severity | Type | Can Cause Crash? |
|---|---|---|---|
| #1 extractSPKI exception | CRITICAL | Exception | Yes |
| #2 getCertData exception | CRITICAL | Exception | Yes |
| #3 DANE-TA chain missing | CRITICAL | Incomplete | No (fails safe) |
| #4 verifyCertAgainstTlsa exception | CRITICAL | Exception | Yes |
| #5 Redundant promisify | HIGH | Performance | No |
| #6 PKIX validation incomplete | HIGH | Incomplete | No |
| #7 Silent DNS failure | HIGH | Security | No |
Verdict: Do not merge until critical issues #1, #2, and #4 are fixed with proper exception handling. Issue #3 requires either code fix or documentation clarification.
Critical fixes: - Wrap extractSPKI, getCertData, hashCertData in try/catch to prevent process crashes from malformed certificates (Issues zone-eu#1, zone-eu#2, zone-eu#4) - Add informative error messages for DANE-TA/PKIX-TA without chain (zone-eu#3) - Document DANE-TA/PKIX-TA limitations in README (zone-eu#3, zone-eu#9) Performance improvements: - Cache promisified dns.resolveTlsa at module load (zone-eu#5) - Check dns.promises.resolveTlsa for native promise support (zone-eu#11) Security improvements: - Add daneLookupFailed flag for strict DNS failure handling (zone-eu#7) - Add isNoRecordsError helper for consistent error handling (zone-eu#10) Documentation: - Add support status column to DANE usage types table - Document that DANE-EE (usage=3) is recommended for SMTP Tests: - Add 14 new tests for malformed certificates and edge cases - Add tests for DANE-TA/PKIX-TA chain requirement errors - Add tests for hex-encoded cert data handling
|
Done, see commit @andris9 |
This commit adds comprehensive DANE (DNS-based Authentication of Named Entities) support for verifying TLS certificates using TLSA records (RFC 6698).
Features:
New configuration options:
New connection properties:
For Node.js versions without native TLSA support, use Tangerine: https://github.com/forwardemail/tangerine
Closes zone-eu/zone-mta#253