-
Notifications
You must be signed in to change notification settings - Fork 0
feat(auth): allow non-admin user role to use HTTP sink/IO nodes #634
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 all commits
55ed4f1
00c19eb
b43d792
72ca68d
6e5d357
fd26e79
8455c91
4f8ebd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,35 +471,101 @@ delete_plugins = false | |
| list_nodes = true | ||
| access_all_sessions = false | ||
|
|
||
| # Users can access all samples except admin-only ones | ||
| # Sample management (mirrors the built-in Permissions::user()); without these | ||
| # flags the allowed_samples list below would be unreachable. | ||
| list_samples = true | ||
| read_samples = true | ||
| write_samples = true | ||
| delete_samples = true | ||
|
|
||
| # Users can access standard samples (mirrors the built-in Permissions::user()). | ||
| allowed_samples = [ | ||
| "oneshot/*.yml", | ||
| "oneshot/*.yaml", | ||
| "dynamic/*.yml", | ||
| "dynamic/*.yaml", | ||
| "demo/*.yml", | ||
| "demo/*.yaml", | ||
| "user/*.yml", | ||
| "user/*.yaml", | ||
| ] | ||
|
Comment on lines
488
to
489
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. 📝 Info: Sample user role allowed_samples diverges slightly from code default The (Refers to lines 475-484) Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Fixed: removed |
||
|
|
||
| # Users can use most nodes except potentially dangerous ones | ||
| # Users can use most nodes except potentially dangerous ones. Mirrors the | ||
| # built-in Permissions::user() default. Allowed: MoQ + the HTTP `mse` sink | ||
| # (serves over the caller's own request). Denied by omission: | ||
| # transport::http::fetcher (SSRF), core::file_writer / core::object_store_writer | ||
| # (arbitrary/external write). The oneshot streamkit::http_input/http_output | ||
| # markers are implicitly allowed on the oneshot path and need no entry here. | ||
| allowed_nodes = [ | ||
| "audio::*", | ||
| "transport::*", | ||
| "core::*", | ||
| "video::*", | ||
| "containers::*", | ||
| "transport::moq::*", | ||
|
staging-devin-ai-integration[bot] marked this conversation as resolved.
|
||
| "transport::http::mse", | ||
| "core::passthrough", | ||
| "core::file_reader", | ||
| "core::pacer", | ||
| "core::param_bridge", | ||
| "core::json_serialize", | ||
| "core::text_chunker", | ||
| "core::script", | ||
| "core::telemetry_tap", | ||
| "core::telemetry_out", | ||
| "core::sink", | ||
| "plugin::*", | ||
| ] | ||
|
Comment on lines
487
to
514
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. 📝 Info: User role node allowlist tightened from broad wildcards to explicit list The sample Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Good catch on the breakage surface. I checked the shipped pipelines: That's intentional and not a regression relative to the secure default: the built-in |
||
|
|
||
| # Users cannot load plugins, so this list is empty | ||
| allowed_plugins = [] | ||
| # Users cannot load/delete plugins, but may use plugins an admin has already loaded. | ||
| allowed_plugins = ["plugin::*"] | ||
|
Comment on lines
515
to
+517
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. 📝 Info: User role permissions widened to include loaded plugins The built-in Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Intentional — this aligns the sample with the built-in |
||
|
|
||
| # Users can list bundled system assets and their uploaded assets | ||
| # (mirrors the built-in Permissions::user(): audio, images and fonts). | ||
| allowed_assets = [ | ||
| "samples/audio/system/*", | ||
| "samples/audio/user/*", | ||
| "samples/images/system/*", | ||
| "samples/images/user/*", | ||
| "samples/fonts/system/*", | ||
| "samples/fonts/user/*", | ||
| ] | ||
|
|
||
| [permissions.roles.gateway] | ||
| # Least-privilege role for trusted intermediaries that build a small set of | ||
| # fixed pipelines, so they need not run as admin. The node/plugin lists below | ||
| # are scoped to the web-capture example; a different gateway (e.g. speech-gateway) | ||
| # would swap in its own nodes and plugins (audio nodes, whisper/kokoro, etc.). | ||
| create_sessions = true | ||
| destroy_sessions = true | ||
| list_sessions = true | ||
| modify_sessions = true | ||
| tune_nodes = true | ||
| load_plugins = false | ||
| delete_plugins = false | ||
| list_nodes = true | ||
| access_all_sessions = false # Only its own sessions | ||
| upload_assets = false | ||
| delete_assets = false | ||
|
|
||
| allowed_samples = [] | ||
|
|
||
| # Exactly the node kinds the web-capture / live-cast pipeline needs, nothing more. | ||
| # A plugin kind must be allowed here too: is_node_allowed() runs before the | ||
| # allowed_plugins check, so a plugin missing from allowed_nodes is rejected first. | ||
| # No fetcher (SSRF) and no file_writer (arbitrary write). The oneshot | ||
| # streamkit::http_output marker is implicitly allowed and needs no entry. | ||
| allowed_nodes = [ | ||
| "plugin::native::servo", # render the page (web-capture) | ||
| "video::pixel_convert", # servo RGBA -> encoder input format | ||
| "video::vp9::encoder", # encode to VP9 | ||
| "containers::webm::muxer", # mux into WebM for MSE / http_output | ||
| "transport::http::mse", # serve the live cast to the browser (MSE) | ||
| "core::pacer", | ||
| "core::sink", | ||
| ] | ||
|
Comment on lines
+554
to
+562
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. 📝 Info: Gateway role's The new Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Fixed: the gateway example (sample config + docs) now grants only the safe core plumbing nodes ( |
||
|
|
||
| # Only the specific plugin(s) the gateway needs (example: servo for web-capture). | ||
| allowed_plugins = ["plugin::native::servo"] | ||
|
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. 🚩 Gateway example references plugin::native::servo Both the docs ( Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Confirmed valid —
Comment on lines
+554
to
+565
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. 📝 Info: Plugin enforcement ordering relied upon by gateway role The gateway role lists Was this helpful? React with 👍 or 👎 to provide feedback. Debug
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. Right — this ordering dependency ( |
||
|
|
||
| allowed_assets = [] | ||
|
|
||
| [permissions.roles.readonly] | ||
| # Read-only role - can only view, not modify | ||
| create_sessions = false | ||
|
|
||
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.
📝 Info: TOML user role now aligned with code, fixing previously-unreachable sample allowlist
Before this PR the
samples/skit.tomluserrole omittedlist_samples/read_samples/write_samples/delete_samples, which default tofalse. Because a TOML-defined role fully replaces the built-inPermissions::user(), theallowed_sampleslist was effectively unreachable for the sample config's user role. Adding these flags (samples/skit.toml:476-479) corrects that and brings the TOML in line with the code defaults inapps/skit/src/permissions.rs. This is a real fix, not a regression.Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Playground
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.
Correct — this was the fix for the second validation finding. Without
list/read/write/delete_samplestheallowed_sampleslist was dead config in the sample'suserrole. Now the sample matchesPermissions::user()and the list is reachable.