[pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[Gabriel Goller]

Allow more complex strings for the acr-value when using openid. The
openid documentation only specifies the acr-value *should* be an URI [0].
Implemented a regex that loosely disallows some of the reserved URI
characters specified in the RFC [1].
A value like "urn:mace:incommon:iap:silver" should work.

[0]: https://openid.net/specs/openid-connect-core-1_0.html
[1]: https://www.rfc-editor.org/rfc/rfc2396.txt

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-api-types/src/lib.rs    | 3 +++
 pbs-api-types/src/openid.rs | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 795ff2a6..2668db3e 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -178,6 +178,9 @@ const_regex! {
     /// any identifier command line tools work with.
     pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
 
+    /// Regex that (loosely) matches URIs according to [RFC 2396](https://www.rfc-editor.org/rfc/rfc2396.txt)
+    pub URI_REGEX = r#"^[^\x00-\x1F\x7F <>#"]*$"#;
+
     pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";
 
     pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)$";
diff --git a/pbs-api-types/src/openid.rs b/pbs-api-types/src/openid.rs
index 2c7646a3..049137e6 100644
--- a/pbs-api-types/src/openid.rs
+++ b/pbs-api-types/src/openid.rs
@@ -4,6 +4,7 @@ use proxmox_schema::{api, ApiStringFormat, ArraySchema, Schema, StringSchema, Up
 
 use super::{
     PROXMOX_SAFE_ID_FORMAT, PROXMOX_SAFE_ID_REGEX, REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA,
+    URI_REGEX,
 };
 
 pub const OPENID_SCOPE_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&PROXMOX_SAFE_ID_REGEX);
@@ -24,11 +25,11 @@ pub const OPENID_SCOPE_LIST_SCHEMA: Schema = StringSchema::new("OpenID Scope Lis
     .default(OPENID_DEFAILT_SCOPE_LIST)
     .schema();
 
-pub const OPENID_ACR_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&PROXMOX_SAFE_ID_REGEX);
+pub const OPENID_ACR_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&URI_REGEX);
 
 pub const OPENID_ACR_SCHEMA: Schema =
     StringSchema::new("OpenID Authentication Context Class Reference.")
-        .format(&OPENID_SCOPE_FORMAT)
+        .format(&OPENID_ACR_FORMAT)
         .schema();
 
 pub const OPENID_ACR_ARRAY_SCHEMA: Schema =
-- 
2.43.0

Re: [pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[Thomas Lamprecht]

Am 01/02/2024 um 15:24 schrieb Gabriel Goller:
> Allow more complex strings for the acr-value when using openid. The
> openid documentation only specifies the acr-value *should* be an URI [0].
> Implemented a regex that loosely disallows some of the reserved URI
> characters specified in the RFC [1].
> A value like "urn:mace:incommon:iap:silver" should work.

on it's own the change itself looks relatively OK, what I'm missing is
a more direct reference to what the issue was in the report, as while
your example closely matches that, it would be still great to actually
mention this explicitly.

The other thing is that the reporter writes that it works fine for
Proxmox VE already, so what's the limitation there, does your patch
aligns it to that, and if not it would great to state why you chose
a different approach (which can be fine, but really should be reasoned
about)

> 
> [0]: https://openid.net/specs/openid-connect-core-1_0.html
> [1]: https://www.rfc-editor.org/rfc/rfc2396.txt
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-api-types/src/lib.rs    | 3 +++
>  pbs-api-types/src/openid.rs | 5 +++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 795ff2a6..2668db3e 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -178,6 +178,9 @@ const_regex! {
>      /// any identifier command line tools work with.
>      pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
>  
> +    /// Regex that (loosely) matches URIs according to [RFC 2396](https://www.rfc-editor.org/rfc/rfc2396.txt)
> +    pub URI_REGEX = r#"^[^\x00-\x1F\x7F <>#"]*$"#;

Could be also good to expand on the "loosely" part, maybe also go
for a name like `GENERIC_URI_REGEX`, which might better signal that
this is not what some have with URI (often mistaken as URL) in mind.

Re: [pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[Gabriel Goller]

On Mon Feb 5, 2024 at 4:45 PM CET, Thomas Lamprecht wrote:
> Am 01/02/2024 um 15:24 schrieb Gabriel Goller:
> > [..]
> on it's own the change itself looks relatively OK, what I'm missing is
> a more direct reference to what the issue was in the report, as while
> your example closely matches that, it would be still great to actually
> mention this explicitly.
In the commit messages or as a comment above the regex?
> The other thing is that the reporter writes that it works fine for
> Proxmox VE already, so what's the limitation there, does your patch
> aligns it to that, and if not it would great to state why you chose
> a different approach (which can be fine, but really should be reasoned
> about)
Hmm AFAIK on pve we don't have any limitation at all, it just has to be
a string. It's probably best if I copy the same regex to pve although I
don't want to suddenly have user input rejected.
The pve regex would get stricter, thus it would be a breaking change.
> > 
> > [0]: https://openid.net/specs/openid-connect-core-1_0.html
> > [1]: https://www.rfc-editor.org/rfc/rfc2396.txt
> > 
> > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> > ---
> >  pbs-api-types/src/lib.rs    | 3 +++
> >  pbs-api-types/src/openid.rs | 5 +++--
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> > index 795ff2a6..2668db3e 100644
> > --- a/pbs-api-types/src/lib.rs
> > +++ b/pbs-api-types/src/lib.rs
> > @@ -178,6 +178,9 @@ const_regex! {
> >      /// any identifier command line tools work with.
> >      pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
> >  
> > +    /// Regex that (loosely) matches URIs according to [RFC 2396](https://www.rfc-editor.org/rfc/rfc2396.txt)
> > +    pub URI_REGEX = r#"^[^\x00-\x1F\x7F <>#"]*$"#;
>
> Could be also good to expand on the "loosely" part, maybe also go
> for a name like `GENERIC_URI_REGEX`, which might better signal that
> this is not what some have with URI (often mistaken as URL) in mind.
Yes, fair point.

Re: [pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[Thomas Lamprecht]

Am 06/02/2024 um 09:59 schrieb Gabriel Goller:
> On Mon Feb 5, 2024 at 4:45 PM CET, Thomas Lamprecht wrote:
>> on it's own the change itself looks relatively OK, what I'm missing is
>> a more direct reference to what the issue was in the report, as while
>> your example closely matches that, it would be still great to actually
>> mention this explicitly.
> In the commit messages or as a comment above the regex?

That would IMO go in the commit message, as it's meta information
quite relevant for git history, but rather noise for the current
code.

>> The other thing is that the reporter writes that it works fine for
>> Proxmox VE already, so what's the limitation there, does your patch
>> aligns it to that, and if not it would great to state why you chose
>> a different approach (which can be fine, but really should be reasoned
>> about)
> Hmm AFAIK on pve we don't have any limitation at all, it just has to be
> a string. It's probably best if I copy the same regex to pve although I
> don't want to suddenly have user input rejected.
> The pve regex would get stricter, thus it would be a breaking change.

It would not break existing setups, so it rather would be for adding
a new realm, or updating that part of an existing one only though?

That I'd see OK to go for the `"break" it, release it and see if some
one complains things` route.
E specially as it's not really a strict regex, any complaint would
mean that the PBS one needs to be further relaxed too.

Re: [pbs-devel] [PATCH proxmox-backup] fix #5190: api-types: openid acr format regex

[Gabriel Goller]

Submitted a v2 and a patch on the pve mailing list!