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

Only enable DTLS if SSL backend is OpenSSL #4239

Merged
merged 2 commits into from
Jan 3, 2025
Merged

Only enable DTLS if SSL backend is OpenSSL #4239

merged 2 commits into from
Jan 3, 2025

Conversation

sauwming
Copy link
Member

@sauwming sauwming commented Jan 3, 2025

No description provided.

@sauwming sauwming requested a review from nanangizz January 3, 2025 06:05
@sauwming sauwming self-assigned this Jan 3, 2025
@sauwming sauwming added this to the release-2.16 milestone Jan 3, 2025
@nanangizz
Copy link
Member

Rather than adding checks multiple times, what if the check is inserted into pjmedia/config.h, e.g: somewhere after :

#ifndef PJMEDIA_SRTP_HAS_DTLS
# define PJMEDIA_SRTP_HAS_DTLS 0
#endif

/* Currently SRTP-DTLS requires OpenSSL */
#if PJMEDIA_SRTP_HAS_DTLS
#  if PJ_SSL_SOCK_IMP != PJ_SSL_SOCK_IMP_OPENSSL
#    undef PJMEDIA_SRTP_HAS_DTLS
#    define PJMEDIA_SRTP_HAS_DTLS 0
#  endif
#endif

@sauwming
Copy link
Member Author

sauwming commented Jan 3, 2025

Overwriting the setting in config.h may not be obvious for users that has enabled PJMEDIA_SRTP_HAS_DTLS and expects it to work.

How about adding it at the beginning of file transport_srtp.c instead?
We can also add #warn there to notify user.

@nanangizz
Copy link
Member

Overwriting the setting in config.h may not be obvious for users that has enabled PJMEDIA_SRTP_HAS_DTLS and expects it to work.

Yes, it disables the DLTS silently, but effectively so does the original patch?

How about adding it at the beginning of file transport_srtp.c instead?

Okay too. The point is better avoid such long check & multiple times, for example if in the future DTLS may also use another TLS backend, so the check will get longer and prone to missing update.

We can also add #warn there to notify user.

Was thinking about compile warning too, but not sure if there is a standard way (or gnu-C89 way :D) for generating it.

@sauwming
Copy link
Member Author

sauwming commented Jan 3, 2025

Overwriting the setting in config.h may not be obvious for users that has enabled PJMEDIA_SRTP_HAS_DTLS and expects it to work.
Yes, it disables the DLTS silently, but effectively so does the original patch?

True, but the build error will point to transport_srtp.c so it's easier to figure out.
(for background info, when I tested with gnutls, the SRTP failed to build and it's not immediately obvious for me why).

How about adding it at the beginning of file transport_srtp.c instead?
Okay too. The point is better avoid such long check & multiple times, for example if in the future DTLS may also use another TLS backend, so the check will get longer and prone to missing update.

Makes sense.

We can also add #warn there to notify user.
Was thinking about compile warning too, but not sure if there is a standard way (or gnu-C89 way :D) for generating it.

I was considering between #warning, #pragma message() and printf(), and it seemed that the second one would be the most acceptable. We've also been using it in various places:

./pjmedia/include/pjmedia/config.h:#       pragma message("Warning: PJMEDIA_HAS_SMALL_FILTER macro is deprecated"\
./pjlib/include/pj/compat/limits.h:#  pragma message("limits.h is not found or not supported. LONG_MIN and "\
./pjlib/include/pj/config.h:#       pragma message("Warning: PJ_ENABLE_EXTRA_CHECK macro is deprecated"\

@sauwming sauwming merged commit 536df27 into master Jan 3, 2025
36 checks passed
@sauwming sauwming deleted the dtls-ossl branch January 3, 2025 08:06
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.

2 participants