From ba1569ee219b27877f49e7f47f6f3da1db4e35af Mon Sep 17 00:00:00 2001 From: Philipp Kunz Date: Sun, 18 May 2025 22:41:41 +0000 Subject: [PATCH] new plan --- readme.plan.md | 192 +++++++++++++++++++------------------------------ 1 file changed, 74 insertions(+), 118 deletions(-) diff --git a/readme.plan.md b/readme.plan.md index 5a9d25c..b4330a7 100644 --- a/readme.plan.md +++ b/readme.plan.md @@ -1,134 +1,90 @@ -# SmartProxy ACME Simplification Plan +# SmartProxy Development Plan -## Overview -This plan addresses the certificate acquisition confusion in SmartProxy v19.0.0 and proposes simplifications to make ACME configuration more intuitive. +cat /home/philkunz/.claude/CLAUDE.md -## Current Issues -1. ACME configuration placement is confusing (route-level vs top-level) -2. SmartAcme initialization logic is complex and error-prone -3. Documentation doesn't clearly explain the correct configuration format -4. Error messages like "SmartAcme not initialized" are not helpful +## Critical Bug Fix: Missing Route Update Callback in updateRoutes Method -## Proposed Simplifications +### Problem Statement +SmartProxy v19.2.2 has a bug where the ACME certificate manager loses its route update callback when routes are dynamically updated. This causes the error "No route update callback set" and prevents automatic certificate acquisition. -### 1. Support Both Configuration Styles -- [x] Reread CLAUDE.md before starting implementation -- [x] Accept ACME config at both top-level and route-level -- [x] Use top-level ACME config as defaults for all routes -- [x] Allow route-level ACME config to override top-level defaults -- [x] Make email field required when any route uses `certificate: 'auto'` +### Root Cause +When `updateRoutes()` creates a new SmartCertManager instance, it fails to set the route update callback that's required for ACME challenges. This callback is properly set in `initializeCertificateManager()` but is missing from the route update flow. -### 2. Improve SmartAcme Initialization -- [x] Initialize SmartAcme when top-level ACME config exists with email -- [x] Initialize SmartAcme when any route has `certificate: 'auto'` -- [x] Provide clear error messages when initialization fails -- [x] Add debug logging for ACME initialization steps +### Implementation Plan -### 3. Simplify Certificate Configuration -- [x] Create helper method to validate ACME configuration -- [x] Auto-detect when port 80 is needed for challenges -- [x] Provide sensible defaults for ACME settings -- [x] Add configuration examples in documentation +#### Phase 1: Fix the Bug +1. **Update the updateRoutes method** in `/mnt/data/lossless/push.rocks/smartproxy/ts/proxies/smart-proxy/smart-proxy.ts` + - [ ] Add the missing callback setup before initializing the new certificate manager + - [ ] Ensure the callback is set after creating the new SmartCertManager instance -### 4. Update Documentation -- [x] Create clear examples for common ACME scenarios -- [x] Document the configuration hierarchy (top-level vs route-level) -- [x] Add troubleshooting guide for common certificate issues -- [x] Include migration guide from v18 to v19 +#### Phase 2: Create Tests +2. **Write comprehensive tests** for the route update functionality + - [ ] Create test file: `test/test.route-update-callback.node.ts` + - [ ] Test that callback is preserved when routes are updated + - [ ] Test that ACME challenges work after route updates + - [ ] Test edge cases (multiple updates, no cert manager, etc.) -### 5. Add Configuration Helpers -- [x] Create `SmartProxyConfig.fromSimple()` helper for basic setups (part of validation) -- [x] Add validation for common misconfigurations -- [x] Provide warning messages for deprecated patterns -- [x] Include auto-correction suggestions +#### Phase 3: Enhance Documentation +3. **Update documentation** to clarify the route update behavior + - [ ] Add section to certificate-management.md about dynamic route updates + - [ ] Document the callback requirement for ACME challenges + - [ ] Include example of proper route update implementation -## Implementation Steps +#### Phase 4: Prevent Future Regressions +4. **Refactor for better maintainability** + - [ ] Consider extracting certificate manager setup to a shared method + - [ ] Add code comments explaining the callback requirement + - [ ] Consider making the callback setup more explicit in the API -### Phase 1: Configuration Support ✅ -1. ✅ Update ISmartProxyOptions interface to clarify ACME placement -2. ✅ Modify SmartProxy constructor to handle top-level ACME config -3. ✅ Update SmartCertManager to accept global ACME defaults -4. ✅ Add configuration validation and helpful error messages +### Technical Details -### Phase 2: Testing ✅ -1. ✅ Add tests for both configuration styles -2. ✅ Test ACME initialization with various configurations -3. ✅ Verify certificate acquisition works in all scenarios -4. ✅ Test error handling and messaging +#### Specific Code Changes +1. In `updateRoutes()` method (around line 535), add: + ```typescript + // Set route update callback for ACME challenges + this.certManager.setUpdateRoutesCallback(async (routes) => { + await this.updateRoutes(routes); + }); + ``` -### Phase 3: Documentation ✅ -1. ✅ Update main README with clear ACME examples -2. ✅ Create dedicated certificate-management.md guide -3. ✅ Add migration guide for v18 to v19 users -4. ✅ Include troubleshooting section +2. Consider refactoring the certificate manager setup into a helper method to avoid duplication: + ```typescript + private async setupCertificateManager( + routes: IRouteConfig[], + certStore: string, + acmeOptions?: any + ): Promise { + const certManager = new SmartCertManager(routes, certStore, acmeOptions); + + // Always set up the route update callback + certManager.setUpdateRoutesCallback(async (routes) => { + await this.updateRoutes(routes); + }); + + if (this.networkProxyBridge.getNetworkProxy()) { + certManager.setNetworkProxy(this.networkProxyBridge.getNetworkProxy()); + } + + return certManager; + } + ``` -## Example Simplified Configuration +### Success Criteria +- [ ] ACME certificate acquisition works after route updates +- [ ] No "No route update callback set" errors occur +- [ ] All tests pass +- [ ] Documentation clearly explains the behavior +- [ ] Code is more maintainable and less prone to regression -```typescript -// Simplified configuration with top-level ACME -const proxy = new SmartProxy({ - // Global ACME settings (applies to all routes with certificate: 'auto') - acme: { - email: 'ssl@example.com', - useProduction: false, - port: 80 // Automatically listened on when needed - }, - - routes: [ - { - name: 'secure-site', - match: { domains: 'example.com', ports: 443 }, - action: { - type: 'forward', - target: { host: 'localhost', port: 8080 }, - tls: { - mode: 'terminate', - certificate: 'auto' // Uses global ACME settings - } - } - } - ] -}); +### Timeline +- Phase 1: Immediate fix (30 minutes) +- Phase 2: Test creation (1 hour) +- Phase 3: Documentation (30 minutes) +- Phase 4: Refactoring (1 hour) -// Or with route-specific ACME override -const proxy = new SmartProxy({ - routes: [ - { - name: 'special-site', - match: { domains: 'special.com', ports: 443 }, - action: { - type: 'forward', - target: { host: 'localhost', port: 8080 }, - tls: { - mode: 'terminate', - certificate: 'auto', - acme: { // Route-specific override - email: 'special@example.com', - useProduction: true - } - } - } - } - ] -}); -``` +Total estimated time: 3 hours -## Success Criteria ✅ -1. ✅ Users can configure ACME at top-level for all routes -2. ✅ Clear error messages guide users to correct configuration -3. ✅ Certificate acquisition works with minimal configuration -4. ✅ Documentation clearly explains all configuration options -5. ✅ Migration from v18 to v19 is straightforward - -## Timeline -- Phase 1: 2-3 days -- Phase 2: 1-2 days -- Phase 3: 1 day - -Total estimated time: 5-6 days - -## Notes -- Maintain backward compatibility with existing route-level ACME config -- Consider adding a configuration wizard for interactive setup -- Explore integration with popular DNS providers for DNS-01 challenges -- Add metrics/monitoring for certificate renewal status \ No newline at end of file +### Notes +- This is a critical bug that affects production use of SmartProxy +- The fix is straightforward but requires careful testing +- Consider backporting to v19.2.x branch if maintaining multiple versions \ No newline at end of file