Posts by ynoga

ynoga · mORMot 1

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,

ynoga · mORMot 1

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,

mORMot Open Source

#1 Re: mORMot 1 » Some more THttpAPIServer » 2020-06-17 15:02:49

#2 mORMot 1 » Some more THttpAPIServer » 2020-06-17 09:08:48

Board footer