Repository navigation
perf(signature): Improve perfomance and correctness - #742
Open
rabbitstack wants to merge 13 commits into
Open
rabbitstack wants to merge 13 commits into
rabbitstack wants to merge 13 commits into
Conversation
Open acquires a catalog admin context and opens the file before it can fail. When the hash is not in any catalog, CryptCatalogAdminEnumCatalogFromHash returns 0 and CryptCatalogInfoFromContext fails. That is the normal, not catalog-signed outcome, so Open returns an error and Close is never called. This can leak: - admin context - open file handle - catalog context
Catalog certificate parsing is uncached and reads whole .cat files and then pkcs7.Parse on it, for every module. Cache the parsed cert per catalog file, keyed by path, size and mtime, and deduplicate concurrent parses.
Store the result inside singleflight callback to fix the race window after Do returns.
Catalog-signed files, which are most of Windows, have no embedded certificate. Request's hot path re-runs a full PE parse on the caller's thread, on every request for that module. Introduce a new atomic field, that is set when the cert parsing is attempted, and will not try to parse the cert again.
Every pointer the native side reads now lives in heap memory. The structs hang off a field of the heap-allocated WintrustData, so the compiler must treat them as escaping. The strings are separate heap buffers. A goroutine stack move can no longer invalidate them. The objects stay reachable until after the second WinVerifyTrust call. Before, they could be collected between VerifyCatalog returning and the deferred Close.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of this PR / why it is needed?
This PR brings a batch of changes to improve the signature store performance and correctness. For more information, review individual commits.
What type of change does this PR introduce?
/kind bug-fix (non-breaking change which fixes an issue)
/kind improvement
Any specific area of the project related to this PR?
/area instrumentation
Special notes for the reviewer
Does this PR introduce a user-facing change?