Skip to content
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

Add TLS/SSL backend: Windows Schannel #3867

Merged
merged 22 commits into from
May 8, 2024
Merged

Add TLS/SSL backend: Windows Schannel #3867

merged 22 commits into from
May 8, 2024

Conversation

nanangizz
Copy link
Member

@nanangizz nanangizz commented Feb 21, 2024

Introduction

Currently the library has implemented TLS/SSL using various backends such as OpenSSL, BoringSSL, GnuTLS, Apple Network framework. This PR will add another backend: Windows SSP Schannel, which is a native TLS/SSL security provider for Windows. As usual, this backend is implemented for the PJLIB SSL socket API.

How to enable

Enable TLS/SSL and use Schannel backend implementation. In config_site.h, specify:

#define PJ_HAS_SSL_SOCK 1
#define PJ_SSL_SOCK_IMP PJ_SSL_SOCK_IMP_SCHANNEL

For app, generally this should be similar to using the other backends (see here), except for few things:

  • TLS certificates are looked up from the Certificate Store, specifically the current user store and the local machine store. Application can specify the certificate lookup criteria via pj_ssl_cert_lookup_criteria, which currently supports lookup by subject, by fingerprint, or by friendly name. Currently certificates specified via file, path, or buffer (i.e: pj_ssl_cert_load_from_files/files2() and pj_ssl_cert_load_from_buffer()) will be ignored.
  • For manual verification, it will lookup the certificate chain in the current user store only in folder Trusted Root Certification Authorities.
  • Enabling/disabling cipher suite and curves should be configured via OS settings (global scope), see here.

Limitations

  • Currently this does not cover DTLS-SRTP key negotiation for SRTP media transport.
  • Specifying enabled cipher suites and curves cannot be done via PJLIB pj_ssl_sock_param.ciphers/curves or PJSIP/PJSUA pjsip_tls_setting.ciphers/curves. This is because the Schannel API for enable/disable/prioritize cipher suites & curves seems to be for global scope instead of per TLS session (e.g: see here).

Notes

  • TLS 1.0 and TLS 1.1 will be deprecated soon, see here.
  • TLS 1.3 is supported since Windows 11 or Windows Server 2022, more info here.

@nanangizz nanangizz marked this pull request as ready for review March 14, 2024 08:35
pjlib/include/pj/ssl_sock.h Outdated Show resolved Hide resolved
pjlib/src/pj/ssl_sock_imp_common.c Show resolved Hide resolved
pjsip/include/pjsua2/siptypes.hpp Show resolved Hide resolved
pjsip/include/pjsua2/siptypes.hpp Outdated Show resolved Hide resolved
*
* Currently this is used by Windows Schannel backend only, it will lookup
* in the Current User store first, if not found it will lookup in the
* Local Machine store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this can potentially be used for iOS and Android in the future, perhaps have a quick look at their key stores as well, to make sure that our API and struct are compatible for future extension (in other words, so that we don't need to create a new API such as pj_ssl_cert_load_from_store2()).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, had a thought about this a bit, that's why pj_ssl_cert_load_from_store() uses an 'expandable' param pj_ssl_cert_lookup_criteria for future use.

Nevertheless, tried to do a bit research for Android. From here, it seems that a cert (& its private key) cannot be extracted out to be used by third-party security lib (such as OpenSSL), so the keystore seems to be for native SSL backend only. And from here, it looks like accessing cert in the keystore system is done via UI prompt dialog for the user selecting the cert to use, then application can assign an "alias" (perhaps some kind of "friendly-name" on Windows) for the cert for future use (so app don't need to show the UI prompt dialog again). So if in the future we have a native Android SSL backend, perhaps pj_ssl_cert_load_from_store() can be used to lookup TLS cert using the "alias". The pj_ssl_cert_lookup_criteria & pj_ssl_cert_load_from_store() may not be able to handle any access auth though, which can be password, PIN, biometrics, etc.

pjlib/src/pj/ssl_sock_schannel.c Show resolved Hide resolved
pjlib/src/pj/ssl_sock_schannel.c Outdated Show resolved Hide resolved
pjlib/src/pj/ssl_sock_schannel.c Show resolved Hide resolved
pjlib/src/pj/ssl_sock_schannel.c Outdated Show resolved Hide resolved
pjlib/src/pj/ssl_sock_schannel.c Show resolved Hide resolved
Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include it in CI test:
Perhaps can replace the GnuTLS, or combine it with windows-with-video-libvpx-unit-test-1
Running the pjlib-test only should suffice.

Replace OpenSSL to Schannel in `windows-with-video-libvpx-unit-test-1`
Disabled DTLS for Schannel test

/* Initialize decrypted circular buffer */
status = circ_init(pf, &sch_ssock->decrypted_buf, read_cap);
if (status != PJ_SUCCESS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be deleted here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe a bit more please, e.g: what can be deleted, what is the potential problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to check status since the next line is the on_return label.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't see the label. IIRC it is on purpose, it relatively gives better readibility & avoid bug when new code is added below that. Hopefully compiler optimization can "delete" it (haven't checked further, but something like this)?

@nanangizz nanangizz merged commit 1dab9b6 into master May 8, 2024
36 checks passed
@nanangizz nanangizz deleted the schannel branch May 8, 2024 03:25
dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Jun 12, 2024
* Add missing openssl SECLEVEL=0 support (pjsip#3890)

Previous SECLEVEL support allowed for levels 1-5.
However, openssl defines levels 0-5. [1]

Recent openssl versions (3.0+) have moved previous
popular ciphers/key lengths (i.e. RSA1024withSHA1)
into level 0, so it is now a reasonable choice to use.

Add support for level 0.

[1] https://www.openssl.org/docs/man3.2/man3/SSL_CTX_set_security_level.html

* Enable Late Offer Answer Mode (LOAM) feature in the pjsua (pjsip#3869)

* Fix warnings for 32-bit compiler and misc fixes. (pjsip#3896)

* Add some missing unlocks (pjsip#3893)

* Prevent race condition in DTLS media stop (pjsip#3901)

* Fix data race reported by ThreadSanitizer in caching pool (pjsip#3897)

* Fixed Metal renderer memory leak (pjsip#3909)

* Fixed DTLS clock stoppage race (pjsip#3905)

* Improve IP address change IPv4 <-> IPv6 (pjsip#3910)

* pjsua_acc: Fix warnings for comparison between ‘pjsua_nat64_opt’ and ‘enum pjsua_ipv6_use’ (pjsip#3915)

* Fix to ext_fmts accessed out of stack scope. (pjsip#3916)

* Add check in siprtp sample app for inactive audio media (pjsip#3927)

* Add function to initialize MediaFormat audio & video (pjsip#3925)

* Fixed incorrect SDP buffer length calculation (pjsip#3924)

* Support Push Notification in iOS sample app (pjsip#3913)

* Fixed PJSUA2 API to get/set Opus config (pjsip#3935)

* Fix bad address length check in pj_ioqueue_sendto(). (pjsip#3941)

* Fix warning of uninitialized value in fuzz-crypto (pjsip#3946)

* Print log on successful send (pjsip#3942)

* Fixed CI Mac build failure (pjsip#3947)

* Update Android JNI audio dev to use 16bit PCM only (pjsip#3945)

* Add TLS/SSL backend: Windows Schannel (pjsip#3867)

* pjsip_find_msg: Log warning if Content-Length field not found (pjsip#3960)

* Fix audiodev index (pjsip#3962)

* Fix assertion on call hangup from DTMF callback (pjsip#3970)

* Fix yaml error in github feature template (pjsip#3972)

* Fix version string in Python setup (pjsip#3976)

* Prevent pjmedia_codec_param.info.enc_ptime_denum division by zero in stream (pjsip#3975)

---------

Co-authored-by: naf <[email protected]>
Co-authored-by: Goodicus <[email protected]>
Co-authored-by: Amilcar Ubiera <[email protected]>
Co-authored-by: Santiago De la Cruz <[email protected]>
Co-authored-by: sauwming <[email protected]>
Co-authored-by: Nanang Izzuddin <[email protected]>
Co-authored-by: dshamaev-intermedia <[email protected]>
Co-authored-by: CI Bot <[email protected]>
Co-authored-by: Pau Espin Pedrol <[email protected]>
Co-authored-by: Riza Sulistyo <[email protected]>
Co-authored-by: Andreas Peldszus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants