open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.5k stars 1.32k forks source link

Feature request: encode JWT timestamps without exponent #6013

Closed kenjenkins closed 1 year ago

kenjenkins commented 1 year ago

What is the underlying problem you're trying to solve?

We (Pomerium) are currently using OPA to generate JWTs, using io.jwt.encode_sign(). We've received a user complaint that the iat and exp timestamps in these JWTs are formatted as a floating point value (e.g. 1.683060782e+09) rather than the more common integer format (1683060782). This apparently causes an error in some JWT parsing libraries.

I don't see any easy way to tell io.jwt.encode_sign() to format these timestamps as plain integers (without a decimal point and exponent).

I do see that there is an io.jwt.encode_sign_raw() function, but it's not clear to me how we can use it, as the json.marshal() function exhibits the same behavior:

$ opa eval -fpretty --stdin <<EOF
> now := round(time.now_ns() / 1e9)
> five_minutes := now + (60 * 5)
> payload := json.marshal({
>     "iat": now,
>     "exp": five_minutes,
> })
> EOF
+-----------------+-----------------+-----------------------------------------------------+
|  five_minutes   |       now       |                       payload                       |
+-----------------+-----------------+-----------------------------------------------------+
| 1.686777838e+09 | 1.686777538e+09 | "{\"exp\":1.686777838e+09,\"iat\":1.686777538e+09}" |
+-----------------+-----------------+-----------------------------------------------------+

Describe the ideal solution

Change the behavior of the io.jwt.encode_sign() function to format timestamps as plain integers, without decimal point and exponent.

Describe a "Good Enough" solution

Provide some mechanism to control the serialization format of integers in the json.marshal() function.

I think we might be able to use a workaround like to_number(format_int(now, 10)), but this feels very hacky to me:

$ opa eval -fpretty --stdin <<EOF
> now := round(time.now_ns() / 1e9)
> five_minutes := now + (60 * 5)
> payload := json.marshal({
>     "iat": to_number(format_int(now, 10)),
>     "exp": to_number(format_int(five_minutes, 10)),
> })
> EOF
+-----------------+-----------------+-------------------------------------------+
|  five_minutes   |       now       |                  payload                  |
+-----------------+-----------------+-------------------------------------------+
| 1.686778339e+09 | 1.686778039e+09 | "{\"exp\":1686778339,\"iat\":1686778039}" |
+-----------------+-----------------+-------------------------------------------+

Any guidance would be appreciated.

Additional Context

User issue in the Pomerium repository: https://github.com/pomerium/pomerium/issues/4149

ashutosh-narkar commented 1 year ago

@kenjenkins can you please provide an example of how to repro this. If you look at this example and say replace the exp field, the resulting JWT seems to have the right format. Maybe I'm missing something here.

kenjenkins commented 1 year ago

Hi @ashutosh-narkar, thanks for the reply. From my testing, if we do any arithmetic with an integer value (e.g. round(time.now_ns() / 1e9)), then the resulting JWT will have the floating point format.

In the example you linked from the documentation, if I change the "exp" value to an arithmetic expression (e.g. 1300819380 + 5), the resulting JWT payload then has the floating point format:

io.jwt.encode_sign({
    "typ": "JWT",
    "alg": "HS256"
}, {
    "iss": "joe",
    "exp": 1300819380 + 5,
    "aud": ["bob", "saul"],
    "http://example.com/is_root": true,
    "privateParams": {
        "private_one": "one",
        "private_two": "two"
    }
}, {
    "kty": "oct",
    "k": "AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow"
})

Example output:

"eyJhbGciOiAiSFMyNTYiLCAidHlwIjogIkpXVCJ9.eyJhdWQiOiBbImJvYiIsICJzYXVsIl0sICJleHAiOiAxLjMwMDgxOTM4NWUrMDksICJodHRwOi8vZXhhbXBsZS5jb20vaXNfcm9vdCI6IHRydWUsICJpc3MiOiAiam9lIiwgInByaXZhdGVQYXJhbXMiOiB7InByaXZhdGVfb25lIjogIm9uZSIsICJwcml2YXRlX3R3byI6ICJ0d28ifX0.JJvPnDHmJ5fJX7Mc71yiho6ND_BLm1wNX0r7_j9A9zo"

Decoded payload section:

echo eyJhdWQiOiBbImJvYiIsICJzYXVsIl0sICJleHAiOiAxLjMwMDgxOTM4NWUrMDksICJodHRwOi8vZXhhbXBsZS5jb20vaXNfcm9vdCI6IHRydWUsICJpc3MiOiAiam9lIiwgInByaXZhdGVQYXJhbXMiOiB7InByaXZhdGVfb25lIjogIm9uZSIsICJwcml2YXRlX3R3byI6ICJ0d28ifX0 \
 | base64 --decode
{"aud": ["bob", "saul"], "exp": 1.300819385e+09, "http://example.com/is_root": true, "iss": "joe", "privateParams": {"private_one": "one", "private_two": "two"

Please let me know if that doesn't make sense.

kenjenkins commented 1 year ago

It looks like the number formatting happens in the method FloatToNumber().

If we change that method to format integers without an exponent, then JWT serialization behaves like I would expect: https://github.com/open-policy-agent/opa/blob/7f068b216638ab056c1f19412be5b45326efc282/topdown/builtins/builtins.go#L251-L257

The modified example from https://github.com/open-policy-agent/opa/issues/6013#issuecomment-1592142484 would then return a different result.

io.jwt.encode_sign({
    "typ": "JWT",
    "alg": "HS256"
}, {
    "iss": "joe",
    "exp": 1300819380 + 5,
    "aud": ["bob", "saul"],
    "http://example.com/is_root": true,
    "privateParams": {
        "private_one": "one",
        "private_two": "two"
    }
}, {
    "kty": "oct",
    "k": "AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow"
})

Output:

"eyJhbGciOiAiSFMyNTYiLCAidHlwIjogIkpXVCJ9.eyJhdWQiOiBbImJvYiIsICJzYXVsIl0sICJleHAiOiAxMzAwODE5Mzg1LCAiaHR0cDovL2V4YW1wbGUuY29tL2lzX3Jvb3QiOiB0cnVlLCAiaXNzIjogImpvZSIsICJwcml2YXRlUGFyYW1zIjogeyJwcml2YXRlX29uZSI6ICJvbmUiLCAicHJpdmF0ZV90d28iOiAidHdvIn19.yVqf2tqzV6HsLj1o-eh2oFfVTql6WNG__BvKZjwVsjg"

Decoded payload section:

echo eyJhdWQiOiBbImJvYiIsICJzYXVsIl0sICJleHAiOiAxMzAwODE5Mzg1LCAiaHR0cDovL2V4YW1wbGUuY29tL2lzX3Jvb3QiOiB0cnVlLCAiaXNzIjogImpvZSIsICJwcml2YXRlUGFyYW1zIjogeyJwcml2YXRlX29uZSI6ICJvbmUiLCAicHJpdmF0ZV90d28iOiAidHdvIn19 \
 | base64 --decode
{"aud": ["bob", "saul"], "exp": 1300819385, "http://example.com/is_root": true, "iss": "joe", "privateParams": {"private_one": "one", "private_two": "two"}}

Would it be feasible to make a change like https://github.com/open-policy-agent/opa/commit/7f068b216638ab056c1f19412be5b45326efc282, or would this be considered breaking?

srenatus commented 1 year ago

Would it be feasible to make a change like https://github.com/open-policy-agent/opa/commit/7f068b216638ab056c1f19412be5b45326efc282, or would this be considered breaking?

I think it's a good idea to approach it like that. Nobody wants an exponent when it's an int. 🤔 What do you think, @ashutosh-narkar ?

ashutosh-narkar commented 1 year ago

Sounds good to me. @kenjenkins if you'd like to contribute that would be great.

kenjenkins commented 1 year ago

Sure, I'd be happy to open a PR.

@ashutosh-narkar, one question: I don't see any unit tests in the topdown/builtins package — should I add one there exercising this behavior, or is there some other place where this should be tested?

ashutosh-narkar commented 1 year ago

Thanks @kenjenkins ! You can find tests in this folder. For ex. tests for the io.jwt.encode_sign builtin can be found here.