ngit-grasp/d6ee-defensive-relay-features.md

# Defensive Relay Features

**ID:** d6ee

> **⚠️ COORDINATION REQUIRED:** This issue is part of the Administrator Observability and Management Strategy (issue ec1f).
> **Before starting work:** Check issue ec1f for current phase, dependencies, and coordination requirements.
> This issue contributes to **Phase 4 (Enforcement)**. Only Phase 1 (config-based protection) is currently approved.

## Issue Summary

~~Implement defensive rate limiting and resource controls to protect the relay from abuse. Focus on per-connection and per-IP rate limits using rust-nostr relay-builder capabilities and custom extensions where needed.~~

**UPDATE 2026-01-13:** Need to review and validate the current approach before implementing. Concerns about over-engineering and maintaining code that relay-builder should handle. Per-IP throttling may be more appropriate for git data fetching (separate issue) than general relay operations.

## ✅ DECISION: PHASE 1 ONLY (Config-Based Protection)

**Status:** APPROVED - Implement Phase 1, defer Phase 2/3  
**Priority:** High (addresses critical DoS vulnerability)

### Review Complete - Key Decisions

**✅ Review Tasks Completed (2026-01-14):**
- [x] Task 1: Validated current state - only database + WritePolicy configured
- [x] Task 2: Validated docs - 96.7% accurate, one minor error found
- [x] Task 3: Gap analysis - total connection limit is critical missing piece
- [x] Task 4: Config-only path recommended (70% protection, 15% effort)
- [x] Task 5: Upstream contribution not recommended (no demand, specialized use case)

**Decision Rationale:**
1. **Config-only is sufficient** - Total connection limit addresses primary DoS vector
2. **Defer per-IP enforcement** - Implement only if abuse detected in production
3. **Git throttling separate** - Git endpoint needs separate IP-based throttling (analyze separately)
4. **Upstream later** - If Phase 2 needed, prefer PR to rust-nostr/relay-builder

### Phase 1: Explicit Rate Limits & Connection Limit (IMPLEMENT NOW)

**Scope:**
1. Make RateLimit explicit in code (currently using defaults)
   - max_reqs: 500 (max subscriptions per connection)
   - notes_per_minute: 60 (max events per minute per connection)
2. Add max_connections configuration (default: 500)
3. Update all 4 config locations (CRITICAL - see AGENTS.md)
4. Fix documentation error (filter limit 5000→500)
5. Document Phase 2 deferral decision in defensive-measures.md

**Files to modify:**
- `src/nostr/builder.rs` - Add explicit RateLimit and max_connections
- `src/config.rs` - Add max_connections field
- `.env.example` - Add NGIT_MAX_CONNECTIONS
- `nix/module.nix` - Add maxConnections option
- `docs/reference/configuration.md` - Document new option
- `docs/explanation/defensive-measures.md` - Fix filter limit error + document Phase 2 deferral

**Effort:** 2-3 hours

**Acceptance Criteria:**
- [ ] RateLimit explicitly configured in builder.rs with comments
- [ ] max_connections configurable via NGIT_MAX_CONNECTIONS (default: 500)
- [ ] All 4 config locations synced
- [ ] Documentation error fixed (filter limit 5000→500)
- [ ] Documentation updated with Phase 2 deferral decision
- [ ] Integration test: Connection rejected after reaching total limit
- [ ] All tests passing
- [ ] Changes committed

### Phase 2 & 3: Per-IP Enforcement (DEFERRED - Future Work)

**Status:** Deferred until abuse detected in production

**When to implement:**
- IF ConnectionTracker shows IPs exceeding 10 connections
- IF event spam patterns emerge from single IPs
- Monitor for 2-4 weeks after Phase 1 deployment

**Preferred Implementation Path:**
1. **First choice:** Contribute to rust-nostr/relay-builder as PR
   - Propose IP-based rate limiting as optional feature
   - Let upstream maintain the code
   - Benefits entire Nostr ecosystem
   - See: docs/explanation/defensive-analysis-of-other-relays.md for research

2. **Fallback:** Implement in ngit-grasp if upstream not interested
   - Use original Phase 2 & 3 plan (see below)
   - Per-IP connection enforcement (4-6 hours)
   - Per-IP event rate limiting (6-8 hours)

**Implementation details preserved below for future reference.**

### Git Endpoint IP-Based Throttling (MOVED TO SEPARATE ISSUE)

**Status:** ✅ Spun out to issue ff38

**Rationale:**
- Git data fetching has different threat model than Nostr relay
- HTTP endpoints, not WebSocket connections
- Different attack vectors (bandwidth, CPU for pack generation)
- Should NOT interact with relay code

**See:** Issue ff38 (git-endpoint-ip-throttling) for full analysis and implementation plan

---

## PREVIOUS IMPLEMENTATION PLAN (FOR REFERENCE ONLY)

## Findings

### rust-nostr relay-builder Capabilities

**Built-in Features (✅ Available):**
- Per-connection subscription limits (`RateLimit.max_reqs`, default: 500)
- Per-connection event rate limits (`RateLimit.notes_per_minute`, default: 60)
- Total connection limit (`max_connections(n)`)
- Subscription ID length limit (`max_subid_length`, default: 250)
- Filter result limits (`max_filter_limit`, default: 5000)

**Custom ngit-grasp Features (✅ Implemented):**
- Per-IP connection monitoring (`ConnectionTracker` in `src/metrics/connection.rs`)
  - Status: Monitoring only, does NOT enforce limits
  - Tracks connections per IP, flags abusers at threshold (default: 10)
  - Privacy-preserving: IP addresses never exposed in Prometheus metrics
- Event blacklist (block all events from specific authors)
- Repository blacklist (block specific repositories/developers)
- WritePolicy plugin system (access to IP address for custom validation)

**Not Supported:**
- Per-IP subscription limits (tracked per connection, not per IP)
- Per-IP event rate limits (tracked per connection, not per IP)

## Architecture Decision

### Rate Limiting Strategy

**Per-Connection Limits (relay-builder):**
- Use built-in `RateLimit` configuration
- Default values: 500 concurrent subscriptions, 60 events/min
- Prevents single connection from overwhelming relay

**Per-IP Limits (custom implementation):**
- Connection enforcement: Extend `ConnectionTracker` to reject connections
- Event rate limiting: Add `IpRateLimiter` to `Nip34WritePolicy`
- Hardcoded sensible defaults (configurable in future if needed)

**Privacy Model:**
- IP addresses tracked internally, NEVER exposed in Prometheus metrics
- Only aggregate counts exposed (total rate limited, flagged IPs)
- Rate limit events logged with IP for operator debugging

### Metrics Changes: OUT OF SCOPE

**Decision:** Do NOT modify metrics in this issue. Current metrics implementation needs architectural review.

**Reasoning:**
- Current metrics use "abuse detection" threshold model
- New rate limiting uses enforcement model
- These should be aligned but require broader metrics redesign
- Deferring to separate issue to avoid scope creep

**Future work:**
- Create separate issue for metrics architectural review
- Align metrics with policy enforcement triggers
- Add rate limiting specific metrics (events rejected, IPs in cooldown)

## Implementation Plan

### Phase 1: Configure Built-in Rate Limits ⚡ (Quick Win)

**Estimated Time:** 1-2 hours  
**Can run in parallel:** No (sequential)

**Scope:**
1. Configure `RateLimit` in relay builder with defaults (500 subs, 60 events/min)
2. Add total connection limit (500 connections, configurable)

**Files to modify:**
- `src/nostr/builder.rs` - Add RateLimit and max_connections to LocalRelayBuilder
- `src/config.rs` - Add `max_connections` config field
- `.env.example` - Add NGIT_MAX_CONNECTIONS example
- `nix/module.nix` - Add maxConnections NixOS option
- `docs/reference/configuration.md` - Document new option

**Acceptance Criteria:**
- ✅ Relay configured with RateLimit (500 subs, 60 events/min)
- ✅ Total connection limit configurable via NGIT_MAX_CONNECTIONS (default: 500)
- ✅ Integration test: Connection rejected after reaching total limit
- ✅ All tests passing
- ✅ Changes committed with message: "Add per-connection rate limits and total connection limit"
- ✅ Documentation updated in defensive-measures.md

**Configuration sync (CRITICAL):**
- Update all FOUR locations: `src/config.rs`, `docs/reference/configuration.md`, `nix/module.nix`, `.env.example`

---

### Phase 2: Per-IP Connection Enforcement

**Estimated Time:** 4-6 hours  
**Can run in parallel:** No (depends on Phase 1)

**Scope:**
1. Extend `ConnectionTracker` to enforce per-IP connection limit (hardcoded: 10)
2. Add actix-web middleware to check limit before WebSocket upgrade
3. Return 429 Too Many Requests with retry-after header

**Files to create/modify:**
- `src/metrics/connection.rs` - Modify `on_connect()` to return Result, check limit
- `src/middleware/ip_limits.rs` - NEW: Actix middleware for connection enforcement
- `src/middleware/mod.rs` - NEW: Module exports
- `src/lib.rs` - Export middleware module
- `src/main.rs` - Add middleware to actix-web app

**Implementation Details:**

**ConnectionTracker changes:**
```rust
pub struct ConnectionTracker {
    max_per_ip: u32, // NEW: hardcoded to 10
    // ... existing fields
}

pub fn on_connect(&self, ip: IpAddr) -> Result<(), String> {
    // Check count BEFORE incrementing
    let current = self.connections.get(&ip).map(|i| i.count).unwrap_or(0);
    if current >= self.max_per_ip {
        return Err(format!("Connection limit ({}) exceeded", self.max_per_ip));
    }
    // Existing increment logic...
    Ok(())
}
```

**Actix middleware approach:**
```rust
// Middleware wraps relay handler
// Checks ConnectionTracker.on_connect() before WebSocket upgrade
// Returns 429 if limit exceeded
// Allows connection if under limit
```

**Acceptance Criteria:**
- ✅ ConnectionTracker enforces 10 connections per IP (hardcoded)
- ✅ 11th connection from same IP returns 429 Too Many Requests
- ✅ Error logged with IP address for operator debugging
- ✅ Integration test: Multiple connections from same IP rejected after limit
- ✅ Integration test: Connections from different IPs allowed
- ✅ All tests passing (including Phase 1 tests)
- ✅ Changes committed with message: "Enforce per-IP connection limit (max 10)"
- ✅ Documentation updated in defensive-measures.md

---

### Phase 3: Per-IP Event Rate Limiting

**Estimated Time:** 6-8 hours  
**Can run in parallel:** No (depends on Phase 1 & 2)

**Scope:**
1. Implement token bucket rate limiter
2. Integrate with `Nip34WritePolicy` to check IP rate before event validation
3. Hardcoded limit: 100 events per minute per IP

**Files to create/modify:**
- `src/nostr/ip_rate_limiter.rs` - NEW: Token bucket implementation
- `src/nostr/builder.rs` - Add IpRateLimiter to Nip34WritePolicy
- `src/nostr/mod.rs` - Export ip_rate_limiter module

**Implementation Details:**

**Token Bucket Algorithm:**
```rust
pub struct IpRateLimiter {
    limiters: DashMap<IpAddr, TokenBucket>,
    events_per_minute: u32, // Hardcoded: 100
}

struct TokenBucket {
    tokens: f64,              // Current tokens (0.0 to events_per_minute)
    last_refill: Instant,     // Last refill time
    capacity: f64,            // Max tokens (events_per_minute)
    refill_rate: f64,         // Tokens per second (events_per_minute / 60.0)
}

impl IpRateLimiter {
    pub fn check_and_consume(&self, ip: IpAddr) -> bool {
        // Refill tokens based on time elapsed
        // If tokens >= 1.0, consume 1 token and return true
        // Else return false (rate limited)
    }
}
```

**Nip34WritePolicy integration:**
```rust
impl WritePolicy for Nip34WritePolicy {
    fn admit_event(&self, event: &Event, addr: &SocketAddr) -> BoxedFuture<WritePolicyResult> {
        // Check IP rate limit FIRST (before blacklist checks)
        if !self.ip_rate_limiter.check_and_consume(addr.ip()) {
            tracing::warn!(ip = %addr.ip(), "Event rejected: IP rate limit exceeded");
            return WritePolicyResult::reject("rate limited: too many events from your IP");
        }
        
        // Existing validation (blacklist, GRASP-01, etc.)...
    }
}
```

**Cleanup task:**
- Spawn background task to remove stale IP entries (no activity for 5 minutes)
- Prevents unbounded memory growth
- Runs every 60 seconds

**Acceptance Criteria:**
- ✅ IpRateLimiter implements token bucket (100 events/min per IP)
- ✅ Integrated into WritePolicy (checked before event validation)
- ✅ 101st event from same IP within 1 minute rejected
- ✅ Events from different IPs allowed
- ✅ Stale IP entries cleaned up (no memory leak)
- ✅ Unit tests: Token bucket refill logic
- ✅ Unit tests: Rate limit enforcement
- ✅ Integration test: Send >100 events/min from single IP, verify rejection
- ✅ Integration test: Events accepted after rate limit window passes
- ✅ All tests passing (including Phase 1 & 2 tests)
- ✅ Changes committed with message: "Add per-IP event rate limiting (100 events/min)"
- ✅ Documentation updated in defensive-measures.md

---

### Phase 4: Documentation & Polish

**Estimated Time:** 2-3 hours  
**Can run in parallel:** No (depends on all phases)

**Scope:**
1. Update all documentation to reflect implemented features
2. Add operational notes for relay operators
3. Update design docs with architectural decisions

**Files to modify:**
- `docs/explanation/defensive-measures.md` - Move features from "Not Implemented" to "Implemented"
- `README.md` - Update defensive measures section
- `AGENTS.md` - Add notes about new rate limiting code patterns (if needed)

**Acceptance Criteria:**
- ✅ defensive-measures.md accurately reflects all implemented features
- ✅ README.md defensive measures section updated
- ✅ Configuration docs show all new options
- ✅ All four config locations synced (src/config.rs, docs, nix/module.nix, .env.example)
- ✅ Changes committed with message: "docs: Update defensive measures documentation"

---

## Testing Strategy

### Unit Tests
- `ConnectionTracker::on_connect()` enforcement logic (Phase 2)
- `IpRateLimiter::check_and_consume()` token bucket (Phase 3)
- Token refill calculations (Phase 3)
- Config parsing for max_connections (Phase 1)

### Integration Tests
- Connection rejection after total limit (Phase 1)
- Connection rejection after per-IP limit (Phase 2)
- Event rejection after per-IP rate limit (Phase 3)
- Rate limit window expiry (Phase 3)
- Multi-IP scenarios (all phases)

### Manual Testing (after all phases)
- Use `grasp-audit` to simulate attack scenarios
- Test with multiple clients from same IP
- Test with rapid event publishing
- Verify metrics don't expose IP addresses

---

## Configuration Summary

### Phase 1: New Config Options
```bash
# Total connection limit (default: 500)
NGIT_MAX_CONNECTIONS=500
```

### Phase 2 & 3: Hardcoded Values
- Per-IP connection limit: **10 connections** (hardcoded in ConnectionTracker)
- Per-IP event rate limit: **100 events/min** (hardcoded in IpRateLimiter)

**Rationale for hardcoding:**
- Proven defensive values based on common relay patterns
- Reduces configuration surface area
- Can be made configurable later if operators need flexibility
- Simpler initial implementation

---

## Privacy & Logging

**IP Address Handling:**
- ✅ Tracked internally in DashMap for rate limiting
- ✅ Logged when limits exceeded (operator debugging)
- ❌ NEVER exposed in Prometheus metrics (only aggregate counts)
- ❌ NEVER logged in normal operation (only when rate limited)

**Log Examples:**
```
WARN: Connection rejected from 192.168.1.100: Connection limit (10) exceeded
WARN: Event rejected from 192.168.1.100: IP rate limit exceeded (100 events/min)
```

---

## Out of Scope (Future Work)

### Metrics Architectural Review (Separate Issue)
**Current state:**
- Metrics use "abuse detection threshold" model (monitoring only)
- New rate limiting uses "enforcement" model (reject at limit)
- These should be unified

**Future work:**
- Create new issue for metrics redesign
- Add enforcement-specific metrics:
  - `ngit_connections_rejected_total{reason="total_limit|per_ip_limit"}`
  - `ngit_events_rejected_total{reason="ip_rate_limit|blacklist|..."}` 
  - `ngit_ip_rate_limited_active` (gauge of IPs currently in cooldown)
- Align with policy enforcement points
- Remove or repurpose "abuse threshold" concept

### Per-IP Subscription Limits
**Why out of scope:**
- Requires complex middleware to intercept REQ messages
- relay-builder tracks subscriptions per connection, not per IP
- Lower priority than event rate limiting

### HTTP Endpoint Protection
**Why out of scope:**
- Different attack surface (HTTP vs WebSocket)
- Needs separate rate limiting strategy
- Lower priority for git-focused relay

### Query Filtering (QueryPolicy)
**Why out of scope:**
- Not a critical defensive measure for git relay
- Can be added later if abuse patterns emerge

### Per-IP Git Data Throttling (Potential Separate Issue)
**Why might need separate issue:**
- Git data fetching (packs, objects) has different characteristics than Nostr events
- Already have domain-based throttling for outbound git fetches (`ThrottleManager`)
- Inbound git push/fetch might benefit from per-IP limits
- Should be scoped separately from general relay rate limiting
- **TODO:** Consider creating new issue for git-specific per-IP throttling

---

## Risks & Mitigations

### Risk 1: Legitimate users behind NAT/proxy
**Scenario:** Multiple clients behind single IP hit connection/rate limits

**Mitigation:**
- Per-IP connection limit set reasonably high (10)
- Per-IP event rate limit set reasonably high (100/min)
- Monitor logs for false positives
- Can add IP whitelist in future if needed

### Risk 2: Middleware integration complexity
**Scenario:** Actix middleware doesn't work with relay-builder

**Mitigation:**
- Test middleware approach early in Phase 2
- Fallback: Log warnings but don't enforce (monitoring mode)
- Alternative: Contribute hooks to relay-builder upstream

### Risk 3: Memory growth from IP tracking
**Scenario:** DashMap grows unbounded with unique IPs

**Mitigation:**
- Cleanup task removes stale entries (5+ min inactive)
- Proven pattern already used in ConnectionTracker
- Monitor memory in production

---

## Success Criteria

**How we'll know this is working:**

1. **Protection active:**
   - 11th connection from same IP rejected (Phase 2)
   - 101st event/min from same IP rejected (Phase 3)
   - Logs show IP addresses when limits hit

2. **No false positives:**
   - Legitimate users can connect and publish normally
   - Sync from other relays works without rate limiting issues
   - No user complaints about false rejections

3. **Resource usage stable:**
   - No memory leaks from IP tracking
   - Cleanup task removes stale entries
   - CPU usage reasonable with token bucket calculations

4. **Privacy maintained:**
   - IP addresses NEVER in Prometheus metrics
   - Only aggregate counts exposed
   - Metrics requests don't expose tracked IPs

---

## Progress Tracking

- [ ] Phase 1: Configure built-in rate limits (1-2h)
- [ ] Phase 2: Per-IP connection enforcement (4-6h)
- [ ] Phase 3: Per-IP event rate limiting (6-8h)
- [ ] Phase 4: Documentation & polish (2-3h)

**Total estimated time:** 13-19 hours

---

## Progress

### 2026-01-14 [Session 14:30] - REVIEW COMPLETE, PHASE 1 APPROVED
- **Status Change:** Review complete, proceeding with Phase 1 only
- Completed: All 5 review tasks using subagents
- **Decision:** Implement Phase 1 (config-only) with explicit rate limits
- **Deferred:** Phase 2 & 3 (per-IP enforcement) until abuse detected in production
- **Separate concern:** Git endpoint IP-based throttling (analyze separately)

**Review Findings:**
- Task 1: Current config uses only database + WritePolicy, all else is defaults
- Task 2: Documentation 96.7% accurate (1 minor error: filter limit 5000→500)
- Task 3: Critical gap is total connection limit (DoS vulnerability)
- Task 4: Config-only approach gives 70% protection for 15% effort
- Task 5: Upstream contribution not recommended (specialized use case, no demand)

**Phase 1 Scope (APPROVED):**
- Add explicit RateLimit configuration (make defaults visible in code)
- Add max_connections config option (default: 500)
- Update all 4 config locations (src/config.rs, docs, nix/module.nix, .env.example)
- Fix documentation error (filter limit)
- Effort: 2-3 hours

**Phase 2 & 3 Deferred (Future Work if Abuse Detected):**
- Per-IP connection enforcement (10 per IP)
- Per-IP event rate limiting (100 events/min per IP)
- **Preferred approach:** Contribute to rust-nostr/relay-builder as PR
- **Fallback:** Implement in ngit-grasp if upstream not interested
- Document implementation plan for future reference

**Git Endpoint Throttling (Separate Analysis Needed):**
- Git data fetching has different threat model than Nostr relay
- Needs IP-based throttling with sensible limits
- Should NOT interact with relay code
- Next: Analyze requirements with architect subagent

### 2026-01-13 [Session 21:15] - IMPLEMENTATION PAUSED
- **Status Change:** Paused implementation for validation review
- Concern: May be over-engineering with custom per-IP rate limiting
- Concern: Should relay-builder handle IP-based throttling, not ngit-grasp?
- Concern: Per-IP throttling might be more relevant for git data fetching (separate issue)
- Added: Review tasks section to validate current state before proceeding
- Decision: Complete review tasks before any implementation
- Next: Work through review tasks to determine simplest path forward

**Key Questions:**
- Is per-connection limiting (already in relay-builder) sufficient?
- Should we contribute IP rate limiting to upstream relay-builder?
- Should git data fetching have separate per-IP throttling issue?
- What's the minimal viable protection (prefer config over code)?

### 2026-01-13 [Session 20:43] - RESEARCH COMPLETED
- Completed: Comprehensive research on defensive features in major Nostr relays (strfry, nostr-rs-relay, khatru)
- Completed: Analysis of ngit-grasp's current defensive capabilities
- Completed: Research on Rust rate limiting ecosystem (governor crate)
- Created: `docs/explanation/defensive-analysis-of-other-relays.md` with detailed findings
- ~~Decision: The existing implementation plan is well-aligned with industry best practices~~
- ~~Next: Proceed with Phase 1 implementation (configure built-in rate limits)~~ **SUPERSEDED - see above**

**Key Findings:**
- Most relays have permissive defaults; khatru is most opinionated (2 events per 3min/IP)
- Governor crate is industry standard for Rust rate limiting (GCRA algorithm)
- ngit-grasp already has good infrastructure (ConnectionTracker, ThrottleManager pattern)
- Proposed defaults are well-balanced compared to other relays
- **However:** Need to validate if this complexity is necessary for our use case

## References

- rust-nostr relay-builder: `nostr-relay-builder` v0.44.0 (git rev 4767ad13)
- Token bucket algorithm: https://en.wikipedia.org/wiki/Token_bucket
- Actix-web middleware: https://actix.rs/docs/middleware/
- Current implementation: `src/metrics/connection.rs` (ConnectionTracker)
- Design docs: `docs/explanation/defensive-measures.md`
- Research: `docs/explanation/defensive-analysis-of-other-relays.md` (comprehensive analysis)