From 1ef5cd92298b409e0298fb98226021a6ae30dd87 Mon Sep 17 00:00:00 2001 From: Phil Skentelbery Date: Sat, 22 Nov 2025 17:46:38 -0700 Subject: [PATCH] fix(dns): query _gondulf subdomain for domain verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../ADR-011-dns-txt-subdomain-prefix.md | 76 +++++++ docs/designs/dns-verification-bug-fix.md | 195 ++++++++++++++++++ .../2025-11-22-dns-verification-bug-fix.md | 151 ++++++++++++++ src/gondulf/dns.py | 23 ++- tests/unit/test_dns.py | 108 ++++++++++ 5 files changed, 548 insertions(+), 5 deletions(-) create mode 100644 docs/decisions/ADR-011-dns-txt-subdomain-prefix.md create mode 100644 docs/designs/dns-verification-bug-fix.md create mode 100644 docs/reports/2025-11-22-dns-verification-bug-fix.md diff --git a/docs/decisions/ADR-011-dns-txt-subdomain-prefix.md b/docs/decisions/ADR-011-dns-txt-subdomain-prefix.md new file mode 100644 index 0000000..7135d12 --- /dev/null +++ b/docs/decisions/ADR-011-dns-txt-subdomain-prefix.md @@ -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 \ No newline at end of file diff --git a/docs/designs/dns-verification-bug-fix.md b/docs/designs/dns-verification-bug-fix.md new file mode 100644 index 0000000..a09ce9b --- /dev/null +++ b/docs/designs/dns-verification-bug-fix.md @@ -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. \ No newline at end of file diff --git a/docs/reports/2025-11-22-dns-verification-bug-fix.md b/docs/reports/2025-11-22-dns-verification-bug-fix.md new file mode 100644 index 0000000..238b565 --- /dev/null +++ b/docs/reports/2025-11-22-dns-verification-bug-fix.md @@ -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 diff --git a/src/gondulf/dns.py b/src/gondulf/dns.py index ec429c5..91c4cb5 100644 --- a/src/gondulf/dns.py +++ b/src/gondulf/dns.py @@ -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: diff --git a/tests/unit/test_dns.py b/tests/unit/test_dns.py index cf22e01..15792cb 100644 --- a/tests/unit/test_dns.py +++ b/tests/unit/test_dns.py @@ -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."""