brave-intl / challenge-bypass-server

https://privacypass.github.io
BSD 3-Clause "New" or "Revised" License
5 stars 6 forks source link

fix: fix time drift between client and server when redeeming v3 #711

Closed pavelbrm closed 1 month ago

pavelbrm commented 1 month ago

This PR adds a 1 hour leeway to the check for issuer v3 signing key, as occasionally clients might fail if requests come through around the time of key rotation.

It also fixes a bug in the v3 redemption handler where issuer expiry check would never work – it mistakenly required the expiry time be both zero and after now at the same time, which was clearly a mistake (as anywhere else that check requires the time to not be zero).

Additionally, the PR offers small refactors to improve readability and maintainability, as well as some test coverage.

For more details, please see the linked issue.

github-actions[bot] commented 1 month ago

[puLL-Merge] - brave-intl/challenge-bypass-server@711

Description

This PR introduces a number of changes related to token issuance and redemption with V3 issuers. The main changes include:

The motivation seems to be to improve the token issuance and redemption flow for V3 issuers, by making the code clearer and adding more robust validation around issuer expiration and signing key selection.

Changes ### Changes #### btd/issuer.go - Refactor `VerifyTokenRedemption` to iterate through `keys` using index - Derive unblinded token and shared key separately, handling metrics for each - Compare server signature to client signature and break loop if valid #### kafka/signed_token_redeem_handler.go - Use new `HasExpired` method to check if issuer is expired - Minor formatting changes #### model/issuer.go - Add `HasExpired(now time.Time)` method to check if issuer is expired - Add `FindSigningKeys(now time.Time)` method to find active signing keys for a V3 issuer - Add private `findActiveKeys` method to find keys active at a given time, with some leeway - Add `parseSigningKeys` helper to convert `IssuerKeys` to `crypto.SigningKey` #### model/issuer_keys.go - Add `isActiveV3(now time.Time, lw time.Duration)` method to check if key is active at a time with some leeway - Add `isValidV3()` method to validate a V3 key - Add package-private `isTimeWithin` helper #### model/*_test.go - Add unit tests for the new `Issuer` and `IssuerKeys` methods #### server/issuers.go - Minor refactor to use `ExpiresAtTime` helper #### server/tokens.go - Refactor `blindedTokenRedeemHandlerV3` to use new `Issuer` methods for validation and key finding - Add `isEmpty()` method to `blindedTokenRedeemRequest` #### server/tokens_test.go - Add unit test for `blindedTokenRedeemRequest.isEmpty()` Overall, the changes look good and well-tested. Using the new `Issuer` methods helps simplify the redemption handler logic. Let me know if you have any other questions!