Vars in request context instead of location config; OAuth2Require

[smanolache]

Hello,

I've made several modifications to the code:

The values of the variables were stored in the local config. A request with a valid token set them. A second request passing no token at all would retrieve the values of the previous token because the local config object would be shared among the requests.
The variables are now stored in an ngx_hash in a request context. It is destroyed after each request.

If the request passes no token the previous code still returned NGX_OK. Now we check if there was a OAuth2TokenVerify configuration directive for the location. If yes then we return NGX_HTTP_UNAUTHORIZED. An NGX_OK is returned only if there was no OAuth2TokenVerify configuration directive, meaning that the request is not 'for us'.

I've added the optional configuration directive OAuth2Require. It takes between zero and seven arguments. Every argument is a variable name. The authorization callback returns NGX_OK only if every variable is defined and its value is the string '1'. We can combine the Nginx 'map' configuration directive and the variables that hold token values in order to synthesize the variables used by OAuth2Require. In this way we may check that the token holds desired values, for example that 'iss' matches a given regular expression or that the 'scope' is belongs to a required set.

This new configuration directive is optional. If absent then the module behaves like before, i.e. it returns NGX_OK if the token can be decrypted, parsed, and it is not expired.

I've modelled the configuration directive on auth_jwt_require in the commercial distribution of nginx (https://nginx.org/en/docs/http/ngx_http_auth_jwt_module.html#auth_jwt_require).

Here's how it can be used:

# inside the http block
        map $http_authorization $source_token {
                "~*^Bearer\s+(?<token>[\S]+)$" $token;
        }
        map $pfc_jwks_uri_iss   $valid_issuer {
                https://trusted.issuer.com/  1;
        }
        map $pfc_jwks_uri_scope $valid_scope {
                "~api-my-own-service:readonly"   1;
        }
        
        ...
# inside the location
        OAuth2TokenVerify $source_token jwks_uri https://keys.host/keys;
        OAuth2Claim sub $pfc_jwks_uri_sub;
        OAuth2Claim scope $pfc_jwks_uri_scope;
        OAuth2Claim iss $pfc_jwks_uri_iss;

        OAuth2Require $valid_issuer $valid_scope;

[zandbelt]

thanks, that looks very good indeed; it would be even better if you could pull (most of) that code up to liboauth2, into https://github.com/OpenIDC/liboauth2/blob/master/src/server/nginx.c and https://github.com/OpenIDC/liboauth2/blob/master/include/oauth2/nginx.h so that it is re-usable for ngx_openidc_module as well

[smanolache]

I'll look into that, but I can't do anything before the end of next week.

[zandbelt]

I have adopted the "variables in request context" changes since they're clearly a security risk.

Wrt. OAuth2Require: am I right that it obsoletes OAuth2Claim ? Alternatively, would it be possible to merge the map functionality into OAuth2Claim ?

[pladen]

I don't think it obsoletes OAuth2Claim, as OAuth2Claim is just a mapping between token claim and ngx var.
The "map trick" is just a casual way to make "if" with ngx. There are a lot of ngx config that makes use of that.

OAuth2Require allows to set one (or more) variable values as mandatory to give access to location.
So there is no reason IMHO to merge these directives in any way.
BTW nginx+ jwt module works this way too (jwt claim into variable map into require)

[zandbelt]

@pladen thanks for chiming in, that clarifies things for me a bit; however, it still feels that OAuth2Require doesn't do anything that is specific to OAuth 2.0 or ngx_oauth_module; isn't there (or shouldn't there be) a generic directive that can be applied here, i.e. that matches a variable value against a map result?

[pladen]

Yes, I get your point.
You probably can make something with the "if directive" but it becomes very tricky if you need AND / OR logic because nginx does not have boolean operators (yes !).

When you practice nginx daily, you avoid IF as much a possible because "if is evil" :
https://www.f5.com/company/blog/nginx/avoiding-top-10-nginx-configuration-mistakes#if

So OAuth2Require have sense and this is why @smanolache adds it.
But I understand it would have been better implemented through a generic nginx directive.

To sum up, without OAuth2Require it is possible to just make
if ( $jwt_sub ) { return 403 ; }
But difficult to make something like (following code does not work)
if ( ($jwt_sub) && ($jwt_issuer="myidp") && ($jwt_scope ~= "readaccess") )

So, please, keep it !

[zandbelt]

I have now adopted - I think - all suggested changes with minor modifications and merged them into the liboauth2 master branch, to be released with liboauth2 1.6.3. I'm closing this as any further refinements/comments can be done as part of a new thread/issue/discussion. Looking for your feedback before releasing liboauth2 1.6.3 and ngx_oauth2_module 3.5.0, thanks!

[smanolache]

Thanks for having merged my proposals.

Here's an example of how I combine maps, ifs, and requirements:

In /etc/nginx/nginx.conf, the http section:

map $http_authorization    $source_token {
    "~*^Bearer\s+(?<token>[\S]+)$"    $token;
}

map $zkapi_token_iss $valid_issuer {
    https://trusted.issuer.net/    1;
}

# GET, HEAD
map $zkapi_token_scope $may_read {
    "~api-zkapi-v1-npd:read"       1;
    "~api-zkapi-v1-npd:admin"      1;
}

# PUT
map $zkapi_token_scope $may_create {
    "~api-zkapi-v1-npd:create"     1;
    "~api-zkapi-v1-npd:admin"      1;
}

In the virtual host configuration:

location /zk/v1/ {
    if ($request_method = GET) {
        OAuth2TokenVerify   $source_token jwks_uri https://key.server.net/jwks;
        OAuth2Claim         sub    $zkapi_token_sub;
        OAuth2Claim         scope  $zkapi_token_scope;
        OAuth2Claim         iss    $zkapi_token_iss;

        OAuth2Require       $valid_issuer  $may_read;
    }

    if ($request_method = PUT) {
        OAuth2TokenVerify   $source_token jwks_uri https://key.server.net/jwks;
        OAuth2Claim         sub    $zkapi_token_sub;
        OAuth2Claim         scope  $zkapi_token_scope;
        OAuth2Claim         iss    $zkapi_token_iss;

        OAuth2Require      $valid_issuer  $may_create;
    }
}

OAuth2Claim directives contain a mapping between token fields (sub, scope, iss) and nginx variables zkapi_token_sub, etc). The maps contain a mechanism to set other variables (valid_issuer, may_read, may_create) depending on values of the zkapi_token_* variables. Finally OAuth2Require checks that valid_issuer, may_create, etc are set.

[zandbelt]

thank you for your contribution!

[smanolache]

I see your point about whether OAuth2Require belongs here or not. I think it can be argued both ways.

On one hand one could say that the module provides authentication only, i.e. no authorization. It authenticates the data found in the token. It is up to the application code to authorize the request, to take action depending on the data in the token.

On the other hand one could argue that OAuth2Require factors out the authorization into a common code, alleviating the application codes. It is also flexible enough to cover most application use-cases. And finally, it is only "additive" to the module, i.e. if the OAuth2Require directive is not used then the module behaves as if the requirements code was never added to it, in an authentication-mode only.