External plugin support for KBS#1188
External plugin support for KBS#1188bpradipt wants to merge 8 commits intoconfidential-containers:mainfrom
Conversation
ce4183c to
b9298bd
Compare
fitzthum
left a comment
There was a problem hiding this comment.
A couple comments. Generally I think this is a fine approach, but I would try to reduce the scope as much as possible. Probably don't need the example implementations (in this repo at least).
Also, why are we modifying the existing plugins?
f500ee6 to
98ee342
Compare
The signature for handle() required changing for ext plugin:
Would you prefer not touching the built-in plugins at all ? |
Hmm. Those are reasonable features, but might be best to start with the simplest possible approach and then improve it if people have the requirement. |
|
Hi @bpradipt I am excited to see the work and it's a pity I didn't participate in the community weekly meeting and related discussions. I have had the similar idea with you that the core value of KBS is 1) RCAR handshake protocol 2) Regarding the second point, I once proposed an RFC to address the issue of unified abstraction of different attestation service tokens/results encountered by different plugins. That thread might change some API but we can perfect them later. Our original design allowed for custom backend implementations of the plugin, meaning that backend plugins could connect to external services via gRPC, RESTful, or any other communication method (IPC, net, etc.). Before diving into reviewing the huge PR, at a quick glance. I'm pleased to see the implementation utilizing a sub-plugin ( As @fitzthum points out, we might not need the sdk code to be included in the repo. There are some precedents; we added a Go SDK to CDH, but it was never maintained and was eventually removed. Therefore, it's recommended to just keep the proto in the repo. Anyway, I will take a review once these "weight loss work" is done for the PR. |
b327d48 to
03a512e
Compare
| /// from the session store. For Bearer token auth, KBS has already verified | ||
| /// the token before reaching the plugin handler, so is_attested is set to | ||
| /// true; tee_type is not available without re-parsing the JWT claims. | ||
| pub async fn get_plugin_context(&self, request: &HttpRequest) -> crate::plugins::PluginContext { |
There was a problem hiding this comment.
I like the idea of pluginContext as it shares a same idea with mine to transform the complex attestation context into a simple one for different plugins to use.
I want to do an abstraction upon this PluginContext here as a core component working in kbs logic, probably naming to TrustContext.
Currently the is_attested only means it has an attestation token binding to the session, while an attestation token only means the endorsement is verified based on the current CoCoAS blackbox. This can be perfect-ted in future.
| SessionStatus::Authed { request: req, .. } => { | ||
| let tee_type = serde_json::to_string(&req.tee) | ||
| .ok() | ||
| .map(|s| s.trim_matches('"').to_owned()); |
There was a problem hiding this comment.
Not related to this PR. We are always trying to do trim options upon the Tee type's serialization. We need to impl a ToString or some trait for Tee someday.
kbs/src/plugins/plugin_manager.rs
Outdated
|
|
||
| #[cfg(feature = "external-plugin")] | ||
| #[derive(Clone, Debug, Deserialize, PartialEq)] | ||
| pub struct ExternalPluginConfig { |
There was a problem hiding this comment.
I saw a lot of feature flags external-plugin. Can we move the definitions into a separate module implementations/external_plugin.rs
kbs/src/plugins/plugin_manager.rs
Outdated
| pub enum PluginInstance { | ||
| BuiltIn(Arc<dyn ClientPlugin>), | ||
| #[cfg(feature = "external-plugin")] | ||
| External(Arc<GrpcPluginProxy>), | ||
| } |
There was a problem hiding this comment.
I noticed that the user-side toml definitions still follows the previous style, s.t. name item is used to choose external. Thus we can organize the code in a style that external is treated as a same-level plugin as current pkcs11, sample, etc. Thus we might not need this wrapper. Just add a new implementations/external.rs - or if more files are needed, implementations/external/mod.rs to have the external plugins forwarder.
There was a problem hiding this comment.
This is intentional to avoid changing the existing ClientPlugin trait. External plugins need to forward request context (session ID, TEE type, attestation status) as gRPC metadata, which required a separate dispatch path (handle_with_context). Adding &PluginContext toClientPlugin::handle() would have been a breaking change for all existing plugin implementations.
Infact my earlier implementation was around the approach you described but resulted in changing the existing plugins. So after @fitzthum comment I went with the current approach.
Happy to revisit if there's a preferred way to extend the trait without breaking existing plugins.
There was a problem hiding this comment.
In fact, in my view, the only difference between external plugin calls and internal integrations is whether or not a gRPC forwarder is involved; they are essentially the same. Both calls need to accept the same parameters, regardless of how the parameters are designed. There is no problems upon the trust relations.
body: &[u8],
query: &HashMap<String, String>,
path: &[&str],
method: &Method,
context: &PluginContext,| [workspace] | ||
| members = [ | ||
| "kbs", | ||
| "rust-sdk", |
There was a problem hiding this comment.
I wonder if we need this rust-sdk. IMOO, a protobuf is enough. But we can keep this for some time and see if the maintaince is good.
| config.plugin_name.clone(), | ||
| )); | ||
|
|
||
| let timeout = config.timeout_ms.map(Duration::from_millis); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
There was a problem hiding this comment.
Looks like a false positive. Wrongly attributing ensure_auth_key_pair to external_plugin.rs
9e28938 to
85da2b2
Compare
| use crate::plugins::external::plugin_api::{ | ||
| kbs_plugin_client::KbsPluginClient, GetCapabilitiesRequest, NeedsEncryptionRequest, | ||
| PluginRequest, ValidateAuthRequest, | ||
| }; |
There was a problem hiding this comment.
We can also include the code of crate::plugins::external into this file and delete external.rs?
- Add KbsPlugin service with Handle and GetCapabilities RPCs - Define PluginRequest with body, query, path, method fields - Define PluginResponse with body, status_code, content_type fields - Reserve field 5 for future attestation context - Set up buf tooling for proto linting and CI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Introduces the gRPC proxy backend for external plugins and wires it into the KBS plugin dispatch layer: - api_server.rs: switch to async PluginManager::new() (external plugin init requires gRPC TLS channel setup); add PUT/DELETE route handlers so external plugins can implement full REST APIs; build PluginContext from session/attestation state to pass TEE metadata without raw tokens; replace plugin.handle() with plugin.dispatch() returning PluginOutput (body + optional HTTP status + content type); downcast PluginCallError so external plugins can return structured HTTP error responses. - implementations/mod.rs: feature-gate the grpc_proxy module and re-export GrpcPluginProxy under the external-plugin feature flag. Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Basic metrics for external plugin Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Rust SDK for external plugin Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Adds an external gRPC resource plugin example (`resource_plugin.rs`) that replicates the built-in KBS resource plugin behaviour exactly: - POST (store) is admin-gated (validate_auth = true) - GET (retrieve) is attestation-gated (validate_auth = false) - GET responses are JWE-encrypted with the TEE public key (needs_encryption = true) - POST responses are plain acknowledgements (needs_encryption = false) - Returns gRPC NotFound on GET for missing keys (mirrors built-in 404) The plugin maintains an in-memory key-value store using `Arc<RwLock<HashMap<String, Vec<u8>>>>` and registers itself with the capabilities name "extstore". The default listen address is 127.0.0.1:50053 and can be overridden via `RESOURCE_PLUGIN_LISTEN_ADDR`. Also adds a full e2e roundtrip test in `kbs/test/Makefile`: - Builds the plugin binary and starts it on :50053 - Starts KBS on :8088 with a config that routes requests through the plugin - POSTs a random 16-byte secret via the admin-gated endpoint - GETs it back via kbs-client - Diffs the result against the original Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Introduction, Quick Start, Writing a Plugin, Building and Packaging - Quick Start examples in Rust, Go, and Python - Handler implementation guide with request/response field tables - gRPC metadata access patterns for session context - Error handling with gRPC-to-HTTP status code mapping - Containerization and CI/CD integration patterns - KBS TOML configuration with TLS mode examples (insecure/tls/mtls) - Standalone and Kubernetes deployment patterns with manifests - Health check integration with grpc.health.v1.Health - Per-plugin Prometheus metrics documentation (requests, duration, errors) - Links to proto file, SDK docs, and echo plugin examples Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Runs the `make e2e-ext-plugin` target (plaintext, TLS, attestation, metrics) on PRs that touch plugin source, config, or test files. Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Creating this as a draft PR to make it easier to discuss and collaborate.