From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: [pbs-devel] applied: [PATCH v3 proxmox-backup 1/2] access: limit editing pam credentials to superuser
Date: Fri, 15 Jan 2021 08:52:11 +0100 [thread overview]
Message-ID: <1610696077.f5m2htfx2v.astroid@nora.none> (raw)
In-Reply-To: <20210113162615.1258366-1-o.bektas@proxmox.com>
with fixup removing unnecessary ref/deref and moving the realm lookup
back before saving the config, as it is our safeguard against bogus
realms:
diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index d9458524..f03389f8 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -236,17 +236,20 @@ pub fn create_user(
config.set_data(user.userid.as_str(), "user", &user)?;
+ let realm = user.userid.realm();
+
+ // Fails if realm does not exist!
+ let authenticator = crate::auth::lookup_authenticator(realm)?;
+
user::save_config(&config)?;
if let Some(password) = password {
let user_info = CachedUserInfo::new()?;
let current_auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
- let target_realm = &userZZ.userid.realm();
- if *target_realm == "pam" && !user_info.is_superuser(¤t_auth_id) {
+ if realm == "pam" && !user_info.is_superuser(¤t_auth_id) {
bail!("only superuser can edit pam credentials!");
}
- let authenticator = crate::auth::lookup_authenticator(target_realm)?;
- authenticator.store_password(&user.userid.name(), &password)?;
+ authenticator.store_password(user.userid.name(), &password)?;
}
Ok(())
On January 13, 2021 5:26 pm, Oguz Bektas wrote:
> modifying @pam users credentials should be only possible for root@pam,
> otherwise it can have unintended consequences.
>
> also enforce the same limit on user creation (except self_service check,
> since it makes no sense during user creation)
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v2->v3:
> * apply restrictions for 'create_user' as well
> * make if condition more readable with variables
> * fix issue with regular pam users being unable to edit their own
> passwords (self_service)
>
>
> src/api2/access/user.rs | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 484919bf..d9458524 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -218,7 +218,11 @@ pub fn list_users(
> },
> )]
> /// Create new user.
> -pub fn create_user(password: Option<String>, param: Value) -> Result<(), Error> {
> +pub fn create_user(
> + password: Option<String>,
> + param: Value,
> + rpcenv: &mut dyn RpcEnvironment
> +) -> Result<(), Error> {
>
> let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
>
> @@ -230,14 +234,19 @@ pub fn create_user(password: Option<String>, param: Value) -> Result<(), Error>
> bail!("user '{}' already exists.", user.userid);
> }
>
> - let authenticator = crate::auth::lookup_authenticator(&user.userid.realm())?;
> -
> config.set_data(user.userid.as_str(), "user", &user)?;
>
> user::save_config(&config)?;
>
> if let Some(password) = password {
> - authenticator.store_password(user.userid.name(), &password)?;
> + let user_info = CachedUserInfo::new()?;
> + let current_auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> + let target_realm = &user.userid.realm();
> + if *target_realm == "pam" && !user_info.is_superuser(¤t_auth_id) {
> + bail!("only superuser can edit pam credentials!");
> + }
> + let authenticator = crate::auth::lookup_authenticator(target_realm)?;
> + authenticator.store_password(&user.userid.name(), &password)?;
> }
>
> Ok(())
> @@ -350,6 +359,7 @@ pub fn update_user(
> email: Option<String>,
> delete: Option<Vec<DeletableProperty>>,
> digest: Option<String>,
> + rpcenv: &mut dyn RpcEnvironment,
> ) -> Result<(), Error> {
>
> let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> @@ -392,6 +402,13 @@ pub fn update_user(
> }
>
> if let Some(password) = password {
> + let user_info = CachedUserInfo::new()?;
> + let current_auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> + let self_service = current_auth_id.user() == &userid;
> + let target_realm = userid.realm();
> + if !self_service && target_realm == "pam" && !user_info.is_superuser(¤t_auth_id) {
> + bail!("only superuser can edit pam credentials!");
> + }
> let authenticator = crate::auth::lookup_authenticator(userid.realm())?;
> authenticator.store_password(userid.name(), &password)?;
> }
> --
> 2.20.1
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
prev parent reply other threads:[~2021-01-15 7:52 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-13 16:26 [pbs-devel] " Oguz Bektas
2021-01-13 16:26 ` [pbs-devel] [PATCH v3 proxmox-backup 2/2] access: restrict password changes on @pam realm " Oguz Bektas
2021-01-15 7:52 ` [pbs-devel] applied: " Fabian Grünbichler
2021-01-15 7:52 ` Fabian Grünbichler [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1610696077.f5m2htfx2v.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox