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

[Tim Marx]

> 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

> 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.

> > +			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) = @_;
> >  
> >