From fcc052bed81dac5a7664b815e3a3cf1ffb1b1659 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 29 Jul 2024 13:39:28 -0400 Subject: [PATCH] When proxying an upload to a special remote, verify the hash. While usually uploading to a special remote does not verify the content, the content in a repository is assumed to be valid, and there is no trust boundary. But with a proxied special remote, there may be users who are allowed to store objects, but are not really trusted. Another way to look at this is it's the equivilant of git-annex-shell checking the hash of received data, which it does (see StoreContent implementation). --- Annex/Proxy.hs | 54 +++++++++++++++++++-------------- Annex/Verify.hs | 11 +++++-- CHANGELOG | 1 + doc/todo/git-annex_proxies.mdwn | 10 ------ 4 files changed, 41 insertions(+), 35 deletions(-) diff --git a/Annex/Proxy.hs b/Annex/Proxy.hs index 16d4fc2cdd..6a5a0bf421 100644 --- a/Annex/Proxy.hs +++ b/Annex/Proxy.hs @@ -18,6 +18,7 @@ import P2P.IO import Remote.Helper.Ssh (openP2PShellConnection', closeP2PShellConnection) import Annex.Concurrent import Annex.Tmp +import Annex.Verify import Logs.Proxy import Logs.Cluster import Logs.UUID @@ -136,9 +137,9 @@ proxySpecialRemote protoversion r ihdl ohdl owaitv oclosedv = go (Left <$> readTMVar oclosedv) receivebytestring = atomically recv >>= \case - Right (Left b) -> return b + Right (Left b) -> return (Just b) Right (Right _m) -> giveup "did not receive ByteString from P2P MVar" - Left () -> giveup "connection closed" + Left () -> return Nothing where recv = (Right <$> takeTMVar ohdl) @@ -161,6 +162,8 @@ proxySpecialRemote protoversion r ihdl ohdl owaitv oclosedv = go withTmpDirIn (fromRawFilePath othertmpdir) "proxy" $ \tmpdir -> a (toRawFilePath tmpdir P. keyFile k) + -- Verify the content received from the client, to avoid bad content + -- being stored in the special remote. proxyput af k = do liftIO $ sendmessage $ PUT_FROM (Offset 0) withproxytmpfile k $ \tmpfile -> do @@ -169,13 +172,18 @@ proxySpecialRemote protoversion r ihdl ohdl owaitv oclosedv = go Left err -> liftIO $ propagateerror err liftIO receivemessage >>= \case Just (DATA (Len len)) -> do + iv <- startVerifyKeyContentIncrementally Remote.AlwaysVerify k h <- liftIO $ openFile (fromRawFilePath tmpfile) WriteMode - liftIO $ receivetofile h len + gotall <- liftIO $ receivetofile iv h len liftIO $ hClose h + verified <- if gotall + then fst <$> finishVerifyKeyContentIncrementally' True iv + else pure False if protoversion > ProtocolVersion 1 then liftIO receivemessage >>= \case - Just (VALIDITY Valid) -> - store + Just (VALIDITY Valid) + | verified -> store + | otherwise -> liftIO $ sendmessage FAILURE Just (VALIDITY Invalid) -> liftIO $ sendmessage FAILURE _ -> giveup "protocol error" @@ -183,25 +191,27 @@ proxySpecialRemote protoversion r ihdl ohdl owaitv oclosedv = go _ -> giveup "protocol error" liftIO $ removeWhenExistsWith removeFile (fromRawFilePath tmpfile) - receivetofile h n = do - b <- liftIO receivebytestring - liftIO $ atomically $ - putTMVar owaitv () - `orElse` - readTMVar oclosedv - n' <- storetofile h n (L.toChunks b) - -- Normally all the data is sent in a single - -- lazy bytestring. However, when the special - -- remote is a node in a cluster, a PUT is - -- streamed to it in multiple chunks. - if n' == 0 - then return () - else receivetofile h n' + receivetofile iv h n = liftIO receivebytestring >>= \case + Just b -> do + liftIO $ atomically $ + putTMVar owaitv () + `orElse` + readTMVar oclosedv + n' <- storetofile iv h n (L.toChunks b) + -- Normally all the data is sent in a single + -- lazy bytestring. However, when the special + -- remote is a node in a cluster, a PUT is + -- streamed to it in multiple chunks. + if n' == 0 + then return True + else receivetofile iv h n' + Nothing -> return False - storetofile _ n [] = pure n - storetofile h n (b:bs) = do + storetofile _ _ n [] = pure n + storetofile iv h n (b:bs) = do + writeVerifyChunk iv h b B.hPut h b - storetofile h (n - fromIntegral (B.length b)) bs + storetofile iv h (n - fromIntegral (B.length b)) bs proxyget offset af k = withproxytmpfile k $ \tmpfile -> do -- Don't verify the content from the remote, diff --git a/Annex/Verify.hs b/Annex/Verify.hs index 735ef929ac..697ffeadc0 100644 --- a/Annex/Verify.hs +++ b/Annex/Verify.hs @@ -19,6 +19,7 @@ module Annex.Verify ( isVerifiable, startVerifyKeyContentIncrementally, finishVerifyKeyContentIncrementally, + finishVerifyKeyContentIncrementally', verifyKeyContentIncrementally, IncrementalVerifier(..), writeVerifyChunk, @@ -199,13 +200,17 @@ startVerifyKeyContentIncrementally verifyconfig k = ) finishVerifyKeyContentIncrementally :: Maybe IncrementalVerifier -> Annex (Bool, Verification) -finishVerifyKeyContentIncrementally Nothing = +finishVerifyKeyContentIncrementally = finishVerifyKeyContentIncrementally' False + +finishVerifyKeyContentIncrementally' :: Bool -> Maybe IncrementalVerifier -> Annex (Bool, Verification) +finishVerifyKeyContentIncrementally' _ Nothing = return (True, UnVerified) -finishVerifyKeyContentIncrementally (Just iv) = +finishVerifyKeyContentIncrementally' quiet (Just iv) = liftIO (finalizeIncrementalVerifier iv) >>= \case Just True -> return (True, Verified) Just False -> do - warning "verification of content failed" + unless quiet $ + warning "verification of content failed" return (False, UnVerified) -- Incremental verification was not able to be done. Nothing -> return (True, UnVerified) diff --git a/CHANGELOG b/CHANGELOG index 08404c65b3..471936cf93 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,7 @@ git-annex (10.20240731) UNRELEASED; urgency=medium git-annex remotedaemon is killed while locking a key to prevent its removal. * When proxying a download from a special remote, avoid unncessary hashing. + * When proxying an upload to a special remote, verify the hash. * Propagate --force to git-annex transferrer. * Added a build flag for servant, enabling annex+http urls and git-annex p2phttp. diff --git a/doc/todo/git-annex_proxies.mdwn b/doc/todo/git-annex_proxies.mdwn index 7d2af32c0f..36ca9b3f8b 100644 --- a/doc/todo/git-annex_proxies.mdwn +++ b/doc/todo/git-annex_proxies.mdwn @@ -40,16 +40,6 @@ Planned schedule of work: When using ssh and not the http server, the node that had the incomplete copy also doesn't get the file, altough no error is displayed. -* When proxying a PUT to a special remote, no verification of the received - content is done, it's just written to a file and that is sent to the - special remote. This violates a usual invariant that any data being - received into a repository gets verified in passing. Although on the - other hand, when sending data to a special remote normally, there is also - no verification. On the third hand, a p2p http proxy (or for that matter - a ssh server) may have users who are allowed to store objects, but are - not really trusted, and if they can upload garbage without verification, - that could be bad. - ## items deferred until later for p2p protocol over http * `git-annex p2phttp` should support serving several repositories at the same -- 2.30.2