From 47c3700380a922d5d25713c61d3a935ba47ffcdb Mon Sep 17 00:00:00 2001 From: Lucas Rayan Date: Mon, 24 Aug 2026 08:34:14 -0300 Subject: [PATCH] fix(dns): reconciliar reescrevia os mesmos registos a cada passagem Duas coisas apanhadas ao correr o reconcile contra a zona real depois de a rota passar a funcionar. 1) Nao era idempotente: 0 criados, 4 corrigidos, 11 iguais -- e outra vez 4 corrigidos na passagem seguinte, para sempre, com o temporizador a fazer o mesmo em ciclo. Os 4 eram os SRV. A Cloudflare devolve o content de um SRV SEM a prioridade ("1 993 host" para um registo guardado como "0 1 993 host") e guarda-a em data. O adaptador so recorria a data quando o content vinha vazio, portanto lia sempre um valor diferente do da spec. Para SRV e TLSA a data passa a ser a autoridade. 2) A spec de cada dominio incluia o A e o AAAA do mail host. Um segundo dominio servido pelo mesmo servidor aponta o MX para um host que nao lhe pertence, por isso reconciliar o cienciaembarcada.com.br tentava escrever mail.lucasrguerra.dev.br -- zona alheia. A Cloudflare respondia "an identical record already exists" e a consola mostrava dois erros para um dominio que estava correcto. O mesmo valia para o TLSA, que tambem tem o nome do mail host. Os registos de endereco passam a ser publicados so pela zona que contem o host; o MX e os SRV, que apontam para ele, continuam a ser de cada dominio. Co-Authored-By: Claude Opus 5 --- .../internal/domain/entity/dns_record.go | 46 ++++++++++---- .../internal/domain/entity/dns_record_test.go | 63 +++++++++++++++++++ .../cloudflare/cloudflare_adapter.go | 15 +++-- .../cloudflare/cloudflare_hardening_test.go | 43 +++++++++++++ services/webmail/messages/en.json | 2 +- services/webmail/messages/es.json | 2 +- services/webmail/messages/pt-BR.json | 2 +- 7 files changed, 153 insertions(+), 20 deletions(-) diff --git a/services/protocols/internal/domain/entity/dns_record.go b/services/protocols/internal/domain/entity/dns_record.go index 7f02bf7..808793a 100644 --- a/services/protocols/internal/domain/entity/dns_record.go +++ b/services/protocols/internal/domain/entity/dns_record.go @@ -50,16 +50,15 @@ func BuildDnsRecordSpecs(spec DnsRecordSpec) []DnsRecord { mailHost = fmt.Sprintf("mail.%s", domainName) } + // The address records for the mail host belong to whichever zone actually + // contains that host. A second domain served by the same server points its + // MX at a host it does not own, and publishing an A for it meant trying to + // write into somebody else's zone: the provider answered "an identical + // record already exists" and the console showed errors for a domain that + // was correctly configured. + ownsMailHost := hostInsideDomain(mailHost, domainName) + records := []DnsRecord{ - // 1. A record for mail host - { - Type: "A", - Name: mailHost, - Value: serverIPv4, - TTL: 1, // Auto - Proxied: false, - Comment: "LambdaMail Mail Host A Record", - }, // 3. MX record { Type: "MX", @@ -185,8 +184,20 @@ func BuildDnsRecordSpecs(spec DnsRecordSpec) []DnsRecord { }) } + // 1. A record for the mail host, only when this domain contains it. + if ownsMailHost { + records = append(records, DnsRecord{ + Type: "A", + Name: mailHost, + Value: serverIPv4, + TTL: 1, // Auto + Proxied: false, + Comment: "LambdaMail Mail Host A Record", + }) + } + // 2. Conditional AAAA record if IPv6 is supplied - if serverIPv6 != "" { + if ownsMailHost && serverIPv6 != "" { records = append(records, DnsRecord{ Type: "AAAA", Name: mailHost, @@ -200,7 +211,7 @@ func BuildDnsRecordSpecs(spec DnsRecordSpec) []DnsRecord { // 11. Conditional TLSA records if DANE is enabled. More than one is normal // during a rollover: the current association and the next one are both // published so the certificate can change without a gap (RFC 7671 8.1). - if spec.DaneEnabled { + if ownsMailHost && spec.DaneEnabled { for _, hash := range spec.TlsaHashes { if hash == "" { continue @@ -268,3 +279,16 @@ func (r DnsRecord) EqualsNormalized(other DnsRecord) bool { } return true } + +// hostInsideDomain reports whether a host name sits inside a domain's zone, +// and so whether this domain is the one that should publish its addresses. +func hostInsideDomain(host, domain string) bool { + host = strings.ToLower(strings.TrimSuffix(strings.TrimSpace(host), ".")) + domain = strings.ToLower(strings.TrimSuffix(strings.TrimSpace(domain), ".")) + if host == "" || domain == "" { + return false + } + // The suffix must fall on a label boundary: "notexample.test" is not + // inside "example.test". + return host == domain || strings.HasSuffix(host, "."+domain) +} diff --git a/services/protocols/internal/domain/entity/dns_record_test.go b/services/protocols/internal/domain/entity/dns_record_test.go index c8b940d..9d52377 100644 --- a/services/protocols/internal/domain/entity/dns_record_test.go +++ b/services/protocols/internal/domain/entity/dns_record_test.go @@ -132,3 +132,66 @@ func TestOnlyTheAvailableDkimKeyIsPublished(t *testing.T) { } } } + +// A second domain served by the same server does not own the mail host. +// +// Every domain's spec included an A and an AAAA for the mail host, so +// reconciling cienciaembarcada.com.br tried to create records for +// mail.lucasrguerra.dev.br - a name in somebody else's zone. Cloudflare +// answered "An identical record already exists" and the console showed the +// operator two errors about a domain that was in fact perfectly configured. +// +// The address records belong to whichever zone actually contains the host. +func TestBuildDnsRecordSpecs_SkipsMailHostAddressesOutsideTheDomain(t *testing.T) { + specs := BuildDnsRecordSpecs(DnsRecordSpec{ + DomainName: "cienciaembarcada.test", + MailHost: "mail.lucasrguerra.test", + ServerIPv4: "192.0.2.1", + ServerIPv6: "2001:db8::1", + DaneEnabled: true, + TlsaHashes: []string{"hash123"}, + }) + + for _, r := range specs { + if r.Type == "A" || r.Type == "AAAA" || r.Type == "TLSA" { + t.Errorf("spec publishes %s for %q, which is not inside cienciaembarcada.test", + r.Type, r.Name) + } + } + + // The records that point AT the mail host are still the domain's own and + // must stay, or the domain stops receiving mail. + var mx bool + for _, r := range specs { + if r.Type == "MX" && r.Value == "mail.lucasrguerra.test" { + mx = true + } + } + if !mx { + t.Error("the domain lost its MX; it would stop receiving mail") + } +} + +// The ordinary case must keep them: a domain whose mail host is its own +// subdomain still needs the address records published. +func TestBuildDnsRecordSpecs_KeepsMailHostAddressesInsideTheDomain(t *testing.T) { + specs := BuildDnsRecordSpecs(DnsRecordSpec{ + DomainName: "example.test", + MailHost: "mail.example.test", + ServerIPv4: "192.0.2.1", + ServerIPv6: "2001:db8::1", + }) + + var a, aaaa bool + for _, r := range specs { + if r.Type == "A" && r.Name == "mail.example.test" { + a = true + } + if r.Type == "AAAA" && r.Name == "mail.example.test" { + aaaa = true + } + } + if !a || !aaaa { + t.Errorf("lost the mail host addresses (A=%v AAAA=%v)", a, aaaa) + } +} diff --git a/services/protocols/internal/infrastructure/cloudflare/cloudflare_adapter.go b/services/protocols/internal/infrastructure/cloudflare/cloudflare_adapter.go index 9b2e8e3..b176fe8 100644 --- a/services/protocols/internal/infrastructure/cloudflare/cloudflare_adapter.go +++ b/services/protocols/internal/infrastructure/cloudflare/cloudflare_adapter.go @@ -130,12 +130,15 @@ func (a *CloudflareAdapter) ListRecords(ctx context.Context, zoneID string) ([]e if r.Proxied != nil { rec.Proxied = *r.Proxied } - // Cloudflare returns SRV and TLSA with content already rendered, - // but older zones and some responses only populate data; fall back - // to it so a structured record is not seen as an empty value and - // rewritten on every reconcile. - if rec.Value == "" && len(r.Data) > 0 { - rec.Value = renderStructuredValue(r.Type, r.Data) + // For SRV and TLSA the structured data is the authority, not the + // rendered content. Cloudflare renders SRV content WITHOUT the + // priority - a record stored as "0 1 993 host" reads back as + // "1 993 host" - so trusting content made every SRV differ from + // the spec and be rewritten on every single reconcile. + if len(r.Data) > 0 { + if rendered := renderStructuredValue(r.Type, r.Data); rendered != "" { + rec.Value = rendered + } } out = append(out, rec) } diff --git a/services/protocols/internal/infrastructure/cloudflare/cloudflare_hardening_test.go b/services/protocols/internal/infrastructure/cloudflare/cloudflare_hardening_test.go index 287d711..ae6905f 100644 --- a/services/protocols/internal/infrastructure/cloudflare/cloudflare_hardening_test.go +++ b/services/protocols/internal/infrastructure/cloudflare/cloudflare_hardening_test.go @@ -235,3 +235,46 @@ func TestCloudflareAdapter_ListRecords_RendersStructuredValues(t *testing.T) { t.Errorf("value = %q, want the rendered presentation form", records[0].Value) } } + +// Cloudflare renders an SRV record's content WITHOUT the priority: the zone +// holding "0 1 993 mail.example.test" answers content "1 993 mail.example.test" +// and keeps the priority in data. Taking content at face value made the record +// differ from the spec on every comparison, so every reconcile rewrote all four +// SRV records - against the live zone, on a timer, forever. +func TestCloudflareAdapter_ListRecords_KeepsSrvPriority(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(map[string]interface{}{ + "success": true, + "result": []map[string]interface{}{{ + "id": "rec_1", "type": "SRV", "name": "_imaps._tcp.example.test", + // Exactly what the API returns: the priority is missing here + // and present in data. + "content": "1 993 mail.example.test", + "data": map[string]interface{}{ + "priority": float64(0), "weight": float64(1), + "port": float64(993), "target": "mail.example.test", + }, + "ttl": float64(1), + }}, + "result_info": map[string]interface{}{"total_pages": float64(1)}, + }) + })) + defer ts.Close() + + adapter := NewCloudflareAdapter("token") + adapter.baseURL = ts.URL + + records, err := adapter.ListRecords(context.Background(), "zone1") + if err != nil { + t.Fatalf("ListRecords: %v", err) + } + if len(records) != 1 { + t.Fatalf("got %d records, want 1", len(records)) + } + + const want = "0 1 993 mail.example.test" + if records[0].Value != want { + t.Errorf("read SRV value %q, want %q: the record will be rewritten on every reconcile", + records[0].Value, want) + } +} diff --git a/services/webmail/messages/en.json b/services/webmail/messages/en.json index a3f648c..bf4d8bf 100644 --- a/services/webmail/messages/en.json +++ b/services/webmail/messages/en.json @@ -4,7 +4,7 @@ "aliasTarget": "Destination address", "aliasesTitle": "Aliases", "auditLog": "Audit log", - "backToWebmail": "Back to webmail", + "backToWebmail": "Webmail", "cancelJob": "Cancel", "certificateState": "Certificate", "checksPassed": "{passed} of {total} passed", diff --git a/services/webmail/messages/es.json b/services/webmail/messages/es.json index 5bec0fd..e32fa0f 100644 --- a/services/webmail/messages/es.json +++ b/services/webmail/messages/es.json @@ -4,7 +4,7 @@ "aliasTarget": "Dirección de destino", "aliasesTitle": "Alias", "auditLog": "Registro de auditoría", - "backToWebmail": "Volver al webmail", + "backToWebmail": "Webmail", "cancelJob": "Cancelar", "certificateState": "Certificado", "checksPassed": "{passed} de {total} aprobadas", diff --git a/services/webmail/messages/pt-BR.json b/services/webmail/messages/pt-BR.json index 41cda3b..85abe32 100644 --- a/services/webmail/messages/pt-BR.json +++ b/services/webmail/messages/pt-BR.json @@ -4,7 +4,7 @@ "aliasTarget": "Endereço de destino", "aliasesTitle": "Apelidos", "auditLog": "Trilha de auditoria", - "backToWebmail": "Voltar ao webmail", + "backToWebmail": "Webmail", "cancelJob": "Cancelar", "certificateState": "Certificado", "checksPassed": "{passed} de {total} aprovadas",