-
Notifications
You must be signed in to change notification settings - Fork 41
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
Experimental: Cache in client certificate verification (lib and wasm) #2166
Experimental: Cache in client certificate verification (lib and wasm) #2166
Conversation
521d5da
to
eac7a7f
Compare
Test Results 4 files ± 0 52 suites ±0 10m 11s ⏱️ +16s Results for commit 052de8f. ± Comparison against base commit d106a3f. This pull request removes 8 and adds 24 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
eac7a7f
to
9c4b7fe
Compare
Security assessmentsSystem overviewWithout cacheThe operations that takes the most time and resources are verifying the signature and downloading each certificate from the aggregator. Without cache the following checks are done:
With cache
Recap table
RisksWith those characteristics the cache system could easily be compromised (non-exhaustive list) :
Potential enhancementsWe could do almost of the checks by storing almost all metadata from the certificate, except the signature and the signers list (to limit the amount of data to store). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Just some minor suggestions.
9c4b7fe
to
add1b08
Compare
As a `unstable` feature
To split its code in more manageable part.
By using private static function so behavior are still implemented in splitted modules.
As it will be its only consumer, the client should not have to use it directly.
And keep them at max level `warn` in release build (to keep the previous default behavior, see: https://docs.rs/slog/latest/slog/#notable-details).
f2dae8c
to
67e851d
Compare
- Rename new event from `CertificateValidatedFromCache` to `CertificateFetchedFromCache` - Better naming for the cache default duration in client (to make it explicit that the number is one week) - Make some tests more clear with better naming and more assertion - Change `ClientBuilder::with_certificate_verifier_cache` to take an option, allowing to disable existing cache and simplifying usage in the wasm library. - Simplify local storage acquisition in `LocalStorageCertificateVerifierCache` by raising an error if it's missing instead of asking consumer to handle an option (it should not be used if not available anyway). - Adjusted some comments and logs
…using certificate chain builder
67e851d
to
cb9c722
Compare
cb9c722
to
4a700f2
Compare
…nabled By not using the cache until we cross an epoch boundary to make sure that the avk in certificate to check was included in a certificate from the previous epoch.
29e2c60
to
3ff980c
Compare
af4c1d1
to
722d969
Compare
* mithril-client-cli from `0.10.5` to `0.10.6` * mithril-client-wasm from `0.7.2` to `0.7.3` * mithril-client from `0.10.4` to `0.10.5` * mithril-common from `0.4.97` to `0.4.98` * [example] client-snapshot from `0.1.21` to `0.1.22` * [js] mithril-client-wasm from `0.7.2` to `0.7.3` * [js] mithril-explorer from `0.7.23` to `0.7.24`
722d969
to
052de8f
Compare
Content
This PR add a experimental cache system to the certificate chain verification of the client library.
This cache have two implementation:
Client library
CertificateVerifierCache
trait which define how to: get one cached value, add one value to a cache, reset all cached values.unstable
featureunstable
featureMemoryCertificateVerifierCache
struct:unstable
featureCertificateVerifierCache
using a in-memory HashMapcertificate_client
module from a single file to a folder with the following organization:api.rs
: define the mainCertificateClient
struct and the public traits that extends it (such as theCertificateVerifierCache
).fetch.rs
: implementation and tests of Certificate data fetching (get, list)verify.rs
: implementation and tests of Certificate chain verificationverify-cache
: a submodule directory to regroup all cache implementations that are part of the client libraryCertificateValidatedFromCache
Client Wasm library
LocalStorageCertificateVerifierCache
struct:CertificateVerifierCache
using browsers local storageWe could instead store them in a single key but that would mean serializing and/or de-serializing the whole cache at each access, alternatively it could be stored in-memory with a synchronization system.
enable_certificate_chain_verification_cache
: default false, when used conjointly withunstable
it load theLocalStorageCertificateVerifierCache
.certificate_chain_verification_cache_duration_in_seconds
: modify cache item expiration duration, default to604800
(one week).LocalStorageCertificateVerifierCache
and the local storage is not available on the current platform (such as with nodejs) a warning is logged in the console and the cache system remain disabled.Explorer
LocalStorageCertificateVerifierCache
Demo
Here's a screencast of the cache in action:
Screencast.from.2024-12-12.16-30-18.webm
Pre-submit checklist
Comments
As the security of this cache system is quite low (see comment below), this should be only see as experimental and not used in production.
Issue(s)
Relates to #1484