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.

Sign up for GitHub

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

Jump to bottom

Implement sending of IGMPv3 query #80

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Implement sending of IGMPv3 query #80

wants to merge 7 commits into from

Conversation

zocker007
Copy link

According to RFC 2236, IGMPv2/v1 clients should ignore additional fields in query, but build the checksum over the whole IP-Packet. So this is designed for future compatibility and it should be by design to send IGMPv3 queries, when a v3 capable router is in the network.

@pali
Copy link
Owner

pali commented Jun 21, 2021

Hello! Could you describe why is this change needed? Is it fixing some bug or are there any issues with current implementation?

@zocker007
Copy link
Author

This PR is a preparation for following commits regarding IGMPv3 source specific multicast. The first step here is to implement sending of IGMPv3 queries.

@pali
Copy link
Owner

pali commented Jun 22, 2021

Ok. Now I see the reason.

Anyway, I'm not sure if it is safe to "upgrade" all packets unconditionally. RFC documents are strict how client should behave but because I know there are tons of broken software it is not really a good idea to upgrade code to always use IGMPv3 packets only.

What do you think about it?

Would not it be better to add e.g. config option for specifying which IGMP protocol version should be used on particular interface?

@zocker007
Copy link
Author

Yes, thats a good point. I think an config option would be good. I will show how to add a config flag and use it.

@pali
Copy link
Owner

pali commented Jun 24, 2021

Ok, please look at this how hard is to add config option and if you can do it during your IGMPv3 support on downstream interfaces.

@zocker007 zocker007 changed the title Implement sending of IGMPv3 query always Implement sending of IGMPv3 query Feb 18, 2022
@zocker007
Copy link
Author

Ok I have added a config option to enable IGMPv3 on queries.

@horolux
Copy link

horolux commented Feb 21, 2022

Hi, does this mean that IGMPv3 will now be supported (new version released soon)?

@TheJulianJES
Copy link

TheJulianJES commented Apr 5, 2022 •
edited
Loading

@horolux I'm not sure if this PR is going anywhere atm, but it looks like there might be two versions of igmpproxy with IGMPv3 support:
https://github.com/zocker007/igmpproxy from @zocker007
https://github.com/Uglymotha/igmpv3proxy from @Uglymotha

I'm guessing you'll have to build the versions manually.

I have yet to try their versions, but I'm hoping I'll get to it in the future (both forks also don't have Issue trackers open right now).
I very much appreciate the work of both (and pali too of course!)

Edit: It also seems like the original igmpproxy includes some sort of "IGMPv3 client support" since v0.2.
Edit 2: Feels a bit weird to ask here, but in case you're seeing this Uglymotha/zocker007, are there pre-built (debian) (arm64) packages for your custom igmpproxy versions?

@pali
Copy link
Owner

pali commented Apr 6, 2022

I'm planning to look and review this change properly. Just need to find some free time for it.

pali
pali requested changes Apr 9, 2022
View reviewed changes
Copy link
Owner

@pali pali left a comment

Choose a reason for hiding this comment

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

Hello! I have looked at this change. Basically it looks good, just some minor issues. See below.

Anyway, what about renaming option igmpv3 to proto with parameter? E.g. in config option would be written proto igmpv3 or proto igmpv2 (or igmpv1). This would make it more generic for future extensions.

src/igmpv3.h Outdated Show resolved Hide resolved
src/igmpv3.h Outdated Show resolved Hide resolved
doc/igmpproxy.conf.5.in Outdated
Comment on lines 81 to 89
.B igmpv3
.RS
Enables IGMPv3 queries. On default settings, IGMPv1/2 queries are send out.
With this setting, you can enable IGMPv3 frames for sending queries. This is needed
for IGMPv3 capable hosts to send IGMPv3 reports. When a IGMPv3 capable host receives
a IGMPv1/2 query, it answers with a IGMPv1/2 report.
.RE
Copy link
Owner

Choose a reason for hiding this comment

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

This option affects downstream interfaces, so it should be in phyint section, not the global.

src/os-freebsd.h Outdated Show resolved Hide resolved
src/igmp.c
Comment on lines +302 to +307
igmp->igmp_cksum = inetChksum((unsigned short *)igmp,
IGMP_V3_QUERY_MINLEN + datalen);
} else {
igmp->igmp_cksum = inetChksum((unsigned short *)igmp,
IP_HEADER_RAOPT_LEN + datalen);
}
Copy link
Owner

Choose a reason for hiding this comment

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

You have stored query length in the query_minlen variable. So you can move inetChksum code outside of the if and call it based on the query_minlen.

@pali pali mentioned this pull request Apr 9, 2022
Open
@itobi1

This comment was marked as off-topic.

Sign in to view
@itobi1 itobi1 mentioned this pull request Apr 25, 2022
Closed
@itobi1

This comment was marked as off-topic.

Sign in to view
@pali
Copy link
Owner

pali commented Dec 20, 2022

@zocker007 Could you address above comments for this PR?

@zhao-wuji-ahh
Copy link

@pali Hi，why don't merge the changes about igmpv3 query in version 0.4? I still don't know how to enable the igmpv3...

@pali
Copy link
Owner

pali commented Jan 17, 2023

why don't merge the changes about igmpv3

Because there is no change ready. See review comments above.

@tasheers
Copy link

@pali could you please look at this PR and merge as @zocker007 has made the changes since last time ? look forward to using igmpv3

@OlympusHo
Copy link

I use the github igmpproxy：eb0aec2a508e21e7d87429ed24b2511a35a6a891 and
#80, it can send igmpv3 query on downstream interfaces, but when the downstream device sends an IGMPv3 message (with a source address), the packet captured on the WAN side of the router shows 'any sources'. how can i solve this ?

@Uglymotha
Copy link
Contributor

Uglymotha commented Jun 25, 2024 via email

@OlympusHo
Copy link

OlympusHo commented Jul 1, 2024 •
edited
Loading

You can’t. Palis version is igmpv2 only, you can have it send an igmpv3 query (easy) but it will not proxy (or even process) an igmpv3 group report (complex). Read chapters 5, 6 and 7 from RFC3376 and you will find that it is not easy to implement a proxy, that has to play group member (chapter 5) downstream and router upstream (chapter 6), whilst maintaining igmpv2 compatibility (chapter 7). I will state once again that the igmpv3 proxy in my repo should be mostly functional but largely untested. I had to halt development since I have not been using any IPTV services myself for the past 2 years. Please get the igmpv3 proxy from my repo and see what that does for you, I am willing to help as much as I can. GitHub - Uglymotha/igmpv3proxy: IGMP multicast routing daemonhttps://github.com/Uglymotha/igmpv3proxy Configuration loglevel 7 logfile /tmp/igmp.log defaultfilterany phyint ... downstream ratelimit 0 threshold 1 phyint ... upstream ratelimit 0 threshold 1 nodefaultfilter filter 0.0.0.0/0 224.0.0.0/4 a
…
-Sietse Van: OlympusHo @.> Verzonden: Tuesday, 25 June 2024 11:02 Aan: pali/igmpproxy @.> CC: Sietse van Zanen @.>; Mention @.> Onderwerp: Re: [pali/igmpproxy] Implement sending of IGMPv3 query (#80) I use the github igmpproxy：eb0aec2a508e21e7d87429ed24b2511a35a6a891 and #80<#80>, it can send igmpv3 query on downstream interfaces, but when the downstream device sends an IGMPv3 message (with a source address), the packet captured on the WAN side of the router shows 'any sources'. how can i solve this ? — Reply to this email directly, view it on GitHub<#80 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC3S6AJTDPS4CZLCSJIUXFLZJEWZTAVCNFSM6AAAAABJ3MNDTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBYGM3DENJWGQ. You are receiving this because you were mentioned.Message ID: @.***>

Thanks for your reply! Can you open issue on https://github.com/Uglymotha/igmpv3proxy? When I configure the file as follows, the igmpproxy fails to start. Here is the log.
Configuration
phyint eth1.4 upstream ratelimit 0 threshold 1
altnet 0.0.0.0/0

phyint br-lan downstream ratelimit 0 threshold 1

igmpproxy-v3 start fail log:
Config: Got a phyint token.
Config: IF: Config for interface eth1.4.
Config: IF: Got upstream token.
Config: IF: Got ratelimit token '0'.
Config: IF: Got threshold token '1'.
Config: IF: Got altnet token 0.0.0.0/0.
Config: IF: Altnet: Parsed altnet to default.
IF name : eth1.4
Next ptr : 0
Ratelimit : 0
Threshold : 1
State : 1
Allowednet ptr : 54babe00
Config: Got a phyint token.
Config: IF: Config for interface br-lan.
Config: IF: Got downstream token.
Config: IF: Got ratelimit token '0'.
Config: IF: Got threshold token '1'.
IF name : br-lan
Next ptr : 0
Ratelimit : 0
Threshold : 1
State : 2
Allowednet ptr : 0
buildIfVc: Interface lo Index: 1 Addr: 127.0.0.1, Flags: 0x0049, Network: 127/8
buildIfVc: Interface br-lan Index: 18 Addr: 192.168.50.1, Flags: 0x1043, Network: 192.168.50/24
Found config for br-lan
adding VIF, Ix 0 Fl 0x8 IP 0x00000012 br-lan, Threshold: 1, Ratelimit: 0
Network for [br-lan] : 192.168.50/24
There must be at least 1 Vif as upstream.

@Uglymotha
Copy link
Contributor

Uglymotha commented Jul 8, 2024 via email

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

@pali pali pali requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@zocker007 @pali @horolux @TheJulianJES @itobi1 @zhao-wuji-ahh @tasheers @OlympusHo @Uglymotha