From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B7B241FF13E for ; Fri, 20 Feb 2026 13:22:49 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1F72F7216; Fri, 20 Feb 2026 13:23:41 +0100 (CET) Date: Fri, 20 Feb 2026 13:23:33 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH backup 1/2] fix #7054: client: remove trailing newlines from credentials To: Maximiliano Sandoval , pbs-devel@lists.proxmox.com References: <20260220111951.280082-1-m.sandoval@proxmox.com> <20260220111951.280082-2-m.sandoval@proxmox.com> In-Reply-To: <20260220111951.280082-2-m.sandoval@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1771589909.upbjfmjdzy.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1771590205652 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: SNSF3TUJLZ5XF26PXF4M2KXAZPGAHOQ7 X-Message-ID-Hash: SNSF3TUJLZ5XF26PXF4M2KXAZPGAHOQ7 X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On February 20, 2026 12:17 pm, Maximiliano Sandoval wrote: > For repositories and fingerprints we simply strip trailing whitespaces. > For passwords, we refer to the password regex at proxmox-schema: > `^[[:^cntrl:]]*$`, we can only strip trailing control characters without > potentially breaking existing passwords. >=20 > The encryption password is just a blob of bytes handled locally by the > client, we cannot remove trailing whitespace here without potential > breakage. Creation of such passwords (via > proxmox_sys::tty::read_and_verify_password) only verifies valid utf-8 > and len >=3D 5. >=20 > In order to prevent an allocation for credentials that do not need to be > stripped we perform a preemptive check with str::ends_with before > allocating. >=20 > Signed-off-by: Maximiliano Sandoval > --- > pbs-client/src/tools/mod.rs | 11 +++++++++++ > 1 file changed, 11 insertions(+) >=20 > diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs > index 7a496d14c..8dc7c2cd5 100644 > --- a/pbs-client/src/tools/mod.rs > +++ b/pbs-client/src/tools/mod.rs > @@ -169,6 +169,17 @@ fn get_secret_impl(env_variable: &str, credential_na= me: &str) -> Result Ok(Some(password)) > } else if let Some(password) =3D get_credential(credential_name)? { > String::from_utf8(password) nit: while we are here, could we maybe rename this variable? ;) > + .map(|s| { > + if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_= PBS_FINGERPRINT) > + && s.ends_with(char::is_whitespace) the s.ends_with here, and the trim below are redundant, just trim below.. this is not a hot path where the slight performance penalty of trim_end matters, and having two potentially deviating conditions is a source of bugs. > + { > + s.trim_end().to_string() > + } else if credential_name =3D=3D CRED_PBS_PASSWORD && s.= ends_with(char::is_control) { > + s.trim_end_matches(char::is_control).to_string() same here, but this is even more wrong - we should just trim a single newline, not silently drop control character bytes just because they are not allowed in passwords! the restriction of the password schema makes it safe to drop that newline (since it can never be part of the password). > + } else { > + s > + } > + }) > .map(Option::Some) > .map_err(|_err| format_err!("credential {credential_name} is= not utf8 encoded")) > } else { > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20