[pbs-devel] [PATCH v2 proxmox-backup 1/2] upid: add workaround for parsing broken termproxy userids

[pbs-devel] [PATCH v2 proxmox-backup 1/2] upid: add workaround for parsing broken termproxy userids

[Stefan Reiter]

Commit aafe8609 "d/postinst: fixup userid for older termproxy tasks"
does the fixup after the upgrade, which is fine for CLI upgrades, but
doesn't work for the GUI, as the termproxy instance used for upgrading
will write it's own tasklog (with the still broken version) after the
upgrade and postinst.

Instead, add a (temporary) workaround to the UPID parser to handle this.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2:
* don't just match root

As discussed off-list with Dominik, it's debatable if we want this patch
together with the second one, or if the latter is enough on it's own. It does
fix a breakage, but it adds workaround code, and this is a Beta after all...

 src/server/upid.rs | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/server/upid.rs b/src/server/upid.rs
index 9fc5085b..786bf6ab 100644
--- a/src/server/upid.rs
+++ b/src/server/upid.rs
@@ -107,20 +107,32 @@ impl UPID {
     }
 }
 
+// This is a workaround for a bug which resulted in only the username instead of
+// the userid to be written to the 'active' file for older termproxy tasks
+// FIXME: Remove in future version
+fn parse_userid(worker_type: &str, userid: &str) -> Result<Userid, Error> {
+    if worker_type == "termproxy" && !userid.ends_with("@pam") {
+        format!("{}@pam", userid).parse()
+    } else {
+        userid.parse()
+    }
+}
 
 impl std::str::FromStr for UPID {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         if let Some(cap) = PROXMOX_UPID_REGEX.captures(s) {
+            let worker_type = cap["wtype"].to_owned();
+            let userid = parse_userid(&worker_type, &cap["userid"])?;
             Ok(UPID {
                 pid: i32::from_str_radix(&cap["pid"], 16).unwrap(),
                 pstart: u64::from_str_radix(&cap["pstart"], 16).unwrap(),
                 starttime: i64::from_str_radix(&cap["starttime"], 16).unwrap(),
                 task_id: usize::from_str_radix(&cap["task_id"], 16).unwrap(),
-                worker_type: cap["wtype"].to_string(),
+                worker_type,
                 worker_id: if cap["wid"].is_empty() { None } else { Some(cap["wid"].to_string()) },
-                userid: cap["userid"].parse()?,
+                userid,
                 node: cap["node"].to_string(),
             })
         } else {
-- 
2.20.1

[pbs-devel] [PATCH v2 proxmox-backup 2/2] d/postinst: always fixup termproxy user id and for all users

[Stefan Reiter]

Anyone with a PAM account and Sys.Console access could have started a
termproxy session, adapt the regex.

Always run the sed expression to make sure eventually all occurences of
the broken syntax are fixed.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2: new patch

This one is definitely necessary though, as otherwise broken entries will just
remain forever. This way they'll at least be fixed up eventually at some
upgrade (and for all users, not just root@pam).

 debian/postinst | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/debian/postinst b/debian/postinst
index 9ab78798..bc414ccb 100644
--- a/debian/postinst
+++ b/debian/postinst
@@ -15,12 +15,8 @@ case "$1" in
 	fi
 	deb-systemd-invoke $_dh_action proxmox-backup.service proxmox-backup-proxy.service >/dev/null || true
 
-	if test -n "$2"; then
-		if dpkg --compare-versions "$2" 'le' '0.8.10-1'; then
-			echo "Fixing up termproxy user id in task log..."
-			flock -w 30 /var/log/proxmox-backup/tasks/active.lock sed -i 's/:termproxy::root: /:termproxy::root@pam: /' /var/log/proxmox-backup/tasks/active
-		fi
-	fi
+	echo "Fixing up termproxy user id in task log..."
+	flock -w 30 /var/log/proxmox-backup/tasks/active.lock sed -i 's/:termproxy::\([^@]\+\): /:termproxy::\1@pam: /' /var/log/proxmox-backup/tasks/active
     ;;
 
     abort-upgrade|abort-remove|abort-deconfigure)
-- 
2.20.1

Re: [pbs-devel] [PATCH v2 proxmox-backup 2/2] d/postinst: always fixup termproxy user id and for all users

[Thomas Lamprecht]

On 01.09.20 14:27, Stefan Reiter wrote:
> Anyone with a PAM account and Sys.Console access could have started a
> termproxy session, adapt the regex.
> 
> Always run the sed expression to make sure eventually all occurences of
> the broken syntax are fixed.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v2: new patch
> 
> This one is definitely necessary though, as otherwise broken entries will just
> remain forever. This way they'll at least be fixed up eventually at some
> upgrade (and for all users, not just root@pam).
> 
>  debian/postinst | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 9ab78798..bc414ccb 100644
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -15,12 +15,8 @@ case "$1" in
>  	fi
>  	deb-systemd-invoke $_dh_action proxmox-backup.service proxmox-backup-proxy.service >/dev/null || true
>  
> -	if test -n "$2"; then
> -		if dpkg --compare-versions "$2" 'le' '0.8.10-1'; then
> -			echo "Fixing up termproxy user id in task log..."
> -			flock -w 30 /var/log/proxmox-backup/tasks/active.lock sed -i 's/:termproxy::root: /:termproxy::root@pam: /' /var/log/proxmox-backup/tasks/active
> -		fi
> -	fi
> +	echo "Fixing up termproxy user id in task log..."
> +	flock -w 30 /var/log/proxmox-backup/tasks/active.lock sed -i 's/:termproxy::\([^@]\+\): /:termproxy::\1@pam: /' /var/log/proxmox-backup/tasks/active

I mean, guard it at least with a grep, so that this log and the flock only gets called
when required..

And anyway, this is a beta, why adding already that much legacy handling code
here? I mean if the old fixup worked, OK, that wasn't much, but this is gets
ugly fast, quite inclined to NAK it altogether...

Why not just document (i.e., post it as reply in the forum) the sed command, and
refer to it if a user runs into this?
Much simpler and avoids adding cruft already now.