go-acme / lego

Let's Encrypt/ACME client and library written in Go
https://go-acme.github.io/lego/
MIT License
7.84k stars 1.01k forks source link

route53: avoid unexpected records deletion #1976

Closed ldez closed 1 year ago

ldez commented 1 year ago

Fixes #1756

ldez commented 1 year ago

@kingcdavid can you try this PR?

kingcdavid commented 1 year ago

Int Tests passed

➜  lego git:(fix/route53-protect-records) go test -v -run TestLiveTTL github.com/go-acme/lego/v4/providers/dns/route53
=== RUN   TestLiveTTL
2023/07/27 13:48:37 [INFO] Wait for route53 [timeout: 2m0s, interval: 4s]
2023/07/27 13:49:15 [INFO] Wait for route53 [timeout: 2m0s, interval: 4s]
--- PASS: TestLiveTTL (74.20s)
PASS
ok      github.com/go-acme/lego/v4/providers/dns/route53    74.205s

But i can't actually re-create the original issue to see if it's fixed, What CNAME needs to be implemented?

ldez commented 1 year ago

I think it's a wildcard CNAME.

kingcdavid commented 1 year ago

so was able to recreate but it now errors when trying to delete the records:

2023/07/27 15:38:08 [WARN] [<redacted>] acme: cleaning up failed: route53: failed to change record set: operation error Route 53: ChangeResourceRecordSets, https response error StatusCode: 400, RequestID: <redacted>, InvalidChangeBatch: [Tried to delete resource record set [name='<redacted>', type='TXT'] but the values provided do not match the current values] 

i think we need to do something like this (tested as working)

diff --git a/providers/dns/route53/route53.go b/providers/dns/route53/route53.go
index d48fd3b6..862cdd51 100644
--- a/providers/dns/route53/route53.go
+++ b/providers/dns/route53/route53.go
@@ -185,14 +185,19 @@ func (d *DNSProvider) CleanUp(domain, token, keyAuth string) error {
                return nil
        }

-       var toDelete []awstypes.ResourceRecord
+       var toChange []awstypes.ResourceRecord
        for _, record := range records {
-               if deref(record.Value) == `"`+info.Value+`"` {
-                       toDelete = append(toDelete, record)
+               if deref(record.Value) != `"`+info.Value+`"` {
+                       toChange = append(toChange, record)
                }
        }
+       action := awstypes.ChangeActionUpsert
+       if len(toChange) == 0 {
+               toChange = records
+               action = awstypes.ChangeActionDelete
+       }

-       if len(toDelete) == 0 {
+       if len(toChange) == 0 {
                return nil
        }

@@ -200,10 +205,10 @@ func (d *DNSProvider) CleanUp(domain, token, keyAuth string) error {
                Name:            aws.String(info.EffectiveFQDN),
                Type:            "TXT",
                TTL:             aws.Int64(int64(d.config.TTL)),
-               ResourceRecords: toDelete,
+               ResourceRecords: toChange,
        }

-       err = d.changeRecord(ctx, awstypes.ChangeActionDelete, hostedZoneID, recordSet)
+       err = d.changeRecord(ctx, action, hostedZoneID, recordSet)
        if err != nil {
                return fmt.Errorf("route53: %w", err)
        }
ldez commented 1 year ago

Thank you, I adapted the algorithm and rename some variables to improve the readability.

Offtopic: I recommend you add your company email (xxxxx.xxxx@glxxxxxxxn.com) to your GiHub profile to avoid this weird behavior https://github.com/go-acme/lego/pull/1970/commits