Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acme-dns: continue the process when the CNAME is handled by the storage #2443

Merged
merged 4 commits into from
Feb 17, 2025

Conversation

ldez
Copy link
Member

@ldez ldez commented Feb 17, 2025

@ldez
Copy link
Member Author

ldez commented Feb 17, 2025

My implementation is wrong, I will fix that

@ldez ldez marked this pull request as draft February 17, 2025 14:43
@ldez ldez marked this pull request as ready for review February 17, 2025 14:46
Copy link
Member

@dmke dmke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense 👍

@ldez ldez enabled auto-merge (squash) February 17, 2025 14:52
@ldez ldez changed the title acme-dns: fix present acme-dns: continue the process when the CNAME is handled by the storage Feb 17, 2025
@ldez ldez disabled auto-merge February 17, 2025 15:00
@ldez ldez merged commit f9c1e24 into go-acme:master Feb 17, 2025
7 checks passed
@ldez ldez deleted the fix/acme-dns-present branch February 17, 2025 15:00
@skippyprime
Copy link

Well, I found one more issue (and I think this is the last) in the flow.

Since the call to Fetch() will return an error for a new challenge, account will be nil. The register function will create the new account and must return that to the caller so it can be used in the UpdateTXTRecord call. It is currently always nil for new challenges.

The Present function should probably look like this:

func (d *DNSProvider) Present(domain, _, keyAuth string) error {
	ctx := context.Background()

	// Compute the challenge response FQDN and TXT value for the domain based on the keyAuth.
	info := dns01.GetChallengeInfo(domain, keyAuth)

	// Check if credentials were previously saved for this domain.
	account, err := d.storage.Fetch(ctx, domain)
	if err != nil {
		if !errors.Is(err, storage.ErrDomainNotFound) {
			return err
		}

		// The account did not exist.
		// Create a new one and return an error indicating the required one-time manual CNAME setup.
		account, err = d.register(ctx, domain, info.FQDN)
		if err != nil {
			return err
		}
	}

	// Update the acme-dns TXT record.
	return d.client.UpdateTXTRecord(ctx, account, info.Value)
}

And register function needs to return the new account:

func (d *DNSProvider) register(ctx context.Context, domain, fqdn string) (goacmedns.Account, error) {
	newAcct, err := d.client.RegisterAccount(ctx, d.config.AllowList)
	if err != nil {
		return newAcct, err
	}

	var cnameCreated bool

	// Store the new account in the storage and call save to persist the data.
	err = d.storage.Put(ctx, domain, newAcct)
	if err != nil {
		cnameCreated = errors.Is(err, internal.ErrCNAMEAlreadyCreated)
		if !cnameCreated {
			return newAcct, err
		}
	}

	err = d.storage.Save(ctx)
	if err != nil {
		return newAcct, err
	}

	if cnameCreated {
		return newAcct, nil
	}

	// Stop issuance by returning an error.
	// The user needs to perform a manual one-time CNAME setup in their DNS zone
	// to complete the setup of the new account we created.
	return newAcct, ErrCNAMERequired{
		Domain: domain,
		FQDN:   fqdn,
		Target: newAcct.FullDomain,
	}
}

@ldez
Copy link
Member Author

ldez commented Feb 17, 2025

#2445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants