mORMot Open Source

ynoga

Member

Registered: 2020-06-17

Posts: 2

Some more THttpAPIServer

Hello,

After fiddling a bit with THttpAPIServer (and more specifically the authentication part), we've noticed several potential "pitfall" that seems worth mentioning.

type
  THttpServerRequest = class
  protected
    fRemoteIP, fURL, fMethod, fInHeaders, fInContent, fInContentType,
    fAuthenticatedUser, fOutContent, fOutContentType, fOutCustomHeaders: SockString;
    [...] 


procedure THttpApiServer.Execute;
[...]
var 
    [...]
    Context: THttpServerRequest;
    [...]
begin 
    [...]
    Context := THttpServerRequest.Create(self,0,self);
    // main loop reusing a single Context instance for this thread
    ReqID := 0;
    Context.fServer := self;
    repeat
      [...]
      // retrieve next pending request, and read its headers
      fillchar(Req^,sizeof(HTTP_REQUEST),0);
      Err := Http.ReceiveHttpRequest(fReqQueue,ReqID,0,Req^,length(ReqBuf),bytesRead);
      [...]
        // retrieve any SetAuthenticationSchemes() information
        if byte(fAuthenticationSchemes)<>0 then // set only with HTTP API 2.0
          for i := 0 to Req^.V2.RequestInfoCount-1 do
          if Req^.V2.pRequestInfo^[i].InfoType=HttpRequestInfoTypeAuth then
            with PHTTP_REQUEST_AUTH_INFO(Req^.V2.pRequestInfo^[i].pInfo)^ do
            case AuthStatus of
            HttpAuthStatusSuccess:
            if AuthType>HttpRequestAuthTypeNone then begin
              byte(Context.fAuthenticationStatus) := ord(AuthType)+1;
              if AccessToken<>0 then
                GetDomainUserNameFromToken(AccessToken,Context.fAuthenticatedUser);

1) Context.fAuthenticatedUser

Context.fAuthenticatedUser and fAuthenticationStatus are set once - when the authentication challenge has been completed - but not reseted between each request (context is created outside of the repeat loop). As Req^.V2.pRequestInfo^.pInfo.AuthStatus is not saved in context, an unsuspecting application relying only on those value might make false asumption about which user has been authentified.

Resetting both Context.fAuthenticationStatus and Context.fAuthenticatedUser at the beginning of each request seems reasonable from our point of view. Do you think it'll be wise to do so ?

    // main loop reusing a single Context instance for this thread
    ReqID := 0;
    Context.fServer := self;
    repeat
        Context.fAuthenticationStatus := hraNone;
        Context.fAuthenticatedUser := '';

2) Potential handle leaks

According to spec https://docs.microsoft.com/en-us/window … ersion-2-0, "Authentication Procedure",
The access token obtained from HttpReceiveHttpRequest() lifecycle is application responsability and must be closed by the caller.
"7) The application sends the final 200 OK response, and it must close the handle to the access token."

As we make no use of it outside of THttpApiServer.Execute, closing the access token just after retrieving domain user name seems reasonable

            HttpAuthStatusSuccess:
            if AuthType>HttpRequestAuthTypeNone then begin
              byte(Context.fAuthenticationStatus) := ord(AuthType)+1;
              if AccessToken<>0 then begin
                GetDomainUserNameFromToken(AccessToken,Context.fAuthenticatedUser);
                CloseHandle(AccessToken);
              end;

I can make a pull request for both items, for 1 or for none depending on your opinion.
Thanks a lot for mORMot,
Regards,

HgAlexx

Member

Registered: 2020-06-10

Posts: 6

Re: Some more THttpAPIServer

For context, this is one of my teammate, working on the same project as me

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,659

Website

Re: Some more THttpAPIServer

Both your remarks seem to make sense.
From your testings, do they work as expected?

Please submit a pull request.

ynoga

Member

Registered: 2020-06-17

Posts: 2

Re: Some more THttpAPIServer

Hello !

Thanks a lot for looking into this. Modification is in place since a few days, and we've not seen any adverse effects due to those modifications in our environment.
I'll send you the PR.
Regards,

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,659

Website

Re: Some more THttpAPIServer

We didn't get any feedback about this authentication part of http.sys - most of our users are following mpv advice and use mORMot-level authentication.
So I guess if it is OK for you, it is fine for us.