feat: allow size >= 2 for jwt payload

[bamthomas]

Could we change the check for the number of fields of the JWT payload to greater or equal to 2 instead of 2. I can't see any drawbacks of doing this, but maybe I'm missing something. There should be iat and sub fields, but why couldn't we use more?

In our app we use exp for expiration timestamp for example and a role field as well.

The tests are green with the change and I didn't add any test .

Thanks for your answer.

[rsto]

Thanks for your PR. The current implementation deliberately only supports a minimal set of JSON Web Token as we wanted to keep it small. I now understand that this is too restrictive and re-reading section 4 of RFC 7519 seems to prove your point:

The set of claims that a JWT must contain to be considered valid is
context dependent and is outside the scope of this specification.
Specific applications of JWTs will require implementations to
understand and process some claims in particular ways. However, in
the absence of such requirements, all claims that are not understood
by implementations MUST be ignored.

I'm OK with merging this PR as-is. However, I would want to at least also enforce the exp claim, probably also nbf. The nbf claim would take precedence over exp and that precedence over the iat+http_jwt_max_age imapd.conf option.

Would you be interested in implementing that inluding cunit tests? If not, I'll merge this PR and revisit implementing exp and nbf later.

[bamthomas]

thanks for your answer @rsto . Agreed with nbf > exp > iat+http_jwt_max_age precedence.

I can try (it's a long time since I did C) but with some help eventually it could work :)

[rsto]

Here is the steps I would take:

• add a new test function validate_claims to cunit/http_jwt.testc that tests tests the nbf, exp and iat (using http_jwt_max_age) for if both http_jwt_max_age is set to 0 or some non-zero value. The nbf and exp claims should be evaluated in both cases, the iat claim only if http_jwt_max_age is set or iat is in the future (which the implementation treats as an error). I would just copy the code of test_validate and remove all but the iat tests. You'll need to re-run automake/autoconf after that
• then I would update validate_payload in imap/http_jwt.c to make sure that all newly added tests pass.

Does that help for a start?

[bamthomas]

yes definitely, I'll do this at the end of this week.

[rsto]

One more thing: I don't want to hold you back using Cyrus for your use cases. So I could equally merge this PR and you submit a new PR with the updated code. As long as it's only on the development branch it doesn't matter too much to me if the claims are supported. Just let me know what you prefer.

[bamthomas]

as we are using cyrus version 3.6.1, it is also equal, we still have the forked code running on the server. So if it can avoid "administrative" tasks with PR management, it's OK.
Do what is the most convenient.

[rsto]

👍 I'll just wait for the changes on this PR then, thanks.

[elliefm]

You have a merge commit in this PR. Please don't merge into your PR branch, it turns the history into spaghetti when the PR in turn gets merged. If you need to bring your branch up to date with the current upstream master, you should rebase on top of it and then force push.

Please rebase and force push your branch to get the merge commit out. I assume you know how to do this, but I'm happy to help if you need it. Thanks :)

[bamthomas]

ooops I've just seen your comment, and it was too late. Sorry about the noise.
I rebased and forced to remove the merge.
If you want a clean PR timeline I can make another one.

Some notes about implementation :

• now a token without expiration information is not accepted (there should be iat + max_age or exp)
• exp has precedence over iat + max_age
• if nbf is present and has a valid value it is checked
• I used negative checks !(now < exp) and not now >= exp to be similar to the spec wording
• I added a max nb fields constant MAX_JWT_PAYLOAD_FIELD_NB and set it to 5
• I splited the claims test into 2 to init the module with a max_age value different from 0
• I had to change the auth test because it was flaky with B replacement for A (not enough entropy to break base64 encoding maybe)
• in the tests there are several blocks like :

time_t nowPlusOneHour = time(NULL) + 3600;
int length = snprintf( NULL, 0, "%ld", nowPlusOneHour );
char p[] = "{\"sub\": \"test\", \"exp\":%ld}";
char payload[strlen(p) + length];
sprintf(payload, p, nowPlusOneHour); 

To update the exp/iat/nbf timestamps how to make them simpler?

requested change has been actioned

[rsto]

Thanks for the PR and the extensive testing! I think the following points should be handled differently

now a token without expiration information is not accepted (there should be iat + max_age or exp)

I think iat without the max_age config set should just be ignored. RFC 7519 does not say otherwise and any Cyrus installations that already use iat without max_age should just work as-is.

I added a max nb fields constant MAX_JWT_PAYLOAD_FIELD_NB and set it to 5

That's not necessary IMO. We should just validate any known claims and ignore any other, just as the spec says.

nowPlusOneHour

Cyrus codebase mostly uses snake case or some variations of it. I'm sorry that we do not have any style guide which would make this clear.

I have ideas how to make the test code nicer to read, but I need to do some experimentation with it as well. If you are OK, I could take your PR and do my changes on top of it. If you prefer to keep working on it on your own, please let me know.

[bamthomas]

Ok with all you said (following the spec, do not break existing servers, and code style).

For code style I found this but I haven't found a naming convention.

I'm doing the changes now.

And yes I'd be curious about your improvements, to learn about modern C coding. So don't hesitate to do your changes on top of it.

[bamthomas]

I kept the rejection of the token if there is max_age and no iat (as it was before).

[rsto]

This now landed on the main branch: #4526 !

[rsto]

I tagged this also to include in the 3.8 branch.