Skip to content

KBS /resource: support CoCo-AS token without public key in it #307

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.

Sign up for GitHub

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

Closed
jialez0 wants to merge 1 commit into from
Closed

KBS /resource: support CoCo-AS token without public key in it #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions kbs/src/api/src/http/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ pub(crate) async fn attestation_policy(
request: HttpRequest,
input: web::Bytes,
user_pub_key: web::Data<Option<Ed25519PublicKey>>,
insecure: web::Data<bool>,
insecure_api: web::Data<bool>,
attestation_service: web::Data<Arc<AttestationService>>,
) -> Result<HttpResponse> {
if !insecure.get_ref() {
if !insecure_api.get_ref() {
let user_pub_key = user_pub_key
.as_ref()
.as_ref()
Expand All @@ -38,10 +38,10 @@ pub(crate) async fn resource_policy(
request: HttpRequest,
input: web::Json<serde_json::Value>,
user_pub_key: web::Data<Option<Ed25519PublicKey>>,
insecure: web::Data<bool>,
insecure_api: web::Data<bool>,
policy_engine: web::Data<PolicyEngine>,
) -> Result<HttpResponse> {
if !insecure.get_ref() {
if !insecure_api.get_ref() {
let user_pub_key = user_pub_key
.as_ref()
.as_ref()
Expand Down Expand Up @@ -85,10 +85,10 @@ pub(crate) async fn set_resource(
request: HttpRequest,
data: web::Bytes,
user_pub_key: web::Data<Option<Ed25519PublicKey>>,
insecure: web::Data<bool>,
insecure_api: web::Data<bool>,
repository: web::Data<Arc<RwLock<dyn Repository + Send + Sync>>>,
) -> Result<HttpResponse> {
if !insecure.get_ref() {
if !insecure_api.get_ref() {
let user_pub_key = user_pub_key
.as_ref()
.as_ref()
Expand Down
3 changes: 3 additions & 0 deletions kbs/src/api/src/http/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ pub enum Error {
#[error("The request is invalid: {0}")]
InvalidRequest(String),

#[error("Cannot return plain resource data without HTTPS")]
InsecureResponse,

#[error("Json Web Encryption failed: {0}")]
JWEFailed(String),

Expand Down
80 changes: 48 additions & 32 deletions kbs/src/api/src/http/resource.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub(crate) async fn get_resource(
#[cfg(feature = "as")] map: web::Data<SessionMap>,
token_verifier: web::Data<Arc<RwLock<dyn AttestationTokenVerifier + Send + Sync>>>,
#[cfg(feature = "policy")] policy_engine: web::Data<PolicyEngine>,
https_enabled: web::Data<crate::HttpsFlag>,
) -> Result<HttpResponse> {
#[allow(unused_mut)]
let mut claims_option = None;
Expand All @@ -46,31 +47,6 @@ pub(crate) async fn get_resource(
Error::AttestationClaimsParseFailed(format!("illegal attestation claims: {e}"))
})?;

let pkey_value = claims
.get("customized_claims")
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `customized_claims` in the attestation claims thus no `tee-pubkey`",
)))?
.as_object()
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"`customized_claims` should be a JSON map",
)))?
.get("runtime_data")
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `runtime_data` in the attestation claims thus no `tee-pubkey`",
)))?
.as_object()
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"`runtime_data` should be a JSON map",
)))?
.get("tee-pubkey")
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `tee-pubkey` in the attestation claims",
)))?;
let pubkey = TeePubKey::deserialize(pkey_value).map_err(|e| {
Error::AttestationClaimsParseFailed(format!("illegal attestation claims: {e}"))
})?;

let resource_description = ResourceDesc {
repository_name: request
.match_info()
Expand Down Expand Up @@ -123,13 +99,24 @@ pub(crate) async fn get_resource(
.await
.map_err(|e| Error::ReadSecretFailed(e.to_string()))?;

let jwe = jwe(pubkey, resource_byte)?;

let res = serde_json::to_string(&jwe).map_err(|e| Error::JWEFailed(e.to_string()))?;

Ok(HttpResponse::Ok()
.content_type("application/json")
.body(res))
// If public key is included in claims, use JWE to wrap resource byte,
// else check if HTTPS is enabled.
match get_pkey(claims) {
Copy link
Contributor

@mkulke mkulke Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly we want to support requests without public key for KBS deployments without HTTPs. In this case there would never be a be pkey, right? Does it still make sense to attempt to fetch it or should we rather start the with the HttpsEnabled == false check and return the unwrapped resource? (Instead of swallowing the get_pkey() error).

Copy link
Member Author

@jialez0 jialez0 Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned, this PR hopes to support requests without pubkey in KBS with HTTPS enabled. However, "HTTPS enabled" is a minimum security measure. Even if HTTPS is enabled, pubkeys may still exist. In this case, the returned resource data is protected by both HTTPS and JWE. Only when the pubkey cannot be found, data will be returned when HTTPS is enabled. If HTTPS is not enabled, it is completely unsafe, and KBS will refuse to return data.

Copy link
Contributor

@mkulke mkulke Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. thanks for clarifying. So, it looks like having no pubkey in the claims is a legitimate code path, i.e. the signature is actually something like: fn get_pkey() => Option<Result<TeePubKey>>. A result should capture unexpected states. An alternative would be to match on a concrete error type:

Err(err) => {
   if let err = Error:NoPubkeyInClaims && https.enabled {
   ....
   }
   Err(Error::InsecureResponse)
}

Failing to parse ( TeePubKey::deserialize(pkey_value)) should probably result in an error and not in a fallback to https, because this shouldn't happen in any case.

If customized_claims is specified as always having runtime_data.tee-pubkey we probably want to error out, too.

Ok(pubkey) => {
let jwe = jwe(pubkey, resource_byte)?;
let res = serde_json::to_string(&jwe).map_err(|e| Error::JWEFailed(e.to_string()))?;
Ok(HttpResponse::Ok()
.content_type("application/json")
.body(res))
}
Err(_) => {
if !https_enabled.enabled {
return Err(Error::InsecureResponse);
}
let res = URL_SAFE_NO_PAD.encode(resource_byte);
Ok(HttpResponse::Ok().body(res))
Copy link
Member

@fitzthum fitzthum Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by this PR. Isn't it fundamental that the secure channel is bound to the hw evidence? In other words, doesn't the KBS want to know that when it encrypts a value using a public key, it can only be decrypted inside the enclave that has the corresponding private key? I'm not sure it's valid to support secret delivery without this. What use case are you looking at?

Copy link
Member Author

@jialez0 jialez0 Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fitzthum I am trying to get CDH to support calling AA to obtain a token from AS, and then using this token to access KBS to obtain resources. However, currently, AA does not carry a pubkey in the tokens obtained from AS.

Another possible path is to add a runtime data parameter to the API for obtaining AS tokens for AA to pass the pubkey, and CDH is responsible for generating and managing this pubkey, so that KBS can still wrap and return resources using JWE.

Copy link
Member Author

@jialez0 jialez0 Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the PR for AA to add runtime data parameter. If we accept that, maybe this PR can be closed.

cc @mkulke

}
}
}

#[cfg(feature = "as")]
Expand Down Expand Up @@ -243,3 +230,32 @@ pub(crate) fn jwe(tee_pub_key: TeePubKey, payload_data: Vec<u8>) -> Result<Respo
tag: "".to_string(),
})
}

pub(crate) fn get_pkey(claims: Value) -> Result<TeePubKey> {
let pkey_value = claims
.get("customized_claims")
Copy link
Contributor

@mkulke mkulke Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define/reuse a type CustomizedClaim instead of manually performing the parsing by stepping through properties?

.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `customized_claims` in the attestation claims thus no `tee-pubkey`",
)))?
.as_object()
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"`customized_claims` should be a JSON map",
)))?
.get("runtime_data")
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `runtime_data` in the attestation claims thus no `tee-pubkey`",
)))?
.as_object()
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"`runtime_data` should be a JSON map",
)))?
.get("tee-pubkey")
.ok_or(Error::AttestationClaimsParseFailed(String::from(
"No `tee-pubkey` in the attestation claims",
)))?;
let pubkey = TeePubKey::deserialize(pkey_value).map_err(|e| {
Error::AttestationClaimsParseFailed(format!("illegal attestation claims: {e}"))
})?;

Ok(pubkey)
}
11 changes: 10 additions & 1 deletion kbs/src/api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,18 @@ impl ApiServer {
};

let insecure_api = self.insecure_api;
let https_enabled = HttpsFlag {
enabled: !self.insecure,
};

let http_server = HttpServer::new(move || {
#[allow(unused_mut)]
let mut server_app = App::new()
.wrap(middleware::Logger::default())
.app_data(web::Data::new(http_timeout))
.app_data(web::Data::new(user_public_key.clone()))
.app_data(web::Data::new(insecure_api));
.app_data(web::Data::new(insecure_api))
.app_data(web::Data::new(https_enabled));

cfg_if::cfg_if! {
if #[cfg(feature = "as")] {
Expand Down Expand Up @@ -343,3 +347,8 @@ impl ApiServer {
}
}
}

#[derive(Clone, Copy, Debug)]
pub(crate) struct HttpsFlag {
enabled: bool,
}