glacier, S3: Fix bug that caused embedded creds to not be encypted using the remote's key.

encryptionSetup must be called before setRemoteCredPair. Otherwise,
the RemoteConfig doesn't have the cipher in it, and so no cipher is used to
encrypt the embedded creds.

This is a security fix for non-shared encryption methods!

For encryption=shared, there's no security problem, just an
inconsistentency in whether the embedded creds are encrypted.

This is very important to get right, so used some types to help ensure that
setRemoteCredPair is only run after encryptionSetup. Note that the external
special remote bypasses the type safety, since creds can be set after the
initial remote config, if the external special remote program requests it.
Also note that IA remotes never use encryption, so encryptionSetup is not
run for them at all, and again the type safety is bypassed.

This leaves two open questions:

1. What to do about S3 and glacier remotes that were set up
   using encryption=pubkey/hybrid with embedcreds?
   Such a git repo has a security hole embedded in it, and this needs to be
   communicated to the user. Is the changelog enough?

2. enableremote won't work in such a repo, because git-annex will
   try to decrypt the embedded creds, which are not encrypted, so fails.
   This needs to be dealt with, especially for ecryption=shared repos,
   which are not really broken, just inconsistently configured.

   Noticing that problem for encryption=shared is what led to commit
   cc54ff9e49260cd94f938e69e926a273e231ef4e, which tried to
   fix the problem by not decrypting the embedded creds.

This commit was sponsored by Josh Taylor.

1 files changed, 30 insertions, 5 deletions

diff --git a/Remote/Helper/Encryptable.hs b/Remote/Helper/Encryptable.hs
index dd032ce33..e8e9d3bf1 100644
--- a/Remote/Helper/Encryptable.hs
+++ b/Remote/Helper/Encryptable.hs
@@ -5,7 +5,17 @@
  - Licensed under the GNU GPL version 3 or higher.
  -}
 
-module Remote.Helper.Encryptable where
+module Remote.Helper.Encryptable (
+	EncryptionIsSetup,
+	encryptionSetup,
+	noEncryptionUsed,
+	encryptionAlreadySetup,
+	remoteCipher,
+	embedCreds,
+	cipherKey,
+	storeCipher,
+	extractCipher,
+) where
 
 import qualified Data.Map as M
 
@@ -16,11 +26,26 @@ import Types.Crypto
 import qualified Annex
 import Utility.Base64
 
+-- Used to ensure that encryption has been set up before trying to
+-- eg, store creds in the remote config that would need to use the
+-- encryption setup.
+data EncryptionIsSetup = EncryptionIsSetup | NoEncryption
+
+-- Remotes that don't use encryption can use this instead of
+-- encryptionSetup.
+noEncryptionUsed :: EncryptionIsSetup
+noEncryptionUsed = NoEncryption
+
+-- Using this avoids the type-safe check, so you'd better be sure
+-- of what you're doing.
+encryptionAlreadySetup :: EncryptionIsSetup
+encryptionAlreadySetup = EncryptionIsSetup
+
 {- Encryption setup for a remote. The user must specify whether to use
  - an encryption key, or not encrypt. An encrypted cipher is created, or is
  - updated to be accessible to an additional encryption key. Or the user
  - could opt to use a shared cipher, which is stored unencrypted. -}
-encryptionSetup :: RemoteConfig -> Annex RemoteConfig
+encryptionSetup :: RemoteConfig -> Annex (RemoteConfig, EncryptionIsSetup)
 encryptionSetup c = maybe genCipher updateCipher $ extractCipher c
   where
 	-- The type of encryption
@@ -28,7 +53,7 @@ encryptionSetup c = maybe genCipher updateCipher $ extractCipher c
 	-- Generate a new cipher, depending on the chosen encryption scheme
 	genCipher = case encryption of
 		_ | M.member "cipher" c || M.member "cipherkeys" c -> cannotchange
-		Just "none" -> return c
+		Just "none" -> return (c, NoEncryption)
 		Just "shared" -> use "encryption setup" . genSharedCipher
 			=<< highRandomQuality
 		-- hybrid encryption is the default when a keyid is
@@ -48,7 +73,7 @@ encryptionSetup c = maybe genCipher updateCipher $ extractCipher c
 	cannotchange = error "Cannot set encryption type of existing remotes."
 	-- Update an existing cipher if possible.
 	updateCipher v = case v of
-		SharedCipher _ | maybe True (== "shared") encryption -> return c'
+		SharedCipher _ | maybe True (== "shared") encryption -> return (c', EncryptionIsSetup)
 		EncryptedCipher _ variant _
 			| maybe True (== if variant == Hybrid then "hybrid" else "pubkey") encryption ->
 				use "encryption update" $ updateEncryptedCipher newkeys v
@@ -57,7 +82,7 @@ encryptionSetup c = maybe genCipher updateCipher $ extractCipher c
 		showNote m
 		cipher <- liftIO a
 		showNote $ describeCipher cipher
-		return $ storeCipher c' cipher
+		return (storeCipher c' cipher, EncryptionIsSetup)
 	highRandomQuality = 
 		(&&) (maybe True ( /= "false") $ M.lookup "highRandomQuality" c)
 			<$> fmap not (Annex.getState Annex.fast)