KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

Xynnn007 · 2024-09-27T07:42:58Z

This PR includes the following work

Refactoring the KBS codebase to have plugin mechanism in, including attestation/resource/policy engine
Update the config file format to make it more tidy and scalable.
Combine CoCo/ITA Token verification logic.
Add client plugin mechanism in. All client plugin calls will be filtered by attestation token.

I am sorry to have such a huge PR and did some disruptive changes. I originally want to add a simple plugin mechanism but soon found that some architectural problems and hidden bugs, thus fixed them by the way. At last resulting such many changed lines.

This PR still works fine with kbs-client on the guest-components side and did not change the protocol.

Needs some attestion from ITA folks @mythi @jodh-intel to see if the changes upon ITA config need any change.
Nebula folks @cclaudio about #451

cc @fitzthum @jialez0 @mkulke

Fixes #486
Related to #502

This refactoring combines all RCAR (attestation) related code into one module. This would help to better modularization and error handling. Signed-off-by: Xynnn007 <[email protected]>

Actually, the ITA token and CoCo Token are both JWTs. They both need a JWK to verify the JWT. The difference is the way to gather the JWK. This commit combined the two logic, and add two ways to get the JWK. 1. From the configured JwkSet when launching KBS 2. From the JWT's Header's jwk field. The two ways will check the jwk endorsement in different ways. The first way is to configure the trusted JwkSet from the config. The second way is to configure the trusted CA in config. Then get the public key cert chain from Jwk's x5c field. The both ways are also supported in this patch. Rust does not provide a mature crate to verify cert chain, thus openssl is used in this patch. We also abondon rustls and openssl feature of KBS because openssl is by default used. Then we use openssl by default to make the code base simpler. Signed-off-by: Xynnn007 <[email protected]>

kbs/src/admin/mod.rs

kbs/src/api_server.rs

kbs/src/policy_engine/opa/mod.rs

kbs/src/policy_engine/mod.rs

mkulke · 2024-09-27T08:18:32Z

kbs/src/attestation/backend.rs

+            .map_err(|e| Error::RcarAttestFailed { source: e })
+    }
+
+    async fn __attest(


this convention of creating "private functions w/ underscore prefixes doesn't look like idiomatic rust. can we inline __myfunc() in myfunc() or if they need to be separated can we put it into a module?

Do you mean to use #[inline] upon pub async fn attest(...)?

no, just move the __attest code into the attest function (unless there is a reason to keep them apart, then we can move it it into a module maybe)

Oh. The wrapping is for error conversion. If move all logic from __attest to attest, we need a lot of map_err from anyhow::Error to Error

if Error is using thiserror macros, we should be able to use a Misc(#[from] anyhow::Error) entry in the error enum, or it even works automatically, I don't remember

Yes. The #[from] could automatically convert ALL anyhow:Error to Error with ?. In this case we want to do some classfication upon the different anyhow::Error due to Auth and Attest of RCAR

ok, if we want to mix anyhow and concrete error types we have to resort to gymnastics like this function wrapping. If we bother about the concrete errors I'd suggest declaring them explicitly in an error enum, at least in new code. so instead of

request.cookie(KBS_SESSION_ID).context("cookie not found")?;

#[error("cookie not found")] CookieNotFound, ... request.cookie(KBS_SESSION_ID).ok_or(Error::CookieNotFound)?;

Right. My original aim is to avoid too many error types. Overall, they are only RCAR attest failed, because of an anyhow::Error and RCAR auth failed, because of an anyhow::Error. The concrete error inside could be all coverted by anyhow. I like the concise error type definition style myself

fitzthum

Great work. Lots of positive changes here.

I mention one problem in a couple of my comments. Let me expand on it here.

For some of our endpoints, we have GET and POST functionality. The GET request is usually called by the client in the guest, but the POST request has too different cases. Sometimes the POST request is a privileged admin function that is called by the kbs-client running in a trusted environment. Sometimes it is called by the client in the guest.

In some cases we could actually support both. For resources and plugins, for instance, we might want to have an admin be able to configure the resource/plugin, but also have the guest be able to make a POST request. Unfortunately we only have one endpoint for these two things. Maybe we should decide on a convention for how these two different things can be implemented.

I think this will also help resolve some confusion around the so-called client and admin APIs, which are currently intermixed.

kbs/src/resource/backend.rs

fitzthum · 2024-09-27T16:57:43Z

kbs/src/api_server.rs

+}
+
+/// Client APIs. /kbs/v0/XXX
+pub(crate) async fn client(


Client is sort of a confusing name for this since we have APIs that can be used by the CDH but also we have the APIs to set the policy.

Yes. You are right. I want to move the set_policy, set_resource APIs to kbs/v0/admin/.... Thus we could have a distinguished barrier from client api and admin api. Maybe we can relatively change the logic of kbs-client. This change will not influence anything on the guest-components side. wdyt?

That's fine with me although I'm not sure it's entirely RESTful

Nice catch for the RESTful design and client/admin APIs. Maybe I went to the wrong design way at first. We might only need one plugin system, and adds admin_auth to the handle(). This means it is the API that decides whether admin auth needs to be checked.

In this way, we can avoid extra kbs/v0/admin/<plugin-name>, but only have one API kbs/v0/<plugin-name>.

Wdyt?

Yeah the problem with that is that you can't implement a plugin that has both GET / POST for the guest and GET / POST for an admin, which some people might want to do (although note that posting from the guest is a bit risky since the requests aren't encrypted).

It looks like having an /admin route is somewhat common so maybe that's ok. Honestly I don't have a strong preference here.

kbs/src/api_server.rs

fitzthum · 2024-09-27T17:10:10Z

kbs/src/plugins/sample.rs

+        _path: String,
+        _method: &Method,
+    ) -> Result<HttpResponse> {
+        let response = HttpResponse::Ok().body("sample plugin response");


Can you expand this example slightly to have different logic for GET and POST and to verify the admin credentials for the POST case.

kbs/src/api_server.rs

fitzthum · 2024-09-27T18:02:12Z

btw I wonder if we should implement the resource backend as a plugin now

Xynnn007 · 2024-09-28T00:06:18Z

@fitzthum Thanks for the review and nice suggestion. About admin and client api, my idea is to move old admin APIs like POST kbs/v0/<plugin> to POST kbs/v0/admin/<plugin>. All admin APIs should be under kbs/v0/admin, no matter it is resource setting or policy setting. This would help us to have a clear border. ~~Let me put a separate commit for review to see if it is good.~~ Please see https://github.com/confidential-containers/trustee/compare/bddfcf5241d71fa1f3263648ac8cfd311cac9df4..91b69d06d33c714de6c591d8f1013feb1e1c686b

btw, about the policy filter for APIs. Now it still works fine with legacy ways, s.t. GET resources. I want to also do a re-design for the policy engine module when refactoring the code. But it seems like an independent work thus not included inside this PR. Mainly for

change the policy format to allow filter both client plugins (including resource) and admin plugins
new point from yours, to add support for GET/POST requests
To have relative static policy (defines roles and privileges) and data (defines how to be a role) and dynamic input (claims to be checked to be a role) to leverage policy engine like the example.

This commit does some refactoring upon policy engine module. Including 1. Change ResourcePolicyError to PolicyEngineError. This is because in future, different client plugins would share same policy engine thus the new name will match better. 2. add a new `set_policy` api for PolicyEngine. This api will handle SetPolicyInput format request rather than the plaintext of policy. This would help to integrate into the KBS server. The plugin mechanism is by default enabled, thus we delete `opa` and `policy` feature. By default integrate `regorus` crate for policy. Signed-off-by: Xynnn007 <[email protected]>

This module brings all admin authentication logic together. Currently it allows to use a public key to verify the admin access. Signed-off-by: Xynnn007 <[email protected]>

The resource module brings all resource storage logic together thus helps modularization. Also, it changes both `read_secret_resource` and `write_secret_resource` to Fn rather than FnMut. This leaves the synchronization handling to concrete underlying plugins, thus promote the performance because we can avoid a global Mutex. Signed-off-by: Xynnn007 <[email protected]>

The Plugins module could provide a plugin way for developers to extend the ability of KBS client APIs. This also provides a Sample implementation for example. Signed-off-by: Xynnn007 <[email protected]>

This is mostly a refactoring patch for KBS. It brings API serving into one function, and will perform different sub-function due to the requested plugin name. This also changes all configuration codes to have a default value. This patch would have some compatibility issue as it changes the old configuration format. The old configuration format is not well classified. This patch tidies the configuration items. Signed-off-by: Xynnn007 <[email protected]>

This patch fixes example configurations of KBS inside this codebase. Also, it fixes the CI test. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

Now the KBS could be built with support for all backend ASes and enable one of them runtimely due to configuration file. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

mythi

I checked 542f64c only. Can that be split to a PR of its own? it's a bit too much content in one PR.

mythi · 2024-09-30T07:04:48Z

kbs/src/token/mod.rs

-    Jwk,
+
+    #[serde(rename = "ITA")]
+    Ita,


this shouldn't be a user facing name/option.

What name do you prefer to use?

mythi · 2024-09-30T07:16:26Z

kbs/src/token/mod.rs

+#[derive(Deserialize, Debug, Clone, PartialEq, Default)]
 pub struct AttestationTokenVerifierConfig {
    #[serde(default)]
-    pub attestation_token_type: AttestationTokenVerifierType,
+    pub r#type: AttestationTokenVerifierType,

-    /// Trusted Certificates file (PEM format) path (for "CoCo") or a valid Url
-    /// (file:// and https:// schemes accepted) pointing to a local JWKSet file
+    /// Trusted Certificates file (PEM format) paths use to verify Attestation
+    /// Token Signature.
+    #[serde(default)]
+    pub trusted_certs_paths: Vec<String>,
+
+    /// Urls (file:// and https:// schemes accepted) pointing to a local JWKSet file
    /// or to an OpenID configuration url giving a pointer to JWKSet certificates
    /// (for "Jwk") to verify Attestation Token Signature.
    #[serde(default)]
-    pub trusted_certs_paths: Vec<String>,
+    pub trusted_jwk_sets: Vec<String>,
+
+    /// Whether a JWK that directly comes from the JWT token is allowed to verify
+    /// the signature. This is insecure as it will not check the endorsement of
+    /// the JWK. If this option is set to false, the JWK will be looked up from
+    /// the key store configured during launching the KBS with kid field in the JWT,
+    /// or be checked against the configured trusted CA certs.
+    #[serde(default = "bool::default")]
+    pub insecure_key: bool,


I tried hard not to break users then Jwk type was added. I don't see these changes absolutely necessary either. how would users know when to use trusted_certs_pathsortrusted_jwk_sets`? IMO, for a proper token verifier unification, we don't have a "type" at all and users would only configure their trust sources.

This is hard to do. Like I said in #519 (comment). Currently KBS will receive JWTs from either CoCoAS or ITA. I have no good idea to detect which concrete JWT is runtimely. So the design here is to configure the token type in the config file.

In future, I wonder if we can change the authentication way here. That means

Attestation module would be responsible to interactive with backend attestation services and get a yes/no result.

KBS will provision a certificate that certs the tee public key and an id inside due to previous yes/no result.

In this way, the storage and other plugins will only need to check the endorsement of the client certificate and the id. Then the policy engine (OPA) would do much easier job -- control API access due to the client id, rather than complex different attestation claims.

fitzthum · 2024-09-30T15:18:37Z

@Xynnn007

All admin APIs should be under kbs/v0/admin, no matter it is resource setting or policy setting. This would help us to have a clear border.

I think this is good. Maybe we should also have a generic admin endpoint for each plugin. Like I said, I think it would be good to move our existing resource implementation to a plugin. This would prove that the plugin interface is robust. That might be too much work for this PR, though.

One other thing, do you think we should plugin as part of the route for plugins. As it is people can't implement a plugin named attest because it will collide with the attest endpoint. Maybe that's not too big of a deal. Having a specific plugin route would make the function serving it a bit smaller.

cclaudio · 2024-10-01T18:40:24Z

kbs/src/plugins/sample.rs

+
+pub struct Sample {
+    _item: String,
+}


Can you update the kbs-client to take requests for plugins (e.g. Sample plugin)?

Yes we can! This would give a good implementation example

kbs/src/plugins/sample.rs

cclaudio · 2024-10-04T14:46:48Z

kbs/src/api_server.rs

+                })?;
+            let body = body.to_vec();
+            let response = plugin
+                .handle(body, query.into(), sub_path.into(), request.method())


I think the plugin may/should want to encrypt the response payload. If so, the handle() should be able to access the TEE public key and call jwe(). You could pass the TEE public key via parameter.

I added a encrypted() method for plugin, that will tell KBS if the response needs to be encrypted via JWE.

Xynnn007 · 2024-10-08T07:14:13Z

btw I wonder if we should implement the resource backend as a plugin now

Resource backend is somehow different. If we want to implement it as a backend plugin, we would use a config item like

...
[[plugins]]
name = "ResourceStorage"
type = "LocalFs"
dir_path = "/tmp/kbs-resource"

While what we are using now is

...
[repository]
type = "LocalFs"
dir_path = "/tmp/kbs-resource"

The difference

If we use plugin mechanism, if and only if the ResourceStorage is explicitly claimed in the configuration file, the API will be open. This hints that delete feature resource and include that may be a good choice.
If we use legacy way, whether the API is open is decided during compilation time.

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 · 2024-10-08T09:53:58Z

@fitzthum I added one commit that tries to make resource a plugin. Also, combine admin and client plugins into one plugins system. The concrete plugin would decide whether it could be accessed upon client auth (attestation) or admin auth. Please take a look if it is good, then I can do some commit tidy work

fitzthum · 2024-10-08T16:35:42Z

I added one commit that tries to make resource a plugin. Also, combine admin and client plugins into one plugins system. The concrete plugin would decide whether it could be accessed upon client auth (attestation) or admin auth. Please take a look if it is good, then I can do some commit tidy work

I really like moving the resource stuff to a plugin. I think it makes sense to get rid of the resource feature. In the future there might be some complex plugins that are behind feature flags (in addition to being enabled at runtime), but imo we should always build the resource support.

Like I said, I could go either way on the admin routes. Maybe another reviewer has an opinion there.

Otherwise I think this is good.

Xynnn007 · 2024-10-09T01:32:46Z

Like I said, I could go either way on the admin routes. Maybe another reviewer has an opinion there.

Otherwise I think this is good.

Nice. I wonder if @cclaudio could give some suggestions upon the design.

~~Also, let me put another PR for token affairs as @mythi mentioned at first.~~

I added #524 about Token parts. Hopefully that makes sense and get merged. Then we can go back to this PR to do some tidy work.

Xynnn007 added 2 commits September 27, 2024 15:28

KBS: refactor attestation module

ff2e518

This refactoring combines all RCAR (attestation) related code into one module. This would help to better modularization and error handling. Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 marked this pull request as ready for review September 27, 2024 08:05

Xynnn007 requested a review from a team as a code owner September 27, 2024 08:05

mkulke reviewed Sep 27, 2024

View reviewed changes

Xynnn007 force-pushed the refactor-kbs branch from 54c5b4d to bddfcf5 Compare September 27, 2024 09:07

fitzthum reviewed Sep 27, 2024

View reviewed changes

Xynnn007 force-pushed the refactor-kbs branch from bddfcf5 to 91b69d0 Compare September 28, 2024 00:21

Xynnn007 added 9 commits September 28, 2024 15:36

KBS: add Admin auth module

4d89589

This module brings all admin authentication logic together. Currently it allows to use a public key to verify the admin access. Signed-off-by: Xynnn007 <[email protected]>

KBS: add Plugins module

9f08369

The Plugins module could provide a plugin way for developers to extend the ability of KBS client APIs. This also provides a Sample implementation for example. Signed-off-by: Xynnn007 <[email protected]>

KBS: fix CI and exampled configurations

8e83068

This patch fixes example configurations of KBS inside this codebase. Also, it fixes the CI test. Signed-off-by: Xynnn007 <[email protected]>

AS: reorder the dep in lexicographic order

823cc81

Signed-off-by: Xynnn007 <[email protected]>

KBS: change default feature to all backend AS and resource

82f4118

Now the KBS could be built with support for all backend ASes and enable one of them runtimely due to configuration file. Signed-off-by: Xynnn007 <[email protected]>

KBS: move all admin APIs under /kbs/v0/admin

db46ea2

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the refactor-kbs branch from 91b69d0 to db46ea2 Compare September 28, 2024 07:36

mythi suggested changes Sep 30, 2024

View reviewed changes

mythi mentioned this pull request Oct 1, 2024

ita: updated trusted_certs_paths property documentation #515

Merged

cclaudio reviewed Oct 1, 2024

View reviewed changes

kbs/src/plugins/sample.rs Show resolved Hide resolved

cclaudio reviewed Oct 4, 2024

View reviewed changes

fixup! KBS: add resource module

bc176e4

KBS: abondon admin API and make resource a plugin

c058c6a

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 mentioned this pull request Oct 9, 2024

KBS: combine CoCo Token and Jwk Token verifier #524

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

Xynnn007 commented Sep 27, 2024 •

edited

Loading

mkulke Sep 27, 2024

Xynnn007 Sep 27, 2024

mkulke Sep 27, 2024

Xynnn007 Sep 27, 2024

mkulke Sep 27, 2024 •

edited

Loading

Xynnn007 Sep 27, 2024

mkulke Sep 27, 2024

Xynnn007 Sep 27, 2024

fitzthum left a comment

fitzthum Sep 27, 2024

Xynnn007 Sep 27, 2024

fitzthum Sep 30, 2024

Xynnn007 Oct 8, 2024

fitzthum Oct 8, 2024

fitzthum Sep 27, 2024

fitzthum commented Sep 27, 2024

Xynnn007 commented Sep 28, 2024 •

edited

Loading

mythi left a comment

mythi Sep 30, 2024

Xynnn007 Oct 8, 2024

mythi Sep 30, 2024

Xynnn007 Oct 8, 2024 •

edited

Loading

fitzthum commented Sep 30, 2024

cclaudio Oct 1, 2024 •

edited

Loading

Xynnn007 Oct 8, 2024

cclaudio Oct 4, 2024 •

edited

Loading

Xynnn007 Oct 8, 2024

Xynnn007 Oct 8, 2024

Xynnn007 commented Oct 8, 2024

Xynnn007 commented Oct 8, 2024

fitzthum commented Oct 8, 2024

Xynnn007 commented Oct 9, 2024 •

edited

Loading

KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

Are you sure you want to change the base?

KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

Conversation

Xynnn007 commented Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkulke Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum commented Sep 27, 2024

Xynnn007 commented Sep 28, 2024 • edited Loading

mythi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

fitzthum commented Sep 30, 2024

cclaudio Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclaudio Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 commented Oct 8, 2024

Xynnn007 commented Oct 8, 2024

fitzthum commented Oct 8, 2024

Xynnn007 commented Oct 9, 2024 • edited Loading

Xynnn007 commented Sep 27, 2024 •

edited

Loading

mkulke Sep 27, 2024 •

edited

Loading

Xynnn007 commented Sep 28, 2024 •

edited

Loading

Xynnn007 Oct 8, 2024 •

edited

Loading

cclaudio Oct 1, 2024 •

edited

Loading

cclaudio Oct 4, 2024 •

edited

Loading

Xynnn007 commented Oct 9, 2024 •

edited

Loading