labstack / echo

High performance, minimalist Go web framework
https://echo.labstack.com
MIT License
29.4k stars 2.21k forks source link

KeyAuth with no token prefix in Authorization header #2503

Closed bigunmd closed 1 year ago

bigunmd commented 1 year ago

Issue Description

Greets!

KeyAuth middleware is not supporting Auth Scheme where Authorizaiton header contains token without prefix. For instance, PASETO based flow does not requre to specify "Bearer " type prefix for token. In case of existing KeyAuth middleware passing an empty string or empty space string to AuthScheme configuration parameter does not solve the problem.

Btw, using X-Api-Key type of header is not constrained to such behaviour and works just fine without token prefixes.

Could you please provide a way to bypass such behaviour for Authorization header? Thanks in advance.

Actual behaviour

package middleware

// .......

// KeyAuthWithConfig returns an KeyAuth middleware with config.
// See `KeyAuth()`.
func KeyAuthWithConfig(config KeyAuthConfig) echo.MiddlewareFunc {
    // Defaults
    if config.Skipper == nil {
        config.Skipper = DefaultKeyAuthConfig.Skipper
    }
    // Defaults <------ CAN NOT DEFINE EMPTY PREFIX AUTH SCHEME!
    if config.AuthScheme == "" {
        config.AuthScheme = DefaultKeyAuthConfig.AuthScheme
    }
    if config.KeyLookup == "" {
        config.KeyLookup = DefaultKeyAuthConfig.KeyLookup
    }
    if config.Validator == nil {
        panic("echo: key-auth middleware requires a validator function")
    }
// .....

And then other shenanigans appear to happen here, in the extractros func, where is support for backward compat is added

package middleware

// .......

func createExtractors(lookups string, authScheme string) ([]ValuesExtractor, error) {
    if lookups == "" {
        return nil, nil
    }
    sources := strings.Split(lookups, ",")
    var extractors = make([]ValuesExtractor, 0)
    for _, source := range sources {
        parts := strings.Split(source, ":")
        if len(parts) < 2 {
            return nil, fmt.Errorf("extractor source for lookup could not be split into needed parts: %v", source)
        }

        switch parts[0] {
        case "query":
            extractors = append(extractors, valuesFromQuery(parts[1]))
        case "param":
            extractors = append(extractors, valuesFromParam(parts[1]))
        case "cookie":
            extractors = append(extractors, valuesFromCookie(parts[1]))
        case "form":
            extractors = append(extractors, valuesFromForm(parts[1]))
        case "header":
            prefix := ""
            if len(parts) > 2 {
                prefix = parts[2]
            } else if authScheme != "" && parts[1] == echo.HeaderAuthorization {
                // backwards compatibility for JWT and KeyAuth:
                // * we only apply this fix to Authorization as header we use and uses prefixes like "Bearer <token-value>" etc
                // * previously header extractor assumed that auth-scheme/prefix had a space as suffix we need to retain that
                //   behaviour for default values and Authorization header.
                prefix = authScheme
                if !strings.HasSuffix(prefix, " ") {
                    prefix += " "
                }
            }
            extractors = append(extractors, valuesFromHeader(parts[1], prefix))
        }
    }
    return extractors, nil
}
aldas commented 1 year ago

probably easiest and quickest way is to use custom middleware that works like keyauth. No need to figth current implementation

    e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
        return func(c echo.Context) error {
            auth := c.Request().Header.Get(echo.HeaderAuthorization)
            if auth == "" {
                return echo.ErrUnauthorized
            }
            // same as "KeyAuthValidator func(auth string, c echo.Context) (bool, error)" implementation would do
            if auth != "IsNotStoredInSomewhere" { // fictional check from store
                return echo.ErrUnauthorized
            }
            return next(c)
        }
    })
bigunmd commented 1 year ago

Got it, thanks. This is the way. Was just surprised with such things.