feat: add mTLS authentication support#27
Conversation
pchuri
left a comment
There was a problem hiding this comment.
I left three inline comments for issues I found while testing the new auth flow.
| authType: { | ||
| type: 'string', | ||
| enum: ['basic', 'bearer', 'mtls'], | ||
| default: 'bearer' |
There was a problem hiding this comment.
Defaulting authType to bearer breaks backward compatibility for existing stored basic-auth configs. Older configs only have server, username, and token; once conf applies this schema default, getRequiredConfig() stops inferring basic auth from username, and JiraClient sends Authorization: Bearer ... instead of basic auth. Can we keep this unset for legacy configs and only infer bearer/basic when authType is actually missing?
| hasMtlsConfig || | ||
| (this.has('server') && this.has('token')) || | ||
| (process.env.JIRA_HOST && process.env.JIRA_API_TOKEN) || | ||
| (process.env.JIRA_DOMAIN && process.env.JIRA_USERNAME && process.env.JIRA_API_TOKEN) |
There was a problem hiding this comment.
If authType is mtls, this still treats the legacy token-based paths as fully configured. In practice, jira config can report success with authType=mtls and a stale token, and then the next getRequiredConfig() call fails because the cert/key are missing. It would be safer to make isConfigured() respect the selected auth mode here.
| return null; | ||
| } | ||
| return `Bearer ${this.config.token}`; | ||
| } |
There was a problem hiding this comment.
This silently switches an explicit authType: 'basic' config into bearer auth whenever username is empty. Since this PR now exposes --auth-type basic, a misconfigured basic setup should probably fail fast instead of sending Authorization: Bearer ... and changing auth modes under the hood.
|
Thanks for putting this together and for adding mTLS support here. I left a few inline comments from my review; when you have a moment, could you please take a look at them? |
pchuri
left a comment
There was a problem hiding this comment.
I reviewed the follow-up auth fix and left two additional inline comments for edge cases that still look open.
| process.env.JIRA_TLS_CLIENT_KEY; | ||
| if (hasMtlsEnv) return true; | ||
|
|
||
| if (process.env.JIRA_HOST && process.env.JIRA_API_TOKEN) return true; |
There was a problem hiding this comment.
This still lets an incomplete mTLS environment config look valid if a stale JIRA_API_TOKEN is exported. For example, with JIRA_AUTH_TYPE=mtls, JIRA_HOST=jira.example.com, and no cert/key, isConfigured() falls through to this branch and returns true, but getRequiredConfig() immediately throws an mTLS configuration error. It looks like the env-path needs the same auth-mode gating you added for stored config.
| return this.has('tlsClientCert') && this.has('tlsClientKey'); | ||
| } | ||
| if (authType === 'basic') { | ||
| return this.has('username') && this.has('token'); |
There was a problem hiding this comment.
has('username') / has('token') only check key presence, so authType=basic still passes validation when either value is an empty string. I reproduced this with username: '' and token: 'abc': isConfigured() returned true and getRequiredConfig() returned a basic-auth config, with the failure only happening later in JiraClient. It would be better to validate non-empty values here as well.
Address review feedback on PR pchuri#27: - When JIRA_AUTH_TYPE=mtls is set via env, treat it as authoritative so a stale JIRA_API_TOKEN can no longer make isConfigured() return true while required cert/key env vars are missing. - Add a hasNonEmpty() helper and use it for username, token, tlsClientCert, and tlsClientKey in isConfigured() and the stored-config branches of getRequiredConfig() so that empty or whitespace-only stored values are rejected instead of silently passing validation. - Extend tests/config.test.js with coverage for env-mTLS precedence, partial env-mTLS, env overriding complete stored basic auth, and empty/whitespace-only credentials across basic, bearer, mTLS, and legacy configurations.
pchuri
left a comment
There was a problem hiding this comment.
I left one more inline comment for an environment-variable edge case that still looks open.
| // fall through to the JIRA_API_TOKEN path just because a stale token is | ||
| // still in the environment. | ||
| if (process.env.JIRA_HOST && process.env.JIRA_AUTH_TYPE === 'mtls') { | ||
| return Boolean(process.env.JIRA_TLS_CLIENT_CERT && process.env.JIRA_TLS_CLIENT_KEY); |
There was a problem hiding this comment.
This still treats whitespace-only env vars as valid because Boolean(...) only checks presence, not content. I reproduced JIRA_AUTH_TYPE=mtls, JIRA_TLS_CLIENT_CERT=' ', and JIRA_TLS_CLIENT_KEY=' ': isConfigured() returned true, but getRequiredConfig() immediately failed on the bogus file path. The same issue also seems to exist for the token-based env branches below (JIRA_API_TOKEN=' ', JIRA_USERNAME=' '). It would be safer to trim and validate env values the same way you now do for stored config.
Add support for JIRA deployments that authenticate at the TLS layer with client certificates instead of Basic or Bearer credentials. Changes: - Add `mtls` as a supported `--auth-type` - Add CLI flags for `--tls-client-cert`, `--tls-client-key`, and `--tls-ca-cert` - Support mTLS config in stored configuration and environment variables - Configure the HTTP client with an `https.Agent` using the provided cert/key/CA files - Skip the `Authorization` header in mTLS mode - Validate that mTLS mode has the required client certificate and key - Document mTLS usage in the README - Add comprehensive tests covering config and client setup Environment variables for mTLS: - JIRA_AUTH_TYPE=mtls - JIRA_TLS_CLIENT_CERT=/path/to/client.pem - JIRA_TLS_CLIENT_KEY=/path/to/client.key - JIRA_TLS_CA_CERT=/path/to/ca.pem (optional)
- Remove schema default authType=bearer to preserve basic-auth inference
for legacy stored configs that have only {server, username, token}.
- Make isConfigured() respect the selected authType so an mTLS config
with a stale token no longer reports as configured while
getRequiredConfig() throws on the missing cert/key.
- Fail fast in JiraClient constructor when authType is explicitly basic
but no username is provided, instead of silently falling back to
Bearer auth. Legacy configs without an authType keep the previous
inference behavior.
- Add tests covering: legacy basic-auth inference, mTLS with stale
token, explicit basic without username at both Config and JiraClient
layers, and the explicit bearer branch of isConfigured().
Address review feedback on PR pchuri#27: - When JIRA_AUTH_TYPE=mtls is set via env, treat it as authoritative so a stale JIRA_API_TOKEN can no longer make isConfigured() return true while required cert/key env vars are missing. - Add a hasNonEmpty() helper and use it for username, token, tlsClientCert, and tlsClientKey in isConfigured() and the stored-config branches of getRequiredConfig() so that empty or whitespace-only stored values are rejected instead of silently passing validation. - Extend tests/config.test.js with coverage for env-mTLS precedence, partial env-mTLS, env overriding complete stored basic auth, and empty/whitespace-only credentials across basic, bearer, mTLS, and legacy configurations.
…flow - Expand ~ / ~/ prefixes in mTLS certificate paths (client cert, client key, CA cert) so the documented examples work without the user having to substitute their home directory. Node's fs APIs do not perform tilde expansion themselves, which previously caused ENOENT during the config validation step. - Include cloudId in the explicit basic-auth branch of getRequiredConfig so scoped tokens still route through the Atlassian Platform API Gateway when authType=basic. - Hoist the fs require in bin/commands/config.js to the top of the file to match the rest of the codebase.
71d3c61 to
dab1923
Compare
|
Hi @anandghegde — I hope it's okay that I pushed directly to this branch. Since the PR had been sitting on conflicts after #29 (cloudId / Atlassian Platform API Gateway support) merged, I rebased it onto current What I did
Validation
If anything in the rebase looks off or the fixup commit oversteps, happy to back any of it out — otherwise I'll plan to merge once you (or another reviewer) give it a look. |
- Add presence checks for --auth-type and --tls-client-cert/key/ca-cert in the config command options test (parity with --cloud-id). - Add mTLS flag-handling tests: a happy path that exercises authType + cert/key/ca storage, plus error paths for invalid --auth-type values and for missing certificate/key files (verifies that config.set is not called when validation fails). - Add a regression test that scoped tokens (cloudId) survive the explicit basic-auth branch of getRequiredConfig, guarding the fix in dab1923.
# [2.6.0](v2.5.0...v2.6.0) (2026-04-28) ### Features * add mTLS authentication support ([#27](#27)) ([e62a8ca](e62a8ca))
|
🎉 This PR is included in version 2.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Merged and released as v2.6.0 — now on npm as @pchuri/jira-cli@2.6.0. Thanks for the contribution, @anandghegde — mTLS support is a great addition for the self-hosted / zero-trust crowd. |
|
@pchuri sorry i couldnt attend to the suggestions on time. Thanks for the fixes! |
Summary
This adds support for JIRA deployments that authenticate at the TLS layer with client certificates instead of Basic or Bearer credentials. This is common for self-hosted Jira Data Center / Server deployments in enterprise zero-trust environments.
Follows the same pattern as pchuri/confluence-cli#102.
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 modeEnvironment variables for mTLS
CLI usage
Validation
npm test— 189/189 tests pass (15 new tests for mTLS)npm run lint— clean