public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms
Date: Tue, 11 Nov 2025 11:40:34 +0100	[thread overview]
Message-ID: <1762856246.0k9so820g3.astroid@yuna.none> (raw)
In-Reply-To: <20251103162605.354733-1-s.rufinatscha@proxmox.com>

On November 3, 2025 5:26 pm, Samuel Rufinatscha wrote:
> PVE and PBS both allow creating realms with names of length ≥ 2.
> However, when creating a user, PBS rejected realms with 2 characters
> (e.g. `test@aa`), while PVE accepted them. This issue was reported
> in our bug tracker [1]. Since the issue appears in the underlying
> `proxmox/proxmox-auth-api` crate, also PDM userid handling is
> affected.
> 
> The issue is caused by a mismatch between realm creation and parsing
> rules in `proxmox/proxmox-auth-api`. `REALM_ID_SCHEMA` allows
> min_length(2), but `PROXMOX_AUTH_REALM_STRING_SCHEMA` enforced
> min_length(3).
> 
> This patch lowers the minimum realm length in
> `PROXMOX_AUTH_REALM_STRING_SCHEMA` from 3 to 2 to align PBS and PMG
> with PVE.
> 
> ## Testing
> 
> Please see the attached unit tests.
> The changes were further verified using a rebuilt PBS .deb
> deployment. PDM was tested using a non-package binary through the
> provided client CLI.
> 
> ## Maintainer notes:
> 
> Bump the `proxmox-auth-api` dependency, no breaking change.
> PBS and PDM to use the new dependency.

this part here we'd usually put into the patch notes (the part below the
`---`), which doesn't show up in git history. you can manage those notes
using `git notes ..`, including (if you set your config accordingly),
preserving/merging them across rebases.

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> 
> [1] Bugzilla: https://bugzilla.proxmox.com/show_bug.cgi?id=6913
> 
> Fixes: #6913
> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
> ---
>  proxmox-auth-api/src/types.rs | 68 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
> index 9bde661c..aa09fb93 100644
> --- a/proxmox-auth-api/src/types.rs
> +++ b/proxmox-auth-api/src/types.rs
> @@ -95,7 +95,7 @@ pub const PROXMOX_GROUP_ID_SCHEMA: Schema = StringSchema::new("Group ID")
>  pub const PROXMOX_AUTH_REALM_STRING_SCHEMA: StringSchema =
>      StringSchema::new("Authentication domain ID")
>          .format(&proxmox_schema::api_types::SAFE_ID_FORMAT)
> -        .min_length(3)
> +        .min_length(2)
>          .max_length(32);
>  pub const PROXMOX_AUTH_REALM_SCHEMA: Schema = PROXMOX_AUTH_REALM_STRING_SCHEMA.schema();
>  
> @@ -769,6 +769,72 @@ fn test_token_id() {
>      assert_eq!(auth_id.to_string(), "test@pam!bar".to_string());
>  }
>  
> +#[test]
> +fn test_realm_validation() {
> +    let empty_realm: Result<Realm, _> = "".to_string().try_into();
> +    let one_char_realm: Result<Realm, _> = "a".to_string().try_into();
> +    let two_char_realm: Result<Realm, _> = "aa".to_string().try_into();
> +    let long_realm: Result<Realm, _> = "a".repeat(33).try_into();
> +    let valid_realm: Result<Realm, _> = "pam".to_string().try_into();
> +
> +    assert!(empty_realm.is_err(), "Empty realm should fail validation");
> +    assert!(
> +        one_char_realm.is_err(),
> +        "1-char realm should fail validation"
> +    );
> +    assert!(
> +        two_char_realm.is_ok(),
> +        "2-char realm should pass validation"
> +    );
> +    assert!(valid_realm.is_ok(), "Typical realm should pass validation");
> +    assert!(
> +        long_realm.is_err(),
> +        "Realm >32 chars should fail validation"
> +    );
> +}
> +
> +#[test]
> +fn test_userid_validation() {
> +    let empty_str: Result<Userid, _> = "".parse();
> +    let invalid_no_realm: Result<Userid, _> = "user".parse();
> +    let invalid_empty_realm: Result<Userid, _> = "user@".parse();
> +    let invalid_one_char_realm: Result<Userid, _> = "user@a".parse();
> +    let valid_two_char_realm: Result<Userid, _> = "user@aa".parse();
> +    let valid_long_realm: Result<Userid, _> = "user@pam".parse();
> +    let invalid_long_realm: Result<Userid, _> = format!("user@{}", "a".repeat(33)).parse();
> +    let invalid_empty_username: Result<Userid, _> = "@aa".parse();
> +
> +    assert!(empty_str.is_err(), "Empty userid should fail");
> +    assert!(
> +        invalid_no_realm.is_err(),
> +        "Userid without realm should fail"
> +    );
> +    assert!(
> +        invalid_empty_realm.is_err(),
> +        "Userid with empty realm should fail"
> +    );
> +    assert!(
> +        invalid_one_char_realm.is_err(),
> +        "Userid with 1-char realm should fail"
> +    );
> +    assert!(
> +        valid_two_char_realm.is_ok(),
> +        "Userid with 2-char realm should pass"
> +    );
> +    assert!(
> +        valid_long_realm.is_ok(),
> +        "Userid with normal realm should pass"
> +    );
> +    assert!(
> +        invalid_long_realm.is_err(),
> +        "Userid with realm >32 chars should fail"
> +    );
> +    assert!(
> +        invalid_empty_username.is_err(),
> +        "Userid with empty username should fail"
> +    );
> +}

these two are more or less tests validating our schema deserializer, but
as the types are rather core types they also don't hurt.

AFAICT we don't have in-depth tests in proxmox-schema that verify that
the schema constraints validation actually works as expected, there's
just some basic tests for query parameter handling and schema types
themselves - might be an area worth improving ;)

> +
>  serde_plain::derive_deserialize_from_fromstr!(Userid, "valid user id");
>  serde_plain::derive_serialize_from_display!(Userid);
>  
> -- 
> 2.47.3
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

  reply	other threads:[~2025-11-11 10:39 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-03 16:26 Samuel Rufinatscha
2025-11-11 10:40 ` Fabian Grünbichler [this message]
2025-11-11 13:49   ` Samuel Rufinatscha
2025-11-14 10:34 ` [pbs-devel] applied: " Fabian Grünbichler

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=1762856246.0k9so820g3.astroid@yuna.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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal