Lnd: Incorrect HTTP status code when macaroon is not provided over REST API

[Kixunil]

Background

The HTTP error code returned by lnd using REST API is not correct. It returns 500 Internal server error instead of 4xx. This is confusing when debugging.

Your environment

• version of lnd 0.5.1
• which operating system (uname -a on *Nix) Linux <redacted> 3.16.0-4-amd64 #1 SMP Debian 3.16.51-3 (2017-12-13) x86_64 GNU/Linux

Steps to reproduce

• Configure restlisten=127.0.0.1:9737
• Run wget --content-on-error --no-check-certificate -O - https://127.0.0.1:9737/v1/getinfo

Expected behaviour

Error 403 Forbidden or other reasonable 4xx error is returned.

Actual behaviour

500 Internal server error is returned

Ironically, since error 500 means there's something broken with the server, it's strictly true (server sending bad status code), but it doesn't help at all.

[philippgille]

Just adding some info here: I think a 401 would be the most appropriate:

401 Unauthorized
Although the HTTP standard specifies "unauthorized", semantically this response means "unauthenticated". That is, the client must authenticate itself to get the requested response.

Source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#Client_error_responses

More details: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401

The specific difference to 403 is that a 401 means "try again with credentials", while 403 means "I got your credentials but they don't allow you to access the requested resource".

So when no macaroons are sent, the server should respond with "try again with credentials" - i.e. 401.

[Kixunil]

I was thinking about it but many browsers automatically assume basic authentication (but maybe only with some additional header?), so I'm unsure if it's actually suitable.

[philippgille]

I think that depends on the WWW-Authenticate header, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate.

There's a link to a list of alternative standard authentication schemes (alternatives to Basic), but I'm not sure if macaroons fit to any of those: https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml

It seems to be okay to define your own scheme, so maybe Macaroon can be used? See https://tools.ietf.org/html/draft-ietf-httpbis-p7-auth-19#section-4.4.

[Kixunil]

In that case, I agree for it to return 401. It should be made sure to return 403 in case of invalid macaroon.

[wiz]

Ah this really got me today 😓

[philippgille]

I might give this a shot on the weekend, but I might need some guidance for my first code contribution.

[philippgille]

Before I create a PR I'd love to get feedback from someone who knows the LND code base regarding if I'm on the right track:

• LND uses grpc-gateway, it gets started here or here
• grpc-gateway has default mappings for gRPC errors (see here)
  
  
  
  • codes.Unauthenticated maps to http.StatusUnauthorized, codes.PermissionDenied to http.StatusForbidden, only unmapped errors are http.StatusInternalServerError
    
    
    
    
    
    • This indicates that maybe some function in LND doesn't return the correct gRPC error code
    
    
    
  
  

Digging deeper:

• The getinfo RPC with its gateway info is defined here
• The gateway's getinfo error is handled here (which is generated code)
  
  
  
  • Apparently the gateway's error handling can be customized in two ways:
    
    
    
    
    
    1. https://github.com/grpc-ecosystem/grpc-gateway/blob/master/docs/_docs/customizingyourgateway.md#replace-a-response-forwarder-per-method
    
    
    
    2. https://mycodesmells.com/post/grpc-gateway-error-handler
       
       
       
       
       
       
       
       • Example: https://github.com/mycodesmells/golang-examples/blob/master/grpc/cmd/server/main.go#L151
       
       
       
       
    
    
    
  
  
  • Custom gateway error handling might be necessary for a proper WWW-Authenticate header as mentioned in my previous comment
  
  
• Macaroon auth is handled in a single location via gRPC interceptor
  
  
  
  • It is set up here
  
  
  • The required permissions per RPC call are listed here
  
  
  • ValidateMacaroon() is called here
    
    
    
    
    
    • It just returns a regular Go error (that just returns text via its Error() method)
      
      
      
      
      
      
      
      • For example in case of no found macaroon: return fmt.Errorf("expected 1 macaroon, got %d", len(md["macaroon"]))
      
      
      
      
      • In case of an encoding error during hex.DecodeString() the returned error from the stdlib is returned
      
      
      
      
    
    
    
  
  
  • The returned error again is returned as is by the custom grpc.UnaryInterceptor
    
    
    
    
    
    • This seems to be the root cause (gRPC can't know how to map some Go error with only text as content to an authentication error code or something else)
    
    
    
  
  

So the solution seems to be to let ValidateMacaroon() return a Go error via gRPC's status.Error() (see GoDoc) with using the proper gRPC error code (either PermissionDenied or Unauthenticated according to above comments) (see GoDoc), as well as probably customizing the gateway response for the proper WWW-Authenticate header.

I looked for tests that would need to be extended and found TestValidateMacaroon in service_test.go (see here). I didn't find any tests for the gateway yet though.

P.S.: I didn't debug into the code yet, I just went through the code in an editor.

[halseth]

@philippgille Good detective work!

From looking at how grpc intereptors are used, what you might need is for the error from macaroon validation to conform to the GRPCStatus interface: https://github.com/grpc/grpc-go/blob/master/status/status.go#L126

[philippgille]

@halseth: Thanks for letting me know that I'm on the right track!

what you might need is for the error from macaroon validation to conform to the GRPCStatus interface

Yes, my proposal was to use gRPC's status.Error(...):

So the solution seems to be to let ValidateMacaroon() return a Go error via gRPC's status.Error() (see GoDoc)

And according to the GoDoc of status.FromError() that you linked to:

FromError returns a Status representing err if it was produced from this package

This should be exactly what we need.

I'll work on a PR then :)