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

Kodi - Open ports to allow smb/samba browsing #3503

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

Conversation

zguithues
Copy link
Contributor

@zguithues zguithues commented Nov 11, 2017

browsing smb/samba shares requires having ports 1025 - 65535 open.
Source: https://github.com/dnschneid/crouton/wiki/How-to-mount-network-shares-on-Chromebook-(sshfs,-cifs,-nfs-etc)

this patch grabs the ip address from wlan0 and opens the ports when the source ip is within the local subnet. if wlan0 has no ip, it does not modify iptables.

I'm not sure how many ChromeOS devices have an eth0, or any other interfaces that should be accounted for. or perhaps there is a better way to accomplish the same thing.

iptables gets reset on reboots, so it's probably not a big deal that the rule doesn't get cleared. however, i would love to clear the port forward when kodi closes, or the chroot exits.
do you have any ideas for this @dnschneid ?

browsing smb/samba shares requires having ports 1025 - 65535 open.
this patch grabs the ip address from wlan0 and opens the ports when the source ip is within the local subnet.
Copy link
Contributor

@divx118 divx118 left a comment

Choose a reason for hiding this comment

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

Thanks for creating this pull request.

@@ -14,5 +14,10 @@ By default, it will log into the primary user on the first chroot found.

Options are directly passed to enter-chroot; run enter-chroot to list them."

# Forward ports needed to browse smb shares
if test -n "$(ip -4 -o addr show dev wlan0| awk '{split($4,a,"/");print a[1]}')"; then
Copy link
Contributor

@divx118 divx118 Nov 11, 2017

Choose a reason for hiding this comment

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

This would assume you are using wlan0, you also could be using a usb ethernet adapter or another wlan device. Maybe something like ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

running that command returns a "1"

i agree that we should take into consideration other devices, i just don't know how...

Copy link
Contributor

Choose a reason for hiding this comment

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

@zguithues hmm strange I just tried it on some different linux devices and my chromebook and that seems to work. What do you get when you run ip route get 1 for output. Maybe the awk part isn't full proof. If not, there sure will be other ways to get the used ip-address, without looking at the device.

# Forward ports needed to browse smb shares
if test -n "$(ip -4 -o addr show dev wlan0| awk '{split($4,a,"/");print a[1]}')"; then
iptables -I INPUT 1 -p udp --source $(ip -4 -o addr show dev wlan0| awk '{split($4,a,"/");print a[1]}')/255.255.255.0 --dport 1025:65535 -j ACCEPT
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Max line length should be 80

Copy link
Collaborator

Choose a reason for hiding this comment

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

@divx118 method works for me, here's the output of each:

@zguithues -

chronos@localhost ~ $ ip -4 -o addr show dev wlan0| awk '{split($4,a,"/");print a[1]}'
192.168.1.225

@divx118 -

chronos@localhost ~ $ ip route get 1
1.0.0.0 via 192.168.1.1 dev wlan0 src 192.168.1.225 uid 1000 
    cache 
 
chronos@localhost ~ $ ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}'
192.168.1.225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i got it working @divx118. I was missing the space between the 2 " in the split($2,a," ").

chronos@localhost / $ ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}' 192.168.2.103

i'll update the patch. thanks!

switched from: ip -4 -o addr show dev wlan0| awk '{split($4,a,"/");print a[1]}'
to: ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}'

also used a variable to get line length down.
@zguithues
Copy link
Contributor Author

OK, i've updated the command, and switched to using a variable so i could easily keep the line length down.

The only issue i still have with this is clearing the rule after kodi closes. I'm not sure the best way to do this, but it's not really that big of a deal...

removed the -n from the test to check whether it succeeds, rather than whether the return exists.
@zguithues
Copy link
Contributor Author

ok, previously this would fail if no network exists, i changed the test, removed the -n. This seems to work from my end. please let me know if you see any other issues with the code @DennisLfromGA @divx118

thanks a bunch for your help guys!

Copy link
Contributor

@divx118 divx118 left a comment

Choose a reason for hiding this comment

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

Looks good to me except those little things I mentioned. Thanks

@@ -17,7 +17,7 @@ Options are directly passed to enter-chroot; run enter-chroot to list them."
# Forward ports needed to browse smb shares
MYIP=$(ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}')

if test -n $MYIP; then
if test $MYIP; then
Copy link
Contributor

@divx118 divx118 Nov 12, 2017

Choose a reason for hiding this comment

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

if [ -n "$MYIP" ]; then should work. It also would be more consistent with the syntax elsewhere used in crouton scripts. Try to always quote strings.

@@ -17,7 +17,7 @@ Options are directly passed to enter-chroot; run enter-chroot to list them."
# Forward ports needed to browse smb shares
MYIP=$(ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}')

if test -n $MYIP; then
if test $MYIP; then
iptables -I INPUT 1 -p udp \
--source $MYIP/255.255.255.0 \
Copy link
Contributor

Choose a reason for hiding this comment

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

"$MYIP"/255.255.255.0 \ quotes

@zguithues
Copy link
Contributor Author

thanks @divx118 !

MYIP=$(ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}')

if [ -n "$MYIP" ]; then
iptables -I INPUT 1 -p udp \
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I overlooked this, but indentation is standard 4 spaces (no tabs) in the crouton scripts so it should look like

if [ -n "$MYIP" ]; then
    iptables -I INPUT 1 -p udp \
        --source "$MYIP"/255.255.255.0 \
        --dport 1025:65535 -j ACCEPT
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@divx118 divx118 left a comment

Choose a reason for hiding this comment

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

Great, now let's see if @dnschneid approves it too.

@DennisLfromGA
Copy link
Collaborator

LGTM2 👍

Copy link
Owner

@dnschneid dnschneid left a comment

Choose a reason for hiding this comment

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

For reverting the iptables, maybe add a script inside the chroot that you launch as part of the enter-chroot command, which in turn forks off a subshell that traps HUP or TERM (which gets triggered by unmount-chroot; you'll have to experiment) and then resets the settings?

@@ -14,5 +14,14 @@ By default, it will log into the primary user on the first chroot found.

Options are directly passed to enter-chroot; run enter-chroot to list them."

# Forward ports needed to browse smb shares
MYIP=$(ip route get 1 | awk -F 'src ' '{ split($2,a," ");print a[1];exit}')
Copy link
Owner

Choose a reason for hiding this comment

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

Quote the output, and space out the awk commands. If it doesn't fit on one line, you can make the awk script span multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My limited knowledge indicated 2 places that obviously needed spacing, did i miss anything else?

I think i added the quote properly...

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, that looks better.

@zguithues
Copy link
Contributor Author

@dnschneid Thanks for the direction, but unfortunately that's mostly Chinese to me...

can you point me in the direction of something i can build off of?

@dnschneid
Copy link
Owner

Unfortunately, I don't think there's anything like it at the moment, and this feature has a weird set of requirements

  1. root permission, regardless of which user is running kodi
  2. needs to be triggered when the chroot unmounts
  3. needs to only be triggered when launching kodi

I guess if we accept the caveat that you can only launch one kodi at a time, you can just get rid of the exec keyword in the startkodi script and add a trap on INT/HUP/0 to run the ip command.

@zguithues
Copy link
Contributor Author

zguithues commented Nov 17, 2017

i'll poke around and see what i can come up with.

i'd be happy for suggestions/recommendations from the peanut gallery ;)

@dnschneid do you consider the removal of this iptables rule to be imperative to the merging of this patch?

@RUDIVANTORRE
Copy link

z

@dnschneid
Copy link
Owner

I agree with @RUDIVANTORRE. It should be pretty straightforward to make the change, so I think the merge can wait until you have it working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants