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

JWT issuer validation #6175

Merged
merged 16 commits into from
Jun 11, 2024

Conversation

maxtropets
Copy link
Contributor

@maxtropets maxtropets commented May 10, 2024

@maxtropets maxtropets changed the title WIP WIP: JWT issuer validation May 10, 2024
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch 4 times, most recently from c5c28c9 to b656996 Compare May 21, 2024 14:47
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch 3 times, most recently from 8d2f033 to ff4e113 Compare May 23, 2024 12:29
@maxtropets maxtropets changed the title WIP: JWT issuer validation JWT issuer validation May 23, 2024
@maxtropets maxtropets self-assigned this May 23, 2024
@maxtropets maxtropets requested review from achamayou and eddyashton and removed request for achamayou and eddyashton May 23, 2024 13:10
@maxtropets maxtropets marked this pull request as ready for review May 23, 2024 13:11
@maxtropets maxtropets requested a review from a team as a code owner May 23, 2024 13:11
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch 2 times, most recently from b2bc164 to a66369a Compare May 28, 2024 10:18
@maxtropets
Copy link
Contributor Author

As noted above, kept the old tables but moved them under "Legacy" namespace. We can work with them if needed but kept them explicitly obsolete.

@maxtropets maxtropets marked this pull request as draft May 28, 2024 10:46
@maxtropets
Copy link
Contributor Author

Converted back to "Draft" to include #5177 and #6204

@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch 3 times, most recently from 66dcd1e to a5d6e0b Compare May 29, 2024 15:47
@maxtropets maxtropets marked this pull request as ready for review May 29, 2024 16:04
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch from a5d6e0b to 0097276 Compare May 29, 2024 16:10
Copy link
Member

@eddyashton eddyashton left a comment

Choose a reason for hiding this comment

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

Partial review - need to take another look at the C++ changes.

tests/jwt_test.py Show resolved Hide resolved
tests/jwt_test.py Show resolved Hide resolved
tests/js-custom-authorization/custom_authorization.py Outdated Show resolved Hide resolved
tests/js-custom-authorization/custom_authorization.py Outdated Show resolved Hide resolved
tests/js-custom-authorization/custom_authorization.py Outdated Show resolved Hide resolved
src/endpoints/authentication/jwt_auth.cpp Outdated Show resolved Hide resolved
src/node/rpc/jwt_management.h Outdated Show resolved Hide resolved
src/node/rpc/jwt_management.h Outdated Show resolved Hide resolved
src/node/rpc/jwt_management.h Show resolved Hide resolved
src/node/rpc/jwt_management.h Outdated Show resolved Hide resolved
src/node/rpc/jwt_management.h Outdated Show resolved Hide resolved
src/endpoints/authentication/jwt_auth.cpp Outdated Show resolved Hide resolved
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch from 3cd6faf to f2ebf24 Compare June 7, 2024 13:12
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch from f2ebf24 to fdbf221 Compare June 7, 2024 13:12
@maxtropets maxtropets requested a review from achamayou June 7, 2024 13:47
@maxtropets
Copy link
Contributor Author

I've run jwt perf test on main VS branch

./tests.sh -VV -R pi_ls_jwt -L "perf"
python3 tests/infra/piccolo/throughput_analysis.py # patched with proper paths

Results main

(env) mtropets@mtropets:~/workspace/CCF$ python3 tests/infra/piccolo/throughput_analysis.py
+------+-------+------+------------+
| Reqs |  Time | Pass | Throughput |
+------+-------+------+------------+
| 1000 | 0.046 | 0.0  |  21914.4   |

Result branch

python3 tests/infra/piccolo/throughput_analysis.py 
+------+-------+------+------------+
| Reqs |  Time | Pass | Throughput |
+------+-------+------+------------+
| 1000 | 0.064 | 0.0  |  15716.4   |
+------+-------+------+------------+

Seems like a pretty decent performance hit, will dig up

@maxtropets
Copy link
Contributor Author

I've run jwt perf test on main VS branch

./tests.sh -VV -R pi_ls_jwt -L "perf"
python3 tests/infra/piccolo/throughput_analysis.py # patched with proper paths

Results main

(env) mtropets@mtropets:~/workspace/CCF$ python3 tests/infra/piccolo/throughput_analysis.py
+------+-------+------+------------+
| Reqs |  Time | Pass | Throughput |
+------+-------+------+------------+
| 1000 | 0.046 | 0.0  |  21914.4   |

Result branch

python3 tests/infra/piccolo/throughput_analysis.py 
+------+-------+------+------------+
| Reqs |  Time | Pass | Throughput |
+------+-------+------+------------+
| 1000 | 0.064 | 0.0  |  15716.4   |
+------+-------+------+------------+

Seems like a pretty decent performance hit, will dig up

However, I don't know how heavy is the typical TX. In the test, we do one append per TX, which might differ from the real-world scenario, and so, I have no clue about the real impact of 25% slower JWT authentication.

Meaning, if the auth itself is 10x faster than the request itself - it doesn't really matter, otherwise it might matter.

@maxtropets
Copy link
Contributor Author

One more perf update

Removed issuer validation, but left new schema in place. Results

(env) mtropets@mtropets:~/workspace/CCF$ python3 tests/infra/piccolo/throughput_analysis.py 
+------+-------+------+------------+
| Reqs |  Time | Pass | Throughput |
+------+-------+------+------------+
| 1000 | 0.058 | 0.0  |  17306.4   |
+------+-------+------+------------+

Most likely, the perf impact is 25%=10%+15%

  • 10% for issuer validation
  • 15% for new schemadeserialization

@achamayou
Copy link
Member

@maxtropets you need to rev up the openapi version of the gov schema, run the test again locally, and check in the updated openapi golden file to get it to pass.

25% hit on end-to-end throughput is substantial, but will be offset to some extent by the application being able to remove some of that logic. It would be good to run the same test (full logging) with jwt removed, to compare (the closest we have is basic, but that's a smaller app, and clocks about ~57kTx/s on the CI). It would also be good to run with a profiler to see what exactly makes this worse.

But because this change is a correctness fix, I think we merge it now, and investigate separately if there is a way to improve the performance. Ideally we do this now, and figure out quickly if further schema changes are needed or not. Improvements that don't impact the schema can happen in later releases.

@achamayou
Copy link
Member

@maxtropets ah one thing we need on this PR is a proper CHANGELOG entry, explaining the change to application developers, and service operators, with details about actions they need to take.

@maxtropets
Copy link
Contributor Author

the closest we have is basic, but that's a smaller app

@achamayou aren't they actually the same test? Seems like the only difference is JWT auth.

@achamayou
Copy link
Member

the closest we have is basic, but that's a smaller app

@achamayou aren't they actually the same test? Seems like the only difference is JWT auth.

No, there are two subtle differences. One is that the logging API involves JSON wrappers, with extra (de)serialisation/copies. The other is that the logging js app is fairly large (it has lots of endpoints) and so involves loading quite a bit more bytecode at every execution.
So while it is logically doing the same thing, it's running quite a bit slower.

CHANGELOG.md Outdated Show resolved Hide resolved
@maxtropets maxtropets enabled auto-merge (squash) June 10, 2024 09:40
@maxtropets maxtropets force-pushed the f/5809-jwt-issuer-policy-check branch from 0b10a04 to 6cb0ee5 Compare June 10, 2024 09:42
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@maxtropets maxtropets merged commit fbc400b into microsoft:main Jun 11, 2024
21 checks passed
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.

3 participants