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

pyatls: consider clock skew, add logging, version 0.0.5 #15

Merged
merged 3 commits into from
May 24, 2024

Conversation

HernanGatta
Copy link
Contributor

@HernanGatta HernanGatta commented May 24, 2024

Clock Skew and Logging

Note: Commits can be reviewed independently.

When validating JWT tokens, the PyJWT package validates the IAT (Issued At) claim strictly: it must be exactly less than the time now. This requires the issuing service and the validating system to have synchronized clocks. Otherwise, even a few milliseconds of skew forwards on the service or backwards in the validator can cause the library to claim that a token was issued in the future.

There is much discussion in that library as to whether verifying that IAT is strictly in the past is a good idea, particularly since the JWT specification does not state that one must do so.

This PR adds a 5s leeway to time validation to account for this. This PR also adds optional logging so that issues like this can be more easily discovered.

@HernanGatta HernanGatta added the fix Bug fix for the user, not a fix to a build script label May 24, 2024
@HernanGatta HernanGatta requested a review from chester-leung May 24, 2024 14:59
@HernanGatta HernanGatta self-assigned this May 24, 2024
@HernanGatta HernanGatta changed the title Clock skew and logging pyatls: consider clock skew and add logging May 24, 2024
@HernanGatta HernanGatta changed the title pyatls: consider clock skew and add logging pyatls: consider clock skew, add logging, version 0.0.5 May 24, 2024
Copy link

github-actions bot commented May 24, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 1 0 0.01s
✅ PYTHON black 18 0 0.74s
✅ PYTHON flake8 18 0 0.51s
✅ PYTHON isort 18 0 0.31s
✅ YAML yamllint 5 0 0.28s

See detailed report in MegaLinter reports

You could have the same capabilities but better runtime performances if you use a MegaLinter flavor:

MegaLinter is graciously provided by OX Security

pub = peer_cert.public_key()

logging.debug("Fetching certificate SubjectPublicKeyInfo")

Choose a reason for hiding this comment

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

Let's standardize these logs a bit more -- e.g., use ... here and above or remove it as part of "Fetching certificate extension value..." and similar

@HernanGatta HernanGatta force-pushed the hegatta/clock-skew-and-logging branch from 35ac376 to 5f9750f Compare May 24, 2024 18:12
When issues occur during aTLS connection establishment, it is unclear
what the problem is, since there is currently no logging.

With this change, one may write in the calling package:

logging.basicConfig()
logging.getLogger().setLevel(logging.DEBUG)

and see useful logging as connections are established.
@HernanGatta HernanGatta force-pushed the hegatta/clock-skew-and-logging branch from 5f9750f to a4af059 Compare May 24, 2024 18:17
@HernanGatta HernanGatta merged commit 8f86719 into dev May 24, 2024
1 check passed
@HernanGatta HernanGatta deleted the hegatta/clock-skew-and-logging branch May 24, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix for the user, not a fix to a build script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants