{"action":"created","issue":{"url":"https://api.github.com/repos/jaraco/keyring/issues/64","labels_url":"https://api.github.com/repos/jaraco/keyring/issues/64/labels{/name}","comments_url":"https://api.github.com/repos/jaraco/keyring/issues/64/comments","events_url":"https://api.github.com/repos/jaraco/keyring/issues/64/events","html_url":"https://github.com/jaraco/keyring/issues/64","id":58652423,"number":64,"title":"New CryptedFileKeyring doesn't follow keyring protocol","user":{"login":"jaraco","id":308610,"avatar_url":"https://avatars.githubusercontent.com/u/308610?v=3","gravatar_id":"","url":"https://api.github.com/users/jaraco","html_url":"https://github.com/jaraco","followers_url":"https://api.github.com/users/jaraco/followers","following_url":"https://api.github.com/users/jaraco/following{/other_user}","gists_url":"https://api.github.com/users/jaraco/gists{/gist_id}","starred_url":"https://api.github.com/users/jaraco/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jaraco/subscriptions","organizations_url":"https://api.github.com/users/jaraco/orgs","repos_url":"https://api.github.com/users/jaraco/repos","events_url":"https://api.github.com/users/jaraco/events{/privacy}","received_events_url":"https://api.github.com/users/jaraco/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-02-23T21:35:31Z","updated_at":"2015-02-23T21:35:38Z","closed_at":"2015-02-23T21:35:32Z","body":"If I understand correctly (and please correct me if not), the new CryptedFileKeyring does not follow the keyring protocol, but instead attempts to implement a similar protocol. Rather than implementing the abstract methods of the BasicFileKeyring, it encrypts and decrypts the entire file, bypassing the machinery implemented in the parent class.\n\nAt the very least, this causes the tests to fail (when attempting to call encrypt and decrypt).\n\nIt may be the case that this implementation is sound as simply needs to not subclass from BasicFileKeyring and have the tests updated accordingly.\n\n----------------------------------------\n- Bitbucket: https://bitbucket.org/kang/python-keyring-lib/issue/64\n- Originally reported by: Jason R. Coombs\n- Originally created at: 2012-06-09T20:30:11.103\n"},"comment":{"url":"https://api.github.com/repos/jaraco/keyring/issues/comments/75639329","html_url":"https://github.com/jaraco/keyring/issues/64#issuecomment-75639329","issue_url":"https://api.github.com/repos/jaraco/keyring/issues/64","id":75639329,"user":{"login":"jaraco","id":308610,"avatar_url":"https://avatars.githubusercontent.com/u/308610?v=3","gravatar_id":"","url":"https://api.github.com/users/jaraco","html_url":"https://github.com/jaraco","followers_url":"https://api.github.com/users/jaraco/followers","following_url":"https://api.github.com/users/jaraco/following{/other_user}","gists_url":"https://api.github.com/users/jaraco/gists{/gist_id}","starred_url":"https://api.github.com/users/jaraco/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jaraco/subscriptions","organizations_url":"https://api.github.com/users/jaraco/orgs","repos_url":"https://api.github.com/users/jaraco/repos","events_url":"https://api.github.com/users/jaraco/events{/privacy}","received_events_url":"https://api.github.com/users/jaraco/received_events","type":"User","site_admin":false},"created_at":"2015-02-23T21:35:38Z","updated_at":"2015-02-23T21:35:38Z","body":"[Sorry for the delay in responding.  I'm in the middle of changing jobs and moving across the continent, so my availability going to be somewhat sporadic.]\r\n\r\nJason, encrypting the entire file is the simplest secure approach, but as you mentioned, \"it is the goal of the Crypto keyring to protect the passwords and not other data in the file.\"  I don't think it's such a great idea to maintain an unencrypted list of servers that a user has access to (OpenSSH hashes its ~/.ssh/known_hosts entries to avoid this) but I'm willing to treat that as a given for now.\r\n\r\nThe problem with passwords is that they're almost always cryptographically weak (low entropy and frequently reused), so it's really important for any program that handles passwords to leak as little information as possible about them (e.g. their length and their equality with other passwords).   Such a program should also make retrieving password somewhat computationally expensive (e.g. by using something like PBKDF2 with many iterations).\r\n\r\nI had actually suggested encrypting all of the *passwords* together, not the entire file.  For example, you could make a JSON array of the passwords, encrypt that, and then put array indexes into the cleartext.\r\n\r\nRegarding your points 2, 3, and 5 above, I agree with them in principle, but I think that getting the file format right should trump the other considerations.  For example, if the BasicFileKeyring API does not allow for the file format that we want, then the API---not the file format---should be changed.  Also, avoiding loading all the passwords into memory is nice if possible, but since the master key is already in memory during decryption, it would be wiser to secure the case where an attacker *doesn't* have access to the master key, than to prioritize the case where the attacker has access to the master key but is somehow unable to use it to decrypt the rest of the file.  We should also be careful to avoid making the system more complex just to implement some \"nice-to-have\" security features that are more likely to contain bugs than they are to prevent a successful attack.\r\n\r\nIf I were going to implement this for python-keyring, I would probably do something like this:\r\n\r\n1. Given the user's master password MP, a random salt MS, and an iteration count i=2000\r\n2. Use PBKDF2-SHA256(MS, MP, i) to generate a 256-bit master key MK\r\n3. Use the master key to derive other keys:\r\n  a. EK = SHA256(\"EK:\" + MK)  # password encryption key\r\n  b. V = SHA256( \"V:\" + MK)  # verification value\r\n4. Store MS, i, and V as plaintext in the global portion of the file.  This is enough to recover the master key from the master password, and to check whether the correct master password has been entered.\r\n5. For each password:\r\n  a. Generate a random IVp for encrypting the password\r\n  b. Encode the password as <length> + \":\" + password, then pad it to a multiple of 64 *bytes*\r\n  c. Encrypt the padded password using EK as the key and IV as the initialization vector (use CBC mode or CTR mode, but definitely not ECB mode), Cp\r\n  d. Base64-encode the IVp and Cp, then store IVp \":\" Cp in the individual password field.\r\n\r\nDoes that make sense?\n\n----------------------------------------\nOriginal comment by: Dwayne Litzenberger\n"}}