Verification of claims about lua-resty-jwt HOT 22 CLOSED

Also - as I'm thinking about it, it seems weird to me to say (in code) is_not_before(leeway). It seems to me that the leeway maybe should also be set on a per-instance basis...because what you are saying is "I'm willing to be within this margin of error on my server"...which, to me, is "instance" - not "function".

from lua-resty-jwt.

Another change I'd like to propose is in the naming of the validators. Instead of (for example) naming the optional one equals_any_of and the required one required_equals_any_of, I would propose the optional one (which, IMO, will be used less) be named opt_equals_any_of and the required one (the more common case) be named equals_any_of. "Required" is kind of implied, I think.

from lua-resty-jwt.

sounds like my proposal combined with @fermaem 's.
have a little discusstion at #19 , check it out

from lua-resty-jwt.

Looks great - I'd love to join in the discussion and/or help put together something that would also be a bit more flexible (i.e. to add the ability to validate a subject, or something else application-specific). Would that be best to do on #19 or would it be better to work up some kind of proposal (in the form of a PR) and carry on the discussion in a new issue (or this issue)?

from lua-resty-jwt.

hmm...
let's discuss in this issue 🎉

from lua-resty-jwt.

Agreed that this now makes the validators table look kind of weird. I would almost say that we replace the timeframe and iss validator options with some exposed functions that can be used just as a table of claims.

from lua-resty-jwt.

yep, i agree.

hi @fermaem , any thoughts?

from lua-resty-jwt.

I'm not sure that every claim validation should be allowed to be overridden now (ie. YAGNI): For instance, the lifecycle validator. On the other hand, it may make sense to provide the user with a callback for him/her to feed/override the base validator with some additional data.

@toonetown Any chance you could wrap up a client API proposal mock?

from lua-resty-jwt.

WARNING: Super long post follows.

I actually really like the simplicity of the API that is used by the auth function by https://github.com/auth0/nginx-jwt (which, BTW, uses this project for their underlying JWT work). Particularly, I really like both the readability and simplicity of defining claims to be checked as a table that matches the claims themselves. i.e.:

{
    sub = "^[a-z]+$",
    iss = function(val)
        for _, value in pairs({ "first", "second" }) do
            if value == val then return true end
        end
        return false
    end
}

The auth0/nginx-jwt project contains a single helper function - table_contains that would allow you to simplify this second statement thusly:

{
    iss = function(val) return nginx_jwt.table_contains({ "first", "second" }, val) end
}

I would, however, propose to simplify the API even one step further by defining it as this:

local verified = jwt:verify_jwt_obj(key, jwt_obj [, claim_spec])
-- OR --
local verified = jwt:verify(key, jet_token [, claim_spec])

From a purely "readability" point of view, this tells me "verify the object with the given key against the given claim_spec."

I would define a claim_spec as a lua table where each key in the table is the name of a claim which is being verified. The value corresponding to each key is a function which has the following signature:

function(val, claim, jwt_obj)

Where val is the value of the claim from jwt_obj (or nil if it doesn't exist), claim is the name of the claim that is being verified, and jwt_obj is the object that is being verified. This function would return either true or false (That is, I would simplify the API a bit from what auth0/nginx-jwt has done and remove the ability to specify a string value). Any validator MAY raise an error, and the validation will be treated as a failure, and the error that was raised will be put into the reason field of the resulting object.

Then, I would provide a library of "validator" functions that would return an appropriate validator function. Since I would anticipate this set of validator functions, I might suggest placing them in a separate file and module (jwt-validators.lua in this example). One could then verify a token (or alternatively, a loaded object using verify_jwt_obj) by doing:

local jwt = require "resty.jwt"
local validators = require "resty.jwt_validators"
local leeway = 300

local verified = jwt:verify(key, jwt_token, {
    sub = validators.required_matches("^[a-z]+$"),
    iss = validators.required_equals_any_of({ "first", "second" }),
    nbf = validators.required_not_before(leeway),
    exp = validators.required_expiration(leeway),
    myclaim = validators.required(),
    ...
})

Or, put another way, what is now defined as:

local jwt_obj = jwt:verify(key, jwt_token,
    {
        lifetime_grace_period = 120,
        require_exp_claim = true,
        valid_issuers = { "my-trusted-issuer", "my-other-trusteed-issuer" },
        claims = { sub = "My Test Subject" }
    }
)

Could be done as:

local jwt_obj = jwt:verify(key, jwt_token,
    {
        exp = validators.required_expiration(120)
        iss = validators.required_equals_any_of({ "my-trusted-issuer", "my-other-trusteed-issuer" }),
        sub = validators.required_equals("My Test Subject")
    }
)

It just seems a bit clearer what is trying to be verified, and what is happening. It also cleans up code which uses this library a bit and makes it more readable.

Suggested functions to include in the jwt_validators module (all of these functions would actually return a validator function - one that takes a single parameter and returns true or false if they match):

--[[
    Returns a validator that returns false if a value doesn't exist.  If
    the value exists and a chain_function is specified, then the value of 
        chain_function(val, claim, jwt_obj)
    will be returned, otherwise, true will be returned.  This allows for 
    specifying that a value is both required *and* it must match some 
    additional check.  This function will be used in the "required_*" shortcut
    functions for simplification.
]]-
function jwt_validators.required([chain_function])

--[[
    Returns a validator that checks if a value exactly equals the given check_value.
    If the value is nil, then this check succeeds.
]]-
function jwt_validators.equals(check_val)

--[[
    Returns a validator that checks if a value matches the given pattern.  If the
    value is nil, then this check succeeds.
]]-
function jwt_validators.matches(pattern)

--[[
    Returns a validator that checks if a value exactly equals any of the given values.
    If the value is nil, then this check succeeds.
]]-
function jwt_validators.equals_any_of(check_values)

--[[
    Returns a validator that checks if a value matches any of the given patterns.
    If the value is nil, then this check succeeds.
]]-
function jwt_validators.matches_any_of(check_patterns)

--[[
    Returns a validator that checks if a date is not before the current time (within
    the given optional leeway).  If leeway is not specified, it defaults to 0.  An
    optional date_value can also be given - which is helpful for writing unit tests.
    If date_value is not specified, then the current time is used.  If the value is 
    nil, then this check succeeds.
]]--
function jwt_validators.not_before([leeway [, date_value]])

--[[
    Returns a validator that checks if the current time is equal to or before
    a date (within the given optional leeway).  If leeway is not specified, it defaults 
    to 0.  An optional date_value can also be given - which is helpful for writing 
    unit tests.  If date_value is not specified, then the current time is used.  If the
    value is nil, then this check succeeds.
]]--
function jwt_validators.expiration([leeway [, date_value]])

--[[
    These functions are just shortcuts to prevent the need to type, for example:
    jwt_validators.required(jwt_validators.equals(check_val)).  They take the same
    parameters as their non-required counterparts (i.e required_equals_any_of takes the
    same parameters as equals_any_of).
]]--
function jwt_validators.required_equals(check_val)
function jwt_validators.required_matches(pattern)
function jwt_validators.required_equals_any_of(check_values)
function jwt_validators.required_matches_any_of(check_patterns)
function jwt_validators.required_not_before([leeway [, date_value]])
function jwt_validators.required_expiration([leeway [, date_value]])

I think that this will provide flexibility as well as ease of use of the library. It also seems like it simplifies things within the library so that there are fewer places for potential bugs. Also, the functions provided by the validators library can be expanded upon and improved over time.

I'd appreciate hearing any feedback or suggestions about this that you would have. I'm also willing to code this up and submit it as a PR if it would be easier to evaluate in that format.

from lua-resty-jwt.

It might even be nice to have the verify functions take multiple claim_spec parameters, and it merges them all together into a single claim_spec to use (later ones override earlier-specified ones). Then, an adapter function could be provided in jwt_validators that returns an appropriate claim_spec which takes validation_options (as they are defined currently surrounding lifecycle). For example:

--[[
    Returns a `claim_spec` table follows the lifetime-specific validation_options
]]--
function jwt_validators.lifetime_spec(validation_options)

You could then do something like this:

local jwt_obj = jwt:verify(key, jwt_token, validators.lifetime_spec(
    {
        lifetime_grace_period = 120,
        require_exp_claim = true
    }
))

Or, to use the full above example:

local jwt_obj = jwt:verify(key, jwt_token,
    validators.lifetime_spec({
        lifetime_grace_period = 120,
        require_exp_claim = true
    }), {
        iss = validators.required_equals_any_of({ "my-trusted-issuer", "my-other-trusteed-issuer" }),
        sub = "My Test Subject"
    }
)

That's just a thought. I kind of like the ability to merge multiple claim_spec tables.

from lua-resty-jwt.

local jwt_obj = jwt:verify(key, jwt_token,
    {
        exp = validators.required_expiration(120)
        iss = validators.required_equals_any_of({ "my-trusted-issuer", "my-other-trusteed-issuer" }),
        sub = validators.required_equals("My Test Subject")
    }
)

i personally like the readability and the power to control everything.
but @fermaem got's his point, this lib should be easy to use without digging into complex manuals.

how about we add a function that takes current validate_options and produce a validators table shown above?
user can still easily control the validation behaviors by using the validate_options without worrying compromising basic validations.
but they can also gain full power by creating their own validators table directly.

from lua-resty-jwt.

@SkyLothar
how about we add a function that takes current validate_options and produce a validators table shown above?
user can still easily control the validation behaviors by using the validate_options without worrying compromising basic validations.
but they can also gain full power by creating their own validators table directly.

That is the route that I am planning on going. I have the first bit of it in place, but I'm working under the following assumptions:

1. If you pass one of the verify functions a single claim_spec
2. AND If that claim spec has at least one of the following keys:
   • lifetime_grace_period
   • require_nbf_claim
   • require_exp_claim
   • valid_issuers
   • claims (see below)
3. THEN it will operate in "legacy" mode (I don't care about the naming...that's just what I've called it so far).

When in "legacy" mode, the functions behave identically to how they are behaving right now.

I would like to fully remove the claims support of triggering "legacy" mode...I'm the one who added it in #25 - and I'm fine with backing out that change in favor of this one.

In the PR so far, I have "deprecated" legacy mode...and it logs a warning when you use it. That is mainly for me in doing testing and seeing how things are working. My goals are to support the current unit tests without modification (except for possibly the error strings that are matched against), and to add as many new unit tests as needed to test the feature completely.

from lua-resty-jwt.

Cool, let's do this. 🎉

from lua-resty-jwt.

Ok. Hopefully I'll be able to finish up PR #26 tomorrow.

from lua-resty-jwt.

Wow! That's a very detailed proposal! Thanks a lot for that! 💯 I like the general idea and the in-depth analysis.

Few thoughts, though:

• I'm not really sold on the optional date_value parameter. I can't think of a real life use_case when one would want to set this value. However, it's possible it's been define for the sole purpose of make the testing easier. Would that be the case, how would you feel about (maybe) setting this at the instance level? (eg. jwt.set_system_clock(function() { return 123456789 } end), default behavior being to return now?)
• Would we want to allow the user to plug in his/her own validators, it may be interesting to define a general purpose callback signature. Maybe something along the lines of bool function is_valid(jwt_obj, claim_name, claim_value). This way, would the validation of a claim depend on another claim value, the implementer would be able to inspect the jwt_obj. (FWIW, being a tad paranoid, I'd favor passing a deeply cloned instance of jwt_obj to the callback rather than the instance by itself to avoid any potential side effects on the object itself).

Thoughts?

from lua-resty-jwt.

@fermaem
I'm not really sold on the optional date_value parameter. I can't think of a real life use_case when one would want to set this value. However, it's possible it's been define for the sole purpose of make the testing easier. Would that be the case, how would you feel about (maybe) setting this at the instance level?

I think that's a great idea - I was trying to figure out how to handle it, and I like that approach better. I'll add it to the tasks

@fermaem
Would we want to allow the user to plug in his/her own validators, it may be interesting to define a general purpose callback signature. Maybe something along the lines of bool function is_valid(jwt_obj, claim_name, claim_value).

That is almost what I proposed for the callback signature - except for reversing the order of the arguments (claim_value, claim_name, jwt_obj) - because in my view, the most common action a validator is going to take is to use the claim_value...so a validator author can take a shortcut and just define their function as taking the claim_value only, since that is all they are interested in.

As a side note, I also figured it might be good to be able to define a "whole object" validator...that is - one that's not tied to a claim, but is tied to the jwt_obj itself. I don't know how to specify these (probably with a key of __jwt_obj__ or something that is very unlikely to be used as a "real" claim name). In that case, the claim_value could either be nil...or we could pass the entire obj in as the claim_value as well...I haven't worked out that part yet.

@fermaem
FWIW, being a tad paranoid, I'd favor passing a deeply cloned instance of jwt_obj to the callback rather than the instance by itself to avoid any potential side effects on the object itself

I'm glad you mentioned that - because it was something that I hadn't even considered...but you are 100% correct! We definitely don't want validators changing the jwt_obj...so passing in a deep clone is a good idea.

For performance reasons, what would you think about passing the same deep copy to all the validators? I don't know that it would be a good idea to deep copy the object for every validator. That way, the original jwt_obj would be preserved...but there would still be the possibility of mutating the object from one function to another.

Another option is we could pass (to all validators) a json-encoded jwt_obj...that way, in the (IMO, less than common) scenario where a validator actually needs access to the jwt_obj, then that validator can take the performance hit of decoding the json string to get access to what it needs. That way, the more common scenario will not have as much of a performance hit (I'd assume that a single json.encode call is roughly the same performance time as a single deep copy) - but it would also have the "immutability" that making a deep copy for each validator call would have.

Thoughts?

from lua-resty-jwt.

Just a status update (aside from what I've been tracking on #26) - I am most of the way finished with updating the legacy options to using the new code, but I am not quite there. I know that @SkyLothar said you were going to do a release this weekend, but perhaps you could hold off until the PR is finished (depending on my schedule, it might be this weekend, but it might also not be ready until Monday)

from lua-resty-jwt.

Thanks for your efforts. These are great proposals!

from lua-resty-jwt.

PR #26 is ready to be merged once it has been reviewed/checked for errors. I have no more plans for changes other than anything that comes up in the review process.

Would you prefer I squash the commits into a single one and resubmit as a separate PR? I don't know if it is easier for you to review as separate commits or as a single commit.

from lua-resty-jwt.

Separate pr is great.
I can squash them into one when i merge the pr.

from lua-resty-jwt.

Done - see #27 for a single-commit version #26 has the commits separated.

from lua-resty-jwt.

#27 has been merged.

from lua-resty-jwt.