Add Admin Roles #1111

fitzthum · 2025-12-16T17:59:48Z

This is a powerful feature, but it's relatively simple to implement.

First, make each admin backend return a role string. This allows us to implement the role logic not in each individual backend, but in the common admin code. This logic is fairly simple. We have a list of admin roles which specify an id and a regular expression. If no roles are specified, all admin will have access to all requests. This provides backwards compatibility.

Since this is a security-critical configuration, I did some defensive programming. For example, the AdminRoles struct does not allow unknown fields when deserializing. The admin id is case insensitive. Etc

I created some unit tests. No integration tests yet due to merge conflicts with the previous admin PR.

Xynnn007 · 2025-12-19T08:54:55Z

kbs/src/admin/allow_all.rs

+    fn validate_admin_token(&self, _request: &HttpRequest) -> Result<String> {
        warn!("Allow All admin backend is set. Anyone can access admin APIs");
-        Ok(())
+        Ok("Anonymous".to_string())


What about

have a separate policy file (no need to be rego, but if rego is good we can use rego) to define admin authZ things (What this PR is intending to do)

the input to the the id/subject or claims parsed from the input JWT of header's bearer.

The output is true/false.

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)

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.

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:

Authorization rules are embedded in startup configuration

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:

Token Verification / Identity Resolution (e.g., extracting credentials, verifying signatures, validating issuers), and

Authorization Decision (allow / deny, or mapping to accessible APIs).

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 stages

Token Verification / Identity Resolution: pluggable implementations that support bearer tokens, JWTs, or other credential formats, and return structured identity or claims.

Authorization Decision: a separate step that evaluates whether the resolved identity is allowed to access a given API.

This separation could exist conceptually first, even if the initial implementation remains simple.

Introduce a simple, standalone authorization policy
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?

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.

@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.

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

Sure, but what do you have in mind generally?

Yes. Almost the PR

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.

When the admin backend validates a token, it should extract a role string and return it. The role will be used by the common admin code to grant access to specific endpoints. Since the ALlowAll endpoint does not have any roles, it will always return "Anonymous" as the role. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

Xynnn007 · 2026-01-06T05:32:11Z

kbs/src/admin/allow_all.rs

+    fn validate_admin_token(&self, _request: &HttpRequest) -> Result<String> {
        warn!("Allow All admin backend is set. Anyone can access admin APIs");
-        Ok(())
+        Ok("Anonymous".to_string())


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

Xynnn007 · 2026-01-06T05:32:37Z

kbs/src/admin/mod.rs

+    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)]


That means by default no endpoints can be accessed?

Yes. That seems less error-prone.

fitzthum · 2026-01-07T19:46:34Z

@mythi @mkulke ptal. this is not a very big PR

fitzthum · 2026-02-06T14:21:42Z

@confidential-containers/trustee-maintainers

Let's get some more reviews here or just move forwards

mkulke

this regex authZ scheme is tempting because of it's simplicity, but often those glob patterns or regexes are security footguns. maybe we can require anchoring (^...$)?

there might also be a semantic problem, if we want to consider HTTP verbs, we cannot add e.g. an allowed_verbs, since we are matching tuples then. we'd have to collapse the verb into the string: ^(GET /admin/foo/[0-9]+|POST /admin/foo)$

kbs/docs/config.md

kbs/src/admin/mod.rs

fitzthum · 2026-02-06T22:37:18Z

there might also be a semantic problem, if we want to consider HTTP verbs, we cannot add e.g. an allowed_verbs, since we are matching tuples then.

Not sure I totally grasp this but at the moment we are only matching on the URI. There is no distinction between POST and GET.

Requiring anchoring seems like a good idea, especially since many of these endpoints can have basically arbitrary URIs that might match the regex in an unintended way.

Generally, I think we should try to get a fairly simple version of this out and then see if/how people actually use the feature before optimizing more.

mkulke · 2026-02-09T08:47:31Z

Not sure I totally grasp this but at the moment we are only matching on the URI. There is no distinction between POST and GET.

ah, I meant that this is a reasonable use case that will probably come up and is not expressible in the current scheme. e.g. "policy_reader" is allowed to "^GET /policies.*$" while "policy_contributor" is also allowed to do "^POST /policies$"

If we want to stick with regexes, I'd suggest making a preceding verb mandatory. or, if we want to add support at a later point, make a leading / mandatory (so we can reserve the empty verb as "all verbs")

mkulke · 2026-02-09T08:49:12Z

Requiring anchoring seems like a good idea, especially since many of these endpoints can have basically arbitrary URIs that might match the regex in an unintended way.

yes, I would start with this. if it turns out to be too restrictive still, it can be relaxed at a later point

kbs/src/admin/mod.rs

mkulke · 2026-02-09T09:12:05Z

kbs/docs/config.md

+
+After the admin token is validated, the role extracted from the token will be checked against
+the roles specified in the config file.
+If a matching role is found, the `allowed_endpoints` regular expression is used to determine


should we define/provide examples what an endpoint is? request.uri seems to be like defined like this:

abc://username:[email protected]:123/path/data?key=value&key2=value2#fragid1 |-| |-------------------------------||--------| |-------------------| |-----| | | | | | scheme authority path query fragment

in the tests we seem to imply an endpoint is "path", but are query and maybe even fragment allowed?

btw, I would expect the anchored test-case to fail since it would start with "http://....", am I misreading the docs or why does this pass?

Hmm I think because I am setting the URI directly in the tests cases with what seems like not correct values. Let me add an integration test for this that will use real requests made to Trustee.

Ok, per the integration tests (which do a full server and client flow), the URI that actix gives us looks like /kbs/v0/attestation-policy. I updated the unit tests to use a similar format.

With this in mind, I actually changed things to require that the regex start with ^/kbs. Hopefully that will help clue people into what these paths look like.

The actix behavior does seem a little weird, but this is what I had in my head when I was first making this PR.

Also note that in any case the method is not included and is out of scope for this approach.

mkulke · 2026-02-11T09:27:41Z

kbs/src/admin/mod.rs

+        // Parse roles to ensure valid regexes and no duplicates.
+        let mut roles = HashMap::new();
+        for role in value.roles {
+            if !role.allowed_endpoints.starts_with("^/kbs") {


anchoring should also check for ends_with("$"), no?

I guess that term is a bit ambiguous. I added a check for $ at the end.

Introduce the admin role configuration at the admin level (rather than the admin backend level). This way, we don't need to duplicate the admin logic for each backend. An admin role matches a particular admin id to a regex. The regex controls which endpoints are allowed. In the future, we could make both fiels regular expressions to more flexibly match different admin ids. If no roles are specified, all admin will have access to all endpoints. This provides backwards compatibility. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

Quick fixup of the integration test. An integration test using roles will follow in a future PR. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

Update the KBS config doc to include the structure of the admin roles configuration and a description of how admin roles work. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

Update the integration tests to add a new "restricted simple admin" setting that creates a Trustee configured to only allow reference values to be accessed by the admin. Plug this new setting into our existing admin endpoint checks. We don't actually have coverage for the reference value endpoint atm, so this doesn't include a positive test Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

The admin backend is only the first step in granting access to an admin endpoint. The admin backend validates the admin token. Next the role extracted from that token is checked. As such, change the logging in the admin backends to talk about the admin token validation passing/failing rather than access being granted to an endpoint. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

fitzthum requested a review from a team as a code owner December 16, 2025 17:59

Xynnn007 reviewed Dec 19, 2025

View reviewed changes

fitzthum force-pushed the admin-3 branch from 972b616 to 46a32f9 Compare December 22, 2025 15:53

fitzthum force-pushed the admin-3 branch from 46a32f9 to 2fb2e3d Compare December 22, 2025 16:37

Xynnn007 reviewed Jan 6, 2026

View reviewed changes

Xynnn007 approved these changes Jan 7, 2026

View reviewed changes

mkulke reviewed Feb 6, 2026

View reviewed changes

kbs/docs/config.md Outdated Show resolved Hide resolved

kbs/src/admin/mod.rs Outdated Show resolved Hide resolved

mkulke reviewed Feb 9, 2026

View reviewed changes

fitzthum force-pushed the admin-3 branch 3 times, most recently from 7935f9b to 0168e00 Compare February 9, 2026 22:09

mkulke reviewed Feb 11, 2026

View reviewed changes

fitzthum added 5 commits February 11, 2026 14:05

tests: fixup integration test for admin roles

228a301

Quick fixup of the integration test. An integration test using roles will follow in a future PR. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

docs: add documentation about admin roles

2892997

Update the KBS config doc to include the structure of the admin roles configuration and a description of how admin roles work. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>

fitzthum force-pushed the admin-3 branch from 0168e00 to 093fa66 Compare February 11, 2026 22:05

Add Admin Roles #1111

Are you sure you want to change the base?

Add Admin Roles #1111

Uh oh!

Conversation

fitzthum commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xynnn007 Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum commented Jan 7, 2026

Uh oh!

fitzthum commented Feb 6, 2026

Uh oh!

mkulke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fitzthum commented Feb 6, 2026

Uh oh!

mkulke commented Feb 9, 2026

Uh oh!

mkulke commented Feb 9, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Xynnn007 Dec 20, 2025 •

edited

Loading

fitzthum Dec 22, 2025 •

edited

Loading

fitzthum Jan 7, 2026 •

edited

Loading

fitzthum Feb 9, 2026 •

edited

Loading