Repository navigation
Fix subdomain allocation and record coexistence validation #170
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
base: main
Are you sure you want to change the base?
Changes from all commits
6e89fd0
eb3e18e
a92b626
fbd5efe
a2154bc
3f64e8d
025daa4
c307aa3
23eb9e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| <?php | ||
|
|
||
| use Boy132\Subdomains\Models\Subdomain; | ||
| use Illuminate\Database\Migrations\Migration; | ||
| use Illuminate\Database\Schema\Blueprint; | ||
| use Illuminate\Support\Facades\DB; | ||
| use Illuminate\Support\Facades\Schema; | ||
|
|
||
| return new class extends Migration | ||
| { | ||
| public function up(): void | ||
| { | ||
| Schema::table('subdomains', function (Blueprint $table) { | ||
| $table->dropUnique(['name', 'domain_id']); | ||
| $table->string('record_identifier')->nullable()->after('record_type'); | ||
| }); | ||
|
|
||
| Subdomain::query()->with('server')->each(function (Subdomain $subdomain) { | ||
| $subdomain->record_identifier = $subdomain->record_type->uniqueIdentifier($subdomain->server); | ||
| $subdomain->saveQuietly(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| }); | ||
|
|
||
| Schema::table('subdomains', function (Blueprint $table) { | ||
| $table->string('record_identifier')->nullable(false)->change(); | ||
| $table->unique(['name', 'domain_id', 'record_identifier']); | ||
| }); | ||
| } | ||
|
|
||
| public function down(): void | ||
| { | ||
| $hasDuplicateSubdomains = DB::table('subdomains') | ||
| ->select('name', 'domain_id') | ||
| ->groupBy('name', 'domain_id') | ||
| ->havingRaw('COUNT(*) > 1') | ||
| ->exists(); | ||
|
|
||
| if ($hasDuplicateSubdomains) { | ||
| throw new RuntimeException( | ||
| 'Cannot roll back subdomain coexistence while multiple record types share the same name and domain. Remove the duplicate records first.', | ||
| ); | ||
| } | ||
|
|
||
| Schema::table('subdomains', function (Blueprint $table) { | ||
| $table->dropUnique(['name', 'domain_id', 'record_identifier']); | ||
| $table->unique(['name', 'domain_id']); | ||
|
macroscopeapp[bot] marked this conversation as resolved.
Resolved
|
||
| $table->dropColumn('record_identifier'); | ||
| }); | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,17 @@ public function getLabel(): string | |
| return $this->name; | ||
| } | ||
|
|
||
| public function uniqueIdentifier(Server $server): string | ||
| { | ||
| if ($this !== self::SRV) { | ||
| return $this->value; | ||
| } | ||
|
|
||
| $serviceType = SRVServiceType::fromServer($server); | ||
|
|
||
| return $serviceType ? $serviceType->value : 'SRV'; | ||
| } | ||
|
|
||
| /** | ||
| * Returns errors that prevent this record type from being used with the provided server and domain. | ||
| * If empty, then this record type is allowed to be used. | ||
|
|
@@ -35,7 +46,7 @@ public function canBeUsedErrors(Server $server, CloudflareDomain $domain): Colle | |
|
|
||
| $targetAddress = ''; | ||
| if ($allocation) { | ||
| if (in_array($allocation->ip, ['0.0.0.0', '::'])) { | ||
| if (in_array($this, [self::A, self::AAAA]) && in_array($allocation->ip, ['0.0.0.0', '::'])) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intentional.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about the messy AI PR, I sent an agent to do this. I do have to push back here though. With this change, 0.0.0.0/:: is still never published, and A/AAAA still reject it. The change only skips the check for record types that never read the IP. SRV publishes subdomain_target plus the allocation’s port, and CNAME publishes subdomain_target alone. Neither puts the allocation IP into DNS, so it doesn’t matter for them. This does matter in practice becuase a node may only have 0.0.0.0 allocations while a hostname already points at it through an existing A record, and we just want to CNAME to that. The same goes for SRV, for example Minecraft servers that only need the port mapped. This also fixes an inconsistency. A server with no allocation can create a CNAME, but a server with a 0.0.0.0 allocation can’t, even though CNAME doesn’t use the allocation at all. If for some reason you still want 0.0.0.0 to be blocked for SRV/CNAME, please let me know :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it should always be blocked. That's intentional, to prevent people from using these invalid ips.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please elaborate why? I don't see an issue for these IPs if they are restricted to CNAME/SRV records only
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because they are not real ips. They shouldn't exist as allocation ips in the first place.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well in that case, the current check doesn't actually enforce that policy for CNAME/SRV. It only blocks one specific setup:
So for CNAME, the invalid IP is never published, and an even weaker setup (no allocation) is already allowed. The README also says the valid-IP requirement "also applies for CNAME and SRV", which the code doesn't currently match for CNAME. I have a node node whose allocations are 0.0.0.0 on purpose (binding on all interfaces behind NAT), where the hostname already has an A record pointing at the node. For those servers like mine, we only want a CNAME, or an SRV for the port. If you'd rather keep blocking this by default, would you accept a per-node option, similar to "Use allocation alias", that allows CNAME/SRV with unspecified allocation IPs? It would be off by default, so the current behaviour stays unless an admin opts in. If you still want it blocked either way, I'll then split this PR so the record coexistence changes can be reviewed on their own. |
||
| $errors->add('Allocation ip is invalid (0.0.0.0 or ::)'); | ||
| } | ||
|
|
||
|
|
@@ -63,7 +74,7 @@ public function canBeUsedErrors(Server $server, CloudflareDomain $domain): Colle | |
| $errors->add('Server has no allocation'); | ||
| } | ||
|
|
||
| if (in_array($targetAddress, ['0.0.0.0', '::'])) { | ||
| if (in_array($this, [self::A, self::AAAA]) && in_array($targetAddress, ['0.0.0.0', '::'])) { | ||
| $errors->add('Allocation target address is invalid (0.0.0.0 or ::)'); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
migrations/011_allow_compatible_record_types.php:18The migration fails at
nullable(false)->change()wheneversubdomainsalready contains rows, becausesaveQuietly()suppresses thesavinglistener that populatesrecord_identifier, leaving every existing rowNULL. Usesave()here so the listener runs before the column is made non-nullable.🤖 Copy this AI Prompt to have your agent fix this: