KeyAuth Middleware perhaps should return 500 when validator return an error

[hyacinthus]

type KeyAuthValidator func(string, echo.Context) (bool, error)

• token not found -> 400
• validator return false -> 401
• validator return error -> 500, or just return it to center error handler, it is 401 now
• validator return true -> ok, next

Now I can't throw a ServerError when validate tokens.

[aldas]

This is not all correct. You have to check in what case valid, err := config.Validator(key, c) here
returns an error.

For example

echo/middleware/key_auth.go

Line 116 in bb7f222

func keyFromHeader(header string, authScheme string) keyExtractor {

returns error when there is no header for key or header is with not matching auth scheme. Those are cases when client is doing something that is not allowed. 500 is more for cases when server does something unexpected.

[hyacinthus]

@aldas Validator is totally user defined, when

valid, err := config.Validator(key, c)

is reached, the middleware have got a key.

Now the first bool return value means the key is valid or not. If the key is "invalid key", like this line

echo/middleware/key_auth.go

Line 104 in bb7f222

Message: "invalid key",

said, I should just return false,nil.

When I return an error, it's must be a server error, like db is down.

[hyacinthus]

If middleware just pass this error to c.Error(), everything is all right too.

[aldas]

you are correct extractor(c) and config.Validator(key, c) are in different. my bad. I read wrong lines.

I'm still personally convinced that it should be 401. For security related stuff it is better to return by default unambitious 401 as it would make attacker harder to blackbox application logic/state. If you really want to know what that original error then just take it form httpError

he : = err.(echo.HttpErrror)
if he.Code == http.StatusUnauthorized && he.Internal != nil {
...

[hyacinthus]

@aldas

I've done like your suggesting before

https://github.com/hack-fan/x/blob/e6250b0fb2bdd4a755c1a6a8633b8d44a46242cf/xecho/error.go#L24-L33

but feel a bit stupid myself...

[aldas]

well, you could send PR to add errorhandler as options for this middleware. something similar to

echo/middleware/jwt.go

Line 213 in bb7f222

if config.ErrorHandler != nil {

and 401 would be sane default when develop did not choose to customize error handling

[hyacinthus]

Thank you, I'll try it.

[aldas]

@aldas

I've done like your suggesting before

https://github.com/hack-fan/x/blob/e6250b0fb2bdd4a755c1a6a8633b8d44a46242cf/xecho/error.go#L24-L33

but feel a bit stupid myself...

I would advise against extracting those errors and sending then to the client. It is totally OK and even advisable to log them but you are exposing your internals to unauthorized clients. Lets imagine that you use header value on SQL query and do not escape it properly - you could be exposing/leaking information about sql injection to the attacker. Or you return third party library errors directly and from them attack can deduce library and version you are potentially using and use vulnerabilities from them.

[hyacinthus]

@aldas Yes, thank you, I should not extract errors to client when it is not debug mode.

I've change it better.

https://github.com/hack-fan/x/blob/74256a10bcb97ac8ca8eb45c2e40ff28fb480c27/xecho/error.go#L26-L30