Adjust Tenable integration by requiring auth and validating host URLs - #8510
Adjust Tenable integration by requiring auth and validating host URLs#8510DMedina6 wants to merge 17 commits into
Conversation
…_url against an allowlist
There was a problem hiding this comment.
Pull request overview
This PR tightens the backend Tenable integration by requiring JWT authentication on Tenable endpoints and introducing a configurable allowlist to validate client-supplied Tenable host URLs before performing login/proxy operations.
Changes:
- Added
JwtAuthGuardto Tenable controller endpoints. - Introduced
TENABLE_HOST_URLS(comma-separated) allowlist and host-origin validation for Tenable login requests. - Exposed
getTenableHostUrls()through config layers and documented the new env var in.env-example.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/backend/src/tenable/tenable.module.ts | Wires configuration access into the Tenable feature module. |
| apps/backend/src/tenable/tenable.controller.ts | Adds JWT auth guard and validates host_url against an allowlist before login. |
| apps/backend/src/config/config.service.ts | Exposes a getTenableHostUrls() accessor for controllers/services. |
| apps/backend/config/app_config.ts | Adds parsing for the TENABLE_HOST_URLS env var. |
| apps/backend/.env-example | Documents the new TENABLE_HOST_URLS configuration. |
Suppressed comments (1)
apps/backend/src/tenable/tenable.controller.ts:99
- The allowlist check validates only the origin, but the code continues to use the raw
host_url(which may include a path/query/fragment) to buildfullUrland to storecreds.host_urlin the session. This can cause proxying to an unintended base path and diverges from what was actually allowlisted. Normalize to the parsed origin for both the login request and the stored session host.
const fullUrl = `${host_url.replace(/\/$/, '')}/rest/currentUser`;
const result = await axios.get(fullUrl, {
headers: {
'x-apikey': `accesskey=${accesskey}; secretkey=${secretkey}`
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…lists and single url, new line delimiter
…ble-url-security
There was a problem hiding this comment.
we'll also need to update the envvar wiki page
There was a problem hiding this comment.
we'll also probably need to update the heimdall helm chart
| return null; | ||
| } | ||
|
|
||
| const match = allowlist.includes(parsed.origin); |
There was a problem hiding this comment.
nominally this will fail if they are using a nonstandard port for the protocol which is allowed, i.e. we add 443 as a port by default for https but they could use basically whatever else and it would be valid but would crash here cause "For URLs using the ftp:, http:, https:, ws:, and wss: schemes, the protocol followed by //, followed by the host. Same as host, the port is only included if it's not the default for the protocol." - https://developer.mozilla.org/en-US/docs/Web/API/URL/origin
i'm pretty sure the envvar says to not include a port on it
There was a problem hiding this comment.
Ports / non-default ports can now be specified for both the environment variable / app-config, and the frontend / client side input so it's consistent and flexible
There was a problem hiding this comment.
huh i thought that the csp directives didn't allow ports but i guess they do:
https://github.com/mitre/heimdall2/blob/master/apps/backend/src/main.ts#L57
"The scheme, port number, and path are optional."
so guess this change is fine
There was a problem hiding this comment.
we'll also probably need to update the tenable integration part of the integrations wiki page
| // Assign the Tenable credentials to the session | ||
| req.session.tenable = {host_url, accesskey, secretkey}; | ||
| // Store the normalized, allowlisted origin rather than the raw client value. | ||
| req.session.tenable = {host_url: allowedHostUrl, accesskey, secretkey}; |
There was a problem hiding this comment.
AI review comment that seems like it could be feasible so please review it:
JwtAuthGuard proves only that the caller has a Heimdall account; these credentials remain associated solely with the browser’s Express session. The Tenable “sign out” UI only clears client state, and /users/logout revokes the JWT without clearing session.tenable. On a shared/reused browser session, a subsequently authenticated user can call the proxy with the prior user’s Tenable keys. Store the authenticated Heimdall user ID with the credentials, verify it on every proxy request, and clear the credentials on both Tenable and Heimdall logout. Please add an A-login → logout → B-login → proxy-is-denied regression test.
You don't need to write an explicit test unless you want to, but please do validate by hand that logging in with a second account does not allow one to reuse the access/secret key.
…all2 into tenable-url-security
…ble-url-security
| } | ||
| // Only protocol + hostname + port are allowed; ports are permitted since | ||
| // Tenable.SC may run on a non-default port. | ||
| const hasExtra = |
There was a problem hiding this comment.
both of the hasExtra processing areas need to exclude everything aside from hostname + protocol + optionally port
considering that this processing code is the same, maybe we can extract out to some common utility function and use it in both places.
|
Add JwtAuthGuard to TenableController (login + proxy endpoints)
Add TENABLE_HOST_URLS config option (comma-separated allowlist)
Reject login requests whose host_url origin isn't in the allowlist
Restricts host_url to http/https, trims whitespace before validation, rejects (instead of silently stripping) any extra path/query/fragment