public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox v2] login: use `ticket` if both it and `ticket_info` are provided
@ 2025-10-01 13:11 Shannon Sterz
  2025-10-01 15:02 ` Christian Ebner
  0 siblings, 1 reply; 4+ messages in thread
From: Shannon Sterz @ 2025-10-01 13:11 UTC (permalink / raw)
  To: pbs-devel

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>
---
 proxmox-login/src/lib.rs | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/proxmox-login/src/lib.rs b/proxmox-login/src/lib.rs
index 4482f2e4..fd4ca850 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 {
-- 
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 v2] login: use `ticket` if both it and `ticket_info` are provided
  2025-10-01 13:11 [pbs-devel] [PATCH proxmox v2] login: use `ticket` if both it and `ticket_info` are provided Shannon Sterz
@ 2025-10-01 15:02 ` Christian Ebner
  2025-10-02  7:50   ` Shannon Sterz
  0 siblings, 1 reply; 4+ messages in thread
From: Christian Ebner @ 2025-10-01 15:02 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Shannon Sterz

On 10/1/25 3:11 PM, Shannon Sterz wrote:
> 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>
> ---

Only one question: Should the `SecondFactorChallenge::response_bytes()` 
also prioritize `ticket`  over `ticket_info` to get the same flow behavior?

Other than that the changes look good to me, as they do follow the same 
intend of parsing the full ticket I did try
to force via the compat mode in 
https://lore.proxmox.com/pdm-devel/DD70FPP8GIBO.2K8CWBW5XPL0K@proxmox.com/T/

Tested ticket parsing and request authentication works with this patch 
applied by using PDM remote add wizard (which uses the proxmox-login) 
for the following version:
- PBS 3.4.0 (fresh install from ISO), 3.4.6, 4.0.11 (fresh install from 
ISO), 4.0.15
- PVE 8.4.0 (fresh install from ISO), 8.4.14, 9.0.3 (fresh install from 
ISO), 9.0.10

Please consider:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@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

* Re: [pbs-devel] [PATCH proxmox v2] login: use `ticket` if both it and `ticket_info` are provided
  2025-10-01 15:02 ` Christian Ebner
@ 2025-10-02  7:50   ` Shannon Sterz
  2025-10-02  7:57     ` [pbs-devel] Superseded: " Shannon Sterz
  0 siblings, 1 reply; 4+ messages in thread
From: Shannon Sterz @ 2025-10-02  7:50 UTC (permalink / raw)
  To: Christian Ebner, Proxmox Backup Server development discussion

On Wed Oct 1, 2025 at 5:02 PM CEST, Christian Ebner wrote:
> On 10/1/25 3:11 PM, Shannon Sterz wrote:
>> 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>
>> ---
>
> Only one question: Should the `SecondFactorChallenge::response_bytes()`
> also prioritize `ticket`  over `ticket_info` to get the same flow behavior?

yes sorry that was an oversight on my end, will send a v3 in a minute.

> Other than that the changes look good to me, as they do follow the same
> intend of parsing the full ticket I did try
> to force via the compat mode in
> https://lore.proxmox.com/pdm-devel/DD70FPP8GIBO.2K8CWBW5XPL0K@proxmox.com/T/
>
> Tested ticket parsing and request authentication works with this patch
> applied by using PDM remote add wizard (which uses the proxmox-login)
> for the following version:
> - PBS 3.4.0 (fresh install from ISO), 3.4.6, 4.0.11 (fresh install from
> ISO), 4.0.15
> - PVE 8.4.0 (fresh install from ISO), 8.4.14, 9.0.3 (fresh install from
> ISO), 9.0.10
>
> Please consider:
>
> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
> Tested-by: Christian Ebner <c.ebner@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

* [pbs-devel] Superseded: Re: [PATCH proxmox v2] login: use `ticket` if both it and `ticket_info` are provided
  2025-10-02  7:50   ` Shannon Sterz
@ 2025-10-02  7:57     ` Shannon Sterz
  0 siblings, 0 replies; 4+ messages in thread
From: Shannon Sterz @ 2025-10-02  7:57 UTC (permalink / raw)
  To: Shannon Sterz, Christian Ebner,
	Proxmox Backup Server development discussion

Superseded-by: https://lore.proxmox.com/all/DD7O7RLF50HZ.1NAU5VCE3AE7A@proxmox.com/T/#t

On Thu Oct 2, 2025 at 9:50 AM CEST, Shannon Sterz wrote:
> On Wed Oct 1, 2025 at 5:02 PM CEST, Christian Ebner wrote:
>> On 10/1/25 3:11 PM, Shannon Sterz wrote:
>>> 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>
>>> ---
>>
>> Only one question: Should the `SecondFactorChallenge::response_bytes()`
>> also prioritize `ticket`  over `ticket_info` to get the same flow behavior?
>
> yes sorry that was an oversight on my end, will send a v3 in a minute.
>
>> Other than that the changes look good to me, as they do follow the same
>> intend of parsing the full ticket I did try
>> to force via the compat mode in
>> https://lore.proxmox.com/pdm-devel/DD70FPP8GIBO.2K8CWBW5XPL0K@proxmox.com/T/
>>
>> Tested ticket parsing and request authentication works with this patch
>> applied by using PDM remote add wizard (which uses the proxmox-login)
>> for the following version:
>> - PBS 3.4.0 (fresh install from ISO), 3.4.6, 4.0.11 (fresh install from
>> ISO), 4.0.15
>> - PVE 8.4.0 (fresh install from ISO), 8.4.14, 9.0.3 (fresh install from
>> ISO), 9.0.10
>>
>> Please consider:
>>
>> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
>> Tested-by: Christian Ebner <c.ebner@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-10-02  7:57 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-10-01 13:11 [pbs-devel] [PATCH proxmox v2] login: use `ticket` if both it and `ticket_info` are provided Shannon Sterz
2025-10-01 15:02 ` Christian Ebner
2025-10-02  7:50   ` Shannon Sterz
2025-10-02  7:57     ` [pbs-devel] Superseded: " Shannon Sterz

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