[pbs-devel] [PATCH proxmox v3] login: use `ticket` if both it and `ticket_info` are provided

[Shannon Sterz <s.sterz@proxmox.com>]

previously the precense of `ticket_info` was assumed to indicate the
HTTPOnly authentication flow. the `ticket` field was ignore in that
case, because the client has no way of validating a ticket anyway.

this commit changes the behaviour to assume that the server is not
trying to "trick us" and that the presence of a `ticket` field
indicates that this value should be used for authentication. if the
`ticket_info` field is also present, it will be ignored.

this fixes an issue where authentication against later versions of
proxmox-backup-server 3.4 failed. including versions up to and
including version 3.4.6-1.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---

changes since v2, thanks @ Christian Ebner
* also prefer the ticket field in
  `SecondFactorChallenge::response_bytes()`

changes since v1:
* rebase on current master

 proxmox-login/src/lib.rs | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/proxmox-login/src/lib.rs b/proxmox-login/src/lib.rs
index 4482f2e4..4b2869a7 100644
--- a/proxmox-login/src/lib.rs
+++ b/proxmox-login/src/lib.rs
@@ -198,22 +198,24 @@ impl Login {
             ));
         }

-        // `ticket_info` is set when the server sets the ticket via an HttpOnly cookie. this also
-        // means we do not have access to the cookie itself which happens for example in a browser.
-        // assume that the cookie is handled properly by the context (browser) and don't worry
-        // about handling it ourselves.
-        if let Some(ref ticket) = response.ticket_info {
-            let ticket = ticket.parse()?;
-            return Ok(TicketResult::HttpOnly(
-                self.authentication_for(ticket, response)?,
-            ));
-        }
-
         // old authentication flow where we needed to handle the ticket ourselves even in the
         // browser etc.
         let ticket: TicketResponse = match response.ticket {
             Some(ref ticket) => ticket.parse()?,
-            None => return Err("no ticket information in response".into()),
+            None => {
+                // `ticket_info` is set when the server sets the ticket via a HttpOnly cookie. this
+                // also means we do not have access to the cookie itself which happens for example
+                // in a browser. assume that the cookie is handled properly by the context
+                // (browser) and don't worry about handling it ourselves.
+                if let Some(ref ticket) = response.ticket_info {
+                    let ticket = ticket.parse()?;
+                    return Ok(TicketResult::HttpOnly(
+                        self.authentication_for(ticket, response)?,
+                    ));
+                }
+
+                return Err("no ticket information in response".into());
+            }
         };

         Ok(match ticket {
@@ -384,15 +386,15 @@ impl SecondFactorChallenge {

         // get the ticket from:
         // 1. the cookie if possible -> new HttpOnly authentication outside of the browser
-        // 2. just the `ticket_info` -> new HttpOnly authentication inside a browser context or
-        //    similar, assume the ticket is handle by that
-        // 3. the `ticket` field -> old authentication flow where we handle the ticket ourselves
+        // 2. the `ticket` field -> old authentication flow where we handle the ticket ourselves
+        // 3. if there is no `ticket` field, check if we have a `ticket_info` field -> new HttpOnly
+        //    authentication inside of a browser (or similar context) that handles the ticket for us
         let ticket: Ticket = cookie_ticket
             .ok_or(ResponseError::from("no ticket in response"))
             .or_else(|e| {
                 response
-                    .ticket_info
-                    .or(response.ticket)
+                    .ticket
+                    .or(response.ticket_info)
                     .ok_or(e)
                     .and_then(|t| t.parse().map_err(|e: TicketError| e.into()))
             })?;
--
2.47.3



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