FTP frontend should avoid caching plaintext of uploads #1083

New Issue

tahoe-lafs · 2010-06-15T23:26:59Z

davidsarah commented

2010-06-15 23:26:59 +00:00

The SFTP frontend was changed to avoid using a cleartext temporary file for the contents of files opened for writing (see [EncryptedTemporaryFile in sftpd.py]source:src/allmydata/frontends/sftpd.py?rev=4478#L294). A similar change should be made to the FTP frontend.

Perhaps EncryptedTemporaryFile should be moved to [fileutil.py]source:/src/allmydata/util/fileutil.py so that it can be used by both SFTP and FTP.

The SFTP frontend was changed to avoid using a cleartext temporary file for the contents of files opened for writing (see [EncryptedTemporaryFile in sftpd.py]source:src/allmydata/frontends/sftpd.py?rev=4478#L294). A similar change should be made to the FTP frontend. Perhaps `EncryptedTemporaryFile` should be moved to [fileutil.py]source:/src/allmydata/util/fileutil.py so that it can be used by both SFTP and FTP.

tahoe-lafs added the

labels 2010-06-15 23:26:59 +00:00

tahoe-lafs added this to the 1.8.0 milestone 2010-06-15 23:26:59 +00:00

davidsarah commented

2010-06-15 23:28:54 +00:00

Note that EncryptedTemporaryFile is not tested directly, only via the SFTP tests. It probably should be tested directly.

Note that `EncryptedTemporaryFile` is not tested directly, only via the SFTP tests. It probably should be tested directly.

tahoe-lafs modified the milestone from 1.8.0 to 1.7.1

2010-06-21 01:53:19 +00:00

davidsarah commented

2010-07-11 21:55:06 +00:00

The FTP frontend unfortunately has no tests (#512). On the other hand, EncryptedTemporaryFile is indirectly tested by the SFTP tests.

The FTP frontend unfortunately has no tests (#512). On the other hand, `EncryptedTemporaryFile` is indirectly tested by the SFTP tests.

zooko commented

2010-07-14 07:09:13 +00:00

Perhaps this should be bumped from v1.7.1 to v1.8.0 simply due to it being a nice new feature instead of a bug fix, clean-up, unfinished-business, etc.?

zooko commented

2010-07-14 07:10:44 +00:00

Oh, it is sort of a bug-fix (potential security problem), clean-up (unify SFTP and FTP backends), and unfinished-business (SFTP got this feature in 1.7.0). So disregard comment:77998. :-)

Oh, it *is* sort of a bug-fix (potential security problem), clean-up (unify SFTP and FTP backends), and unfinished-business (SFTP got this feature in 1.7.0). So disregard [comment:77998](/tahoe-lafs/trac-2024-07-25/issues/1083#issuecomment-77998). :-)

davidsarah commented

2010-07-15 06:36:35 +00:00

The current patch seems to have caused a regression in SFTP that results in the test_openFile_write test hanging (probably something simple, but I haven't had time to track it down). So I wouldn't object to this being bumped to 1.8. Whether we'll fit it into 1.7.1 depends on how much the schedule slips :-)

davidsarah commented

2010-07-15 20:14:52 +00:00

Replying to davidsarah:

The current patch seems to have caused a regression in SFTP that results in the test_openFile_write test hanging...

I was mistaken; this was caused by sftp-no-trunc-files-opened-with-append.dpatch, not by this patch.

Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/1083#issuecomment-78000): > The current patch seems to have caused a regression in SFTP that results in the test_openFile_write test hanging... I was mistaken; this was caused by [sftp-no-trunc-files-opened-with-append.dpatch](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1106/sftp-no-trunc-files-opened-with-append.dpatch), not by this patch.

davidsarah commented

2010-07-17 06:02:25 +00:00

Attachment make-ftp-use-encrypted-temporary-file.dpatch (11005 bytes) added

Move EncryptedTemporaryFile from SFTP frontend to allmydata.util.fileutil, and make the FTP frontend also use it (fi xing #1083). Also, docstrings for non-obvious usage restrictions on methods of EncryptedTemporaryFile.

**Attachment** make-ftp-use-encrypted-temporary-file.dpatch (11005 bytes) added Move [EncryptedTemporaryFile](wiki/EncryptedTemporaryFile) from SFTP frontend to allmydata.util.fileutil, and make the FTP frontend also use it (fi xing #1083). Also, docstrings for non-obvious usage restrictions on methods of [EncryptedTemporaryFile](wiki/EncryptedTemporaryFile).

make-ftp-use-encrypted-temporary-file.dpatch

11 KiB

zooko commented

2010-07-18 02:17:01 +00:00

I just reviewed the patch. +1! Thanks, David-Sarah!

Oh, it needs a NEWS snippet. I'll add that myself.

Should we remove the "test-needed" flag?

I just reviewed the patch. +1! Thanks, David-Sarah! Oh, it needs a NEWS snippet. I'll add that myself. Should we remove the "test-needed" flag?

davidsarah commented

2010-07-18 02:38:13 +00:00

Replying to zooko:

Should we remove the "test-needed" flag?

That would be part of #512, so yes.

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1083#issuecomment-78002): > Should we remove the "test-needed" flag? That would be part of #512, so yes.

zooko commented

2010-07-18 05:39:52 +00:00

committed in changeset:05022dca36780b3b. Thanks!

zooko added the

fixed

label 2010-07-18 05:39:52 +00:00

zooko closed this issue

2010-07-18 05:39:52 +00:00

Sign in to join this conversation.