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.
Support allowing specific unknown critical extensions #2377
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
Support allowing specific unknown critical extensions #2377
Changes from all commits
c67a869
9ac02a5
a10aa3a
eb9ba6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should clear this
EXFLAG_CRITICAL
flag off after thisif
case. It's kind of a misnomer of a name but the purpose of the flag indicates that there is unknown critical extension unhandled. If we don't exit out here at this point we should remove it cause that implies no extensions are unknown at that point on the certificate.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.
Can we also add a comment here about this maybe, I was also confused at first. I read the if condition as "if
IGNORE_CRITICAL
is not set, andEXFLAG_CRITICAL
is set (which to me implied any critical extensions, not just unhandled ones), AND eithercheck_custom_known_critical_extension
orctx->verify_crit_oids
fails", we are in an error state.Since
check_custom_known_critical_extension
fails immediately ifctx->custom_crit_oids
is NULL, ifEXFLAG_CRITICAL
were to mean any critical ext and not just unhandled ones, this if condition would be erroneous.Just to confirm for my own clarity, at this point in code, are we expecting that any "known" critical oids have already been parsed/validated. So if EXFLAG_CRITICAL is still set here, we know that it's only possible because the customer has set custom oids and can therefore fail otherwise?
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.
I was a bit hesitant to do so, mainly because the state of
EXFLAG_CRITICAL
is maintained along with thex
certificate. If that state is cleared, there is now an implication that critical extension is supported and the certificate can be accepted. I was initially afraid of the user not being aware that there were typically unknown critical extensions present in the cert. But if they're already runningX509_verify_cert
with this feature, I guess it would make sense that that state is cleared later on.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.
EXFLAG_CRITICAL
simply indicates an unsupported critical extension, so a critical extension that is already supported wouldn't trigger this. This flag is added during the initial parsing of the certificate.check_custom_known_critical_extension
is supposed to fail under the circumstance you mentioned. Typical OpenSSL/AWS-LC behavior is to fail when an unsupported critical extension is found (hence the flag). The user has to set upctx->custom_crit_oids
with all their "known" unsupported critical extensions they wish to work around here.At this point in the code, we only know if there are unknown critical extensions present with the
EXFLAG_CRITICAL
flag. We could remove the flag after the first validation against this has succeeded, but I'd prefer to do the custom OID validation regardless of whether the flag has been set.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.
Thank you for those details, that clears up a lot for me! It would be great if we can add a comment here explaining EXFLAG_CRITICAL/what we are checking to improve readability in this func :)
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.
No problem, I think it's valuable. Will do :).