Add mTLS authentication support#102
Conversation
|
Thanks for putting this together, @anandghegde! mTLS support is a solid addition for self-hosted and reverse-proxied Confluence environments. The overall approach is clean — you've covered CLI flags, environment variables, and profile storage consistently, and the existing auth flows remain untouched. I left a few notes after reviewing the diff: Issues & Suggestions1. Certificate file existence check (Medium) In if (this.mtls.clientCert) {
if (!fs.existsSync(this.mtls.clientCert)) {
throw new Error(`Client certificate file not found: ${this.mtls.clientCert}`);
}
options.cert = fs.readFileSync(this.mtls.clientCert);
}2. Validate file paths at save time (Low)
3. Complex ternary in This line in const authType = normalizeAuthType(envAuthType || (envMtls && !envToken ? 'mtls' : envAuthType), Boolean(envEmail));Extracting it into a separate variable improves readability: const inferredAuthType = envAuthType || (envMtls && !envToken ? 'mtls' : undefined);
const authType = normalizeAuthType(inferredAuthType, Boolean(envEmail));4.
5. Interactive wizard missing cert path prompts (Low) When a user selects mTLS in the interactive 6. Test temp file cleanup (Nit) In Items 1 and 5 are the ones I'd recommend addressing before merge. The rest are minor improvements that can be follow-ups. Great work overall! |
- Add fs.existsSync checks before readFileSync in buildHttpsAgent for friendlier error messages when cert files are missing - Validate cert/key/CA file paths exist at config save time in validateMtlsConfig - Simplify complex ternary in getConfig env logic into separate variable - Error when authType=mtls is combined with protocol=http in CLI validation, env config, and profile config - Add interactive wizard prompts for client cert, client key, and CA cert paths when mTLS auth is selected during 'confluence init' - Wrap mTLS test bodies in try/finally for reliable temp file cleanup
|
Thank you for the feedback @pchuri . I have made few changes. Let me know if this works |
pchuri
left a comment
There was a problem hiding this comment.
LGTM — well-structured addition that integrates mTLS cleanly across CLI flags, env vars, profiles, and the interactive wizard without disturbing existing auth flows. A few suggestions below, none blocking.
Suggestions
Private key file permission check (follow-up PR)
buildHttpsAgent() reads the client key file without verifying its permissions. On Unix systems, a private key readable by other users (e.g., 0644) is a security risk. Consider adding a warning or error when the key file mode is more permissive than 0600.
Prompt validation duplication
The when/validate logic for tlsClientCert, tlsClientKey, and tlsCaCert is nearly identical in three places — promptForMissingValues(), the interactive wizard in initConfig(), and validateCliOptions(). Extracting a shared helper (e.g., mtlsCertQuestion(name, message, required)) would reduce maintenance surface. Not blocking, but worth a follow-up refactor.
README paths should be platform-neutral
The mTLS examples use macOS-specific paths (/Users/you/.certs/). Suggest changing to /path/to/client.pem or ~/.certs/ for cross-platform readability. This is small enough to fix before merge if you'd like.
Minor nits
- The
--tokenvalidation change (&&→!== undefined) is unrelated to mTLS — consider splitting into a separate commit for cleaner history. - The
http + mtlsincompatibility check is duplicated in CLI validation, env config, and profile config. Could share a single helper.
|
🎉 This PR is included in version 1.30.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
This adds support for Confluence deployments that authenticate at the TLS layer with client certificates instead of Basic or Bearer credentials.
Changes
mtlsas a supported--auth-type--tls-client-cert,--tls-client-key, and--tls-ca-certhttps.Agentusing the provided cert/key/CA filesAuthorizationheader in mTLS modeValidation
npm test -- --runInBandnpm run lint