GNOME Bugzilla – Bug 526582
Add SSL/TLS authentication (AUTH TLS) support
Last modified: 2015-04-10 09:20:32 UTC
Would be good to add SSL/TLS auth support to the FTP backend.
Doesn't gvfs provide sftp://?
Yeah but here it's about ftp over ssl
I've been looking at this and it seems the first problem would be libsoups lack of maturity in the SSL area. There doesn't seem to be a way to set up SSL on a plain SoupSocket, since the APIs for this is internal and only exposed via the HTTP-specific classes. For more info see http://live.gnome.org/LibSoup/ToDo
Created attachment 125194 [details] [review] Ugly hack as proof of concept SSL implementation. Wrote up this hack last night. Haven't tested it much, but it seems to work. Whats wrong in it: - Exploits the fact that the hidden/private API of credentials is not hidden in the ABI of the libsoup shared object. - Does no verification of the certificate at all (trusted issuer, expiration, host matching CN, ...) and thus vulnerable to MITM. - Always on (some servers, like vsftpd, sometimes doesn't allow anonymous to start ssl. I think gvfsd-ftp doesn't ask for user/pass until needed, so we don't know which user we're going to log in as when we're starting ssl.) - ... what else?
There are three things that would make me not accept such a patch: 1) regressions. So if we stop working on sites that worked without security, I'm gonna be very annoyed. 2) Dan (whom I just cc'ed). If he says he'd prefer to have this patch not applied for ugliness reasons or whatever, so be it. 3) no security. I guess checking the certs is one thing missing here. I'm not sure just sending stuff SSH'ed is that much of an improvement? I'm no security expert, so I'm gonna just trust what people say. What I like about this patch though is that it's a lot smaller than I thought it would be. Also, I don't want to ever contact the user about if a secure connection is used or not, because usually users don't know how to do sane decisions on security. Which means we need to solve the regression problem of what do we do if securely connecting. Just connecting insecurely sure seems like a bad idea to me.
Andreas and I talked about this a little on IRC last night. I said that I don't want to expose the existing SSL APIs publicly, but I would be willing to make them available -DLIBSOUP_I_KNOW_THIS_IS_NOT_SUPPORTED-style, basically just for gvfs's use. (And when/if libsoup got a new SSL API, the old one would probably disappear the same release.) (The other possibility I suggested was that we could move the core of the ftp code into libsoup as suggested in bug 557777, which would let it use the SSL code without needing to export it.) As for security/checking certs, this patch would give the ftp backend the same behavior/level of security that the http/dav backend has. I have always wanted to make this work better, but at the same time, the fact is that nobody actually understands SSL dialogs, so the increased security is almost entirely theoretical.
Yeah, I think the hardest part here is getting the user interface inside gvfs/gio right, that makes sure your connection is secure. And from Firefox security handling experience, it's clear that this has to work with the only user involvement being a "site is not secure, aborted" dialog. So we need policies! Someone should file bugs about this against nautilus, gio and gtk about that I guess?
(In reply to comment #7) > So we need policies! > Someone should file bugs about this against nautilus, gio and gtk about that I > guess? There was a discussion of this a year ago: http://mail.gnome.org/archives/desktop-devel-list/2007-December/msg00070.html FF3's new behavior seems to be a point against the idea that it's possible to design a useful UI around this. Rather than getting people to stop putting self-signed certs on the public internet (which was presumably part of the intention), it has just made people get very good at clicking 5 times in a row without reading instead of just once.
interesting timing. http://groups.google.com/group/mozilla.dev.tech.crypto/browse_thread/thread/9c0cc829204487bf?tvc=2 So remember that we also need a dialog that says "A bunch of really smart people who know way more about this than you do can't decide whether or not you should trust this certificate. Do you want to trust it?" :-)
Some random thoughts: 1. Everything is better then sending an unencrypted password, that will at least protect me from $random script kiddie 2. Why not provide ftps://username:options@host where options could be: selfsignedok, nonmatchinghostok, ... So those of us who know what they do can use SSL even with companies, that are incapable of provided "good" certificates. 3. gvfs/gio does not need a UI layer, nautilus and gvfs-mount need one ok, but first the gio module has to exist, which can be used by them...
(In reply to comment #10) > 1. Everything is better then sending an unencrypted password, that will at > least protect me from $random script kiddie > I think this is a compelling argument for using encryption - security by obscurity is at least a tiny bit better than no security. So if someone came up with a patch that added this support and properly fell back if things didn't work out (I guess this would require monitoring the whole first login process and redoing it without AUTH on failures (apart from wrong username/password probably?), I'd be happy to commit it. > 2. Why not provide ftps://username:options@host where options could be: > selfsignedok, nonmatchinghostok, ... So those of us who know what they do can > use SSL even with companies, that are incapable of provided "good" > certificates. > I prefer to implement security right. Giving lots of options because we have no clue how to do it right seems like a bad idea. I guess we'll have to wait for the certificate people (go Mozilla!) to figure out what "right" means in this context though, before we can make any useful decisions.
*** Bug 567352 has been marked as a duplicate of this bug. ***
On a related note.... Microsoft seems to have invented a syntax for FEAT replies that I haven't seen anywhere else and can't find any specs indicating if it's valid or not. (Please point out if you know of any document which describes the used syntax!) See debug output in bug #556809 for example. I believe this is an IIS 7.0 server since googling for that AUTH string returns lots of hits for "ftp7" and this is what Microsoft says about it: "Microsoft has created a new FTP service that has been completely rewritten for Windows Server® 2008." The proof of concept patch which was attached to this bug-report will not see the alternative encryption methods (tls-c, ssl and others) but only match on "AUTH TLS". (The same concatenated syntax is also used for the PROT command in the FEAT-reply. The PROT command should be used to say if we want only the command channel or both command+data channels to be encrypted, but this part is completely left out in the patch.)
*** Bug 586247 has been marked as a duplicate of this bug. ***
Setting patch as obsolete since soup is not used anymore. A solution is needed for TLS on GSocket. Possibly gtls.{c,h} from http://git.gnome.org/cgit/gnio could be used...
filed bug 588189 about tls support in gio
*** Bug 607113 has been marked as a duplicate of this bug. ***
the gio tls stuff from bug 588189 (http://gnome.org/~danw/glib.git, tls branch) is to the point where it would be useful to try out porting stuff to it. Basically, you need to create a GTlsClientContext (you only need one for the whole daemon), then when you want to use TLS on a connection, call g_tls_socket_connection_new_client() to wrap the existing GSocketConnection with a GTlsSocketConnection (and use its input/output streams from then on rather than the original ones). This has the reverse problem of the libsoup-based version; it only works if the certificate is completely valid. You can adjust the context's "validation-flags" property or connect to the "accept-certificate" signal if you want the chance to override, but you probably don't until/unless gvfs itself gets better support for this (since you can't interact with the user, etc anyway). Oh, also, looking at the AUTH TLS spec, it looks like the earlier patch only encrypts the control connection, not the data connections.
hello :) any progress?
Created attachment 195550 [details] [review] ftp: Implement AUTH TLS support The code now tries to use TLS whenever it is supported by the server. Unfortunately this causes various issues so we can not connect at all to some servers. So I don't think the code can go to master in its current state.
That is a 2-year old patch that I wrote to test glib's TLS implementation. It might not even compile anymore. Unfortunately I don't remember the issues that existed, but it involved the setup code - some FTP servers claim to support TLS but don't allow the AUTH command, others do allow the AUTH command, but only as the first command ever. Some others require even other orders of commands. So I never committed it.
Any progress? ftps:// or ftpes:// syntax is a lot better than no security at all... :(
+1 for getting this implemented
+1 - ftps:// support would be quite useful.
*** Bug 680217 has been marked as a duplicate of this bug. ***
Hi. Is there any activity on this? More and more providers have ftps enabled and want to have access to an ftps server via Nautilus.
*** Bug 738948 has been marked as a duplicate of this bug. ***
+1 - Really, it would be very helpful to have ftps:// support. We're not giving out ftp accounts anymore to our hosting clients, which means they (and us) cannot use Nautilus anymore for simple web access Using an ftp client again feels soo antiquated.
I'll have a go at implementing this.
Created attachment 298040 [details] [review] daemon: Add function to confirm certificate Add a utility function to present a certificate to the user during a mount to confirm whether to continue or not. This adds a dependency on Gcr, to parse information from the certificate. Based on a patch by Ondrej Holy.
Created attachment 298041 [details] [review] ftp: Implement TLS support Implement TLS support (aka explicit ftps). This is done by using a different URL scheme, ftps, so that it is only used if explicitly specified. Although the protocol allows tansparently upgrading a normal connection to a secure one, there are several problems with this. FEAT is needed to determine support for it but some servers do not allow this before login. Some servers are configured to allow AUTH TLS but have firewalls that block data connections because they can't inspect the traffic. Servers may disallow TLS on the data connection, making it unclear to the user how secure the connection is. Finally, there may be verification errors which need to be presented to the user, and these are unexpected because they did not choose to use ftps. Making secure ftp opt-in as a separate URL scheme side-steps most of these issues as well as ensuring there are no regressions for normal ftp. When using ftps, we assume that the server implements AUTH TLS so the connection is secured before login. It is also assumed that TLS for data connections is allowed, so both control and data connection are secure before any information is transferred. Verification errors are presented during mounting. If the identity changes on subsequent reconnections, the connection is aborted. While presenting verification errors to the user in this way is perhaps not the best way of handling security, it is fairly standard. The implementation has been successfully tested on vsftpd, ProFTPD, Pure-FTPd, IIS, and FileZilla Server. Based on a patch by Benjamin Otte.
Created attachment 298042 [details] [review] ftp: Use TCP_NODELAY When ftp is layered on top of TLS, it does a write-write-read which causes a large amount of latency due to the combination of Nagle's algorithm and delayed ACKs. Use TCP_NODELAY to disable Nagle's algorithm and prevent this. The flag is used for both secure and normal connections. This should not cause any issues.
Well there it is :-) This doesn't implement implicit ftp support. AFAIK, that's not a real standard and servers should be supporting explicit ftps instead. Also note that it depends on a couple of open bugs.
Created attachment 298063 [details] [review] ftp: Implement TLS support Implement TLS support (aka explicit ftps). This is done by using a different URL scheme, ftps, so that it is only used if explicitly specified. Although the protocol allows tansparently upgrading a normal connection to a secure one, there are several problems with this. FEAT is needed to determine support for it but some servers do not allow this before login. Some servers are configured to allow AUTH TLS but have firewalls that block data connections because they can't inspect the traffic. Servers may disallow TLS on the data connection, making it unclear to the user how secure the connection is. Finally, there may be verification errors which need to be presented to the user, and these are unexpected because they did not choose to use ftps. Making secure ftp opt-in as a separate URL scheme side-steps most of these issues as well as ensuring there are no regressions for normal ftp. When using ftps, we assume that the server implements AUTH TLS so the connection is secured before login. It is also assumed that TLS for data connections is allowed, so both control and data connection are secure before any information is transferred. Verification errors are presented during mounting. If the identity changes on subsequent reconnections, the connection is aborted. While presenting verification errors to the user in this way is perhaps not the best way of handling security, it is fairly standard. The implementation has been successfully tested on vsftpd, ProFTPD, Pure-FTPd, IIS, and FileZilla Server. Based on a patch by Benjamin Otte.
Review of attachment 298040 [details] [review]: Good job, but... The output is pretty in cmd, but not sure about using \t chars, we don't know where it will be shown. Also try to connect using Nautilus and see what is shown... ::: configure.ac @@ +75,3 @@ AC_SUBST(giomodulesdir) +PKG_CHECK_MODULES(GCR, gcr-base-3) Not sure if this shouldn't be optional... ::: daemon/gvfsdaemonutils.c @@ +261,3 @@ + if (errors & G_TLS_CERTIFICATE_INSECURE) + g_string_append_printf (reason, "\n\t%s", _("The certificate's algorithm is considered insecure.")); + if (errors & G_TLS_CERTIFICATE_GENERIC_ERROR) Wouldn't be better to check for (reason->len == 0) instead? And you can also remove following "if (reason->len > 0)" check... @@ +266,3 @@ + /* Remove the initial newline. */ + if (reason->len > 0) + g_string_erase (reason, 0, 1); Hmm, what about using \t%s\n and g_string_truncate (reason, reason->len - 1) to avoid memmove, or just return string with new line at the end, because you already add new one in gvfs_accept_certificate ()? @@ +299,3 @@ + "\tIdentity: %s\n" + "\tVerified by: %s\n" + "\tExpiry: %s\n" Wouldn't be better something like "Expires on:"?
Created attachment 298634 [details] what is shown in nautilus
(In reply to Ondrej Holy from comment #36) > Created attachment 298634 [details] > what is shown in nautilus I tested it with Nautilus but not running GNOME shell. It looked fine. Oh well, I'll figure something out.
(In reply to Ondrej Holy from comment #35) > Review of attachment 298040 [details] [review] [review]: > > Good job, but... > > The output is pretty in cmd, but not sure about using \t chars, we don't > know where it will be shown. Also try to connect using Nautilus and see what > is shown... > I think using \t is OK since we already use \n. The gtk mount operation is also ok, but gnome-shell's is broken (see bug 745713). But it's a simple fix and looks good after that.
Created attachment 298670 [details] gnome-shell fixed
(In reply to Ross Lagerwall from comment #38) > (In reply to Ondrej Holy from comment #35) > > Review of attachment 298040 [details] [review] [review] [review]: > > > > Good job, but... > > > > The output is pretty in cmd, but not sure about using \t chars, we don't > > know where it will be shown. Also try to connect using Nautilus and see what > > is shown... > > > > I think using \t is OK since we already use \n. As I said, it is pretty, so let them there and we can remove them in future if it will be needed :-D > The gtk mount operation is > also ok, but gnome-shell's is broken (see bug 745713). But it's a simple fix > and looks good after that. Thanks!
(In reply to Ondrej Holy from comment #35) > > ::: configure.ac > @@ +75,3 @@ > AC_SUBST(giomodulesdir) > > +PKG_CHECK_MODULES(GCR, gcr-base-3) > > Not sure if this shouldn't be optional... > But if it's optional then the certificate message is going to show no useful information. Also, it's required by quite a few gnome packages on a typical system already: $ rpm -q --whatrequires 'libgcr-base-3.so.1()(64bit)' gcr-3.14.0-2.fc21.x86_64 libgdata-0.16.1-1.fc21.x86_64 gnome-keyring-3.14.0-1.fc21.x86_64 empathy-3.12.7-1.fc21.x86_64 seahorse-3.14.0-3.fc21.x86_64 gnome-online-accounts-3.14.3-1.fc21.x86_64 evolution-3.12.10-1.fc21.x86_64 epiphany-runtime-3.14.2-3.fc21.x86_64 epiphany-3.14.2-3.fc21.x86_64 gcr-devel-3.14.0-2.fc21.x86_64 evolution-data-server-3.12.11-1.fc21.x86_64 gnome-shell-3.14.3-1.fc21.x86_64
Ok, fair enough...
(In reply to Ondrej Holy from comment #35) > Review of attachment 298040 [details] [review] [review]: > + g_string_append_printf (reason, "\n\t%s", _("The certificate's > algorithm is considered insecure.")); > + if (errors & G_TLS_CERTIFICATE_GENERIC_ERROR) > > Wouldn't be better to check for (reason->len == 0) instead? And you can also > remove following "if (reason->len > 0)" check... No, because G_TLS_CERTIFICATE_GENERIC_ERROR may be set in addition to one of the other errors to indicate some other certificate error. > > @@ +266,3 @@ > + /* Remove the initial newline. */ > + if (reason->len > 0) > + g_string_erase (reason, 0, 1); > > Hmm, what about using \t%s\n and g_string_truncate (reason, reason->len - 1) > to avoid memmove, or just return string with new line at the end, because > you already add new one in gvfs_accept_certificate ()? Done. > > @@ +299,3 @@ > + "\tIdentity: %s\n" > + "\tVerified by: %s\n" > + "\tExpiry: %s\n" > > Wouldn't be better something like "Expires on:"? I changed it to "Expires:" as is used in Epiphany and a couple of other browsers. Now the first 3 lines are the same as shown by Epiphany in the certificate view (when not showing details).
Created attachment 299237 [details] [review] daemon: Add function to confirm certificate Add a utility function to present a certificate to the user during a mount to confirm whether to continue or not. This adds a dependency on Gcr, to parse information from the certificate. Based on a patch by Ondrej Holy.
Created attachment 299238 [details] [review] ftp: Implement TLS support Implement TLS support (aka explicit ftps). This is done by using a different URL scheme, ftps, so that it is only used if explicitly specified. Although the protocol allows tansparently upgrading a normal connection to a secure one, there are several problems with this. FEAT is needed to determine support for it but some servers do not allow this before login. Some servers are configured to allow AUTH TLS but have firewalls that block data connections because they can't inspect the traffic. Servers may disallow TLS on the data connection, making it unclear to the user how secure the connection is. Finally, there may be verification errors which need to be presented to the user, and these are unexpected because they did not choose to use ftps. Making secure ftp opt-in as a separate URL scheme side-steps most of these issues as well as ensuring there are no regressions for normal ftp. When using ftps, we assume that the server implements AUTH TLS so the connection is secured before login. It is also assumed that TLS for data connections is allowed, so both control and data connection are secure before any information is transferred. Verification errors are presented during mounting. If the identity changes on subsequent reconnections, the connection is aborted. While presenting verification errors to the user in this way is perhaps not the best way of handling security, it is fairly standard. The implementation has been successfully tested on vsftpd, ProFTPD, Pure-FTPd, IIS, and FileZilla Server. Based on a patch by Benjamin Otte.
Created attachment 299239 [details] [review] ftp: Use TCP_NODELAY When ftp is layered on top of TLS, it does a write-write-read which causes a large amount of latency due to the combination of Nagle's algorithm and delayed ACKs. Use TCP_NODELAY to disable Nagle's algorithm and prevent this. The flag is used for both secure and normal connections. This should not cause any issues.
Created attachment 301021 [details] [review] ftp: Implement TLS support Implement TLS support (aka explicit ftps). This is done by using a different URL scheme, ftps, so that it is only used if explicitly specified. Although the protocol allows tansparently upgrading a normal connection to a secure one, there are several problems with this. FEAT is needed to determine support for it but some servers do not allow this before login. Some servers are configured to allow AUTH TLS but have firewalls that block data connections because they can't inspect the traffic. Servers may disallow TLS on the data connection, making it unclear to the user how secure the connection is. Finally, there may be verification errors which need to be presented to the user, and these are unexpected because they did not choose to use ftps. Making secure ftp opt-in as a separate URL scheme side-steps most of these issues as well as ensuring there are no regressions for normal ftp. When using ftps, we assume that the server implements AUTH TLS so the connection is secured before login. It is also assumed that TLS for data connections is allowed, so both control and data connection are secure before any information is transferred. Verification errors are presented during mounting. If the identity changes on subsequent reconnections, the connection is aborted. While presenting verification errors to the user in this way is perhaps not the best way of handling security, it is fairly standard. The implementation has been successfully tested on vsftpd, ProFTPD, Pure-FTPd, IIS, and FileZilla Server. Based on a patch by Benjamin Otte.
The most recent update just updated the patch now that bug 745255 has been done. There is nothing blocking this from going in now, except for reviews :-)
Review of attachment 299239 [details] [review]: Looks good, however would be good to commit into master after gnome-3-16 branch...
(In reply to Ondrej Holy from comment #49) > Review of attachment 299239 [details] [review] [review]: > > Looks good, however would be good to commit into master after gnome-3-16 > branch... It's already been branched.
Patch looks good to me, too.
(In reply to Ross Lagerwall from comment #50) > (In reply to Ondrej Holy from comment #49) > > Review of attachment 299239 [details] [review] [review] [review]: > > > > Looks good, however would be good to commit into master after gnome-3-16 > > branch... > > It's already been branched. Ah, I missed it, thanks for notice...
Review of attachment 299237 [details] [review]: Looks good, please fix the translations before committing... ::: daemon/gvfsdaemonutils.c @@ +324,3 @@ + GTlsCertificateFlags errors) +{ + const char *choices[] = {"Yes", "No", NULL}; _("Yes"), _("No")
(In reply to Ross Lagerwall from comment #48) > The most recent update just updated the patch now that bug 745255 has been > done. > > There is nothing blocking this from going in now, except for reviews :-) Do you know about the following criticals (e.g. when doing enumeration)? (process:21793): GLib-GIO-CRITICAL **: g_tls_client_connection_copy_session_state: assertion 'G_TLS_CLIENT_CONNECTION_GET_INTERFACE (conn)->copy_session_state != NULL' failed
(In reply to Ondrej Holy from comment #54) > (In reply to Ross Lagerwall from comment #48) > > The most recent update just updated the patch now that bug 745255 has been > > done. > > > > There is nothing blocking this from going in now, except for reviews :-) > > Do you know about the following criticals (e.g. when doing enumeration)? > (process:21793): GLib-GIO-CRITICAL **: > g_tls_client_connection_copy_session_state: assertion > 'G_TLS_CLIENT_CONNECTION_GET_INTERFACE (conn)->copy_session_state != NULL' > failed You need to have the latest glib-networking, as the tls stuff is implemented there.
Dammit, I rebuilt only pure glib before... it seems working correctly with glib-networking master!
Review of attachment 301021 [details] [review]: Thanks, overall looks good to me, just... Can't we detect that only secured connection is allowed and fail in nicer way (yes, this is another bug, but just asking)? $ gvfs-mount ftp://localhost/ Enter password for localhost User: oholy Password: Enter password for localhost User [oholy]: Password: Error mounting location: Unexpected end of stream ::: configure.ac @@ -62,3 @@ AC_SUBST(DISTCHECK_CONFIGURE_FLAGS) -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.43.2 gobject-2.0 gmodule-no-export-2.0 gio-unix-2.0 gio-2.0 ) I think we should to add dependency also on glib-networking to make the dependency more explicit (especially for me :-D)... ::: daemon/gvfsbackendftp.c @@ +395,3 @@ + if (ftp->server_identity) + g_object_unref (ftp->server_identity); g_clear_object? @@ +398,3 @@ + + if (ftp->certificate) + g_object_unref (ftp->certificate); g_clear_object? @@ +462,3 @@ + /* Secure the initial connection if necessary. This may result in a prompt + * for the user. */ + ftp->mount_source = g_object_ref (mount_source); Is the additional object reference needed? Wouldn't be enough: ftp->mount_source = mount_source; ... ftp->mount_source = NULL; @@ +470,3 @@ + return; + } + g_object_unref (ftp->mount_source); However it should be cleared anyway, so another possibility is: ftp->mount_source = g_object_ref (mount_source); ... g_clear_object (&ftp->mount_source);
(In reply to Ondrej Holy from comment #57) > I think we should to add dependency also on glib-networking to make the > dependency more explicit (especially for me :-D)... Or maybe just have the ftps mount code check if G_TLS_CLIENT_CONNECTION_GET_INTERFACE(conn)->copy_session_state is non-NULL, and return an error to the user if not. (libsoup does something similar if you try to connect to an https site but don't have glib-networking installed)
(In reply to Ondrej Holy from comment #57) > Review of attachment 301021 [details] [review] [review]: > > Thanks, overall looks good to me, just... > > -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.43.2 gobject-2.0 > gmodule-no-export-2.0 gio-unix-2.0 gio-2.0 ) > > I think we should to add dependency also on glib-networking to make the > dependency more explicit (especially for me :-D)... > glib-networking doesn't provide a pkgconfig file and will be kept in sync with glib in most cases. (In reply to Dan Winship from comment #58) > (In reply to Ondrej Holy from comment #57) > > I think we should to add dependency also on glib-networking to make the > > dependency more explicit (especially for me :-D)... > > Or maybe just have the ftps mount code check if > G_TLS_CLIENT_CONNECTION_GET_INTERFACE(conn)->copy_session_state is non-NULL, > and return an error to the user if not. (libsoup does something similar if > you try to connect to an https site but don't have glib-networking installed) I don't think this is necessary. It's a non-fatal error if copy_session_state fails -- some ftps servers are configured to require it and some aren't, but those that *do* require it will generate an error that gets passed to the user anyway.
Created attachment 301164 [details] [review] ftp: Implement TLS support Implement TLS support (aka explicit ftps). This is done by using a different URL scheme, ftps, so that it is only used if explicitly specified. Although the protocol allows tansparently upgrading a normal connection to a secure one, there are several problems with this. FEAT is needed to determine support for it but some servers do not allow this before login. Some servers are configured to allow AUTH TLS but have firewalls that block data connections because they can't inspect the traffic. Servers may disallow TLS on the data connection, making it unclear to the user how secure the connection is. Finally, there may be verification errors which need to be presented to the user, and these are unexpected because they did not choose to use ftps. Making secure ftp opt-in as a separate URL scheme side-steps most of these issues as well as ensuring there are no regressions for normal ftp. When using ftps, we assume that the server implements AUTH TLS so the connection is secured before login. It is also assumed that TLS for data connections is allowed, so both control and data connection are secure before any information is transferred. Verification errors are presented during mounting. If the identity changes on subsequent reconnections, the connection is aborted. While presenting verification errors to the user in this way is perhaps not the best way of handling security, it is fairly standard. The implementation has been successfully tested on vsftpd, ProFTPD, Pure-FTPd, IIS, and FileZilla Server. Based on a patch by Benjamin Otte.
Thanks for the review. (In reply to Ondrej Holy from comment #57) > Review of attachment 301021 [details] [review] [review]: > > Thanks, overall looks good to me, just... > > Can't we detect that only secured connection is allowed and fail in nicer > way (yes, this is another bug, but just asking)? > $ gvfs-mount ftp://localhost/ > Enter password for localhost > User: oholy > Password: > Enter password for localhost > User [oholy]: > Password: > Error mounting location: Unexpected end of stream It possibly could be detected, but there's many different ways that an ftp login can fail (e.g. max users exceeded) so this should be handled separately. > > ::: configure.ac > @@ -62,3 @@ > AC_SUBST(DISTCHECK_CONFIGURE_FLAGS) > > -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.43.2 gobject-2.0 > gmodule-no-export-2.0 gio-unix-2.0 gio-2.0 ) > > I think we should to add dependency also on glib-networking to make the > dependency more explicit (especially for me :-D)... > > ::: daemon/gvfsbackendftp.c > @@ +395,3 @@ > > + if (ftp->server_identity) > + g_object_unref (ftp->server_identity); > > g_clear_object? Done. > > @@ +398,3 @@ > + > + if (ftp->certificate) > + g_object_unref (ftp->certificate); > > g_clear_object? Done. > > @@ +462,3 @@ > + /* Secure the initial connection if necessary. This may result in a prompt > + * for the user. */ > + ftp->mount_source = g_object_ref (mount_source); > > Is the additional object reference needed? Wouldn't be enough: > > ftp->mount_source = mount_source; > ... > ftp->mount_source = NULL; > > @@ +470,3 @@ > + return; > + } > + g_object_unref (ftp->mount_source); > > However it should be cleared anyway, so another possibility is: > > ftp->mount_source = g_object_ref (mount_source); > ... > g_clear_object (&ftp->mount_source); OK, I removed the ref.
Review of attachment 301164 [details] [review]: Ok! Before committing fix typo in the message: s/tansparently/transparently/
Pushed to master as ab4e591..4667ada. Thanks for the reviews. Yay, closing 7 year old bugs :-)