fix(dns): query _gondulf subdomain for domain verification

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>
2025-11-22 17:46:38 -07:00
5 changed files with 548 additions and 5 deletions
--- a/docs/decisions/ADR-011-dns-txt-subdomain-prefix.md
+++ 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
--- a/docs/designs/dns-verification-bug-fix.md
+++ 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.
--- a/docs/reports/2025-11-22-dns-verification-bug-fix.md
+++ 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
--- 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:
--- 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."""