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.

Sign up for GitHub

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

Jump to bottom

Add JTI validation #28

Merged
merged 34 commits into from Sep 23, 2022
Merged

Add JTI validation #28

merged 34 commits into from Sep 23, 2022

Conversation

Potherca
Copy link
Member

@Potherca Potherca commented Sep 17, 2022 •
edited

This MR adds a JTI Validator class, in order to validate jti.

In order to do this, the TokenGenerator class needed to be changed, since the DateInterval in that class also needs to be used by JtiValidator.

The DateInterval has been made constructor dependency for both the TokenGenerator and the JtiValidator.
To make this refactor safe, unit-tests have been added (see tests/unit/TokenGeneratorTest.php).

The JtiValidator has been added as constructor dependency to the Utils\DPop class.
Unit-tests were added here as well. During testing, some bug were found and fixed. These fixes will need to be approved by @poef (as lead in DPOP hardening) before this MR can be moved out of "WIP".

As more effort was invested in writing unit-test, the scope of this issue has not yet been completed.

What is still missing, before this MR can come out of WIP is:

  • Logic for bucket rotation
  • A Dummy JtiStorage class

@Potherca
Fix Deprecation error caused by Pdsinterop\Solid\Auth\Exception\Excep…
f472a37 
…tion::jsonSerialize() not having a return type.
@Potherca
Fixes whitespace.
78daf4c
@Potherca
Fixes whitespace.
4a9adb0
@Potherca
Code cleanup
36144c5 
- Change mixed tabs/spaces to spaces
- Add trailing commas to arrays
- Make method opening bracket consistent across methods
- Remove trailing whitespaces
- Add trailing newline to file
@Potherca
Code cleanup
4839e66 
- Add missing return type
- Remove unneeded doc-blocks
- Add missing param in doc-block
@Potherca
Add first rough tests for the TokenGenerator.
13a6ed7
@Potherca
Change TokenGenerator::generateIdToken() to make the $dpopKey param…
8f9961a 
…eter required.
@Potherca
Change TokenGenerator so Token expiration is settable from the outside.
43cc138
@Potherca
Change TokenGenerator::generateIdToken(), adding a parameter to set t…
dd7ba46 
…oken time.
@Potherca
Add interface for Config\Keys.
ec6f595
@Potherca
Add interface for Config\Server.
d903504
@Potherca
Change DPOPTest to be cleaner
1bc88f4 
- Rename tests to mention subject under test
- Add separator comments between different method types
- Move helper funtion to end of class
@Potherca
Add AbstractTestCase to hold common test methods.
8652191
@Potherca
Change TokenGeneratorTest, move expectArgumentCountError method to …
4f25101 
…abstract parent.
@Potherca
Add test descriptions to DPOPTest.
91b1010
@Potherca
Add missing parameter tests to DPOPTest.
2255946
@Potherca
Fix bug caused by calling non-existent class member.
d5c490f
@Potherca
Add first draft tests for Dpop::getWebId().
c1926c3
@Potherca
Add comments for possible problems to DPop class.
f01a65c
@Potherca
Fix various bugs in DPop class + unit-tests
8d2c826 
- Add various checks for unset keys
- Change Exception message to state what makes a token invalid
-
@Potherca Potherca requested a review from poef September 18, 2022 19:29
@Potherca Potherca self-assigned this Sep 18, 2022
@Potherca Potherca mentioned this pull request Sep 18, 2022
Closed
3 tasks
@Potherca
Copy link
Member Author

Potherca commented Sep 21, 2022 •
edited

In person review with @poef provided insights that changes the scope of the current implementation.

Most notably:

  • Make sure all interfaces do not define a __construct()
  • Undefined / Unchecked array/class properties trigger exceptions (invalid token)
  • Generic \Exceptions need to be replaced with project / case specific ones
  • Unit-test level asserts on JWTs need to be on content (not the base64Url encoded string)
  • The JtiStorageInterface is too broad. It needs to be replaced with a more specifc ReplayDetectorInterface kind of thing
    (as the bucket logic does not belong within the responsibilities of this repo)

image

@Potherca Potherca linked an issue Sep 21, 2022 that may be closed by this pull request
Add support for a JWT "jti" value to prevent replay attacks #27
Closed
3 tasks
@Potherca Potherca marked this pull request as ready for review September 21, 2022 14:15
@Potherca Potherca merged commit e705275 into main Sep 23, 2022
@Potherca Potherca deleted the feature/jti branch September 28, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poef poef poef approved these changes

Assignees

@Potherca Potherca

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for a JWT "jti" value to prevent replay attacks
2 participants
@Potherca @poef