session mbedtls, openssl UPDATE optionally enforce EKU and KU#619
Open
niklas-moser wants to merge 1 commit into
Open
session mbedtls, openssl UPDATE optionally enforce EKU and KU#619niklas-moser wants to merge 1 commit into
niklas-moser wants to merge 1 commit into
Conversation
niklas-moser
force-pushed
the
tls_client_enforce_eku_and_ku_extensions
branch
2 times, most recently
from
9b0567c to
8e7e05a
Compare
Roytak
requested changes
Jul 8, 2026
Roytak
left a comment
Roytak
left a comment
Contributor
There was a problem hiding this comment.
Looks good, but needs a bit of refactor.
Comment on lines
+793
to
+820
| /* WG11 6.9.3 / O-RAN: the end-entity (client) certificate MUST carry | ||
| * the clientAuth Extended Key Usage, so explicitly reject any leaf | ||
| * certificate lacking the clientAuth EKU extension. */ | ||
| const mbedtls_x509_sequence *usage; | ||
| int has_client_auth = 0; | ||
|
|
||
| for (usage = &cert->ext_key_usage; usage && usage->buf.p; usage = usage->next) { | ||
| if (!MBEDTLS_OID_CMP(MBEDTLS_OID_CLIENT_AUTH, &usage->buf)) { | ||
| has_client_auth = 1; | ||
| break; | ||
| } | ||
| } | ||
| if (!has_client_auth) { | ||
| ERR(data->session, "Cert verify: fail (client certificate lacks the required clientAuth Extended Key Usage)."); | ||
| *flags |= MBEDTLS_X509_BADCERT_EXT_KEY_USAGE; | ||
| return 0; | ||
| } | ||
|
|
||
| /* WG11 6.9.3 / O-RAN: the end-entity (client) certificate MUST also | ||
| * carry a Key Usage extension with the digitalSignature bit set (the | ||
| * client signs the CertificateVerify handshake message), so reject any | ||
| * leaf certificate missing the Key Usage extension or that bit. */ | ||
| if (!mbedtls_x509_crt_has_ext_type(cert, MBEDTLS_X509_EXT_KEY_USAGE) || | ||
| mbedtls_x509_crt_check_key_usage(cert, MBEDTLS_X509_KU_DIGITAL_SIGNATURE)) { | ||
| ERR(data->session, "Cert verify: fail (client certificate lacks the required digitalSignature Key Usage)."); | ||
| *flags |= MBEDTLS_X509_BADCERT_KEY_USAGE; | ||
| return 0; | ||
| } |
Contributor
There was a problem hiding this comment.
I think that extracting this code into a separate function would look much cleaner. If it fails, then just set the flags and return, also wrap the function in the ifdef.
Comment on lines
+467
to
+506
| /* WG11 6.9.3 / O-RAN: the end-entity (client) certificate MUST carry | ||
| * the clientAuth Extended Key Usage, so explicitly reject any leaf | ||
| * certificate lacking the clientAuth EKU extension. */ | ||
| { | ||
| EXTENDED_KEY_USAGE *eku = X509_get_ext_d2i(cert, NID_ext_key_usage, NULL, NULL); | ||
| int has_client_auth = 0, i; | ||
|
|
||
| if (eku) { | ||
| for (i = 0; i < sk_ASN1_OBJECT_num(eku); ++i) { | ||
| if (OBJ_obj2nid(sk_ASN1_OBJECT_value(eku, i)) == NID_client_auth) { | ||
| has_client_auth = 1; | ||
| break; | ||
| } | ||
| } | ||
| EXTENDED_KEY_USAGE_free(eku); | ||
| } | ||
| if (!has_client_auth) { | ||
| ERR(data->session, "Cert verify: fail (client certificate lacks the required clientAuth Extended Key Usage)."); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| /* WG11 6.9.3 / O-RAN: the end-entity (client) certificate MUST also | ||
| * carry a Key Usage extension with the digitalSignature bit set (the | ||
| * client signs the CertificateVerify handshake message), so reject any | ||
| * leaf certificate missing the Key Usage extension or that bit. */ | ||
| { | ||
| ASN1_BIT_STRING *ku = X509_get_ext_d2i(cert, NID_key_usage, NULL, NULL); | ||
| int has_digital_signature = 0; | ||
|
|
||
| if (ku) { | ||
| /* bit 0 of the Key Usage bit string is digitalSignature */ | ||
| has_digital_signature = ASN1_BIT_STRING_get_bit(ku, 0); | ||
| ASN1_BIT_STRING_free(ku); | ||
| } | ||
| if (!has_digital_signature) { | ||
| ERR(data->session, "Cert verify: fail (client certificate lacks the required digitalSignature Key Usage)."); | ||
| return 0; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do the same as for mbedTLS, we really always try to avoid inner code blocks in this codebase. Otherwise the code looks OK.
| option(ENABLE_PAM "Detect and use PAM" ON) | ||
| option(ENABLE_COMMON_TARGETS "Define common custom target names such as 'doc' or 'uninstall', may cause conflicts when using add_subdirectory() to build this project" ON) | ||
| option(ENABLE_IP_FREEBIND "Enable the IP_FREEBIND/IPV6_FREEBIND options on the listening TCP socket" OFF) | ||
| option(ENABLE_COMPLY_WITH_ORAN_WG11 "Comply with the O-RAN WG11 security spec (e.g. enforce the clientAuth Extended Key Usage and the digitalSignature Key Usage on client certificates), non-default behavior" OFF) |
Contributor
There was a problem hiding this comment.
I think you can omit the "non-default behavior" here.
niklas-moser
force-pushed
the
tls_client_enforce_eku_and_ku_extensions
branch
from
8e7e05a to
824a417
Compare
Author
|
Thank you! I updated the branch, let me know what you think |
Roytak
approved these changes
Jul 8, 2026
Roytak
reviewed
Jul 8, 2026
Roytak
left a comment
Roytak
left a comment
Contributor
There was a problem hiding this comment.
Last comment from me, otherwise LGTM. Will wait for Michal to look at it and merge it when he comes back next week.
|
|
||
| # comply with the O-RAN WG11 security spec (e.g. client cert Extended Key Usage and Key Usage checks) | ||
| if(ENABLE_COMPLY_WITH_ORAN_WG11) | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DCOMPLY_WITH_ORAN_WG11") |
Contributor
There was a problem hiding this comment.
Prepend NC_ to the compile def name, so it's NC_COMPLY_WITH_ORAN_WG11, similarly to NC_ENABLED_SSH_TLS.
…Key Usage (KU) set by client, optional through compile-time flag Signed-off-by: Niklas Moser <niklas.moser@srs.io>
niklas-moser
force-pushed
the
tls_client_enforce_eku_and_ku_extensions
branch
from
824a417 to
e6f89fb
Compare
Author
|
Okay, updated the PR. Thanks! |
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
2 participants
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.
Continuation of this PR, with
develas target branch.