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

fornex.com API v.2.2.0 support #5162

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

zer0init1
Copy link

This is official script from Fornex.com Dev-Team https://github.com/fornex-com/

Bugs: #5161

Copy link

Welcome
First thing: don't send PR to the master branch, please send to the dev branch instead.
Please make sure you've read our DNS API Dev Guide and DNS-API-Test.
Then reply on this message, otherwise, your code will not be reviewed or merged.
We look forward to reviewing your Pull request shortly ✨
注意: 必须通过了 DNS-API-Test 才会被 review. 无论是修改, 还是新加的 dns api, 都必须确保通过这个测试.

@zer0init1 zer0init1 changed the title fornex.com API v.2.20 support fornex.com API v.2.2.0 support May 30, 2024
Copy link

@stenri stenri left a comment

Choose a reason for hiding this comment

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

New dns_fornex.sh doesn't correctly handle sub-domains, removes ALL TXT records from domain, and doesn't handle errors well enough.

# _sub_domain=_acme-challenge.www
# _domain=domain.com
_get_root() {
_get_domain_id() {
Copy link

Choose a reason for hiding this comment

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

This updated script doesn't work correctly with subdomains. For example, if I have thedomain.com domain with "host": "api" record:

{
    "name": "thedomain.com",
    "created": "...",
    "updated": "...",
    "tags": [],
    "entry_set": [
...
      {
        "id": <xxx>,
        "host": "api",
        "type": "A",
        "prio": null,
        "value": "xx.xx.xx.xx",
        "ttl": null
      },
...

and I want to renew certificate for api.thedomain.com, script fails, since _get_domain_id() incorrectly returns api.thedomain.com as _domain. And subsequent calls to DNS API return error that there is no such domain:

Getting domain ID for _acme-challenge.api.thedomain.com
api.thedomain.com/entry_set/
response='{"detail":"No Domain matches the given query."}'
Domain ID for api.thedomain.com is {"detail":"No Domain matches the given query."}
_domain_id='{"detail":"No Domain matches the given query."}'

Notice how previous version of the script uses while true; do loop to find a correct "root" domain to use.
New script must do the same.

fi
i=$(_math "$i" + 1)
done
if ! _rest GET "$domain/entry_set/"; then
Copy link

Choose a reason for hiding this comment

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

Error handling is somehow incorrect here. In the case outlined above when a domain api.thedomain.com doesn't exist, but there is thedomain.com domain with "host": "api" record

{
    "name": "thedomain.com",
    "created": "...",
    "updated": "...",
    "tags": [],
    "entry_set": [
...
      {
        "id": <xxx>,
        "host": "api",
        "type": "A",
        "prio": null,
        "value": "xx.xx.xx.xx",
        "ttl": null
      },
...

this piece of code calls /api.thedomain.com/entry_set/, fornex DNS API 2.2.0 return error string:

No Domain matches the given query.

But script ignores this error, and goes on as if nothing happened:

Getting domain ID for _acme-challenge.api.thedomain.com
thedomain.com/entry_set/
response='{"detail":"No Domain matches the given query."}'
Domain ID for thedomain.com is {"detail":"No Domain matches the given query."}
_domain_id='{"detail":"No Domain matches the given query."}'

The script should handle this and other similar errors correctly, and terminate processing.

response=$(curl -X GET -H "Authorization: Api-Key $FORNEX_API_KEY" "https://fornex.com/api/dns/domain/$domain/entry_set/")

# Extract TXT record IDs using jq
txt_ids=$(echo "$response" | jq -r '.[] | select(.type == "TXT") | .id')
Copy link

Choose a reason for hiding this comment

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

This is a total disaster! dns_fornex_rm() selects and removes ALL TXT records from a domain, regardless of what they contain and who created them. Bye-bye all marketing google-site-verification, yandex-verification and other such records!

Previous version of the script checked TXT record value and only deleted TXT records with the value that script previously added.

I would suggest at least check here that TXT record .host value starts with "_acme-challenge". I.e. use something like this:

  txt_ids=$(echo "$response" | jq -r '.[] | select(.type == "TXT") | select(.host | startswith("_acme-challenge")) | .id')

fi
_info "Adding TXT record for $fulldomain"
# Add the TXT record
if ! _rest POST "$domain/entry_set/" "type=TXT&host=_acme-challenge&value=$txtvalue"; then
Copy link

Choose a reason for hiding this comment

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

To work correctly with subdomains TXT record "host" value must be: _acme-challenge[.subdomain].
I.e. in the case when domain api.thedomain.com doesn't exist, but there is thedomain.com domain with "host": "api" record:

{
    "name": "thedomain.com",
    "created": "...",
    "updated": "...",
    "tags": [],
    "entry_set": [
...
      {
        "id": <xxx>,
        "host": "api",
        "type": "A",
        "prio": null,
        "value": "xx.xx.xx.xx",
        "ttl": null
      },
...

Script must create a TXT record with ".host" == "_acme-challenge.api" value, for example.
Script needs to be modified to account for this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants