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.

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

[cloud] Provide ability to disable executing modified pxl scripts #2062

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ddelnano
Copy link
Member

@ddelnano ddelnano commented Dec 17, 2024

Summary: [cloud] Provide ability to disable executing modified pxl scripts

Certain security conscious users are hesitant to use Pixie because without RBAC anyone with Pixie UI access can write arbitrary BPF code (bpftrace integration), access or export arbitrary data (modifying pxl scripts, writing export scripts). This change aims to address this concern with a global setting to prevent the ability to execute modified scripts. When an adhoc script is executed, the cloud will hash the contents of the script and check it against the scripts known to the scriptmgr service. If it is contained in the scriptmgr service, the script will be allowed to execute.

Note: this does not prevent users from writing new export scripts. Since the query broker can source its scripts from a configmap as of #1326, this is deemed as an appropriate mitigation for cluster admins and I'll follow up with UI support to reflect that a vizier is in "configmap mode".

Relevant Issues: N/A

Type of change: /kind feature

Test Plan: The following checks were performed

  • New tests verify the scriptmgr and api service changes work
  • Skaffold'ed to a testing cluster and verified script modification is blocked and unmodified scripts are allowed to run. In addition to this, the code editor in the UI is made read only and shows an explanation
Screenshots

Screen Shot 2025-01-07 at 8 58 34 AM
Screen Shot 2025-01-07 at 8 58 37 AM
Screen Shot 2025-01-07 at 8 58 59 AM

Changelog Message: Pixie Cloud can now disable executing modified pxl scripts via the PL_SCRIPT_MODIFICATION_DISABLED key in the pl-script-bundle-config ConfigMap. See reference manifests for more details.

@ddelnano ddelnano requested review from a team as code owners December 17, 2024 16:09
@ddelnano ddelnano force-pushed the ddelnano/enforce-scripts-arent-modified branch from cda686a to af0447c Compare December 17, 2024 16:17
Signed-off-by: Dom Del Nano <[email protected]>
@ddelnano ddelnano force-pushed the ddelnano/enforce-scripts-arent-modified branch from 094bbc7 to 4270147 Compare December 17, 2024 16:57
Signed-off-by: Dom Del Nano <[email protected]>
Copy link
Member

@aimichelle aimichelle left a comment

Choose a reason for hiding this comment

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

For a slightly better experience, should we also update the UI so that the editor doesn't allow the user to make any modifications? That way they don't have to wait to edit the script, then try to run the script, in order to know it's not going to work.

src/cloud/api/ptproxy/vizier_pt_proxy.go Outdated Show resolved Hide resolved
src/cloud/scriptmgr/scriptmgrpb/service.proto Outdated Show resolved Hide resolved
@ddelnano ddelnano requested a review from a team as a code owner December 23, 2024 18:39
@ddelnano ddelnano force-pushed the ddelnano/enforce-scripts-arent-modified branch 2 times, most recently from 0a31fa1 to 13ffb77 Compare December 23, 2024 19:20
…ing and renamed to match other SCRIPT_ prefixed settings

Signed-off-by: Dom Del Nano <[email protected]>
@ddelnano ddelnano force-pushed the ddelnano/enforce-scripts-arent-modified branch from 13ffb77 to 1c57303 Compare December 23, 2024 19:50
Copy link
Member

@NickLanam NickLanam left a comment

Choose a reason for hiding this comment

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

UI adjustments LGTM, with the comment below.

@@ -85,7 +86,7 @@ export class CodeEditor extends React.PureComponent<CodeEditorProps, CodeEditorS
scrollBeyondLastColumn: 3, // Prevents hiding text behind the minimap or the scrollbar. Expands the scroll area.
scrollBeyondLastLine: false,
fontFamily: COMMON_THEME.typography.monospace.fontFamily,
readOnly: this.props.isReadOnly === true,
readOnly: this.props.isReadOnly === true || SCRIPT_MODIFICATION_DISABLED,
Copy link
Member

Choose a reason for hiding this comment

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

Notably, CodeEditor doesn't show the user why it's read-only, and assumes the caller will tell them why (I don't remember if any existing caller does so, though - they might not).

Having readOnly set directly inside of CodeEditor this way breaks that assumption further, showing nothing and giving no indication that it should. Might be worth consideration in a future change!

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this in favor of having each instance of the component to provide a read only reason. See testing done screenshots for more details.

@@ -169,7 +171,8 @@ data:
env PL_HYDRA_SERVICE;
env PL_KRATOS_SERVICE;
env SCRIPT_BUNDLE_URLS;
env SCRIPT_BUNDE_DEV;
env SCRIPT_BUNDLE_DEV;
Copy link
Member Author

@ddelnano ddelnano Jan 7, 2025

Choose a reason for hiding this comment

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

This is a typo but something that's only used for development.

@ddelnano ddelnano force-pushed the ddelnano/enforce-scripts-arent-modified branch from 49e06d1 to b0098ca Compare January 7, 2025 17:03
@ddelnano
Copy link
Member Author

ddelnano commented Jan 7, 2025

@aimichelle please see the latest testing done screenshots. The UI now enables the read only setting for the CodeEditor component and shows a tool tip explaining why it's read only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants