public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms
@ 2025-11-03 16:26 Samuel Rufinatscha
  2025-11-11 10:40 ` Fabian Grünbichler
  2025-11-14 10:34 ` [pbs-devel] applied: " Fabian Grünbichler
  0 siblings, 2 replies; 4+ messages in thread
From: Samuel Rufinatscha @ 2025-11-03 16:26 UTC (permalink / raw)
  To: pbs-devel

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.

[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"
+    );
+}
+
 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms
  2025-11-03 16:26 [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms Samuel Rufinatscha
@ 2025-11-11 10:40 ` Fabian Grünbichler
  2025-11-11 13:49   ` Samuel Rufinatscha
  2025-11-14 10:34 ` [pbs-devel] applied: " Fabian Grünbichler
  1 sibling, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2025-11-11 10:40 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms
  2025-11-11 10:40 ` Fabian Grünbichler
@ 2025-11-11 13:49   ` Samuel Rufinatscha
  0 siblings, 0 replies; 4+ messages in thread
From: Samuel Rufinatscha @ 2025-11-11 13:49 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Fabian Grünbichler

On 11/11/25 11:40 AM, Fabian Grünbichler wrote:
> 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>
>

Thanks for the review Fabian - makes absolutely sense! I will keep this
in mind for my future patches.
>>
>> [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 ;)
>

Good point! Having tests for the schema constraints would be a great
follow-up and probably good-to-have, also we could move these tests
then.
>> +
>>   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



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [pbs-devel] applied: [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms
  2025-11-03 16:26 [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms Samuel Rufinatscha
  2025-11-11 10:40 ` Fabian Grünbichler
@ 2025-11-14 10:34 ` Fabian Grünbichler
  1 sibling, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-11-14 10:34 UTC (permalink / raw)
  To: pbs-devel, Samuel Rufinatscha


On Mon, 03 Nov 2025 17:26:05 +0100, 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.
> 
> [...]

Applied, thanks!

[1/1] fix #6913: auth-api: fix user ID parsing for 2-character realms
      commit: 2ac004ec5516beb93000544bb709dccd8c48f116

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-11-14 10:34 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-03 16:26 [pbs-devel] [PATCH proxmox] fix #6913: auth-api: fix user ID parsing for 2-character realms Samuel Rufinatscha
2025-11-11 10:40 ` Fabian Grünbichler
2025-11-11 13:49   ` Samuel Rufinatscha
2025-11-14 10:34 ` [pbs-devel] applied: " Fabian Grünbichler

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