Skip to content
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.

Sign up for GitHub

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

Restrict chromium requests #425

Closed
wants to merge 7 commits into from
Closed

Restrict chromium requests #425

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b2596d
Fix regex validation, detect iframe, embed, object tags
joshuali925 Aug 1, 2022
51890cb
Disallow redirection to non-localhost urls
joshuali925 Aug 2, 2022
949e016
Disallow connection to non-allowlisted urls
joshuali925 Aug 4, 2022
cb317bf
Disable JIT
joshuali925 Aug 5, 2022
4f5309d
Fix workflow
joshuali925 Aug 16, 2022
d76f4fb
Try to fix CI
joshuali925 Aug 17, 2022
6968ccf
Fix localstorage logic
joshuali925 Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions .github/workflows/dashboards-reports-test-and-build-workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ jobs:
with:
repository: opensearch-project/Opensearch-Dashboards
ref: ${{ env.OPENSEARCH_VERSION }}
path: dashboards-reports/OpenSearch-Dashboards
path: OpenSearch-Dashboards

- name: Get node version
id: versions_step
run:
echo "::set-output name=node_version::$(node -p "(require('./OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"
echo "::set-output name=node_version::$(node -p "(require('../OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"

- name: Setup Node
uses: actions/setup-node@v1
Expand All @@ -35,13 +35,13 @@ jobs:


- name: Move Dashboards Reports to Plugins Dir
run: mv dashboards-reports OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
run: mv dashboards-reports ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}

- name: Add Chromium Binary to Reporting for Testing
run: |
sudo apt update
sudo apt install -y libnss3-dev fonts-liberation libfontconfig1
cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
wget https://github.com/opendistro-for-elasticsearch/kibana-reports/releases/download/chromium-1.12.0.0/chromium-linux-x64.zip
unzip chromium-linux-x64.zip
rm chromium-linux-x64.zip
Expand All @@ -51,25 +51,25 @@ jobs:
with:
timeout_minutes: 30
max_attempts: 3
command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap
command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap

- name: Test
uses: nick-invision/retry@v1
with:
timeout_minutes: 30
max_attempts: 3
command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage
command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage

- name: Upload coverage
uses: codecov/codecov-action@v1
with:
flags: dashboards-reports
directory: OpenSearch-Dashboards/plugins/
directory: ../OpenSearch-Dashboards/plugins/
token: ${{ secrets.CODECOV_TOKEN }}

- name: Build Artifact
run: |
cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
yarn build

cd build
Expand Down Expand Up @@ -103,16 +103,16 @@ jobs:
uses: actions/upload-artifact@v1
with:
name: dashboards-reports-linux-x64
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip

- name: Upload Artifact For Linux arm64
uses: actions/upload-artifact@v1
with:
name: dashboards-reports-linux-arm64
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip

- name: Upload Artifact For Windows
uses: actions/upload-artifact@v1
with:
name: dashboards-reports-windows-x64
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip
8 changes: 6 additions & 2 deletions dashboards-reports/server/routes/utils/__tests__/visualReportHelper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ describe('test create visual report', () => {
reportParams as ReportParamsSchemaType,
mockHtmlPath,
mockLogger,
mockHeader
mockHeader,
undefined,
/^(data:image|file:\/\/)/
);
expect(fileName).toContain(`${reportParams.report_name}`);
expect(fileName).toContain('.png');
Expand All @@ -71,7 +73,9 @@ describe('test create visual report', () => {
reportParams as ReportParamsSchemaType,
mockHtmlPath,
mockLogger,
mockHeader
mockHeader,
undefined,
/^(data:image|file:\/\/)/
);
expect(fileName).toContain(`${reportParams.report_name}`);
expect(fileName).toContain('.pdf');
Expand Down
2 changes: 2 additions & 0 deletions dashboards-reports/server/routes/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ const ipv6Regex = /(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:)
const localhostRegex = /localhost:([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])/g;
const iframeRegex = /iframe/g;

export const ALLOWED_HOSTS = /^(0|0.0.0.0|127.0.0.1|localhost|(.*\.)?(opensearch.org|aws.a2z.com))$/;
Copy link
Member

@dblock dblock Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not have an Amazon URL. And how is this used? Why would opensearch.org be in this list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the allowed hosts for all requests and redirections in chromium. The two hosts listed are used by map tiles visualizations

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those 2 are not hosts, rather domains. Perhaps narrow the scope to the specific ones we need like maps.search-services.aws.a2z.com?

Copy link
Member

@dblock dblock Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this needs to be configurable because some users will run in intranets. Is there a GH issue/feature request for this ask? I can open one otherwise.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk @dblock for pointing this one out: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/899/files

It looks like we don't need the a2z one. Could you please confirm, @joshuali925 ? If we can also make the opensearch.org more specific that'd be even better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened opensearch-project/documentation-website#983 to document what's going on with these URLs.

Copy link
Member

@dblock dblock Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: is ALLOWED_HOSTS only used for maps? Maybe split it into ALLOWED_HOSTS and ALLOWED_MAPS_HOST to make it explicit?

Also this needs tests. I don't see anything that checks that ALLOWED_HOSTS is taken into consideration in tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidlago yes, good idea to narrow down to maybe (.+\.)?maps.opensearch.org. Will remove the a2z one if someone is able to confirm in db.'s issue which dashboards versions use which endpoint.

@dblock ALLOWED_HOSTS is used for all requests and redirections happening inside chromium. Reporting plugin cannot tell if the request is coming from a map visualization, so I'm not sure if there are values to split them. Will add tests after the a2z issue is addressed


export const replaceBlockedKeywords = (htmlString: string) => {
// replace <ipv4>:<port>
htmlString = htmlString.replace(ipv4Regex, BLOCKED_KEYWORD);
Expand Down
4 changes: 4 additions & 0 deletions dashboards-reports/server/routes/utils/visual_report/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ body {
padding: 0;
}

iframe, embed, object {
display: none !important;
}

/* nice padding + matches Kibana default UI colors you could also set this to inherit if
the wrapper gets inserted inside a kibana section. I might also remove the manual text color here as well, potentially */
.reportWrapper {
Expand Down
70 changes: 61 additions & 9 deletions dashboards-reports/server/routes/utils/visual_report/visualReportHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
SELECTOR,
CHROMIUM_PATH,
SECURITY_CONSTANTS,
ALLOWED_HOSTS,
} from '../constants';
import { getFileName } from '../helpers';
import { CreateReportResultType } from '../types';
Expand All @@ -27,7 +28,8 @@ export const createVisualReport = async (
queryUrl: string,
logger: Logger,
extraHeaders: Headers,
timezone?: string
timezone?: string,
validRequestProtocol = /^(data:image)/,
): Promise<CreateReportResultType> => {
const {
core_params,
Expand Down Expand Up @@ -76,6 +78,8 @@ export const createVisualReport = async (
'--no-zygote',
'--single-process',
'--font-render-hinting=none',
'--js-flags="--jitless --no-opt"',
'--disable-features=V8OptimizeJavascript',
],
executablePath: CHROMIUM_PATH,
ignoreHTTPSErrors: true,
Expand All @@ -84,6 +88,32 @@ export const createVisualReport = async (
},
});
const page = await browser.newPage();

await page.setRequestInterception(true);
let localStorageAvailable = true;
page.on('request', (req) => {
// disallow non-allowlisted connections. urls with valid protocols do not need ALLOWED_HOSTS check
if (
!validRequestProtocol.test(req.url()) &&
!ALLOWED_HOSTS.test(new URL(req.url()).hostname)
) {
if (req.isNavigationRequest() && req.redirectChain().length > 0) {
logger.error(
'Reporting does not allow redirections to outside of localhost, aborting. URL received: ' +
req.url()
);
} else {
logger.warn(
'Disabled connection to non-allowlist domains: ' + req.url()
);
}
localStorageAvailable = true;
req.abort();
} else {
req.continue();
}
});

page.setDefaultNavigationTimeout(0);
page.setDefaultTimeout(100000); // use 100s timeout instead of default 30s
// Set extra headers that are needed
Expand All @@ -93,13 +123,25 @@ export const createVisualReport = async (
logger.info(`original queryUrl ${queryUrl}`);
await page.goto(queryUrl, { waitUntil: 'networkidle0' });
// should add to local storage after page.goto, then access the page again - browser must have an url to register local storage item on it
await page.evaluate(
/* istanbul ignore next */
(key) => {
localStorage.setItem(key, 'false');
},
SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
);
try {
await page.evaluate(
/* istanbul ignore next */
(key) => {
try {
if (
localStorageAvailable &&
typeof localStorage !== 'undefined' &&
localStorage !== null
) {
localStorage.setItem(key, 'false');
}
} catch (err) {}
},
SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
);
} catch (err) {
logger.error(err);
}
await page.goto(queryUrl, { waitUntil: 'networkidle0' });
logger.info(`page url ${page.url()}`);

Expand Down Expand Up @@ -162,9 +204,19 @@ export const createVisualReport = async (
// wait for dynamic page content to render
await waitForDynamicContent(page);

await addReportStyle(page);
await addReportHeader(page, keywordFilteredHeader);
await addReportFooter(page, keywordFilteredFooter);
await addReportStyle(page);

const numDisallowedTags = await page.evaluate(
() =>
document.getElementsByTagName('iframe').length +
document.getElementsByTagName('embed').length +
document.getElementsByTagName('object').length
);
if (numDisallowedTags > 0) {
throw Error('Reporting does not support "iframe", "embed", or "object" tags, aborting');
}

// create pdf or png accordingly
if (reportFormat === FORMAT.pdf) {
Expand Down
36 changes: 35 additions & 1 deletion dashboards-reports/server/utils/__tests__/validationHelper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
REPORT_TYPE,
TRIGGER_TYPE,
} from '../../routes/utils/constants';
import { validateReport, validateReportDefinition } from '../validationHelper';
import { isValidRelativeUrl, validateReport, validateReportDefinition } from '../validationHelper';

const SAMPLE_SAVED_OBJECT_ID = '3ba638e0-b894-11e8-a6d9-e546fe2bba5f';
const createReportDefinitionInput: ReportDefinitionSchemaType = {
Expand Down Expand Up @@ -152,7 +152,41 @@ describe('test input validation', () => {
`saved object with id dashboard:${SAMPLE_SAVED_OBJECT_ID} does not exist`
);
});

test('validation against query_url', async () => {
const urls: [string, boolean][] = [
['/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=', true],
[
'/_plugin/kibana/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
true,
],
[
'/_dashboards/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
true,
],
[
'/_dashboards/app/dashboards#/edit/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
true,
],
[
'/app/observability-dashboards?security_tenant=private#/notebooks/NYdlPIIB0-fJ8Bh1nLdW?view=output_only',
true,
],
[
'/app/notebooks-dashboards?view=output_only&security_tenant=private#/M4dlPIIB0-fJ8Bh1nLc7?security_tenant=private',
true,
],
[
'/_dashboards/app/visualize&security_tenant=/.%2e/.%2e/.%2e/.%2e/_dashboards?#/view/id',
false,
],
];
expect(urls.map((url) => isValidRelativeUrl(url[0]))).toEqual(
urls.map((url) => url[1])
);
});
});

// TODO: merge this with other mock clients used in testing, to create some mock helpers file
const mockOpenSearchClient = (mockSavedObjectIds: string[]) => {
const client = {
Expand Down
2 changes: 1 addition & 1 deletion dashboards-reports/server/utils/validationHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const isValidRelativeUrl = (relativeUrl: string) => {
export const regexDuration = /^(-?)P(?=\d|T\d)(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)([DW]))?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+(?:\.\d+)?)S)?)?$/;
export const regexEmailAddress = /\S+@\S+\.\S+/;
export const regexReportName = /^[\w\-\s\(\)\[\]\,\_\-+]+$/;
export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only)([?&]security_tenant=.+|)#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;
export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only(&security_tenant=.+)?)(\?security_tenant=.+)?#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;

export const validateReport = async (
client: ILegacyScopedClusterClient,
Expand Down