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

Security: Enable CIDR for allow/deny play/publish #1753

Closed
wants to merge 6 commits into from
Closed

Security: Enable CIDR for allow/deny play/publish #1753

wants to merge 6 commits into from

Conversation

macabu
Copy link
Contributor

@macabu macabu commented May 8, 2020

Previously you could only specify static IPs to the security config, this PR enables specifying ranges of IPs in a network.
Tests added and passing.

Fixes #1190

Thanks

@matclayton
Copy link

This would be very useful for us, any chance this could be merged?

@winlinvip winlinvip self-assigned this Aug 27, 2021
@winlinvip winlinvip added this to the SRS 4.0 release milestone Aug 27, 2021
@winlinvip
Copy link
Member

Thanks for your PR. 👍

I think it's very good to support CIDR for security, and the utest is in this PR which is very nice.

However, I need more time to think about it.

Please keep this PR here and others could pick it to their repository and test it.

@winlinvip winlinvip changed the title [Feature] Enable specifying range of IPs in a subnet for allow/deny play/publish Security: Enable specifying range of IPs in a subnet for allow/deny play/publish Oct 19, 2021
@winlinvip winlinvip changed the title Security: Enable specifying range of IPs in a subnet for allow/deny play/publish Security: Enable CIDR for allow/deny play/publish Oct 19, 2021
@macabu
Copy link
Contributor Author

macabu commented Oct 20, 2021

Thanks for your PR. 👍

I think it's very good to support CIDR for security, and the utest is in this PR which is very nice.

However, I need more time to think about it.

Please keep this PR here and others could pick it to their repository and test it.

Hi @winlinvip , there was another change that didn't reach this PR, it's this commit: https://github.com/mycujoo/srs/commit/6a29f5984e2b2120d5a63ac89514a6719f95aa8d

Unfortunately I don't have access to the repository anymore so I can't update the PR with this commit.

It's missing tests, but the functionality is supposed to make the allow rule take precedence over deny, so if you have something like deny publish 10.0.0.0/8; but allow publish 10.0.1.1; this would allow 10.0.1.1 to publish.

This is trying to copy nginx's behaviour more or less.

I would recommend if you plan to merge or improve this to consider adding that commit as well :)

@winlinvip
Copy link
Member

Really appreciate this patch, thanks a lot.

I notice there is some codes copied from blog, but the LICENSE is not clear, so please rewrite these functions. Please check the code from Go or Nginx, both LICENSE are ok. 😄

// Convert an IPv4 from string to uint32_t. Extracted from https://www.stev.org/post/ccheckanipaddressisinaipmask
extern uint32_t srs_ipv4_to_num(std::string ip);

// Whether the IPv4 is in an IP mask. Extracted from https://www.stev.org/post/ccheckanipaddressisinaipmask
extern bool srs_ipv4_within_mask(std::string ip, std::string network, std::string mask);

@winlinvip
Copy link
Member

winlinvip commented Feb 12, 2022

@macabu @matclayton I have reviewed this PR and some codes are delivered under unspecified license, copied from the blog website, so @duiniuluantanqin fixed this and file a new PR #2914 which is based on this one (the commits are kept and new commits had been added).

Moved to #2914 and this PR will be closed.

Appreciated for all you 👍

@winlinvip winlinvip closed this Feb 12, 2022
@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can SRS Security rules add network segments?
4 participants