-
Notifications
You must be signed in to change notification settings - Fork 140
Add Admin Roles #1111
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
Open
fitzthum
wants to merge
5
commits into
confidential-containers:main
Choose a base branch
from
fitzthum:admin-3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add Admin Roles #1111
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8838c2e
admin: return admin role from admin backend
fitzthum 6a1036c
admin: add admin roles
fitzthum 2eea909
admin: add unit tests for admin roles
fitzthum c1a91a7
tests: fixup integration test for admin roles
fitzthum 2fb2e3d
docs: add documentation about admin roles
fitzthum File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ | |
|
|
||
| use actix_web::HttpRequest; | ||
| use log::{info, warn}; | ||
| use regex::Regex; | ||
| use serde::Deserialize; | ||
| use std::collections::HashMap; | ||
| use std::sync::Arc; | ||
|
|
||
| pub mod allow_all; | ||
|
|
@@ -21,9 +23,9 @@ use simple::{SimpleAdminBackend, SimpleAdminConfig}; | |
| #[derive(Clone)] | ||
| pub(crate) struct Admin { | ||
| backend: Arc<dyn AdminBackend>, | ||
| roles: HashMap<String, Regex>, | ||
| } | ||
|
|
||
| // create a simple backend | ||
| #[derive(Clone, Debug, Deserialize, PartialEq)] | ||
| #[serde(tag = "type")] | ||
| pub enum AdminBackendType { | ||
|
|
@@ -45,6 +47,25 @@ impl Default for AdminBackendType { | |
| pub struct AdminConfig { | ||
| #[serde(flatten)] | ||
| pub admin_backend: AdminBackendType, | ||
| /// Admin roles control which admin personas can access | ||
| /// which endpoints. | ||
| /// | ||
| /// If no admin roles are specified, all admin will be able | ||
| /// to access all endpoints. | ||
| pub roles: Vec<AdminRole>, | ||
| } | ||
|
|
||
| /// An admin role is a rule that grants access for some roles to some endpoints. | ||
| #[derive(Clone, Debug, Default, Deserialize, PartialEq)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct AdminRole { | ||
| /// The admin role that this rule applies to. | ||
| /// The id is case insensitive. | ||
| pub id: String, | ||
| /// A regular expression selecting request paths this rule allows. | ||
| /// In other words, the paths that the above role can access. | ||
| #[serde(default)] | ||
|
Member
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. That means by default no endpoints can be accessed?
Member
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. Yes. That seems less error-prone. |
||
| pub allowed_endpoints: String, | ||
| } | ||
|
|
||
| impl TryFrom<AdminConfig> for Admin { | ||
|
|
@@ -59,23 +80,47 @@ impl TryFrom<AdminConfig> for Admin { | |
| AdminBackendType::DenyAll => Arc::new(DenyAllBackend::default()) as _, | ||
| }; | ||
|
|
||
| Ok(Admin { backend }) | ||
| // Parse roles to ensure valid regexes and no duplicates. | ||
| let mut roles = HashMap::new(); | ||
| for role in value.roles { | ||
| let re = Regex::new(&role.allowed_endpoints)?; | ||
|
|
||
| if roles.insert(role.id.to_lowercase(), re).is_some() { | ||
| return Err(Error::DuplicateAdminRole); | ||
| } | ||
| } | ||
|
|
||
| Ok(Admin { backend, roles }) | ||
| } | ||
| } | ||
|
|
||
| impl Admin { | ||
| pub fn validate_admin_token(&self, request: &HttpRequest) -> Result<()> { | ||
| let res = self.backend.validate_admin_token(request); | ||
| match res { | ||
| Ok(()) => info!("Allowing Admin access for {}", request.full_url().as_str()), | ||
| Err(ref e) => info!( | ||
| "Not allowing Admin access for {} due to: \n{}", | ||
| request.full_url().as_str(), | ||
| e | ||
| ), | ||
| pub fn check_admin_access(&self, request: &HttpRequest) -> Result<()> { | ||
| if let Ok(role) = self.backend.validate_admin_token(request) { | ||
| info!("Admin Role: {role}"); | ||
|
|
||
| // If there are no roles specified, allow all. | ||
| if self.roles.is_empty() { | ||
| info!( | ||
| "No admin roles configured. Allowing Request to {}", | ||
| request.full_url().as_str() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(re) = self.roles.get(&role.to_lowercase()) { | ||
| if re.is_match(&request.uri().to_string()) { | ||
| info!("Allowing Request to {}", request.full_url().as_str()); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| res | ||
| info!( | ||
| "Not allowing Admin access to {}", | ||
| request.full_url().as_str() | ||
| ); | ||
| Err(Error::AdminAccessDenied) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -85,5 +130,112 @@ pub(crate) trait AdminBackend: Send + Sync { | |
| /// When a request is made to an admin endpoint, this method should be called | ||
| /// to validate that the user making the request is authorized | ||
| /// to access admin functionality. | ||
| fn validate_admin_token(&self, request: &HttpRequest) -> Result<()>; | ||
| /// | ||
| /// If the token is valid, the backend will return an admin role. | ||
| fn validate_admin_token(&self, request: &HttpRequest) -> Result<String>; | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
|
|
||
| use super::*; | ||
| use serde_json::json; | ||
|
|
||
| #[test] | ||
| pub fn make_admin_object() { | ||
| // basic (backwards compatible) | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| }); | ||
|
|
||
| let config: AdminConfig = serde_json::from_value(admin_config_json).unwrap(); | ||
| let _admin = Admin::try_from(config).unwrap(); | ||
|
|
||
| // with invalid role (wrong field name) | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| "roles" : [{ | ||
| "id": "Anonymous", | ||
| "allowed_paths": "xyz" | ||
| }] | ||
| }); | ||
|
|
||
| let config = serde_json::from_value::<AdminConfig>(admin_config_json); | ||
| assert!(config.is_err()); | ||
|
|
||
| // with invalid role (bad regex) | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| "roles" : [{ | ||
| "id": "Anonymous", | ||
| "allowed_endpoints": "(xyz" | ||
| }] | ||
| }); | ||
|
|
||
| let config: AdminConfig = serde_json::from_value(admin_config_json).unwrap(); | ||
| let admin = Admin::try_from(config); | ||
| assert!(admin.is_err()); | ||
|
|
||
| // with invalid role (duplicate role) | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| "roles" : [{ | ||
| "id": "Anonymous", | ||
| "allowed_endpoints": "xyz" | ||
| }, | ||
| { | ||
| "id": "Anonymous", | ||
| "allowed_endpoints": "abc" | ||
| }] | ||
| }); | ||
|
|
||
| let config: AdminConfig = serde_json::from_value(admin_config_json).unwrap(); | ||
| let admin = Admin::try_from(config); | ||
| assert!(admin.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| pub fn check_requests() { | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| "roles" : [{ | ||
| "id": "Anonymous", | ||
| "allowed_endpoints": "^/resource/a/.+/c$" | ||
| }] | ||
| }); | ||
|
|
||
| let config: AdminConfig = serde_json::from_value(admin_config_json).unwrap(); | ||
| let admin = Admin::try_from(config).unwrap(); | ||
|
|
||
| // valid request | ||
| let req = actix_web::test::TestRequest::post() | ||
| .uri("/resource/a/b/c") | ||
| .to_http_request(); | ||
| admin.check_admin_access(&req).unwrap(); | ||
|
|
||
| // invalid request | ||
| let req = actix_web::test::TestRequest::post() | ||
| .uri("/resource/b/b/c") | ||
| .to_http_request(); | ||
| assert!(admin.check_admin_access(&req).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| pub fn check_requests_wrong_role() { | ||
| let admin_config_json = json!({ | ||
| "type": "InsecureAllowAll", | ||
| "roles" : [{ | ||
| "id": "Steve", | ||
| "allowed_endpoints": "^/resource/a/.+/c$" | ||
| }] | ||
| }); | ||
|
|
||
| let config: AdminConfig = serde_json::from_value(admin_config_json).unwrap(); | ||
| let admin = Admin::try_from(config).unwrap(); | ||
|
|
||
| let req = actix_web::test::TestRequest::post() | ||
| .uri("/resource/a/b/c") | ||
| .to_http_request(); | ||
| assert!(admin.check_admin_access(&req).is_err()); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about
We can by default give a allow all/deny all policy.
In this way we can have a simple authZ system that is code-policy-decoupled.
The policy can includes all the "role" names and its acceptable paths (e.g. in regex)
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.
I think that's too complicated. Users are already confused by the two policies we have. Let's only add another one if we absolutely have to. What is implemented here is basically a policy mechanism, but it's made as simple as possible. Let's not do anything more complex unless users explicitly ask for it.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yup, what we are aiming for right now in the admin module is a simple authorization (AuthZ) mechanism, i.e., defining which APIs can be accessed.
The solution proposed in this PR enables a quick way to define roles and their accessible APIs, and it is ready for deployment. This approach is very useful for some internal or short-term projects where simplicity and fast iteration are the primary goals.
However, from a longer-term evolution perspective—especially if we expect more frequent policy changes or additional authorization scenarios - there are a few concerns worth discussing:
Currently, the rules are defined in the KBS configuration at startup time and are not independently versioned or reloadable. As a result, updating or modifying authorization rules requires restarting the entire KBS, which may become inconvenient as policies evolve.
2.Mixed abstraction between token verification and authorization decision
The current validate_admin_token abstraction mixes two different concerns:
In retrospect, this separation was not clearly enforced in earlier designs (including my own previous review in https://github.com/confidential-containers/trustee/pull/1014/changes#diff-57ec221f4d45eb2d52007f10e72821894545bdea23a2ca94924f4a55fe39a44a), and this PR makes the boundary more visible. It may be beneficial to clarify this abstraction going forward.
From a directional and longer-term design point of view (not necessarily a requirement for this PR), one possible evolution could be:
Move client authentication toward token-based credentials
As a follow-up direction, the kbs-client could rely on tokens rather than private keys. For example, docker-compose or Kubernetes deployments could generate tokens at launch time and mount them at a well-defined path for the kbs-client to consume. This would be a change that we once talked about and can be considered independently of this PR.
Conceptually split
admin::validate_admin_token(the module's than the plugin's) into two stagesThis separation could exist conceptually first, even if the initial implementation remains simple.
The authorization decision could be driven by a lightweight policy definition (e.g., file-based), instead of being embedded in startup configuration. KBS would still not perform identity issuance or user management; it would only verify externally issued credentials and apply a local authorization policy. More complex authentication or identity logic could remain out of band.
This direction certainly has drawbacks:
Higher implementation complexity in the initial iteration.
Increased learning cost for users, who would need to understand the policy model.
That said, this could be mitigated by providing a default policy (e.g., allow_all) for simple use cases.
Overall, the current PR provides a practical and usable solution, while the points above are intended as considerations for future evolution rather than blockers for the current implementation.
wdyt?
Uh oh!
There was an error while loading. Please reload this page.
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.
Runtime configuration of admin roles is out of scope. Maybe sometime we could add an API to update the roles (and store them in the kv backend), but this is complicated, and currently we don't even have an admin login interface.
For 2, I will just rename the upper function to be more generic. We already have a split between checking the token, which is handled by the admin backends, and allowing access to the endpoint. It isn't perfect since the allow-all backend could be specified and roles could block things on top of that, but I've updated the docs to explain the different.
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.
@Xynnn007 let's revisit this before the PR gets stale. i renamed a few things and updated the docs. I think it's best to use a simple approach for now. We have some internal people asking for this feature. I am hoping to get some feedback from them after we implement something and iterate if needed.
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.
What about this? I will do some changes upon your current PR after getting it merged. including moving key pair to token and some other changes about code structure
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.
Sure, but what do you have in mind generally?
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.
Yes. Almost the PR
Uh oh!
There was an error while loading. Please reload this page.
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.
Ok. I have some comments about that PR but we can save for when it is actually posted. It seems fine in general although it's much more complex.