GNOME Bugzilla – Bug 322966
password required option for sharing
Last modified: 2006-12-08 00:25:07 UTC
Would be nice to have the option of requiring a password for shares.
This should probably be switched to the DAAP component.
I wrote the DAAP code this summer. I did not add password protectiohttp://bugzilla.gnome.org/show_bug.cgi?id=322966n to personal shares because I feel it is not a useful feature. I believe it would be relatively easy to do, modify the share object to request a password (iirc its just normal HTTP auth) after it recieves a connection, so if ya'll feel it is a much needed feature, there you go. I'd be happy to answer any questions about how to do it via email. That being said, why do you feel you need this feature? Imagine your position if you were looking to listen to a variety of music, you fire up RB and browse some shared music on your LAN. You go to click on someones music, and it asks you for a password - how frustrating. Now you need to figure out who that is, where they are and ask for the password. Screw that. Doesn't it go against the whole "sharing music" credo to require a password? Just my two cents though.
Here's a use case: you run an open wireless access point in your house, but you don't want somebody sitting outside in their car listening to your music. In the US sharing bandwidth probably won't violate your terms of service, but sharing your music might incur the wrath of the RIAA. ;-)
(In reply to comment #3) > but sharing your music might incur the wrath of the RIAA. ;-) ...which is annoying and perhaps paranoid, but that's the world we live in, unfortunately.
DAAP passwords provide no meaningful security, as the password is sent in cleartext. The "somebody sitting outside in their car" just has to sniff network traffic for a while and they'll get the password.
Disagree that is provides "no meaningful security". If it uses Basic-Auth then it is cleartext, yes. Even with Basic it is meaningful and useful if it prevents average person using iTunes from snooping. I'm working on this now.
Created attachment 58026 [details] [review] initial patch This seems to work pretty well. It puts the password in gconf for now. I don't think that is a problem considering it is sent in clear text over the wire anyway. I tested this with both howl and avahi. I noticed that when using avahi I'm getting service name collisions. I don't know if that has anything to do with this patch or not since I only started using avahi for this testing. I found a few other bugs that we might want to fix separately. Seems like we should pop up the password dialog when we get auth denied even if the published data doesn't say it requires a password. Seems like there is a problem with retrying to connect to a share when auth fails or is cancelled - probably related to the former.
Created attachment 58048 [details] [review] updated patch OK, the not reconnecting problem was because the server wasn't handling the /logout path. This should fix it. Still not sure about the namespace collision. Might have something to do with the fact that we create a new entry group when any parameters of the service change instead of updating the existing group. The docs seem to recommend against doing this: http://avahi.org/download/doxygen/main.html The easiest way to do that I think is to make rb-daap-mdns into a class and split the implementation howl vs. avahi into separate files. This will make it much easier to maintain the publish state. But I think that is a separate bug. What do you think?
If we're going to do this, we really should give each client a unique session ID (rather than using 42 for all clients) and check a valid ID was presented for all requests. Other than that, this looks OK to me.
I'm glad you mentioned that. I forgot to mention that for some reason all logout requests seem to be using a zero for ID. But I agree and it shouldn't be difficult to create a unique id.
If it is OK with you and James then I'd like to commit this and work on those other issues separately so this patch doesn't get unweildy. OK?
Looks OK to me.
FYI, I'm going ahead with making the MDNS stuff into two classes.
Latest patch does apply cleanly against CVS HEAD as of now for me: $ patch -p0 < patches/rb-daap-pw2.patch patching file daapsharing/rb-daap-connection.c Hunk #5 FAILED at 765. Hunk #6 succeeded at 898 (offset 19 lines). 1 out of 6 hunks FAILED -- saving rejects to file daapsharing/rb-daap-connection.c.rej patching file daapsharing/rb-daap-dialog.c patching file daapsharing/rb-daap-mdns.c patching file daapsharing/rb-daap-mdns.h patching file daapsharing/rb-daap-share.c patching file daapsharing/rb-daap-share.h patching file daapsharing/rb-daap-sharing.c patching file data/rhythmbox.schemas Hunk #1 succeeded at 552 (offset 22 lines). patching file data/glade/daap-prefs.glade patching file lib/rb-preferences.h patching file shell/rb-shell-preferences.c I'll attach the rejected diff.
Created attachment 58137 [details] Rejected diffs from patch Probably due to this recent commit, which was probably applied after the patch was generated: 2006-01-25 James Livingston <jrl@ids.org.au> patch by: Christope Fergeau <teuf@gnome.org> * daapsharing/rb-daap-connection.c: (g_zalloc_wrapper), (http_response_handler): fix a potential buffer overflow issue.
Yes I know about it. Thanks. I should have another patch ready shortly. James, as an aside I think the logic in that fix was reversed.
Created attachment 58172 [details] [review] patch This splits up rb-daap-mdns into two singleton classes with implementations split into separate files. I think this makes it much easier to support and handle things like reconnecting etc. I've tested this with howl and avahi. Does this look OK to commit?
I haven't looked at the earlier patches, but the latest patch has some issues (with Avahi 0.6 on Dapper) * When turning the password on while RB is sharing, I get a name service collision. * in share_password_entry_focus_out_event_cb: if old_pw is NULL, then the new password won't be stored. Apart from that it works well.
Patch applied cleanly, but fails to compile with howl (I did a "make clean", "./autogen.sh", "./configure " make): gcc -DHAVE_CONFIG_H -I. -I. -I.. -DGNOMELOCALEDIR=\"/usr/local//share/locale\" -DG_LOG_DOMAIN=\"Rhythmbox\" -I.. -I../lib -I../lib -I../corba -I../corba -I../rhythmdb -I../library -I../iradio -I../widgets -I../shell -I../sources -DPIXMAP_DIR=\"/usr/local//share/pixmaps\" -DSHARE_DIR=\"/usr/local//share/rhythmbox\" -DDATADIR=\"/usr/local//share\" -DXTHREADS -D_REENTRANT -DXUSE_MTSAFE_API -DORBIT2=1 -pthread -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/X11R6/include -I/usr/include/atk-1.0 -I/usr/include/pango-1.0 -I/usr/include/freetype2 -I/usr/include/freetype2/config -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libgnomeui-2.0 -I/usr/include/libgnome-2.0 -I/usr/include/libgnomecanvas-2.0 -I/usr/include/libart-2.0 -I/usr/include/gconf/2 -I/usr/include/libbonoboui-2.0 -I/usr/include/orbit-2.0 -I/usr/include/libbonobo-2.0 -I/usr/include/gnome-vfs-2.0 -I/usr/lib/gnome-vfs-2.0/include -I/usr/include/bonobo-activation-2.0 -I/usr/include/libxml2 -I/usr/include/libglade-2.0 -I/usr/include/gnome-vfs-module-2.0 -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -I/usr/include/libsoup-2.2 -I/usr/include/libxml2 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/howl-0.9.8 -g -O2 -Wcomment -Wformat -Wnonnull -Wimplicit-int -Wimplicit -Wmain -Wmissing-braces -Wparentheses -Wsequence-point -Wreturn-type -Wswitch -Wtrigraphs -Wunused-function -Wunused-label -Wunused-value -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wall -Werror -std=gnu89 -MT rb-daap-mdns-browser-howl.lo -MD -MP -MF .deps/rb-daap-mdns-browser-howl.Tpo -c rb-daap-mdns-browser-howl.c -fPIC -DPIC -o .libs/rb-daap-mdns-browser-howl.o cc1: warnings being treated as errors rb-daap-mdns-browser-howl.c: In function 'host_is_local': rb-daap-mdns-browser-howl.c:144: warning: implicit declaration of function 'gnome_vfs_address_equal' rb-daap-mdns-browser-howl.c:144: warning: nested extern declaration of 'gnome_vfs_address_equal' make[1]: *** [rb-daap-mdns-browser-howl.lo] Error 1 make[1]: Leaving directory `/home/alex/src/remote-cvs/gnome.org/rhythmbox/daapsharing' make: *** [all] Error 2
Lastly, a DAAP related problem that may or may not be related to passwords. Before applying the patch, I was attempting to connect to an iTunes server with a password that I have been using reliably for several months now. Now it appears in the sidepane, but when I click it, nothing appears, no password prompt, no error message. I thought it was something to do with the mDNSResponder so I restarted that, but still the same problem. I checked and restarted the iTunes server, made sure that the port was open, all seems fine. In the debug log (below) it looks like it's creating a connection OK, then the last thing it writes is about the http 200 error, but it mysteriously says "OK". Any ideas? [0x8ad3128] [rb_daap_connection_constructor] rb-daap-connection.c:1441 (01:16:10): Creating new DAAP connection to 192.168.0.4:3689 [0x8ad3128] [rb_daap_connection_do_something] rb-daap-connection.c:1540 (01:16:10): Getting DAAP server info [0x8ad3128] [http_get] rb-daap-connection.c:927 (01:16:10): Queued message for http://192.168.0.4:3689//server-info [0x8ad3128] [rb_shell_clipboard_set_property] rb-shell-clipboard.c:274 (01:16:10): selected source 0x8de6678 [0x8ad3128] [rb_shell_clipboard_sync] rb-shell-clipboard.c:387 (01:16:10): syncing clipboard [0x8ad3128] [rb_shell_player_set_property] rb-shell-player.c:650 (01:16:10): selected source 0x8de6678 [0x8ad3128] [rb_shell_player_sync_with_selected_source] rb-shell-player.c:2128 (01:16:10): syncing with selected source: 0x8de6678 [0x8ad3128] [rb_shell_player_sync_buttons] rb-shell-player.c:1907 (01:16:10): syncing with source 0x8de6678 [0x8ad3128] [rb_source_header_set_property] rb-source-header.c:232 (01:16:10): selected source 0x8de6678 [0x8ad3128] [rb_statusbar_set_property] rb-statusbar.c:287 (01:16:10): selected source 0x8de6678 [0x8ad3128] [http_response_handler] rb-daap-connection.c:883 (01:16:10): Error getting http://192.168.0.4:3689/: 200, OK
It's definitely this change: 2006-01-25 James Livingston <jrl@ids.org.au> patch by: Christope Fergeau <teuf@gnome.org> * daapsharing/rb-daap-connection.c: (g_zalloc_wrapper), (http_response_handler): fix a potential buffer overflow issue. http://cvs.gnome.org/viewcvs/rhythmbox/daapsharing/rb-daap-connection.c?r1=1.13&r2=1.14 If I revert this, then all is well and I can connect. Should I open a separate bug on this?
No, I just fixed it.
(In reply to comment #21) > Should I open a separate bug on this? Done: bug #328816.
(In reply to comment #22) > No, I just fixed it. Sorry 'about that, was a mid-air collision, I'll test the fix and close the bug myself.
Created attachment 58231 [details] [review] updated patch Should fix the prefs problem but not the name collision one. Some info here: http://avahi.org/ticket/5 http://avahi.org/ticket/6
Created attachment 58232 [details] [review] updated patch Sweet. After making all the changes suggested by lennart the collision problem seems to be fixed. Might be the change to using avahi_entry_group_add_service instead of avahi_entry_group_add_service_strlst. Look OK to commit?
Not yet.
Created attachment 58239 [details] [review] updated patch This makes sure to a base64 encoding for the Authorization token. It also fixes requiring Authorization for /database/ requests. This patch still doesn't address using unique session ids or making rb-connection understand 401 responses. I think we can do those separately. I've tested this with iTunes and Rhythmbox. Please give this a try.
This works well for me. We're aiming to release 0.9.3 in a few days, and I'd notified the i18n list, so it's probably better to wait until just after the release to commit.
This should be fine to commit now, unless you wanted to get per-session client id working first.
Cool. I committed this. I'll open a new bug for the session id stuff. Thanks.
*** Bug 383561 has been marked as a duplicate of this bug. ***