After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 526582 - Add SSL/TLS authentication (AUTH TLS) support
Add SSL/TLS authentication (AUTH TLS) support
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: ftp backend
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gvfs-maint
gvfs-maint
: 567352 586247 607113 680217 738948 (view as bug list)
Depends on: 588189 745099 745255 745713
Blocks: 708306
 
 
Reported: 2008-04-06 20:41 UTC by Guillaume Desmottes
Modified: 2015-04-10 09:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Ugly hack as proof of concept SSL implementation. (2.03 KB, patch)
2008-12-23 13:31 UTC, Andreas Henriksson
none Details | Review
ftp: Implement AUTH TLS support (13.20 KB, patch)
2011-09-03 01:11 UTC, Benjamin Otte (Company)
none Details | Review
daemon: Add function to confirm certificate (7.39 KB, patch)
2015-02-26 22:53 UTC, Ross Lagerwall
none Details | Review
ftp: Implement TLS support (21.04 KB, patch)
2015-02-26 22:53 UTC, Ross Lagerwall
none Details | Review
ftp: Use TCP_NODELAY (2.61 KB, patch)
2015-02-26 22:53 UTC, Ross Lagerwall
none Details | Review
ftp: Implement TLS support (21.07 KB, patch)
2015-02-27 07:59 UTC, Ross Lagerwall
none Details | Review
what is shown in nautilus (20.82 KB, image/png)
2015-03-05 12:36 UTC, Ondrej Holy
  Details
gnome-shell fixed (30.64 KB, image/png)
2015-03-05 23:44 UTC, Ross Lagerwall
  Details
daemon: Add function to confirm certificate (7.28 KB, patch)
2015-03-12 21:08 UTC, Ross Lagerwall
committed Details | Review
ftp: Implement TLS support (21.76 KB, patch)
2015-03-12 21:08 UTC, Ross Lagerwall
none Details | Review
ftp: Use TCP_NODELAY (2.61 KB, patch)
2015-03-12 21:09 UTC, Ross Lagerwall
committed Details | Review
ftp: Implement TLS support (22.26 KB, patch)
2015-04-06 14:07 UTC, Ross Lagerwall
none Details | Review
ftp: Implement TLS support (22.17 KB, patch)
2015-04-08 21:54 UTC, Ross Lagerwall
committed Details | Review

Description Guillaume Desmottes 2008-04-06 20:41:28 UTC
Would be good to add SSL/TLS auth support to the FTP backend.
Comment 1 Martin Jürgens 2008-05-27 17:20:49 UTC
Doesn't gvfs provide sftp://?
Comment 2 Laurent Bigonville 2008-05-27 19:43:55 UTC
Yeah but here it's about ftp over ssl
Comment 3 Andreas Henriksson 2008-11-15 14:19:56 UTC
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
Comment 4 Andreas Henriksson 2008-12-23 13:31:34 UTC
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?
Comment 5 Benjamin Otte (Company) 2008-12-23 13:53:56 UTC
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.
Comment 6 Dan Winship 2008-12-23 15:08:59 UTC
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.
Comment 7 Benjamin Otte (Company) 2008-12-23 15:31:27 UTC
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?
Comment 8 Dan Winship 2008-12-23 15:59:43 UTC
(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.
Comment 9 Dan Winship 2008-12-23 21:29:48 UTC
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?" :-)
Comment 10 Matthias Bläsing 2009-01-08 11:46:31 UTC
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...
Comment 11 Benjamin Otte (Company) 2009-01-08 12:19:34 UTC
(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.
Comment 12 A. Walton 2009-01-13 15:58:24 UTC
*** Bug 567352 has been marked as a duplicate of this bug. ***
Comment 13 Andreas Henriksson 2009-01-28 14:23:11 UTC
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.)
Comment 14 Jon Dowland 2009-06-23 12:32:36 UTC
*** Bug 586247 has been marked as a duplicate of this bug. ***
Comment 15 Andreas Henriksson 2009-07-09 14:58:43 UTC
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...
Comment 16 Dan Winship 2009-07-09 19:40:22 UTC
filed bug 588189 about tls support in gio
Comment 17 Benjamin Otte (Company) 2010-02-07 14:39:22 UTC
*** Bug 607113 has been marked as a duplicate of this bug. ***
Comment 18 Dan Winship 2010-05-22 22:59:20 UTC
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.
Comment 19 Ondra Pelech 2011-09-03 00:02:07 UTC
hello :)
any progress?
Comment 20 Benjamin Otte (Company) 2011-09-03 01:11:13 UTC
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.
Comment 21 Benjamin Otte (Company) 2011-09-03 01:18:17 UTC
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.
Comment 22 Kevin 2011-11-30 19:26:15 UTC
Any progress?
ftps:// or ftpes:// syntax is a lot better than no security at all... :(
Comment 23 Eric Drechsel 2012-04-04 21:27:40 UTC
+1 for getting this implemented
Comment 24 Craig 2012-06-01 19:54:46 UTC
+1 - ftps:// support would be quite useful.
Comment 25 Cosimo Cecchi 2012-07-19 03:51:55 UTC
*** Bug 680217 has been marked as a duplicate of this bug. ***
Comment 26 Torben Andresen 2014-10-14 06:48:49 UTC
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.
Comment 27 Ross Lagerwall 2014-10-21 20:43:07 UTC
*** Bug 738948 has been marked as a duplicate of this bug. ***
Comment 28 Bachi 2015-02-16 16:31:27 UTC
+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.
Comment 29 Ross Lagerwall 2015-02-18 07:24:02 UTC
I'll have a go at implementing this.
Comment 30 Ross Lagerwall 2015-02-26 22:53:34 UTC
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.
Comment 31 Ross Lagerwall 2015-02-26 22:53:43 UTC
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.
Comment 32 Ross Lagerwall 2015-02-26 22:53:50 UTC
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.
Comment 33 Ross Lagerwall 2015-02-26 22:55:37 UTC
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.
Comment 34 Ross Lagerwall 2015-02-27 07:59:50 UTC
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.
Comment 35 Ondrej Holy 2015-03-05 12:35:58 UTC
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:"?
Comment 36 Ondrej Holy 2015-03-05 12:36:58 UTC
Created attachment 298634 [details]
what is shown in nautilus
Comment 37 Ross Lagerwall 2015-03-05 13:04:37 UTC
(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.
Comment 38 Ross Lagerwall 2015-03-05 23:43:43 UTC
(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.
Comment 39 Ross Lagerwall 2015-03-05 23:44:30 UTC
Created attachment 298670 [details]
gnome-shell fixed
Comment 40 Ondrej Holy 2015-03-06 10:08:45 UTC
(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!
Comment 41 Ross Lagerwall 2015-03-09 07:35:12 UTC
(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
Comment 42 Ondrej Holy 2015-03-09 09:12:51 UTC
Ok, fair enough...
Comment 43 Ross Lagerwall 2015-03-12 21:07:48 UTC
(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).
Comment 44 Ross Lagerwall 2015-03-12 21:08:47 UTC
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.
Comment 45 Ross Lagerwall 2015-03-12 21:08:54 UTC
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.
Comment 46 Ross Lagerwall 2015-03-12 21:09:07 UTC
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.
Comment 47 Ross Lagerwall 2015-04-06 14:07:36 UTC
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.
Comment 48 Ross Lagerwall 2015-04-06 14:09:42 UTC
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 :-)
Comment 49 Ondrej Holy 2015-04-07 11:00:22 UTC
Review of attachment 299239 [details] [review]:

Looks good, however would be good to commit into master after gnome-3-16 branch...
Comment 50 Ross Lagerwall 2015-04-07 12:51:37 UTC
(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.
Comment 51 Benjamin Otte (Company) 2015-04-07 12:57:42 UTC
Patch looks good to me, too.
Comment 52 Ondrej Holy 2015-04-07 13:01:17 UTC
(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...
Comment 53 Ondrej Holy 2015-04-07 13:50:57 UTC
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")
Comment 54 Ondrej Holy 2015-04-07 20:42:02 UTC
(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
Comment 55 Ross Lagerwall 2015-04-07 20:45:53 UTC
(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.
Comment 56 Ondrej Holy 2015-04-08 07:38:17 UTC
Dammit, I rebuilt only pure glib before... it seems working correctly with glib-networking master!
Comment 57 Ondrej Holy 2015-04-08 08:15:05 UTC
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);
Comment 58 Dan Winship 2015-04-08 12:03:24 UTC
(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)
Comment 59 Ross Lagerwall 2015-04-08 12:15:34 UTC
(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.
Comment 60 Ross Lagerwall 2015-04-08 21:54:57 UTC
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.
Comment 61 Ross Lagerwall 2015-04-08 21:57:48 UTC
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.
Comment 62 Ondrej Holy 2015-04-09 07:37:57 UTC
Review of attachment 301164 [details] [review]:

Ok! Before committing fix typo in the message: s/tansparently/transparently/
Comment 63 Ross Lagerwall 2015-04-09 21:16:19 UTC
Pushed to master as ab4e591..4667ada. Thanks for the reviews. Yay, closing 7 year old bugs :-)