Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jwt-auth): support setting clock skew for verifying #7500

Merged
merged 3 commits into from Jul 25, 2022

Conversation

tzssangglass
Copy link
Member

Description

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the CI pass, thanks!

@@ -49,6 +49,7 @@ For Consumer:
| exp | integer | False | 86400 | [1,...] | Expiry time of the token in seconds. |
| base64_secret | boolean | False | false | | Set to true if the secret is base64 encoded. |
| vault | object | False | | | Set to true to use Vault for storing and retrieving secret (secret for HS256/HS512 or public_key and private_key for RS256). By default, the Vault path is `kv/apisix/consumer/<consumer_name>/jwt-auth`. |
| lifetime_grace_period | integer | False | 0 | [0,...] | Define the leeway in seconds to account for clock skew between the server that generated the jwt and the server validating it. Value should be zero (0) or a positive integer. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the leeway is a time range from [-N, N], I think it should be shown in the description (after we configure the lifetime_grace_period.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the leeway is a time range from [-N, N], I think it should be shown in the description (after we configure the lifetime_grace_period.

From the documentation of lua-resty-jwt, leeway >= 0, see:https://github.com/cdbattags/lua-resty-jwt#legacytimeframe-options

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, such a lifetime_grace_period field cannot let people be associated with the cloud skew (just from the name) unless they read this document. Show the cloud drift (to the past, or to the future) will make them understand this field better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose the current real time is 17:05:30.

  1. Server A, which generates the JWT, displays the time 17:05:30, when a JWT is generated with a validity of 60 seconds, and Server B, which validates the JWT, displays the time 17:05:20.

  2. Server B, which verifies the JWT, shows the time 17:05:20.

When the real time has passed 65 seconds.

At this point, Server A shows a time of 17:06:35 and Server B shows a time of 17:06:25.

At this point, to verify the JWT on Server B, we need to set the leeway to -10 to keep the time consistent with Server A and the real time.

Otherwise, the JWT should be expired at this time, but Server B sees the time 17:06:25 and thinks the JWT is not expired.

So it is necessary to make the leeway range from [-N, N].

I will verify this scenario later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tokers Unfortunately, the lib lua-resty-jwt currently allows setting lifetime_grace_period to be less than 0. Even though it seems to us that this makes sense.

The error stack for the test is as follows.

stack traceback:
coroutine 0:
	[C]: in function 'error'
	...local/apisix/deps/share/lua/5.1/resty/jwt-validators.lua:81: in function 'ensure_is_non_negative'
	...local/apisix/deps/share/lua/5.1/resty/jwt-validators.lua:323: in function 'set_system_leeway'
	/usr/local/apisix/deps/share/lua/5.1/resty/jwt.lua:713: in function 'get_claim_spec_from_legacy_options'
	/usr/local/apisix/deps/share/lua/5.1/resty/jwt.lua:788: in function 'validate_claims'
	/usr/local/apisix/deps/share/lua/5.1/resty/jwt.lua:822: in function 'verify_jwt_obj'
	/usr/local/apisix/apisix/plugins/jwt-auth.lua:399: in function 'phase_func'
	/usr/local/apisix/apisix/plugin.lua:897: in function 'run_plugin'
	/usr/local/apisix/apisix/init.lua:435: in function 'http_access_phase'
	access_by_lua(nginx.conf:427):4: in main chunk, client: 127.0.0.1, server: localhost, request: "GET /hello HTTP/1.1", host: "127.0.0.1:1984"

ensure_is_non_negative will check that lifetime_grace_period is at least greater than 0.

@tzssangglass tzssangglass requested a review from tokers July 22, 2022 06:10
@spacewander spacewander merged commit a2144b5 into apache:master Jul 25, 2022
@tzssangglass tzssangglass deleted the jwt_system_leeway branch October 10, 2022 10:42
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants