Skip to content

Commit

Permalink
fix: increase TTLs for NODATA responses (#59)
Browse files Browse the repository at this point in the history
* fix: set SOA minimum TTL to 1 day

minimize the need for returning "NODATA-A/AAAA" responses
See: #52

* fix(acme): empty TXT to control TTL of DNS-01

this ensures we ALWAYS control TTL of ACME DNS-01 response
and keep it very low (10s here) to ensure "not set" response is not
cached for hours due to minimal TTL from SOA

* fix: defensive check against bugs in datastore

if dynamodb starts returning empty strings, we want to see that
reflected in metrics

* test: DNS01NotSetValue

making sure we always return "empty" txt
  • Loading branch information
lidel authored Feb 25, 2025
1 parent 65ee56a commit 46e30d6
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 8 deletions.
25 changes: 19 additions & 6 deletions acme/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package acme
import (
"context"
"strings"
"time"

"github.com/coredns/coredns/plugin"
"github.com/ipfs/go-datastore"
Expand All @@ -22,7 +21,10 @@ const (
acmeSubdomain = "_acme-challenge"

// The TTL for the _acme-challenge TXT record is as short as possible
ttl = 10 * time.Second
txtTTL = uint32(10) // seconds

// TXT value returned when broker has no DNS-01 value yet
DNS01NotSetValue = "not set yet"
)

// ServeDNS implements the plugin.Handler interface.
Expand Down Expand Up @@ -59,9 +61,20 @@ func (p acmeReader) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.M
}

val, err := p.Datastore.Get(ctx, datastore.NewKey(peerID.String()))
if err != nil {
containsNODATAResponse = true
dns01ResponseCount.WithLabelValues("NODATA-TXT").Add(1)
if err != nil || len(val) == 0 {
// return "empty" TXT record to have control over TTL that does not depend on minimal TTL from SOA
// (avoiding issue described in https://github.com/ipshipyard/p2p-forge/issues/52)
answers = append(answers, &dns.TXT{
Hdr: dns.RR_Header{
Name: dns.Fqdn(q.Name),
Rrtype: dns.TypeTXT,
Class: dns.ClassINET,
Ttl: txtTTL,
},
Txt: []string{DNS01NotSetValue},
})
// track "empty" TXT separately from NODATA (we do return a record, but DNS-01 value is not set yet)
dns01ResponseCount.WithLabelValues("TXT-EMPTY").Add(1)
continue
}

Expand All @@ -70,7 +83,7 @@ func (p acmeReader) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.M
Name: dns.Fqdn(q.Name),
Rrtype: dns.TypeTXT,
Class: dns.ClassINET,
Ttl: uint32(ttl.Seconds()),
Ttl: txtTTL,
},
Txt: []string{string(val)},
})
Expand Down
39 changes: 39 additions & 0 deletions e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/caddyserver/certmagic"
"github.com/coredns/caddy"
"github.com/coredns/coredns/core/dnsserver"
p2pacme "github.com/ipshipyard/p2p-forge/acme"
"github.com/ipshipyard/p2p-forge/client"
"github.com/libp2p/go-libp2p"
"github.com/libp2p/go-libp2p/core/crypto"
Expand Down Expand Up @@ -277,6 +278,44 @@ func TestSetACMEChallenge(t *testing.T) {
}
}

// Confirm we ALWAYS return empty TXT instead of NODATA to avoid
// issues described in https://github.com/ipshipyard/p2p-forge/issues/52
func TestACMEChallengeNoDNS01Value(t *testing.T) {
t.Parallel()
sk, _, err := crypto.GenerateEd25519Key(rand.Reader)
if err != nil {
t.Fatal(err)
}

h, err := libp2p.New(libp2p.Identity(sk))
if err != nil {
t.Fatal(err)
}

// Note: we don't register – we want DNS-01 to fail

peerIDb36, err := peer.ToCid(h.ID()).StringOfBase(multibase.Base36)
if err != nil {
t.Fatal(err)
}

m := new(dns.Msg)
m.Question = make([]dns.Question, 1)
m.Question[0] = dns.Question{Qclass: dns.ClassINET, Name: fmt.Sprintf("_acme-challenge.%s.%s.", peerIDb36, forge), Qtype: dns.TypeTXT}

r, err := dns.Exchange(m, dnsServerAddress)
if err != nil {
t.Fatalf("Could not send message: %s", err)
}
if r.Rcode != dns.RcodeSuccess || len(r.Answer) == 0 {
t.Fatalf("Expected successful reply with TXT value, got empty %s", dns.RcodeToString[r.Rcode])
}
expectedAnswer := fmt.Sprintf(`%s 10 IN TXT "%s"`, m.Question[0].Name, p2pacme.DNS01NotSetValue)
if r.Answer[0].String() != expectedAnswer {
t.Fatalf("Expected %s reply, got %s", expectedAnswer, r.Answer[0].String())
}
}

func TestIPv4Lookup(t *testing.T) {
_, pk, err := crypto.GenerateEd25519Key(rand.Reader)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions zones/libp2p.direct
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ $ORIGIN libp2p.direct.

;; SOA Records
@ 86400 IN SOA aws1.libp2p.direct. domains.ipshipyard.com. (
2025020101 ; serial
2025022501 ; serial
86400 ; refresh
2400 ; retry
604800 ; expire
300 ; minimum
86400 ; minimum (TTL for "no record" responses)
)

;; DNS Service
Expand Down

0 comments on commit 46e30d6

Please sign in to comment.