[PATCH datacenter-manager/proxmox 0/4] fix #7747: OpenID: allow non HTTP scheme in redirect URL

[PATCH datacenter-manager/proxmox 0/4] fix #7747: OpenID: allow non HTTP scheme in redirect URL

[Thomas Ellmenreich]

As reported in this bug: [1], PDM's '/access/openid/auth-url' endpoint only
accepts URL's that either have a HTTP or HTTPS scheme. Also as the user
mentions, this is a problem for native app authentication flows that might
want to redirect back to the application after successful authentication.

Since PVE already accepts redirect URLs with any scheme, this series also
extends PDM's validation to allow other schemes.

Implementation
--------------

To avoid duplicated regex strings, part of the original HTTP_URL_REGEX
was refactored out, and then reused for the new URL_REGEX.

Testing
-------

I tested this with a simple curl request to both PDM's and PVE's 'auth-url'
endpoints. Providing a HTTP and non HTTP scheme, to see where the process
fails.

```
curl -o - --data '{"realm":"myrealm","redirect-url":"https://pdm1:8443"}' \
    --header 'Content-Type: application/json;charset=UTF-8' \
    -k https://pdm1:8443/api2/extjs/access/openid/auth-url | jq
```

[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=7747


proxmox:

Thomas Ellmenreich (3):
  api-types: reorganise unit tests
  api-types: refactor HTTP_URL_REGEX construction
  api-types: add a scheme generic URL regex

 pbs-api-types/src/lib.rs        |   1 +
 proxmox-schema/src/api_types.rs | 128 ++++++++++++++++++++++++--------
 2 files changed, 97 insertions(+), 32 deletions(-)


proxmox-datacenter-manager:

Thomas Ellmenreich (1):
  fix #7747: openid: allow non HTTP schemes in redirect URL

 lib/pdm-api-types/src/lib.rs    | 1 +
 server/src/api/access/openid.rs | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)


Summary over all repositories:
  4 files changed, 100 insertions(+), 36 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH proxmox 1/4] api-types: reorganise unit tests

[Thomas Ellmenreich]

In preparation for extending the api_types tests, reorganzie them into
their own module. Also, split the large test into several smaller ones
to make them clearer.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 proxmox-schema/src/api_types.rs | 76 +++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/proxmox-schema/src/api_types.rs b/proxmox-schema/src/api_types.rs
index d6a0608c..553eb054 100644
--- a/proxmox-schema/src/api_types.rs
+++ b/proxmox-schema/src/api_types.rs
@@ -270,35 +270,49 @@ pub const DISK_LIST_SCHEMA: Schema = StringSchema::new("A list of disk names, co
     .format(&ApiStringFormat::PropertyString(&DISK_ARRAY_SCHEMA))
     .schema();
 
-#[test]
-fn test_regexes() {
-    assert!(IP_REGEX.is_match("127.0.0.1"));
-    assert!(IP_V4_REGEX.is_match("127.0.0.1"));
-    assert!(!IP_V6_REGEX.is_match("127.0.0.1"));
-
-    assert!(CIDR_V4_REGEX.is_match("127.0.0.1/24"));
-    assert!(CIDR_REGEX.is_match("127.0.0.1/24"));
-
-    assert!(IP_REGEX.is_match("::1"));
-    assert!(IP_REGEX.is_match("2014:b3a::27"));
-    assert!(IP_REGEX.is_match("2014:b3a::192.168.0.1"));
-    assert!(IP_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
-    assert!(!IP_V4_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
-    assert!(IP_V6_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
-
-    assert!(CIDR_V6_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF/60"));
-    assert!(CIDR_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF/60"));
-
-    assert!(IP_BRACKET_REGEX.is_match("127.0.0.1"));
-    assert!(IP_BRACKET_REGEX.is_match("[::1]"));
-    assert!(IP_BRACKET_REGEX.is_match("[2014:b3a::27]"));
-    assert!(IP_BRACKET_REGEX.is_match("[2014:b3a::192.168.0.1]"));
-    assert!(IP_BRACKET_REGEX.is_match("[2014:b3a:0102:adf1:1234:4321:4afA:BCDF]"));
-
-    assert!(ED25519_BASE64_KEY_REGEX.is_match("KNpc7alqlLTaWE6RzuzHGioKs7Nqh/z3YxMJojpSelA="));
-    assert!(!ED25519_BASE64_KEY_REGEX.is_match(""));
-    // 31 bytes of data
-    assert!(!ED25519_BASE64_KEY_REGEX.is_match("6zroXbjGs9sdOpr1n/M5hh+UklBxtQ90tGQDnYzJfw=="));
-    // 33 bytes of data
-    assert!(!ED25519_BASE64_KEY_REGEX.is_match("IiC3Nkh4Fn2ukUZUNmdK5K5CWO53Zmk/eGlKO4m6aCD/"));
+#[cfg(test)]
+mod tests {
+    use super::{
+        CIDR_REGEX, CIDR_V4_REGEX, CIDR_V6_REGEX, ED25519_BASE64_KEY_REGEX, IP_BRACKET_REGEX,
+        IP_REGEX, IP_V4_REGEX, IP_V6_REGEX,
+    };
+
+    #[test]
+    fn test_ip_regexes() {
+        assert!(IP_REGEX.is_match("127.0.0.1"));
+        assert!(IP_V4_REGEX.is_match("127.0.0.1"));
+        assert!(!IP_V6_REGEX.is_match("127.0.0.1"));
+
+        assert!(IP_REGEX.is_match("::1"));
+        assert!(IP_REGEX.is_match("2014:b3a::27"));
+        assert!(IP_REGEX.is_match("2014:b3a::192.168.0.1"));
+        assert!(IP_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
+        assert!(!IP_V4_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
+        assert!(IP_V6_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF"));
+
+        assert!(IP_BRACKET_REGEX.is_match("127.0.0.1"));
+        assert!(IP_BRACKET_REGEX.is_match("[::1]"));
+        assert!(IP_BRACKET_REGEX.is_match("[2014:b3a::27]"));
+        assert!(IP_BRACKET_REGEX.is_match("[2014:b3a::192.168.0.1]"));
+        assert!(IP_BRACKET_REGEX.is_match("[2014:b3a:0102:adf1:1234:4321:4afA:BCDF]"));
+    }
+
+    #[test]
+    fn test_cidr_regexes() {
+        assert!(CIDR_V4_REGEX.is_match("127.0.0.1/24"));
+        assert!(CIDR_REGEX.is_match("127.0.0.1/24"));
+
+        assert!(CIDR_V6_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF/60"));
+        assert!(CIDR_REGEX.is_match("2014:b3a:0102:adf1:1234:4321:4afA:BCDF/60"));
+    }
+
+    #[test]
+    fn test_ed25519_regex() {
+        assert!(ED25519_BASE64_KEY_REGEX.is_match("KNpc7alqlLTaWE6RzuzHGioKs7Nqh/z3YxMJojpSelA="));
+        assert!(!ED25519_BASE64_KEY_REGEX.is_match(""));
+        // 31 bytes of data
+        assert!(!ED25519_BASE64_KEY_REGEX.is_match("6zroXbjGs9sdOpr1n/M5hh+UklBxtQ90tGQDnYzJfw=="));
+        // 33 bytes of data
+        assert!(!ED25519_BASE64_KEY_REGEX.is_match("IiC3Nkh4Fn2ukUZUNmdK5K5CWO53Zmk/eGlKO4m6aCD/"));
+    }
 }
-- 
2.47.3

[PATCH proxmox 2/4] api-types: refactor HTTP_URL_REGEX construction

[Thomas Ellmenreich]

In preparation for the addition of a new URL regex, refactor out the
non scheme part of the regex to a separate constant.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 proxmox-schema/src/api_types.rs | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/proxmox-schema/src/api_types.rs b/proxmox-schema/src/api_types.rs
index 553eb054..81878dbe 100644
--- a/proxmox-schema/src/api_types.rs
+++ b/proxmox-schema/src/api_types.rs
@@ -50,6 +50,15 @@ pub const CIDR_V6_REGEX_STR: &str = concatcp!(r"(?:", IPV6RE_STR, r"/\d{1,3})$")
 #[rustfmt::skip]
 pub const SAFE_ID_REGEX_STR: &str = r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)";
 
+/// Regular expression string to match a host, optional port and optionally a
+/// path, although the path remains completely unvalidated except a few
+/// prohibited characters.
+///
+/// The host can either be a DNS name, bracketed IP or, when without port, a
+/// full IPv6
+#[rustfmt::skip]
+pub const URL_HOST_PORT_PATH_REGEX_STR: &str = concatcp!("(?:(?:(?:", DNS_NAME_STR, "|", IPRE_BRACKET_STR, ")(?::", PORT_REGEX_STR ,")?)|", IPV6RE_STR,")(?:/[^\x00-\x1F\x7F]*)?");
+
 #[rustfmt::skip]
 pub const DNS_LABEL_STR: &str = r"(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
 
@@ -103,7 +112,10 @@ const_regex! {
     pub DNS_ALIAS_REGEX = concatcp!(r"^", DNS_ALIAS_NAME_STR, r"$");
     pub DNS_NAME_OR_IP_REGEX = concatcp!(r"^(?:", DNS_NAME_STR, "|",  IPRE_STR, r")$");
     pub HOST_PORT_REGEX = concatcp!(r"^(?:", DNS_NAME_STR, "|", IPRE_BRACKET_STR, "):", PORT_REGEX_STR ,"$");
-    pub HTTP_URL_REGEX = concatcp!(r"^https?://(?:(?:(?:", DNS_NAME_STR, "|", IPRE_BRACKET_STR, ")(?::", PORT_REGEX_STR ,")?)|", IPV6RE_STR,")(?:/[^\x00-\x1F\x7F]*)?$");
+
+    /// A specialisation of [`URL_REGEX`] regex that only allows http and
+    /// https as the URL scheme
+    pub HTTP_URL_REGEX = concatcp!(r"^https?://", URL_HOST_PORT_PATH_REGEX_STR, "$");
 
     /// Regex to match SHA256 Digest.
     pub SHA256_HEX_REGEX = r"^[a-f0-9]{64}$";
-- 
2.47.3

[PATCH proxmox 3/4] api-types: add a scheme generic URL regex

[Thomas Ellmenreich]

The current URL regex, HTTP_URL_REGEX, is specific to the HTTP and HTTPS
schemes. This patch creates a new regex that is scheme-generic and thus
allows any other valid scheme.

It also adds tests that cover some basic URL edge cases.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 pbs-api-types/src/lib.rs        |  1 +
 proxmox-schema/src/api_types.rs | 40 ++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 21ef733d..924f356b 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -44,6 +44,7 @@ pub use proxmox_schema::api_types::HTTP_URL_SCHEMA;
 pub use proxmox_schema::api_types::MULTI_LINE_COMMENT_SCHEMA;
 pub use proxmox_schema::api_types::NODE_SCHEMA;
 pub use proxmox_schema::api_types::SINGLE_LINE_COMMENT_FORMAT;
+pub use proxmox_schema::api_types::URL_SCHEMA;
 pub use proxmox_schema::api_types::{
     BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA, BLOCKDEVICE_NAME_SCHEMA,
 };
diff --git a/proxmox-schema/src/api_types.rs b/proxmox-schema/src/api_types.rs
index 81878dbe..05e50f26 100644
--- a/proxmox-schema/src/api_types.rs
+++ b/proxmox-schema/src/api_types.rs
@@ -50,6 +50,10 @@ pub const CIDR_V6_REGEX_STR: &str = concatcp!(r"(?:", IPV6RE_STR, r"/\d{1,3})$")
 #[rustfmt::skip]
 pub const SAFE_ID_REGEX_STR: &str = r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)";
 
+/// Regular expression string to match allowed URL schemes.
+#[rustfmt::skip]
+pub const URL_SCHEME_REGEX_STR: &str = r"(?:[a-zA-Z][a-zA-Z0-9\+.\-]*)";
+
 /// Regular expression string to match a host, optional port and optionally a
 /// path, although the path remains completely unvalidated except a few
 /// prohibited characters.
@@ -113,6 +117,12 @@ const_regex! {
     pub DNS_NAME_OR_IP_REGEX = concatcp!(r"^(?:", DNS_NAME_STR, "|",  IPRE_STR, r")$");
     pub HOST_PORT_REGEX = concatcp!(r"^(?:", DNS_NAME_STR, "|", IPRE_BRACKET_STR, "):", PORT_REGEX_STR ,"$");
 
+    /// Matches URL constructed out of a scheme, host, port, and path. With
+    /// the host possibly being an IPv[4|6] address and the port being
+    /// optional. The path remains completely unvalidated except for a few
+    /// prohibited characters.
+    pub URL_REGEX = concatcp!(r"^", URL_SCHEME_REGEX_STR, "://", URL_HOST_PORT_PATH_REGEX_STR, "$");
+
     /// A specialisation of [`URL_REGEX`] regex that only allows http and
     /// https as the URL scheme
     pub HTTP_URL_REGEX = concatcp!(r"^https?://", URL_HOST_PORT_PATH_REGEX_STR, "$");
@@ -166,6 +176,7 @@ pub const SYSTEMD_DATETIME_FORMAT: ApiStringFormat =
 pub const HOSTNAME_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&HOSTNAME_REGEX);
 pub const HOST_PORT_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&HOST_PORT_REGEX);
 pub const HTTP_URL_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&HTTP_URL_REGEX);
+pub const URL_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&URL_REGEX);
 
 pub const DNS_ALIAS_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&DNS_ALIAS_REGEX);
 pub const DNS_NAME_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&DNS_NAME_REGEX);
@@ -243,6 +254,10 @@ pub const PORT_SCHEMA: Schema = IntegerSchema::new("Node port")
     .maximum(65535)
     .schema();
 
+pub const URL_SCHEMA: Schema = StringSchema::new("Url with optional port.")
+    .format(&URL_FORMAT)
+    .schema();
+
 pub const HTTP_URL_SCHEMA: Schema = StringSchema::new("HTTP(s) url with optional port.")
     .format(&HTTP_URL_FORMAT)
     .schema();
@@ -286,7 +301,7 @@ pub const DISK_LIST_SCHEMA: Schema = StringSchema::new("A list of disk names, co
 mod tests {
     use super::{
         CIDR_REGEX, CIDR_V4_REGEX, CIDR_V6_REGEX, ED25519_BASE64_KEY_REGEX, IP_BRACKET_REGEX,
-        IP_REGEX, IP_V4_REGEX, IP_V6_REGEX,
+        IP_REGEX, IP_V4_REGEX, IP_V6_REGEX, URL_REGEX,
     };
 
     #[test]
@@ -327,4 +342,27 @@ mod tests {
         // 33 bytes of data
         assert!(!ED25519_BASE64_KEY_REGEX.is_match("IiC3Nkh4Fn2ukUZUNmdK5K5CWO53Zmk/eGlKO4m6aCD/"));
     }
+
+    #[test]
+    fn test_url_regex_on_valid_urls() {
+        assert!(URL_REGEX.is_match("https://www.example.com/index.html"));
+        assert!(URL_REGEX.is_match("https://[2001:db8::1]/docs/%E2%9C%93.html?lang=en"));
+        assert!(URL_REGEX.is_match("ftp://ftp.example.com/pub/archive/file.zip"));
+        assert!(URL_REGEX.is_match("file://test.com/home/alice/documents/report.pdf"));
+        assert!(URL_REGEX.is_match("https://example.com:443/a%20file.html?name=John%20Doe#top"));
+    }
+
+    #[test]
+    fn test_url_regex_on_invalid_urls() {
+        // is missing the ':' after the scheme
+        assert!(!URL_REGEX.is_match("https//www.example.com/index.html"));
+        // has two port numbers
+        assert!(!URL_REGEX.is_match("http://example.com:80:90/page"));
+        // only has one '/' after the scheme
+        assert!(!URL_REGEX.is_match("ftp:/ftp.example.com/file.txt"));
+        // has userinfo (technically correct, but not accepted by the regex)
+        assert!(!URL_REGEX.is_match("mailto:alice@example.com"));
+        // missing closing angle ']' brachet
+        assert!(!URL_REGEX.is_match("https://[2001:db8::1/path"));
+    }
 }
-- 
2.47.3

[PATCH datacenter-manager 4/4] fix #7747: openid: allow non HTTP schemes in redirect URL

[Thomas Ellmenreich]

Loosen validation on redirect URLs for OpenID authentication to also allow
other valid schemes that are not HTTP or HTTPS.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7747
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 lib/pdm-api-types/src/lib.rs    | 1 +
 server/src/api/access/openid.rs | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/pdm-api-types/src/lib.rs b/lib/pdm-api-types/src/lib.rs
index 89d1b4ad..6ddeab5d 100644
--- a/lib/pdm-api-types/src/lib.rs
+++ b/lib/pdm-api-types/src/lib.rs
@@ -71,6 +71,7 @@ pub use proxmox_schema::api_types::HTTP_URL_SCHEMA;
 pub use proxmox_schema::api_types::MULTI_LINE_COMMENT_SCHEMA;
 pub use proxmox_schema::api_types::NODE_SCHEMA;
 pub use proxmox_schema::api_types::SINGLE_LINE_COMMENT_FORMAT;
+pub use proxmox_schema::api_types::URL_SCHEMA;
 pub use proxmox_schema::api_types::{
     BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA, BLOCKDEVICE_NAME_SCHEMA,
 };
diff --git a/server/src/api/access/openid.rs b/server/src/api/access/openid.rs
index 5048fde3..caa065f3 100644
--- a/server/src/api/access/openid.rs
+++ b/server/src/api/access/openid.rs
@@ -18,9 +18,7 @@ use proxmox_router::{
 use proxmox_schema::{ApiType, ObjectSchema, ParameterSchema, StringSchema, api};
 use proxmox_sortable_macro::sortable;
 
-use pdm_api_types::{
-    HTTP_URL_SCHEMA, OPENID_DEFAULT_SCOPE_LIST, OpenIdRealmConfig, REALM_ID_SCHEMA,
-};
+use pdm_api_types::{OPENID_DEFAULT_SCOPE_LIST, OpenIdRealmConfig, REALM_ID_SCHEMA, URL_SCHEMA};
 use pdm_buildcfg::PDM_RUN_DIR_M;
 
 use crate::auth;
@@ -252,7 +250,7 @@ fn create_ticket_http_only(
                 schema: REALM_ID_SCHEMA,
             },
             "redirect-url": {
-                schema: HTTP_URL_SCHEMA,
+                schema: URL_SCHEMA,
             },
         },
     },
-- 
2.47.3