virtualmin / virtualmin-gpl

Virtualmin web hosting control panel for Webmin
https://www.virtualmin.com
GNU General Public License v3.0
320 stars 102 forks source link

Restore of domain from server1 to server2 duplicates dns entries with server2 ip instead of replacing. Reset features dns properly sets dns to server2 ip and removes server 1 ips #491

Closed abclution closed 1 year ago

abclution commented 1 year ago

Webmin version 2.001 Usermin version 1.860 Virtualmin version 7.3-1 Debian

Hello guys, long time no talk, been over in python land dealing with some things.

Encountering an issue where I am trying to move some domains from one server to another, which I have done many times before. For some reason, I do not know when this started (been away from virtualmin stuff for almost a year!) restoring a domain from server1 to server2 or the reverse, the DNS/bind entries of server1 do not get replaced with server2 ips, instead new additional entries are made with the ips of server2. This is for all the standard m. , autoconfig., mail.* etc etc. The reverse also happens going from server2 -> server1 as well as doing via the transfer domain (which makes sense as it uses the backup system.)

If on the transferred domain, I run the reset features (and include deleting data) for DNS, the correct entries are written and the old server entries are removed.

I'm feeling a bit rusty with virtualmin stuff so I am not sure if its me or a bug.

jcameron commented 1 year ago

This sounds like a bug! When you do a transfer like this, does the domain already exist on the destination system? Or is it being created as part of the transfer process?

abclution commented 1 year ago

Hey @jcameron !

Being created. I'm migrating virtual servers from one server to another and its duplicating (or not overwriting) the entries for all dns entries except for SOA, keeping the first server entries and creating new entries for the second.

iliajie commented 1 year ago

I can also reproduce this problem, where DNS IP is not updated correctly. Also, using Server Configuration ⇾ Change IP Address page doesn't update it.

abclution commented 1 year ago

I can also reproduce this problem, where DNS IP is not updated correctly. Also, using Server Configuration ⇾ Change IP Address page doesn't update it.

Oh thank fsck its not only me. Why do I always run into show stopping bugs. lmao, just lucky I guess.

jcameron commented 1 year ago

Damn, I was able to confirm this bug :-( Looking into it now ..

jcameron commented 1 year ago

Ok this should fix it : https://github.com/virtualmin/virtualmin-gpl/commit/18ee3b6ef0936146aafe93ad16b6f9019cb27e5d

abclution commented 1 year ago

Will test shortly

iliajie commented 1 year ago

Ok this should fix it : 18ee3b6

Jamie, I'm pretty sure this patch won't fix it. Also, why DNSSEC records should not be taken from the backup?

abclution commented 1 year ago

Ok, at first glance this seems fixed for me.

Ended running into another issue while doing server transfers left and right while debugging this issue, so am not 100% certain all is well yet. ;)

Please see here: https://github.com/virtualmin/virtualmin-gpl/issues/493

iliajie commented 1 year ago

Ok, at first glance this seems fixed for me.

What do you mean by "at first glance"? Does it fix the original issue that you have reported earlier or not? Could you please provide a screenshot of how the records looked for you before and after this patch?

Jamie, do we intentionally keep local IPs (like 10.x.x.x and 192.168.x.x) in SPF record not updated? Base IP is getting updated as expected though.

Jamie, I'm pretty sure this patch won't fix it.

Unless, the issue was related to DNSSEC records only?

abclution commented 1 year ago

What do you mean by "at first glance"? Does it fix the original issue that you have reported earlier or not? Could you please provide a screenshot of how the records looked for you before and after this patch?

At first glance meaning until I hit, debugged, and filed that other mentioned bug (its relevant only as much as I am testing this fix via the server transfer feature) and had to figure out what was causing it, the first restore/transfer looked ok.

I am currently cleaning up a mess of orphaned groups from https://github.com/virtualmin/virtualmin-gpl/issues/493 and I noticed a possible issue with this fix ( https://github.com/virtualmin/virtualmin-gpl/commit/18ee3b6ef0936146aafe93ad16b6f9019cb27e5d ) but as I am in the middle of something else will take me a bit more time to validate my findings. Working on it, patience please!

jcameron commented 1 year ago

Also, why DNSSEC records should not be taken from the backup?

Because they get re-generated after the restore.

abclution commented 1 year ago

Also, why DNSSEC records should not be taken from the backup?

Because they get re-generated after the restore.

Sorry this took me so long, I was encountering some sporadic undefined behavior which turned out I hadn't restarted webmin on one of the servers between patches. Sigh. Like I said, I'm rusty with virtualmin atm.

So, back to the subject at hand, while the standard records are not duplicated anymore with this patch, the keys are not regenerated during the restore/transfer, and I get the following error. (This doesn't happen pre-patch)

in DNS Options image

And webmin agrees in Bind that there is no DNSSEC key for the domain

image

Resetting the feature on the domain does fix the DS records and DNSKEY record.

image

Even though I have selected No for "Continue if settings would be lost", fully reset does take effect and in theory is a destructive feature. I am not sure if this is intended either.

iliajie commented 1 year ago

Jamie, you seem to skip DNSSEC records for the domain upon restore but never regenerated it later?

Also, the records supposed to be regenerated using the same DS record .. it's possible right?

jcameron commented 1 year ago

Seems like we're missing something here on the restore - looking into it now.

jcameron commented 1 year ago

Ok this will fix that DNSKEY record issue : https://github.com/virtualmin/virtualmin-gpl/commit/4588a41cc7b7f4ff2c8d7ccafddcfd98b9fbbca2

abclution commented 1 year ago

Ok this will fix that DNSKEY record issue : 4588a41

This seems ok now. Will continue to test.

abclution commented 1 year ago

Sorry I was incorrect, most recent patch fixed the DNSSEC key but the original issue appeared again.

Its duplicating again.

abclution commented 1 year ago

Please ignore/close the pull request, I wrongly though it had stopped duplicated with my fix but it had showed me the records for another domain that moment confusing me.

abclution commented 1 year ago
# With this, records properly created (not duplicated) DNSSECKEY restore broken.
               next if ($r->{'type'} eq 'DNSKEY');

# With this, records properly created (not duplicated) DNSSECKEY restore broken. same as above, wat
               next if (&is_dnssec_record($r));

# With nothing here, DNSSEC key is not working and the entries are duplicated.

# With this, records are not created, DNSSECKEY does work.
               next if ($r->{'type'} ne 'DNSKEY');

# Jamies patch, records are duplicated  DNSSECKEY does work.
                next if (&is_dnssec_record($r) && $r->{'type'} ne 'DNSKEY');
jcameron commented 1 year ago

OK, so it's all good now?

abclution commented 1 year ago

Sorry, no it is not,

With your most recent patch https://github.com/virtualmin/virtualmin-gpl/commit/4588a41cc7b7f4ff2c8d7ccafddcfd98b9fbbca2 it began duplicating again but DNSSEC keys are ok.

And looking at the code, I could not understand why your patch did not work.

I tried to solve it myself but I could not, I though I did but I was mistaken. Those code excerpts are some of the tries I tested and their results. Sorry for the confusion, quite exhausted yesterday

abclution commented 1 year ago

These two lines(used individually) should not produce the same results on a restore, but they do. :/

next if ($r->{'type'} eq 'DNSKEY'); next if (&is_dnssec_record($r));

iliajie commented 1 year ago

Is there a way you could email me the link for your backup (ilia at virtualmin dot com)? It would help a lot. I know I said that I could reproduce the problem, but I actually didn't (it was something else).

abclution commented 1 year ago

@iliajie Sent.

iliajie commented 1 year ago

I didn't get anything. Try support+github-issue-491 at ilia dot engineer email.

abclution commented 1 year ago

@iliajie Sent again.

abclution commented 1 year ago

Just a sec, got a bounce this time. It didn't like the bz2 file. just a sec

iliajie commented 1 year ago

I have taken a closer look using your backup files but I don't see records being duplicated .. However, I noticed that after restoring your domain I can see an error in DNSSEC Setup Key page in Webmin:

image

I remember "fixing" this problem in one of the PRs in the past.


After taking a deeper look, I think I have found a bug, as the real simplest solution to this problem is just to regenerate DNSSEC records (of course using old DS key). Please try this patch (from PR). It should solve your problem https://github.com/virtualmin/virtualmin-gpl/pull/494/files.

iliajie commented 1 year ago

@abclution Also, can you tell us exactly, how did you make that backup you shared with me over the email? Did you use Virtualmin CLI (if so, please post the exact command) or you used the UI?

abclution commented 1 year ago

@iliajie UI

I tested your patch and still not getting anywhere.. noticed as well some other erratic stuff like the SOA not updating to the new server when restored there.. but not all the time .. shits a bit erratic.. still narrowing it down.

Please confirm for me, I only need to restart webmin when making changes to .pl files? In case I'm testing wrongly?

abclution commented 1 year ago

I belive I did it exactly as show in this screenshot, I know I dont need to press the select all button (when backup all features is selected, but its a force of habit.

image

iliajie commented 1 year ago

I am just very confused as running tests. What I cannot understand yet is when I use your backup, DS record always stays the same after restore. When I make a new backup with one of my local domains, with DNSSEC enabled, and then restore it, then DS key always happen to be different.

abclution commented 1 year ago

... have we found the ghost in the machine?

iliajie commented 1 year ago

.. something causes my @keys = &bind8::get_dnssec_key(&get_bind_zone($d->{'dom'})); not return keys ..

iliajie commented 1 year ago

.. oh, you also have Create DNSSEC key and sign new domains enabled on the templates .. so it pre-creates records and then works with my patch as expected .. but there is more to fix .. not sure yet what's wrong ..

iliajie commented 1 year ago

Alright, I got what the problem was, and the latest commit on the PR fixes it for all modes.

It works flawlessly for me, although, I am not sure, if Jamie wouldn't want to come with a different solution.

But restoring DNSSEC now works correctly with this PR and DS key is always preserved.

jcameron commented 1 year ago

There's something deeper going on here that I need to fix ... it's not really related to DNSSEC at all.

iliajie commented 1 year ago

But we have a problem with DNSSEC restore.

jcameron commented 1 year ago

That was just the trigger - the proper fix, in my opinion, is here : https://github.com/virtualmin/virtualmin-gpl/commit/45318b4d1421afe1cc6db535a5c175838140d56a

iliajie commented 1 year ago

Will that regenerate DNSSEC records?

iliajie commented 1 year ago

Okay, I see the fix now. It must solve the problem with IPs not being updated, right? But what about DNSSEC bug which I clearly described?

iliajie commented 1 year ago

Gave it a quick test and this patch expectedly won't fix DNSSEC restore issue. My PR will.

jcameron commented 1 year ago

Can you explain in more detail what's going wrong with the DNSSEC records? With the latest code, I was able to create a domain, back it up, check the DS records, delete it, restore and then check the DS records again. And they were the same.

abclution commented 1 year ago

That was just the trigger - the proper fix, in my opinion, is here : 45318b4

I just tested this and at first glance 100% fixed the duplicated records and missing DNSKEY situation (have not confirmed yet wether is old or new key being kept). aka both issues. Will do additional indepth testing shortly.

Am going to test @iliajie patch as well.

abclution commented 1 year ago

That was just the trigger - the proper fix, in my opinion, is here : 45318b4

Created domain on server1 transferred to server2 & transferred back for good measure. All is well. DNS Records OK, no duplication when restoring to a secondary server. DNSKEY/DS Records OK, Preserved keys + DS records on transfer

image

Have a few more checks in mind but so far looks good. Onward to Ilias patch

abclution commented 1 year ago

Alright, I got what the problem was, and the latest commit on the PR fixes it for all modes.

It works flawlessly for me, although, I am not sure, if Jamie wouldn't want to come with a different solution.

But restoring DNSSEC now works correctly with this PR and DS key is always preserved.

@iliajie Just tested your PR while the DNSSEC/DS records are preserved, I'm still getting duplication of records. I belive you are not seeing that because you are (presumably?) testing backup and restore on a single server, while I am transferring from one server to another. The duplicated records are not clones, just duplicated entries for example ftp.* with the prior server IPs & and new server IPs. On the same server, the records are not duplicated

example.
(Server 1, originating server, 188.40.x.x / 2a01:4f8:X:X:X:X) (Server 2, destination server, 95.217.x.x / 2a01:4f9:X:X:X:X) Image from the restored DNS records on Server 2 image

iliajie commented 1 year ago

With the latest code, I was able to create a domain, back it up, check the DS records, delete it, restore and then check the DS records again.

Negative. This cannot be! You are probably just checking for dsset-dom.com. file. That file is irrelevant, because the moment you turn DNSSEC off and on, it will be regenerated from a newly generated (not from restored) public key (as a DS record is only a hash of public DNSKEY record) which again wasn't restored, and will result into a different DS record.

Can you explain in more detail what's going wrong with the DNSSEC records?

Yes, most definitely!

1. If you restore a domain with default template (when DNSSEC is not enabled by default), then DNSSEC private and public keys from the backup won't be restores simply because @keys = &bind8::get_dnssec_key(&get_bind_zone($d->{'dom'})); will always return empty! This will result into different DS record and additional pain for domain owner, to change it on the registrar side. This particular code from my PR solves this. 1.1. The second case scenario is when a template have DNSSEC enabled by default (in case of @abclution), which will generate at first the random pair of public and private keys. Later, upon the actual restore earlier mentioned @keys will return data, and the code right below, with _dnssec_keyinfo that actually restores public and private keys from the backup will work, resulting in having correct keys restored for the domain. That means, if you disable and then re-enable DNSSEC for the domain it will actually work (has expected DS record). 2. Nevertheless, we still need to toggle DNSSEC for the domain to regenerate the fresh records. Otherwise, considering the latest code that Jamie committed a few days ago, the zone will only have DNSKEY record after the restore, which is invalid. It will though show that DNSSEC is enabled simply because has_domain_dnssec is looking for DNSKEY record and finds it. This particular code from my PR solves this.

Just tested your PR while the DNSSEC/DS records are preserved, I'm still getting duplication of records.

This is impossible because it's regenerated afterwards. Make sure that you update the file in whole, using this link.

testing backup and restore on a single server, while I am transferring from one server to another.

No, I'm using different servers. Although, there is still a separate bug which doesn't update SPF local IP correctly.

The duplicated records are not clones, just duplicated entries for example ftp.* with the prior server IPs & and new server IPs.

Yeah, that should be fixed in Jamie's latest patch, which is already included in my PR.

Give it a try and pay attention to the actual DNSSEC records and dsset-dom.com. file not just after the restore but also after turning DNSSEC off and on again.

Here is the example backup which you could also use to try, with and without my PR, and see that in described case 1. it will never return 5A8B DS record after toggling DNSSEC off/on. And case 2. you case, it will still work after DNSSEC toggling but not before (the zone will miss a lot of records).

abclution commented 1 year ago

Sure, will try it again. I did replace the whole file previously but will try again as well as

after turning DNSSEC off and on again.

which I did NOT do previously.

abclution commented 1 year ago

Sorry @iliajie you mentioned

Otherwise, considering the latest code that Jamie committed a few days ago,

But the code I tested from him is this one. Just FYI https://github.com/virtualmin/virtualmin-gpl/commit/45318b4d1421afe1cc6db535a5c175838140d56a