From 4ff452ffd24aac1f9c9acf8a098becebf6bca786 Mon Sep 17 00:00:00 2001 From: weisd Date: Mon, 3 Mar 2025 15:53:56 +0800 Subject: [PATCH] fix iam service_account bugs --- iam/src/auth/credentials.rs | 10 ++---- iam/src/error.rs | 3 ++ iam/src/store/object.rs | 4 ++- iam/src/sys.rs | 12 +++++-- iam/src/utils.rs | 37 ++++++++++++++++++-- madmin/src/user.rs | 1 + rustfs/src/admin/handlers/service_account.rs | 12 ++++++- 7 files changed, 64 insertions(+), 15 deletions(-) diff --git a/iam/src/auth/credentials.rs b/iam/src/auth/credentials.rs index 01e832b4..905c1771 100644 --- a/iam/src/auth/credentials.rs +++ b/iam/src/auth/credentials.rs @@ -152,10 +152,7 @@ impl Credentials { const IAM_POLICY_CLAIM_NAME_SA: &str = "sa-policy"; self.claims .as_ref() - .map(|x| { - x.get(IAM_POLICY_CLAIM_NAME_SA) - .map_or(false, |_| !self.parent_user.is_empty()) - }) + .map(|x| x.get(IAM_POLICY_CLAIM_NAME_SA).is_some_and(|_| !self.parent_user.is_empty())) .unwrap_or_default() } @@ -164,10 +161,7 @@ impl Credentials { return self .claims .as_ref() - .map(|x| { - x.get(&iam_policy_claim_name_sa()) - .map_or(false, |v| v == INHERITED_POLICY_TYPE) - }) + .map(|x| x.get(&iam_policy_claim_name_sa()).is_some_and(|v| v == INHERITED_POLICY_TYPE)) .unwrap_or_default(); } diff --git a/iam/src/error.rs b/iam/src/error.rs index f99cf89d..4c4ff0a6 100644 --- a/iam/src/error.rs +++ b/iam/src/error.rs @@ -79,6 +79,9 @@ pub enum Error { #[error("action not allowed")] IAMActionNotAllowed, + #[error("invalid expiration")] + InvalidExpiration, + #[error("no secret key with access key")] NoSecretKeyWithAccessKey, diff --git a/iam/src/store/object.rs b/iam/src/store/object.rs index 99bf40db..8b11ae15 100644 --- a/iam/src/store/object.rs +++ b/iam/src/store/object.rs @@ -846,7 +846,9 @@ impl Store for ObjectStore { let name = ecstore::utils::path::dir(item); info!("load svc user: {}", name); if let Err(err) = self.load_user(&name, UserType::Svc, &mut items_cache).await { - return Err(Error::msg(std::format!("load_user failed: {}", err))); + if !is_err_no_such_user(&err) { + return Err(Error::msg(std::format!("load svc user failed: {}", err))); + } }; } diff --git a/iam/src/sys.rs b/iam/src/sys.rs index 6a016708..c22a9d56 100644 --- a/iam/src/sys.rs +++ b/iam/src/sys.rs @@ -197,6 +197,10 @@ impl IamSys { return Err(IamError::IAMActionNotAllowed.into()); } + if opts.expiration.is_none() { + return Err(IamError::InvalidExpiration.into()); + } + // TODO: check allow_site_replicator_account let policy_buf = if let Some(policy) = opts.session_policy { @@ -229,9 +233,13 @@ impl IamSys { } } + // set expiration time default to 1 hour m.insert( "exp".to_string(), - serde_json::Value::Number(serde_json::Number::from(opts.expiration.map_or(0, |t| t.unix_timestamp()))), + serde_json::Value::Number(serde_json::Number::from( + opts.expiration + .map_or(OffsetDateTime::now_utc().unix_timestamp() + 3600, |t| t.unix_timestamp()), + )), ); let (access_key, secret_key) = if !opts.access_key.is_empty() || !opts.secret_key.is_empty() { @@ -385,7 +393,7 @@ impl IamSys { return Ok(()); }; - if u.credentials.is_service_account() { + if !u.credentials.is_service_account() { return Ok(()); } diff --git a/iam/src/utils.rs b/iam/src/utils.rs index 00448fdd..2d301bb9 100644 --- a/iam/src/utils.rs +++ b/iam/src/utils.rs @@ -1,5 +1,5 @@ use ecstore::error::{Error, Result}; -use jsonwebtoken::{encode, Algorithm, DecodingKey, EncodingKey, Header}; +use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey, Header}; use rand::{Rng, RngCore}; use serde::{de::DeserializeOwned, Serialize}; @@ -42,7 +42,7 @@ pub fn gen_secret_key(length: usize) -> crate::Result { pub fn generate_jwt(claims: &T, secret: &str) -> Result { let header = Header::new(Algorithm::HS512); - encode(&header, &claims, &EncodingKey::from_secret(secret.as_bytes())) + jsonwebtoken::encode(&header, &claims, &EncodingKey::from_secret(secret.as_bytes())) } pub fn extract_claims( @@ -58,7 +58,8 @@ pub fn extract_claims( #[cfg(test)] mod tests { - use super::{gen_access_key, gen_secret_key}; + use super::{gen_access_key, gen_secret_key, generate_jwt}; + use serde::{Deserialize, Serialize}; #[test] fn test_gen_access_key() { @@ -76,4 +77,34 @@ mod tests { let b = gen_secret_key(10).unwrap(); assert_ne!(a, b); } + + #[derive(Debug, Serialize, Deserialize, PartialEq)] + struct Claims { + sub: String, + company: String, + } + + #[test] + fn test_generate_jwt() { + let claims = Claims { + sub: "user1".to_string(), + company: "example".to_string(), + }; + let secret = "my_secret"; + let token = generate_jwt(&claims, secret).unwrap(); + + assert!(!token.is_empty()); + } + + // #[test] + // fn test_extract_claims() { + // let claims = Claims { + // sub: "user1".to_string(), + // company: "example".to_string(), + // }; + // let secret = "my_secret"; + // let token = generate_jwt(&claims, secret).unwrap(); + // let decoded_claims = extract_claims::(&token, secret).unwrap(); + // assert_eq!(decoded_claims.claims, claims); + // } } diff --git a/madmin/src/user.rs b/madmin/src/user.rs index 7de50f5b..3a398c51 100644 --- a/madmin/src/user.rs +++ b/madmin/src/user.rs @@ -211,6 +211,7 @@ pub struct UpdateServiceAccountReq { pub new_description: Option, #[serde(rename = "newExpiration", skip_serializing_if = "Option::is_none")] + #[serde(with = "time::serde::rfc3339::option")] pub new_expiration: Option, } diff --git a/rustfs/src/admin/handlers/service_account.rs b/rustfs/src/admin/handlers/service_account.rs index 0af420bd..fbc2e73d 100644 --- a/rustfs/src/admin/handlers/service_account.rs +++ b/rustfs/src/admin/handlers/service_account.rs @@ -53,6 +53,16 @@ impl Operation for AddServiceAccount { .validate() .map_err(|e| S3Error::with_message(InvalidRequest, e.to_string()))?; + let session_policy = if let Some(policy) = &create_req.policy { + let p = Policy::parse_config(policy.as_bytes()).map_err(|e| { + debug!("parse policy failed, e: {:?}", e); + s3_error!(InvalidArgument, "parse policy failed") + })?; + Some(p) + } else { + None + }; + let Some(sys_cred) = get_global_action_cred() else { return Err(s3_error!(InvalidRequest, "get sys cred failed")); }; @@ -95,7 +105,7 @@ impl Operation for AddServiceAccount { name: create_req.name, description: create_req.description, expiration: create_req.expiration, - session_policy: create_req.policy.and_then(|p| Policy::parse_config(p.as_bytes()).ok()), + session_policy, ..Default::default() };