[pve-devel] [RFC PATCH http-server 1/1] allow ticket in auth header as fallback

[Thomas Lamprecht]

On 9/2/19 2:15 PM, Tim Marx wrote:
> 
>> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 2. September 2019 13:31 geschrieben:
>>
>>  
>> It would be great to have a better commit message, or better said:
>> one at all. You cover-letter looks like it could be a good fit for
>> that here, almost as-is.
>>
> 
> I expected this to get some feedback as it is RFC, therefore I wrote all thoughts into the cover letter, but will move the discussion result to the commit message. I thought it would be convenient to have all things summed up in the cover letter for now.
> 
>> On 8/30/19 2:12 PM, Tim Marx wrote:
>>> Signed-off-by: Tim Marx <t.marx at proxmox.com>
>>> ---
>>>  PVE/APIServer/AnyEvent.pm  |  5 +++++
>>>  PVE/APIServer/Formatter.pm | 12 ++++++++++++
>>>  2 files changed, 17 insertions(+)
>>>
>>> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
>>> index 2e8ca47..c8f7b6d 100644
>>> --- a/PVE/APIServer/AnyEvent.pm
>>> +++ b/PVE/APIServer/AnyEvent.pm
>>> @@ -1223,6 +1223,11 @@ sub unshift_read_header {
>>>  		    my $cookie = $r->header('Cookie');
>>>  		    my $ticket = PVE::APIServer::Formatter::extract_auth_cookie($cookie, $self->{cookie_name});
>>>  
>>> +		    if (!defined $ticket) {
>>
>> please use parenthesis with defined checks, e.g.:
>> if (!defined($ticket)) {
>>
> 
> ok
> 
>> You could also do:
>>
>> if (!defined($ticket) && (my $authHeader = $r->header('Authorization'))) {
>>
>> Here, but a bit longer / more nested, so just a suggestion.
>>
>> Not sure about the change from "=" to space " " as a separation.
>> "Authorization" is specified in detail by any ietf or similar RFC,
>> but it seems that it has some basic rules set in place [0].
>>
>> [0]: https://tools.ietf.org/html/rfc7235#section-4.2
>>
> 
> This isn't something specific to the Authorization header, it's defined in the various token schemes e.g. bearer etc. As said, I just opted for the most common schemes on web, which all include a space between type and token. I'm not insisting on it, this wouldn't be a blocker for me, but I had new contributors in mind. It's a simple change which doesn't hurt us and maybe saves someone 5 minutes of debugging, because he assumed a space.
> 
> In addition most other schemes use the "=" in the token part of the auth header, which makes the differentiation between scheme type and actual token more cumbersome.
> 
> Reference for example and other schemes:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

Yes, I found that link, it pointed me to the IETF hosted RFC linked above ;)

> 
>> So with we being the server, and knowing that proxies may not alter
>> this field we could just use the same format as the ticket and reuse
>> the method. Albeit, I'd rename it then to "extract_auth_ticket" or
>> the like.
>>
>> If you find anything that specifies reasons for why a space separation
>> would be better here, or if you just insist on it, we could also extend
>> the regex to allow either " " or "=" as separator and still reuse the
>> same (renamed) "extract_auth_ticket" method. Something like:
>>
>> my $ticket = ($auth_header =~ /(?:^|\s)\Q$type\E(?:[ =])([^;]*)/)[0];
>>
>> should do the trick (did not tested it).
>>
>> Rest looks OK for me.
>>
> 
> We can argue about the name, as always :P
> I would prefer two distinct functions, this way we don't have to think about the cookie stuff if we change something specific to the auth header, but if you insist I will merge them.

As we need to stay backward compatible this won't change often. 
And AFAICT, both the existing, and your proposed method, have nothing to do with
cookie, or headers. They just match a string (independent from where it comes) and
return the ticket, if found, so IMO a unified way, without _cookie or _header in the
name would be a bit nicer, easier to understand code.

You could then even do something like:

my $auth_token = $r->header('Cookie') // $r->header('Authorization');
my $ticket = PVE::APIServer::Formatter::extract_auth_ticket($auth_token, $self->{cookie_name});

and be done. IMO quite a bit nicer.

Single slight oddity is the "$self->{cookie_name}", here one could rename it too (with early class
setup fallback) or just life with it (as you do in this patch).

> 
>>> +			my $authHeader = $r->header('Authorization');
>>> +			$ticket = PVE::APIServer::Formatter::extract_ticket_from_auth_header($authHeader, $self->{cookie_name});
>>> +		    }
>>> +
>>>  		    my ($rel_uri, $format) = &$split_abs_uri($path, $self->{base_uri});
>>>  		    if (!$format) {
>>>  			$self->error($reqstate, HTTP_NOT_IMPLEMENTED, "no such uri");
>>> diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm
>>> index 0c459bd..f626180 100644
>>> --- a/PVE/APIServer/Formatter.pm
>>> +++ b/PVE/APIServer/Formatter.pm
>>> @@ -87,6 +87,18 @@ sub extract_auth_cookie {
>>>      return $ticket;
>>>  }
>>>  
>>> +sub extract_ticket_from_auth_header {
>>> +    my ($auth_header, $type) = @_;
>>> +
>>> +    return undef if !$auth_header;
>>> +
>>> +    my $ticket = ($auth_header =~ /(?:^|\s)\Q$type\E ([^;]*)/)[0];
>>> +
>>> +    $ticket = uri_unescape($ticket) if $ticket;
>>> +
>>> +    return $ticket;
>>> +}
>>> +
>>>  sub create_auth_cookie {
>>>      my ($ticket, $cookie_name) = @_;
>>>  
>>>