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

Add DNS provider for IPv64 #1954

Closed SamTV12345 closed 1 year ago

SamTV12345 commented 1 year ago

Add support for ipv64 provider. It issues a simple https post request to update TXT records. You can read more about this at https://ipv64.net/dyndns_updater_api .

To update a DNS record/create it the API is a bit weird. You have to issue a POST request to the https://ipv64.net/api.php endpoint with

encoded in a form-data

To delete the record. You do exactly the same but this time with the following attributes:

Fixes #1957

ldez commented 1 year ago

Hello, in order for a PR adding a DNS provider to be accepted, you have to:

make test

./lego -m your@email.com --dns YOUR_PROVIDER_NAME -d *.example.com -d example.com -s https://acme-staging-v02.api.letsencrypt.org/directory run

Note the wildcard domain is important.
- [x] pass the linter ([golangci-lint](https://github.com/golangci/golangci-lint#install) must be installed):
```shell
make checks
ldez commented 1 year ago

Hello,

I see some parts are missing, but can you explain why you put this PR on draft?

SamTV12345 commented 1 year ago

Oh sorry. I made it a draft so I can make clear that I am currently working on that pull request and that not another person needs to add this exact provider. I am working on the tests. How can I run the tests only for my provider?

ldez commented 1 year ago

I made it a draft so I can make clear that I am currently working on that pull request and that not another person needs to add this exact provider.

It's better to open an issue before creating a PR in this context.

I am working on the tests. How can I run the tests only for my provider?

go test ./providers/dns/ipv64/...
SamTV12345 commented 1 year ago

I opened it in #1957

ldez commented 1 year ago

We misunderstood each other: I was saying that indicating that you want to work on a provider you simply had to open an issue before making a PR. Now, your PR is open and not ready for review so the draft is justified.

I was just asked to know if you are opening the PR just because you were expecting feedback from me.

ldez commented 1 year ago

I understand that you are trying to learn Go by creating this PR (your code is not really Go idiomatic).

Do you want help?


TDD is a good way to learn a language but your tests seem odd and go too far to be TDD.

I'm a huge fan of TDD, and I practice and promote TDD.

But TDD is not "tests first": TDD is a way to bring out code, to do that you have to write one and only one test at a time. You have to write only the needed code to turn a test to green. So one test red, then green, then refactor (if needed). The design must emerge through the addition of tests.

By writing several tests before writing your code you are doing "tests first" not TDD, and this is really different than TDD.

I know it's off-topic but it's funny to share about TDD.

SamTV12345 commented 1 year ago

Sure if you have the time I would be pleased if you could help me with this pull request. I mostly do my stuff in Rust, Java or Typescript.

SamTV12345 commented 1 year ago

The first part works. Only the deletion of a dns record is failing.

ldez commented 1 year ago

The first part works. Only the deletion of a dns record is failing.

the HTTP method should be DELETE and the value must be provided.

SamTV12345 commented 1 year ago

Is there an easy way to integrate that like e.g. in Java I can write a ternary operator?

ldez commented 1 year ago

Is there an easy way to integrate that like e.g. in Java I can write a ternary operator?

You have factorized some elements that should not be factorized.

SamTV12345 commented 1 year ago

The first part works. Only the deletion of a dns record is failing.

the HTTP method should be DELETE and the value must be provided.

What does the value must be provided mean? The content is already supplied. Now is the question why I get 403.

SamTV12345 commented 1 year ago

Is there an easy way to integrate that like e.g. in Java I can write a ternary operator?

You have factorized some elements that should not be factorized.

What does factorizing mean? Is this a term from Golang?

ldez commented 1 year ago

What does factorizing mean? Is this a term from Golang?

factorizing means the same thing as in the other language.

the problem is UpdateTxtRecord: this method should not exist IMHO.

ldez commented 1 year ago

What does the value must be provided mean? The content is already supplied.

https://github.com/go-acme/lego/pull/1954/files#diff-c6780977ec40a47b455e3793c06169840dd585be0aec2698d8bbbc591032cd1aR37

SamTV12345 commented 1 year ago

I did take the code from here. The flag clean is a little bit weird if you mean that.

SamTV12345 commented 1 year ago

What does the value must be provided mean? The content is already supplied.

https://github.com/go-acme/lego/pull/1954/files#diff-c6780977ec40a47b455e3793c06169840dd585be0aec2698d8bbbc591032cd1aR37

That explains everything. I need to provide the same content for the removal to be succesful. In the addRecord it was supplied as a separate string. Can I grab that in the remove method somewhere?

ldez commented 1 year ago

if you call dns01.GetChallengeInfo 2 times with the same parameters the value will be the same.

The Present and the Cleanup will provide the same parameters.

SamTV12345 commented 1 year ago

if you call dns01.GetChallengeInfo 2 times with the same parameters the value will be the same.

The Present and the Cleanup will provide the same parameters.

Yep. Found that out too. It is working now.

SamTV12345 commented 1 year ago

In the meantime I added:

SamTV12345 commented 1 year ago

The other points on the list are

SamTV12345 commented 1 year ago

Thanks for the help. That looks much cleaner. I haven't done such a deep dive into Golang yet. Scrolling through the code I saw that you put the authorization header as a const because it was duplicated twice. The other parameters like praefix, type and content are currently duplicated twice. Could you please also add them as constants?

ldez commented 1 year ago
$ IIPV64_API_KEY=xxxx \
./dist/lego -m mail@example.com --dns ipv64 -d *.lego.example.com -d lego.example.com -s https://acme-staging-v02.api.letsencrypt.org/directory run
2023/07/19 20:32:48 No key found for account mail@example.com. Generating a P256 key.
2023/07/19 20:32:48 Saved key to /home/ldez/sources/go/src/github.com/go-acme/lego/.lego/accounts/acme-staging-v02.api.letsencrypt.org/mail@example.com/keys/mail@example.com.key
2023/07/19 20:32:49 Please review the TOS at https://letsencrypt.org/documents/LE-SA-v1.3-September-21-2022.pdf
Do you accept the TOS? Y/n
y
2023/07/19 20:32:50 [INFO] acme: Registering account for mail@example.com
!!!! HEADS UP !!!!

Your account credentials have been saved in your Let's Encrypt
configuration directory at "/home/ldez/sources/go/src/github.com/go-acme/lego/.lego/accounts".

You should make a secure backup of this folder now. This
configuration directory will also contain certificates and
private keys obtained from Let's Encrypt so making regular
backups of this folder is ideal.
2023/07/19 20:32:51 [INFO] [*.lego.example.com, lego.example.com] acme: Obtaining bundled SAN certificate
2023/07/19 20:32:51 [INFO] [*.lego.example.com] AuthURL: https://acme-staging-v02.api.letsencrypt.org/acme/authz-v3/7369849274
2023/07/19 20:32:51 [INFO] [lego.example.com] AuthURL: https://acme-staging-v02.api.letsencrypt.org/acme/authz-v3/7369849284
2023/07/19 20:32:51 [INFO] [*.lego.example.com] acme: use dns-01 solver
2023/07/19 20:32:51 [INFO] [lego.example.com] acme: Could not find solver for: tls-alpn-01
2023/07/19 20:32:51 [INFO] [lego.example.com] acme: Could not find solver for: http-01
2023/07/19 20:32:51 [INFO] [lego.example.com] acme: use dns-01 solver
2023/07/19 20:32:51 [INFO] [*.lego.example.com] acme: Preparing to solve DNS-01
2023/07/19 20:32:52 [INFO] [lego.example.com] acme: Preparing to solve DNS-01
2023/07/19 20:32:53 [INFO] [*.lego.example.com] acme: Trying to solve DNS-01
2023/07/19 20:32:53 [INFO] [*.lego.example.com] acme: Checking DNS record propagation using [192.168.1.1:53 [2a02:842b:5a8:b601::1]:53]
2023/07/19 20:32:55 [INFO] Wait for propagation [timeout: 1m0s, interval: 2s]
2023/07/19 20:33:02 [INFO] [*.lego.example.com] The server validated our request
2023/07/19 20:33:02 [INFO] [lego.example.com] acme: Trying to solve DNS-01
2023/07/19 20:33:02 [INFO] [lego.example.com] acme: Checking DNS record propagation using [192.168.1.1:53 [2a02:842b:5a8:b601::1]:53]
2023/07/19 20:33:04 [INFO] Wait for propagation [timeout: 1m0s, interval: 2s]
2023/07/19 20:33:11 [INFO] [lego.example.com] The server validated our request
2023/07/19 20:33:11 [INFO] [*.lego.example.com] acme: Cleaning DNS-01 challenge
2023/07/19 20:33:12 [INFO] [lego.example.com] acme: Cleaning DNS-01 challenge
2023/07/19 20:33:12 [INFO] [*.lego.example.com, lego.example.com] acme: Validations succeeded; requesting certificates
2023/07/19 20:33:13 [INFO] Wait for certificate [timeout: 30s, interval: 500ms]
2023/07/19 20:33:14 [INFO] [*.lego.example.com] Server responded with a certificate.

Scrolling through the code I saw that you put the authorization header as a const because it was duplicated twice. The other parameters like praefix, type and content are currently duplicated twice. Could you please also add them as constants?

It's not because the header is repeated twice (I start to think about constant when a value is repeated 3 times and if there is a real meaning behind it). It's because that helps me to identify the authentication system used by the DNS provider, it's for maintenance.

SamTV12345 commented 1 year ago

Ah got it. Okay. Then I don't have anything to complain 😄 . Was there a bug in the code or why is the testing certificate suddenly accepted by IPv64?

SamTV12345 commented 1 year ago

Seems like golint needs too much time for checking.

ldez commented 1 year ago

Was there a bug in the code or why is the testing certificate suddenly accepted by IPv64?

example.com is just a placeholder :wink:

I used a real domain, and a real email, and redacted the console output to hide the real information.

SamTV12345 commented 1 year ago

Was there a bug in the code or why is the testing certificate suddenly accepted by IPv64?

example.com is just a placeholder 😉

I used a real domain and redacted the console output to hide the real information.

Oh snap. Thought I should simply run the command with the other dns provider. Okay xD.

ldez commented 1 year ago

Seems like golint needs too much time for checking.

It's not golint but golangci-lint.

golint has been deprecated for 2 years.

SamTV12345 commented 1 year ago

The linter shows now:

golangci-lint run
WARN [linters_context] running gomoddirectives failed: failed to get module file: unmarshaling error: invalid character 'w' looking for beginning of value: warning: GOPATH set to GOROOT (/Users/redacted/sdk/go1.20.6) has no effect
{
        "Path": "github.com/go-acme/lego/v4",
        "Main": true,
        "Dir": "/Users/redacted/WebstormProjects/lego",
        "GoMod": "/Users/redacted/WebstormProjects/lego/go.mod",
        "GoVersion": "1.19"
}
: if you are not using go modules it is suggested to disable this linter 

I have no idea if this is a configuration issue on my side or the cause of the linter error.

SamTV12345 commented 1 year ago

Seems like golint needs too much time for checking.

It's not golint but golangci-lint.

golint is deprecated for 2 years.

Ah okay. Golint is a bit more memorizable than golangci-lint. Yes I referred to the last mentioned tool.

ldez commented 1 year ago

warning: GOPATH set to GOROOT (/Users/redacted/sdk/go1.20.6) has no effect

it's because your GOPATH has the same value as GOROOT.

GOROOT is the place of the binary and the source of Go itself. GOPATH is the place of user's sources. The use of GOPATH is deprecated since Go module.

You have to change either the GOROOT or the GOPATH, it depends on your installation.

Note: don't put Go binary and user sources in the same directory.

SamTV12345 commented 1 year ago

Thanks. I set the GOPATH to another place and the linter is now happy.

I don't know if it is relevant but in acme.sh it was named IPv64_Token

ldez commented 1 year ago

I don't know if it is relevant but in acme.sh it was named IPv64_Token

I prefer using a name based on a name inside the UI or API documentation of a DNS provider.

Screenshot UI

Screenshot API

I just follow the DNS provider naming, I think it's easier for the users.

SamTV12345 commented 1 year ago

Okay. I'm fine with that. Just out of curiosity why didn't you add named return types. Now it is string string error and I don't know which string is the domain, prefix based on the information this method gives me. Or is it common in the Golang community to name the return variables accordingly?

ldez commented 1 year ago

There is no real shared convention about that, I'm trying to avoid using named returns because, even if they can be useful for documentation, they decrease readability. An example:

func splitDomain(full string) (subDomain string, domain string, err error) {
    split := dns.Split(full)
    if len(split) < 3 {
        err = fmt.Errorf("unsupported domain: %s", full)
        return
    }

    if len(split) == 3 {
        domain = full
        return
    }

    domain = full[split[len(split)-3]:]
    subDomain = full[:split[len(split)-3]-1]

    return
}

But it's just my opinion.

SamTV12345 commented 1 year ago

True. I agree. In that regard it would be doubled and just confuses the reader with no real benefit.

SamTV12345 commented 1 year ago

I see that merging is blocked because a change is requested. Is that a bug or is there still something to do?

ldez commented 1 year ago

It's because I have to approve the PR :wink:

SamTV12345 commented 1 year ago

Alright. I just sit and wait 😄 . Contact me if I can help you somewhere.