Opened at 2010-06-15T23:26:59Z

Closed at 2010-07-18T05:39:52Z

#1083 closed defect (fixed)

FTP frontend should avoid caching plaintext of uploads

Reported by: davidsarah Owned by: davidsarah
Priority: major Milestone: 1.7.1
Component: code-frontend Version: 1.7β
Keywords: ftpd sftp confidentiality easy Cc:
Launchpad Bug:

Description

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​). A similar change should be made to the FTP frontend.

Perhaps EncryptedTemporaryFile should be moved to fileutil.py​ so that it can be used by both SFTP and FTP.

Attachments (1)

make-ftp-use-encrypted-temporary-file.dpatch​ (10.7 KB) - added by davidsarah at 2010-07-17T06:02:25Z.
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?.

Download all attachments as: .zip

Change History (12)

comment:1 Changed at 2010-06-15T23:28:54Z by davidsarah

  • Keywords test added

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

comment:2 Changed at 2010-06-21T01:53:19Z by davidsarah

  • Milestone changed from 1.8.0 to 1.7.1
  • Owner set to davidsarah
  • Status changed from new to assigned

comment:3 Changed at 2010-07-11T21:55:06Z by davidsarah

  • Keywords test-needed review-needed added; test removed
  • Owner changed from davidsarah to somebody
  • Status changed from assigned to new

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

comment:4 Changed at 2010-07-14T07:09:13Z by zooko

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.?

comment:5 Changed at 2010-07-14T07:10:44Z by zooko

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:4. :-)

comment:6 follow-up: Changed at 2010-07-15T06:36:35Z by davidsarah

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 :-)

comment:7 in reply to: ↑ 6 Changed at 2010-07-15T20:14:52Z by davidsarah

  • Keywords sftp added
  • Owner changed from somebody to davidsarah
  • Status changed from new to assigned

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.

Changed at 2010-07-17T06:02:25Z by davidsarah

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?.

comment:8 follow-up: Changed at 2010-07-18T02:17:01Z by zooko

  • Keywords review-needed removed

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?

comment:9 in reply to: ↑ 8 Changed at 2010-07-18T02:38:13Z by davidsarah

  • Keywords test-needed removed

Replying to zooko:

Should we remove the "test-needed" flag?

That would be part of #512, so yes.

comment:10 Changed at 2010-07-18T03:38:13Z by zooko

  • Keywords reviewed added

comment:11 Changed at 2010-07-18T05:39:52Z by zooko

  • Keywords reviewed removed
  • Resolution set to fixed
  • Status changed from assigned to closed

committed in 05022dca36780b3b. Thanks!

Note: See TracTickets for help on using tickets.