-
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 4 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,66 @@ | |
| // 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 | ||
| ) {} | ||
|
|
||
| // Returns the matching allowlisted origin for host_url, or null if none match | ||
| // or if host_url carries a path/query/fragment beyond a bare origin. | ||
| private resolveAllowedHostUrl(host_url: string): string | null { | ||
| const allowlist = this.configService.getTenableHostUrls(); | ||
| if (allowlist.length === 0) { | ||
| return null; | ||
| } | ||
| // Default to https when no scheme is provided, so "example.com" is treated | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| // the same as "https://example.com". | ||
| const trimmedInput = host_url.trim(); | ||
| const normalizedInput = /^[a-z][a-z\d+.-]*:\/\//i.test(trimmedInput) | ||
| ? trimmedInput | ||
| : `https://${trimmedInput}`; | ||
| try { | ||
| // Throws for unparseable input, caught below and treated as not allowed. | ||
| const parsed = new URL(normalizedInput); | ||
|
|
||
| // Reject any scheme other than http/https (e.g. file:, javascript:, ftp:). | ||
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| return null; | ||
| } | ||
|
|
||
| // Reject anything beyond a bare origin rather than silently stripping it, | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| // so a tampered/malformed host_url fails loudly instead of being sanitized. | ||
| const hasExtra = | ||
| (parsed.pathname !== '' && parsed.pathname !== '/') || | ||
| parsed.search !== '' || | ||
| parsed.hash !== ''; | ||
| if (hasExtra) { | ||
| return null; | ||
| } | ||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Compare origins (scheme + host + port) rather than raw strings so that | ||
| // equivalent URLs (default ports, trailing slash, casing) still match. | ||
| const match = allowlist.find((allowed) => { | ||
|
Check warning on line 77 in apps/backend/src/tenable/tenable.controller.ts
|
||
|
DMedina6 marked this conversation as resolved.
Outdated
|
||
| const trimmedAllowed = allowed.trim(); | ||
| const normalizedAllowed = /^[a-z][a-z\d+.-]*:\/\//i.test(trimmedAllowed) | ||
| ? trimmedAllowed | ||
| : `https://${trimmedAllowed}`; | ||
| try { | ||
| return new URL(normalizedAllowed).origin === parsed.origin; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| // Return the parsed origin (never the raw client value) so nothing beyond | ||
| // scheme+host+port ever propagates into the session or outbound requests. | ||
| return match ? parsed.origin : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| @Post('login') | ||
| /** | ||
|
|
@@ -42,7 +103,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 106 in apps/backend/src/tenable/tenable.controller.ts
|
||
| @Req() req: Request, | ||
| @Body() body: {host_url: string; accesskey: string; secretkey: string} | ||
| ) { | ||
|
|
@@ -52,17 +113,29 @@ | |
| throw new HttpException('Missing credentials', HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| const allowedHostUrl = this.resolveAllowedHostUrl(host_url); | ||
| if (!allowedHostUrl) { | ||
| throw new HttpException( | ||
|
Amndeep7 marked this conversation as resolved.
|
||
| { | ||
| status: HttpStatus.FORBIDDEN, | ||
| message: 'Tenable host URL is not in the configured allowlist', | ||
| code: 'HOST_NOT_ALLOWED' | ||
| }, | ||
| HttpStatus.FORBIDDEN | ||
| ); | ||
| } | ||
|
|
||
| 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