Optimize name constraint matching #4047
Merged
Conversation
This file contains 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
randombit
force-pushed
the
jack/opt-name-constraints
branch
2 times, most recently
from
43a4c83 to
0b70811
Compare
randombit
force-pushed
the
jack/opt-name-constraints
branch
from
a6ad2c2 to
deae034
Compare
FAlbertDev
approved these changes
May 13, 2024
src/lib/x509/name_constraint.cpp
Outdated
Comment on lines
265
to
270
| m_excluded_contains_unknown = false; | ||
| for(const auto& c : m_permitted_subtrees) { | ||
| if(c.base().is_unknown_type()) { | ||
| m_excluded_contains_unknown = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| m_excluded_contains_unknown = false; | |
| for(const auto& c : m_permitted_subtrees) { | |
| if(c.base().is_unknown_type()) { | |
| m_excluded_contains_unknown = true; | |
| } | |
| } | |
| m_excluded_contains_unknown = false; | |
| for(const auto& c : m_excluded_subtrees) { | |
| if(c.base().is_unknown_type()) { | |
| m_excluded_contains_unknown = true; | |
| } | |
| } |
There was a problem hiding this comment.
Fixed using a lambda
m_permitted_contains_unknown = contains_unknown(m_permitted_subtrees);
m_excluded_contains_unknown = contains_unknown(m_excluded_subtrees);
Previously name constraints flattened everything to a std::string and then parsed it back to the desired form during matching. This is slow. It also bounced through a std::function for each match, which has a surprisingly high overhead. This commit also removes some sketchy (untested, unused within library, unclear purpose) string constructors for GeneralName and GeneralSubtree. Technically a SemVer break but I'm pretty sure nobody uses these.
DNS is case insensitive and forcing the DNS name to lowercase means we don't have to tolower while comparing name constraints.
If we match on an excluded tree there is no reason to continue checking the remainder of the constraints.
randombit
force-pushed
the
jack/opt-name-constraints
branch
2 times, most recently
from
f5d098f to
6700358
Compare
If so we can immediately fail if the extension is critical. Also this allows returning early from is_permitted since we don't have to handle the case of looking for critical+unknown permitted subtrees, which otherwise would force us to scan (and match against) the entire list of permitted trees.
With the help of string_view we don't need to create any copies.
RFC 5280 states that minimum must be zero and maximum must be unset, so keeping the fixed values in memory isn't that helpful.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously name constraint matching worked by smashing all of the constraints and names into
std::string, then parsing the values back out of the strings for each match attempt. This is all quite slow and needless. Instead store the names in their best form for matching in astd::variant.