-
Notifications
You must be signed in to change notification settings - Fork 78
Adjust Tenable integration by requiring auth and validating host URLs #8510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
53cfe2d
f2d6a86
6dd3ada
a1322ae
e68e3e5
afcda97
c452f75
6c40c83
13ca3c3
1569459
3d3c39b
7d4eebf
c685679
0e0ab24
694ab1d
71bbddd
d0fbc3b
77c178f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll also probably need to update the heimdall helm chart
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll also probably need to update the tenable integration part of the integrations wiki page |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,12 @@ | |
| Body, | ||
| HttpException, | ||
| HttpStatus, | ||
| All | ||
| All, | ||
| UseGuards | ||
| } from '@nestjs/common'; | ||
| import {TenableService} from './tenable.service'; | ||
| import {JwtAuthGuard} from '../guards/jwt-auth.guard'; | ||
| import {ConfigService} from '../config/config.service'; | ||
| import axios from 'axios'; | ||
| import {Request, Response} from 'express'; | ||
|
|
||
|
|
@@ -30,8 +33,43 @@ | |
| // It allows users to log in with their Tenable credentials and then proxies all subsequent requests | ||
| // to the Tenable API, handling authentication via session storage. | ||
| @Controller('api/tenable') | ||
| @UseGuards(JwtAuthGuard) | ||
| export class TenableController { | ||
| constructor(private readonly tenableService: TenableService) {} | ||
| constructor( | ||
| private readonly tenableService: TenableService, | ||
| private readonly configService: ConfigService | ||
| ) {} | ||
|
|
||
| // Resolves host_url to its allowlisted origin, or null if not allowed | ||
| private resolveAllowedHostUrl(host_url: string): string | null { | ||
| const allowlist = this.configService.getTenableHostUrl(); | ||
| if (allowlist.length === 0) { | ||
| return null; | ||
| } | ||
| try { | ||
| const parsed = new URL(host_url); // requires a protocol; throws otherwise | ||
|
|
||
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| return null; | ||
| } | ||
|
|
||
| // Reject extra path/query/fragment | ||
| const hasExtra = | ||
| (parsed.pathname !== '' && parsed.pathname !== '/') || | ||
| parsed.search !== '' || | ||
| parsed.hash !== ''; | ||
| if (hasExtra) { | ||
| return null; | ||
| } | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
|
|
||
| const match = allowlist.includes(parsed.origin); | ||
|
DMedina6 marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| // Only the parsed origin (never the raw input) is used downstream | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| return match ? parsed.origin : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| @Post('login') | ||
| /** | ||
|
|
@@ -42,7 +80,7 @@ | |
| * @returns An object indicating success and the authenticated user's data from Tenable. | ||
| * @throws {HttpException} If any credentials are missing or if authentication fails. | ||
| */ | ||
| async login( | ||
|
Check failure on line 83 in apps/backend/src/tenable/tenable.controller.ts
|
||
| @Req() req: Request, | ||
| @Body() body: {host_url: string; accesskey: string; secretkey: string} | ||
| ) { | ||
|
|
@@ -52,17 +90,30 @@ | |
| throw new HttpException('Missing credentials', HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| const allowedHostUrl = this.resolveAllowedHostUrl(host_url); | ||
| if (!allowedHostUrl) { | ||
| // 400, not 403: the frontend treats any 403 here as a credentials error. | ||
| throw new HttpException( | ||
|
Amndeep7 marked this conversation as resolved.
|
||
| { | ||
| status: HttpStatus.BAD_REQUEST, | ||
| message: 'Tenable host URL is not in the configured allowlist', | ||
| code: 'HOST_NOT_ALLOWED' | ||
| }, | ||
| HttpStatus.BAD_REQUEST | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| // This helps prevent double slashes in the resulting URL if host_url ends with a slash. | ||
| const fullUrl = `${host_url.replace(/\/$/, '')}/rest/currentUser`; | ||
| // allowedHostUrl is the parsed origin, so no trailing slash to strip. | ||
| const fullUrl = `${allowedHostUrl}/rest/currentUser`; | ||
| const result = await axios.get(fullUrl, { | ||
| headers: { | ||
| 'x-apikey': `accesskey=${accesskey}; secretkey=${secretkey}` | ||
| } | ||
| }); | ||
|
|
||
| // 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}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI review comment that seems like it could be feasible so please review it:
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. |
||
|
|
||
| // Return the authenticated user data | ||
| // Note: result.data is already a plain object, no need to convert it. | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll also need to update the envvar wiki page