Support self-signed SSL certificates#126
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Thank you for this valuable contribution! 🎉First, I want to thank you for adding self-signed SSL certificate support to n8n-mcp. This is a much-needed feature that will help users working with n8n instances using self-signed certificates. Your documentation is particularly impressive - comprehensive and well-structured across all the relevant files. Required ChangesI've reviewed the PR and identified a few critical issues that need to be addressed before we can merge: 🚨 Critical Security FixFile: The current implementation disables SSL verification entirely with // Current (insecure):
this.httpsAgent = new https.Agent({
ca: cert,
rejectUnauthorized: false // ❌ Remove this line
});
// Fixed (secure):
this.httpsAgent = new https.Agent({
ca: cert
// rejectUnauthorized remains true by default
});📝 Schema Validation FixFile: Add the missing schema validation for const n8nApiConfigSchema = z.object({
N8N_API_URL: z.string().url().optional(),
N8N_API_KEY: z.string().min(1).optional(),
N8N_API_TIMEOUT: z.coerce.number().positive().default(30000),
N8N_API_MAX_RETRIES: z.coerce.number().positive().default(3),
N8N_CERT_PATH: z.string().optional(), // ✅ Add this line
});🔧 TypeScript Interface UpdateFile: Update the interface to include the cert property: export interface N8nApiClientConfig {
baseUrl: string;
apiKey: string;
timeout?: number;
maxRetries?: number;
cert?: string; // ✅ Add this line
}Suggested Improvements (Optional but Recommended)Enhanced Error HandlingConsider improving the certificate loading error handling in if (config.N8N_CERT_PATH) {
try {
const certPath = path.resolve(config.N8N_CERT_PATH);
// Check if file exists
if (\!fs.existsSync(certPath)) {
throw new Error(`Certificate file not found: ${certPath}`);
}
// Verify it's a file
const stats = fs.statSync(certPath);
if (\!stats.isFile()) {
throw new Error(`Certificate path is not a file: ${certPath}`);
}
// Read certificate
cert = fs.readFileSync(certPath, 'utf-8');
// Basic PEM format validation
if (\!cert.includes('-----BEGIN CERTIFICATE-----')) {
throw new Error(`Invalid certificate format. Expected PEM format in: ${certPath}`);
}
logger.info(`Custom SSL certificate loaded from: ${certPath}`);
} catch (error) {
logger.error(`Failed to load certificate: ${error.message}`);
throw error;
}
}Test CoverageAdding tests would be valuable, especially for:
What Works Great ✅
Next StepsOnce you've made the critical fixes (especially removing Feel free to ask if you need any clarification or help with these changes. Thanks again for your contribution! 🙏 |
|
@czlonkowski, the |
|
why do you need to handle self signed certs when a certbot / lets encrypt is 100% free and a domain is 5-10$/year... i guess local hosted instances? |
|
@PyroFilmsFX , depending on how a certificate is generated, we may need to add it to the whitelist — for example, due to a missing root certificate or certificate authority. If the user only has access to the application through a web browser, and not to the server or n8n configuration, it would be more convenient to simply add the n8n certificate to the whitelist to get the connection working. |
|
@gifflet are you considering to extend this feature to also support mTLS? |
|
@czlonkowski When will this PR be merged into the main branch? I really need this feature, thanks a lot!!! |
…fication skip - Enhance SSL/TLS configuration with support for custom CA or self-signed certificates via N8N_CERT_PATH. - Introduce N8N_SKIP_SSL_VERIFICATION environment variable to optionally disable SSL cert verification (for development only). - Add detailed SSL/TLS troubleshooting guide documentation with common errors, fixes, and best practices. - Update n8n API client to use custom cert and conditionally disable SSL verification with proper warnings. - Improve logging to warn about insecure SSL skip usage.
|
@czlonkowski @Catsofsuffering Merge conflicts resolved. Ready for review! |
PR Review SummaryThank you for this valuable contribution! Self-signed SSL certificate support is an important feature for enterprise deployments. After thorough review by both code and deployment reviewers, we've identified several issues that need to be addressed. Critical Issues (Must Fix)1. Duplicate
|
| Concern | Status |
|---|---|
| Docker volume mounting | ✅ Correctly uses :ro flag |
| Kubernetes documentation | ❌ Missing - please add Secret/ConfigMap examples |
| Certificate rotation | |
| File permissions check | ❌ Consider warning if cert is world-readable |
What Works Well 👍
- ✅ Two-tier security model (cert path vs skip verification)
- ✅ Appropriate warning logging when SSL disabled
- ✅ HTTPS agent correctly shared across client, health check, and webhook
- ✅ Excellent documentation (345-line troubleshooting guide!)
- ✅ Fully backward compatible (opt-in only)
- ✅ Docker examples with read-only volume flag
Summary
Status: Changes Requested
Please address:
- Fix duplicate
path.resolve()call - Add SSL support to
getN8nApiConfigFromContext() - Resolve merge conflict marker in docs
- Add unit tests (target 70%+ coverage)
- Block
N8N_SKIP_SSL_VERIFICATIONin production
The feature itself is well-designed and the documentation is excellent. Once these issues are fixed, this will be ready to merge!
Review conducted by code-reviewer and deployment-engineer agents
|
Adding To the env in mcp config will allow self signed certs to work, no need for code changes, just update docs e.g. |
This PR introduces support for self-signed SSL certificates when connecting to an n8n instance. Users can now specify a certificate path using the new N8N_CERT_PATH environment variable. Key changes include: