Skip to content

Add RTMPS support#113

Merged
mat-hek merged 7 commits intomembraneframework:masterfrom
olegokunevych:ssl_support
Jul 18, 2025
Merged

Add RTMPS support#113
mat-hek merged 7 commits intomembraneframework:masterfrom
olegokunevych:ssl_support

Conversation

@olegokunevych
Copy link
Contributor

No description provided.

@olegokunevych olegokunevych requested a review from varsill as a code owner June 16, 2025 15:49
@olegokunevych
Copy link
Contributor Author

@varsill thank you for code review, I've found out it useful and committed corresponding changes. Let me know if we need to add/modify something else in this PR.
PS I am wondering that CI format checker is failing, I've done mix format locally

@olegokunevych olegokunevych requested a review from varsill June 23, 2025 09:32
@mat-hek
Copy link
Member

mat-hek commented Jun 25, 2025

Hi @olegokunevych, even after the fixes, this code looks like a very complex wrapper over :ssl.handshake. Was it generated by some kind of LLM? If so, it's quite hard to trust it, especially since it's about security.

@olegokunevych
Copy link
Contributor Author

Hey @mat-hek thank you for looking over the PR. I've used Claude 4 LLM to assist me in adding this wrapper, it have actually helped with our requirements of configuring SSL options of the membrane_rtmp_plugin library in different ways: using ENVs, passing it as start_link arguments or Elixir App config. I am open for a discussion about how library API should be used to configure SSL options, and of course it needs thorough security validation. I've checked different scenarios which applicable for our deployment variations, they are: no SSL, SSL without verifying peer, SSL with verifying peer. However, I haven't checked all available SSL options, which might be used by users, do you have in mind a specific list of use cases how users may use library options?
Anyway, this PR did work RTMPS for our use case, and I am open for further discussion which particular changes you'd like to see in this PR. I am pretty limited in time which I can invest into this feature, but looking forward to hearing from you any ideas :)

@olegokunevych
Copy link
Contributor Author

@mat-hek @varsill I've simplified an API and removed all the validations for SSL options. Let me know what you think about the current approach.
IMHO it should be responsibility of library to validate passed options, but we can start with this implementation

@mat-hek mat-hek merged commit f218d53 into membraneframework:master Jul 18, 2025
3 checks passed
@varsill
Copy link
Contributor

varsill commented Jul 18, 2025

Once again, thank you very much @olegokunevych for your contribution! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants