phil/Gondulf
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(dns): query _gondulf subdomain for domain verification

Browse Source 
The DNS TXT verification was querying the base domain instead of
_gondulf.{domain}, causing verification to always fail even when
users had correctly configured their DNS records.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Phil Skentelbery
2025-11-22 17:46:38 -07:00
parent bf69588426
commit 1ef5cd9229
5 changed files with 548 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
docs/decisions/ADR-011-dns-txt-subdomain-prefix.md Normal file
View File

@@ -0,0 +1,76 @@
# ADR-011. DNS TXT Record Subdomain Prefix
Date: 2024-11-22
## Status
Accepted
## Context
For DNS-based domain verification, we need users to prove they control a domain by setting a TXT record. There are two common approaches:
1. **Direct domain TXT record**: Place the verification value directly on the domain (e.g., TXT record on `example.com`)
2. **Subdomain prefix**: Use a specific subdomain for verification (e.g., TXT record on `_gondulf.example.com`)
The direct approach seems simpler but has significant drawbacks:
- Conflicts with existing TXT records (SPF, DKIM, DMARC, domain verification for other services)
- Clutters the main domain's DNS records
- Makes it harder to identify which TXT record is for which service
- Some DNS providers limit the number of TXT records on the root domain
The subdomain approach is widely used by major services:
- Google uses `_domainkey` for DKIM
- Various services use `_acme-challenge` for Let's Encrypt domain validation
- GitHub uses `_github-challenge` for domain verification
- Many OAuth/OIDC providers use service-specific prefixes
## Decision
We will use the subdomain prefix approach with `_gondulf.{domain}` for DNS TXT record verification.
The TXT record requirements:
- **Location**: `_gondulf.{domain}` (e.g., `_gondulf.example.com`)
- **Value**: `gondulf-verify-domain`
- **Type**: TXT record
This approach follows industry best practices and RFC conventions for using underscore-prefixed subdomains for protocol-specific purposes.
## Consequences
### Positive Consequences
1. **No Conflicts**: Won't interfere with existing TXT records on the main domain
2. **Clear Purpose**: The `_gondulf` prefix clearly identifies this as Gondulf-specific
3. **Industry Standard**: Follows the same pattern as DKIM, ACME, and other protocols
4. **Clean DNS**: Keeps the main domain's DNS records uncluttered
5. **Multiple Services**: Users can have multiple IndieAuth servers verified without conflicts
6. **Easy Removal**: Users can easily identify and remove Gondulf verification when needed
### Negative Consequences
1. **Slightly More Complex**: Users must understand subdomain DNS records (though this is standard)
2. **Documentation Critical**: Must clearly document the exact subdomain format
3. **DNS Propagation**: Subdomain records may propagate differently than root domain records
4. **Wildcard Conflicts**: May conflict with wildcard DNS records (though underscore prefix minimizes this)
### Implementation Considerations
1. **Clear Instructions**: The error messages and documentation must clearly show `_gondulf.{domain}` format
2. **DNS Query Logic**: The code must prefix the domain with `_gondulf.` before querying
3. **Validation**: Must handle cases where users accidentally set the record on the wrong location
4. **Debugging**: Logs should clearly show which domain was queried to aid troubleshooting
## Alternative Considered
**Direct TXT on root domain** was considered but rejected due to:
- High likelihood of conflicts with existing TXT records
- Poor service isolation
- Difficulty in identifying ownership of TXT records
- Goes against industry best practices
## References
- RFC 8552: Scoped Interpretation of DNS Resource Records through "Underscored" Naming
- DKIM (RFC 6376): Uses `_domainkey` subdomain
- ACME (RFC 8555): Uses `_acme-challenge` subdomain
- Industry examples: GitHub (`_github-challenge`), various OAuth providers

195
docs/designs/dns-verification-bug-fix.md Normal file
View File

@@ -0,0 +1,195 @@
# DNS Verification Bug Fix Design
## Purpose
Fix critical bug in DNS TXT record verification where the code queries the wrong domain location, preventing successful domain verification even when users have correctly configured their DNS records.
## Problem Statement
### Current Incorrect Behavior
The DNS verification service currently queries the wrong domain for TXT records:
1. **User instructions** (correctly shown in template): Set TXT record at `_gondulf.{domain}`
2. **User action**: Creates TXT record at `_gondulf.thesatelliteoflove.com` with value `gondulf-verify-domain`
3. **Code behavior** (INCORRECT): Queries `thesatelliteoflove.com` instead of `_gondulf.thesatelliteoflove.com`
4. **Result**: Verification always fails
### Root Cause
In `src/gondulf/dns.py`, the `verify_txt_record` method passes the domain directly to `get_txt_records`, which then queries that exact domain. The calling code in `src/gondulf/routers/authorization.py` also passes just the base domain without the `_gondulf.` prefix.
## Design Overview
The fix requires modifying the DNS verification logic to correctly prefix the domain with `_gondulf.` when querying TXT records for Gondulf domain verification purposes.
## Component Details
### 1. DNSService Updates (`src/gondulf/dns.py`)
#### Option A: Modify `verify_txt_record` Method (RECOMMENDED)
Update the `verify_txt_record` method to handle Gondulf-specific verification by prefixing the domain:
```python
def verify_txt_record(self, domain: str, expected_value: str) -> bool:
"""
Verify that domain has a TXT record with the expected value.
For Gondulf domain verification (expected_value="gondulf-verify-domain"),
queries the _gondulf.{domain} subdomain as per specification.
Args:
domain: Domain name to verify (e.g., "example.com")
expected_value: Expected TXT record value
Returns:
True if expected value found in TXT records, False otherwise
"""
try:
# For Gondulf domain verification, query _gondulf subdomain
if expected_value == "gondulf-verify-domain":
query_domain = f"_gondulf.{domain}"
else:
query_domain = domain
txt_records = self.get_txt_records(query_domain)
# Check if expected value is in any TXT record
for record in txt_records:
if expected_value in record:
logger.info(
f"TXT record verification successful for domain={domain} "
f"(queried {query_domain})"
)
return True
logger.debug(
f"TXT record verification failed: expected value not found "
f"for domain={domain} (queried {query_domain})"
)
return False
except DNSError as e:
logger.warning(f"TXT record verification failed for domain={domain}: {e}")
return False
```
#### Option B: Create Dedicated Method (ALTERNATIVE - NOT RECOMMENDED)
Add a new method specifically for Gondulf verification:
```python
def verify_gondulf_domain(self, domain: str) -> bool:
"""
Verify Gondulf domain ownership via TXT record at _gondulf.{domain}.
Args:
domain: Domain name to verify (e.g., "example.com")
Returns:
True if gondulf-verify-domain found in _gondulf.{domain} TXT records
"""
gondulf_subdomain = f"_gondulf.{domain}"
return self.verify_txt_record(gondulf_subdomain, "gondulf-verify-domain")
```
**Recommendation**: Use Option A. It keeps the fix localized to the DNS service and maintains backward compatibility while fixing the bug with minimal changes.
### 2. No Changes Required in Authorization Router
With Option A, no changes are needed in `src/gondulf/routers/authorization.py` since the fix is entirely contained within the DNS service. The existing call remains correct:
```python
dns_verified = dns_service.verify_txt_record(domain, "gondulf-verify-domain")
```
### 3. Template Remains Correct
The template (`src/gondulf/templates/verification_error.html`) already shows the correct instructions and needs no changes.
## Data Models
No data model changes required.
## API Contracts
No API changes required. This is an internal bug fix.
## Error Handling
### DNS Query Errors
The existing error handling in `get_txt_records` is sufficient:
- NXDOMAIN: Domain doesn't exist (including subdomain)
- NoAnswer: No TXT records found
- Timeout: DNS server timeout
- Other DNS exceptions: General failure
All these cases correctly return False for verification failure.
### Logging Updates
Update log messages to include which domain was actually queried:
- Success: Include both the requested domain and the queried domain
- Failure: Include both domains to aid debugging
## Security Considerations
1. **No New Attack Vectors**: The fix doesn't introduce new security concerns
2. **DNS Rebinding**: Not applicable (we're only reading TXT records)
3. **Cache Poisoning**: Existing DNS resolver safeguards apply
4. **Subdomain Takeover**: The `_gondulf` prefix is specifically chosen to avoid conflicts
## Testing Strategy
### Unit Tests Required
1. **Test Gondulf domain verification with correct TXT record**
- Mock DNS response for `_gondulf.example.com` with value `gondulf-verify-domain`
- Verify `verify_txt_record("example.com", "gondulf-verify-domain")` returns True
2. **Test Gondulf domain verification with missing TXT record**
- Mock DNS response for `_gondulf.example.com` with no TXT records
- Verify `verify_txt_record("example.com", "gondulf-verify-domain")` returns False
3. **Test Gondulf domain verification with wrong TXT value**
- Mock DNS response for `_gondulf.example.com` with value `wrong-value`
- Verify `verify_txt_record("example.com", "gondulf-verify-domain")` returns False
4. **Test non-Gondulf TXT verification still works**
- Mock DNS response for `example.com` (not prefixed) with value `other-value`
- Verify `verify_txt_record("example.com", "other-value")` returns True
- Ensures backward compatibility for any other TXT verification uses
5. **Test NXDOMAIN handling**
- Mock NXDOMAIN for `_gondulf.example.com`
- Verify `verify_txt_record("example.com", "gondulf-verify-domain")` returns False
### Integration Test
1. **End-to-end authorization flow test**
- Set up test domain with `_gondulf.{domain}` TXT record
- Attempt authorization flow
- Verify DNS verification passes
### Manual Testing
1. Configure real DNS record: `_gondulf.yourdomain.com` with value `gondulf-verify-domain`
2. Test authorization flow
3. Verify successful DNS verification
4. Check logs show correct domain being queried
## Acceptance Criteria
1. ✅ DNS verification queries `_gondulf.{domain}` when verifying Gondulf domain ownership
2. ✅ Users with correctly configured TXT records can successfully verify their domain
3. ✅ Log messages clearly show which domain was queried for debugging
4. ✅ Non-Gondulf TXT verification (if used elsewhere) continues to work
5. ✅ All existing tests pass
6. ✅ New unit tests cover the fix
7. ✅ Manual testing confirms real DNS records work
## Implementation Notes
1. **Critical Bug**: This is a P0 bug that completely breaks domain verification
2. **Simple Fix**: The fix is straightforward - just add the prefix when appropriate
3. **Test Thoroughly**: While the fix is simple, ensure comprehensive testing
4. **Verify Logs**: Update logging to be clear about what domain is being queried
## Migration Considerations
None required. This is a bug fix that makes the code work as originally intended. No database migrations or data changes needed.

151
docs/reports/2025-11-22-dns-verification-bug-fix.md Normal file
View File

@@ -0,0 +1,151 @@
# Implementation Report: DNS Verification Bug Fix
**Date**: 2025-11-22
**Developer**: Claude (Developer Agent)
**Design Reference**: /docs/designs/dns-verification-bug-fix.md
## Summary
Fixed a critical bug in the DNS TXT record verification that caused domain verification to always fail. The code was querying the base domain (e.g., `example.com`) instead of the `_gondulf.{domain}` subdomain (e.g., `_gondulf.example.com`) where users are instructed to place their TXT records. The fix modifies the `verify_txt_record` method in `src/gondulf/dns.py` to prefix the domain with `_gondulf.` when the expected value is `gondulf-verify-domain`. All tests pass with 100% coverage on the DNS module.
## What Was Implemented
### Components Modified
1. **`src/gondulf/dns.py`** - DNSService class
- Modified `verify_txt_record` method to query the correct subdomain
- Updated docstring to document the Gondulf-specific behavior
- Updated all logging statements to include both the requested domain and the queried domain
2. **`tests/unit/test_dns.py`** - DNS unit tests
- Added new test class `TestGondulfDomainVerification` with 7 test cases
- Tests verify the critical bug fix behavior
- Tests ensure backward compatibility for non-Gondulf TXT verification
### Key Implementation Details
The fix implements Option A from the design document - modifying the existing `verify_txt_record` method rather than creating a new dedicated method. This keeps the fix localized and maintains backward compatibility.
**Core logic added:**
```python
# For Gondulf domain verification, query _gondulf subdomain
if expected_value == "gondulf-verify-domain":
query_domain = f"_gondulf.{domain}"
else:
query_domain = domain
```
**Logging updates:**
- Success log now shows: `"TXT record verification successful for domain={domain} (queried {query_domain})"`
- Failure log now shows: `"TXT record verification failed: expected value not found for domain={domain} (queried {query_domain})"`
- Error log now shows: `"TXT record verification failed for domain={domain} (queried {query_domain}): {e}"`
## How It Was Implemented
### Approach
1. **Reviewed design document** - Confirmed Option A (modify existing method) was the recommended approach
2. **Reviewed standards** - Checked coding.md and testing.md for requirements
3. **Implemented the fix** - Single edit to `verify_txt_record` method
4. **Added comprehensive tests** - Created new test class covering all scenarios from design
5. **Ran full test suite** - Verified no regressions
### Deviations from Design
No deviations from design.
The implementation follows the design document exactly:
- Used Option A (modify `verify_txt_record` method)
- Added the domain prefixing logic as specified
- Updated logging to show both domains
- No changes needed to authorization router or templates
## Issues Encountered
No significant issues encountered.
The fix was straightforward as designed. The existing code structure made the change clean and isolated.
## Test Results
### Test Execution
```
============================= test session starts ==============================
platform linux -- Python 3.11.14, pytest-9.0.1, pluggy-1.6.0
plugins: anyio-4.11.0, asyncio-1.3.0, mock-3.15.1, cov-7.0.0, Faker-38.2.0
collected 487 items
[... all tests ...]
================= 482 passed, 5 skipped, 36 warnings in 20.00s =================
```
### Test Coverage
- **Overall Coverage**: 90.44%
- **DNS Module Coverage**: 100% (`src/gondulf/dns.py`)
- **Coverage Tool**: pytest-cov 7.0.0
### Test Scenarios
#### New Unit Tests Added (TestGondulfDomainVerification)
1. **test_gondulf_verification_queries_prefixed_subdomain** - Critical test verifying the bug fix
- Verifies `verify_txt_record("example.com", "gondulf-verify-domain")` queries `_gondulf.example.com`
2. **test_gondulf_verification_with_missing_txt_record** - Tests NoAnswer handling
- Verifies returns False when no TXT records exist at `_gondulf.{domain}`
3. **test_gondulf_verification_with_wrong_txt_value** - Tests value mismatch
- Verifies returns False when TXT value doesn't match
4. **test_non_gondulf_verification_queries_base_domain** - Backward compatibility test
- Verifies other TXT verification still queries base domain (not prefixed)
5. **test_gondulf_verification_with_nxdomain** - Tests NXDOMAIN handling
- Verifies returns False when `_gondulf.{domain}` doesn't exist
6. **test_gondulf_verification_among_multiple_txt_records** - Tests multi-record scenarios
- Verifies correct value found among multiple TXT records
7. **test_gondulf_verification_with_subdomain** - Tests subdomain handling
- Verifies `blog.example.com` queries `_gondulf.blog.example.com`
#### Existing Tests (All Pass)
All 22 existing DNS tests continue to pass, confirming no regressions:
- TestDNSServiceInit (1 test)
- TestGetTxtRecords (7 tests)
- TestVerifyTxtRecord (7 tests)
- TestCheckDomainExists (5 tests)
- TestResolverFallback (2 tests)
### Test Results Analysis
- All 29 DNS tests pass (22 existing + 7 new)
- 100% coverage on dns.py module
- Full test suite (487 tests) passes with no regressions
- 5 skipped tests are unrelated (SQL injection tests awaiting implementation)
- Deprecation warnings are unrelated to this change (FastAPI/Starlette lifecycle patterns)
## Technical Debt Created
No technical debt identified.
The fix is clean, well-tested, and follows the existing code patterns. The implementation matches the design exactly.
## Next Steps
1. **Manual Testing** - Per the design document, manual testing with a real DNS record is recommended:
- Configure real DNS record: `_gondulf.yourdomain.com` with value `gondulf-verify-domain`
- Test authorization flow
- Verify successful DNS verification
- Check logs show correct domain being queried
2. **Deployment** - This is a P0 critical bug fix that should be deployed to production as soon as testing is complete.
## Sign-off
Implementation status: Complete
Ready for Architect review: Yes

23
src/gondulf/dns.py
View File

@@ -94,32 +94,45 @@ class DNSService:
"""
Verify that domain has a TXT record with the expected value.
For Gondulf domain verification (expected_value="gondulf-verify-domain"),
queries the _gondulf.{domain} subdomain as per specification.
Args:
domain: Domain name to verify
domain: Domain name to verify (e.g., "example.com")
expected_value: Expected TXT record value
Returns:
True if expected value found in TXT records, False otherwise
"""
try:
txt_records = self.get_txt_records(domain)
# For Gondulf domain verification, query _gondulf subdomain
if expected_value == "gondulf-verify-domain":
query_domain = f"_gondulf.{domain}"
else:
query_domain = domain
txt_records = self.get_txt_records(query_domain)
# Check if expected value is in any TXT record
for record in txt_records:
if expected_value in record:
logger.info(
f"TXT record verification successful for domain={domain}"
f"TXT record verification successful for domain={domain} "
f"(queried {query_domain})"
)
return True
logger.debug(
f"TXT record verification failed: expected value not found "
f"for domain={domain}"
f"for domain={domain} (queried {query_domain})"
)
return False
except DNSError as e:
logger.warning(f"TXT record verification failed for domain={domain}: {e}")
logger.warning(
f"TXT record verification failed for domain={domain} "
f"(queried {query_domain}): {e}"
)
return False
def check_domain_exists(self, domain: str) -> bool:

108
tests/unit/test_dns.py
View File

@@ -201,6 +201,114 @@ class TestVerifyTxtRecord:
assert result is True
class TestGondulfDomainVerification:
"""Tests for Gondulf domain verification (queries _gondulf.{domain})."""
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_queries_prefixed_subdomain(self, mock_resolve):
"""
Test Gondulf domain verification queries _gondulf.{domain}.
This is the critical bug fix test - verifies we query the correct
subdomain (_gondulf.example.com) not the base domain (example.com).
"""
mock_rdata = MagicMock()
mock_rdata.strings = [b"gondulf-verify-domain"]
mock_resolve.return_value = [mock_rdata]
service = DNSService()
result = service.verify_txt_record("example.com", "gondulf-verify-domain")
assert result is True
# Critical: verify we queried _gondulf.example.com, not example.com
mock_resolve.assert_called_once_with("_gondulf.example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_with_missing_txt_record(self, mock_resolve):
"""Test Gondulf verification fails when no TXT records exist at _gondulf subdomain."""
mock_resolve.side_effect = dns.resolver.NoAnswer()
service = DNSService()
result = service.verify_txt_record("example.com", "gondulf-verify-domain")
assert result is False
mock_resolve.assert_called_once_with("_gondulf.example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_with_wrong_txt_value(self, mock_resolve):
"""Test Gondulf verification fails when TXT value doesn't match."""
mock_rdata = MagicMock()
mock_rdata.strings = [b"wrong-value"]
mock_resolve.return_value = [mock_rdata]
service = DNSService()
result = service.verify_txt_record("example.com", "gondulf-verify-domain")
assert result is False
mock_resolve.assert_called_once_with("_gondulf.example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_non_gondulf_verification_queries_base_domain(self, mock_resolve):
"""
Test non-Gondulf TXT verification still queries base domain.
Ensures backward compatibility - other TXT verification uses
should not be affected by the _gondulf prefix fix.
"""
mock_rdata = MagicMock()
mock_rdata.strings = [b"some-other-value"]
mock_resolve.return_value = [mock_rdata]
service = DNSService()
result = service.verify_txt_record("example.com", "some-other-value")
assert result is True
# Should query example.com directly, not _gondulf.example.com
mock_resolve.assert_called_once_with("example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_with_nxdomain(self, mock_resolve):
"""Test Gondulf verification handles NXDOMAIN for _gondulf subdomain."""
mock_resolve.side_effect = dns.resolver.NXDOMAIN()
service = DNSService()
result = service.verify_txt_record("example.com", "gondulf-verify-domain")
assert result is False
mock_resolve.assert_called_once_with("_gondulf.example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_among_multiple_txt_records(self, mock_resolve):
"""Test Gondulf verification finds value among multiple TXT records."""
mock_rdata1 = MagicMock()
mock_rdata1.strings = [b"v=spf1 include:example.com ~all"]
mock_rdata2 = MagicMock()
mock_rdata2.strings = [b"gondulf-verify-domain"]
mock_rdata3 = MagicMock()
mock_rdata3.strings = [b"other-record"]
mock_resolve.return_value = [mock_rdata1, mock_rdata2, mock_rdata3]
service = DNSService()
result = service.verify_txt_record("example.com", "gondulf-verify-domain")
assert result is True
mock_resolve.assert_called_once_with("_gondulf.example.com", "TXT")
@patch("gondulf.dns.dns.resolver.Resolver.resolve")
def test_gondulf_verification_with_subdomain(self, mock_resolve):
"""Test Gondulf verification works correctly with subdomains."""
mock_rdata = MagicMock()
mock_rdata.strings = [b"gondulf-verify-domain"]
mock_resolve.return_value = [mock_rdata]
service = DNSService()
result = service.verify_txt_record("blog.example.com", "gondulf-verify-domain")
assert result is True
# Should query _gondulf.blog.example.com
mock_resolve.assert_called_once_with("_gondulf.blog.example.com", "TXT")
class TestCheckDomainExists:
"""Tests for check_domain_exists method."""