diff --git a/ChangeLog.md b/ChangeLog.md --- a/ChangeLog.md +++ b/ChangeLog.md @@ -1,5 +1,41 @@ # ChangeLog +## 0.3.10 + +Two for the server: one that answered on a stream it should never have +been given, and one that made a lost flight cost seconds. + +* Hand the application a stream only once its first frame is read. + `openStream` created the stream and gave it to the application in one + step, before the frame that opened it had been looked over, so a first + STREAM frame past the flow control limit was answered by the + application in the moment before the connection was closed over it. + An HTTP/3 server sent a response on a stream the peer had never + opened, and the peer called that a STREAM_STATE_ERROR, as RFC 9000 Sec + 19.8 says to, before the FLOW_CONTROL_ERROR arrived; h3spec's "MUST + send FLOW_CONTROL_ERROR if a STREAM frame with a large offset is + received" failed for that reason. The stream now reaches the + application after the frame has been checked, so a frame that closes + the connection closes it with the application none the wiser. + [#131](https://github.com/kazu-yamamoto/quic/pull/131) + +* Keep the Initial keys of the version the client addressed us in. A + server that settles on a compatible version answers in it and replaces + its Initial keys with the ones for that version (RFC 9368), but the + client hears of the choice only when the answer arrives and until then + retransmits in the version it started with. Without the keys for that + version the server could not pick the handshake up from those and + waited out its own PTO instead -- a second, then two, then four, with + the client's retransmissions falling into it unread. On the test that + loses the server's whole first flight, the handshake is done at about + 1.4 seconds where it took about 7.0. This is the other half of #128, + which stopped the same sequence from hanging for good; what was left + was the waiting. + [#132](https://github.com/kazu-yamamoto/quic/pull/132) + +* The library builds without a warning: two imports left over from the + crypton 2.1.0 work are gone. + ## 0.3.9 A stream limit the peer could walk past, two ways for a connection to diff --git a/Network/QUIC/Connection/Crypto.hs b/Network/QUIC/Connection/Crypto.hs --- a/Network/QUIC/Connection/Crypto.hs +++ b/Network/QUIC/Connection/Crypto.hs @@ -13,6 +13,8 @@ setNegotiated, -- dropSecrets, + keepInitialKeys, + getPrevInitialKeys, -- initializeCoder, initializeCoder1RTT, @@ -95,6 +97,26 @@ dropSecrets Connection{..} lvl = do writeArray coders lvl initialCoder writeArray protectors lvl initialProtector + when (lvl == InitialLevel) $ writeIORef prevInitialKeys Nothing + +-- | Keeping the Initial keys we have, before they are replaced with the ones +-- for a version we chose ourselves. +-- +-- A server that settles on a compatible version (RFC 9368) answers in it, but +-- the client hears of that only when the answer arrives. Until then it +-- retransmits its Initial packets in the version it started with, and without +-- the keys for that version they are so much noise: the server cannot pick up +-- the handshake from them, and has to wait out its own PTO instead -- a +-- second, then two, then four, with the client's retransmissions falling into +-- it unread. +keepInitialKeys :: Connection -> IO () +keepInitialKeys conn@Connection{..} = do + coder <- getCoder conn InitialLevel False + protector <- getProtector conn InitialLevel + writeIORef prevInitialKeys $ Just (coder, protector) + +getPrevInitialKeys :: Connection -> IO (Maybe (Coder, Protector)) +getPrevInitialKeys Connection{..} = readIORef prevInitialKeys ---------------------------------------------------------------- diff --git a/Network/QUIC/Connection/Stream.hs b/Network/QUIC/Connection/Stream.hs --- a/Network/QUIC/Connection/Stream.hs +++ b/Network/QUIC/Connection/Stream.hs @@ -62,8 +62,8 @@ -- -- A MAX_STREAMS that does not raise it is ignored (RFC 9000, section 4.6). -- One of them reordered on the way, or retransmitted after a newer one has --- gone out, would otherwise lower the limit. 'setTxMaxData' and --- 'setTxMaxStreamData' guard the limits on data the same way. +-- gone out, would otherwise lower the limit. @setTxMaxData@ and +-- @setTxMaxStreamData@ guard the limits on data the same way. set :: TVar Concurrency -> Int -> IO () set tvar mx = atomically $ modifyTVar' tvar raise where diff --git a/Network/QUIC/Connection/Types.hs b/Network/QUIC/Connection/Types.hs --- a/Network/QUIC/Connection/Types.hs +++ b/Network/QUIC/Connection/Types.hs @@ -310,6 +310,9 @@ , coders :: IOArray EncryptionLevel Coder , coders1RTT :: IOArray Bool Coder1RTT , protectors :: IOArray EncryptionLevel Protector + , prevInitialKeys :: IORef (Maybe (Coder, Protector)) + -- ^ The Initial keys for the version the peer addressed us in, kept + -- over a compatible version change (RFC 9368) , currentKeyPhase :: IORef (Bool, PacketNumber) , negotiated :: IORef Negotiated , connMyAuthCIDs :: IORef AuthCIDs @@ -410,6 +413,7 @@ coders <- newArray (InitialLevel, HandshakeLevel) initialCoder coders1RTT <- newArray (False, True) initialCoder1RTT protectors <- newArray (InitialLevel, RTT1Level) initialProtector + prevInitialKeys <- newIORef Nothing currentKeyPhase <- newIORef (False, 0) negotiated <- newIORef initialNegotiated connMyAuthCIDs <- newIORef myAuthCIDs @@ -463,9 +467,10 @@ clientConnection ClientConfig{..} verInfo myAuthCIDs peerAuthCIDs = newConnection Client ccParameters' verInfo myAuthCIDs peerAuthCIDs where - ccParameters' = ccParameters - { maxDatagramFrameSize = ccMaxDatagramFrameSize - } + ccParameters' = + ccParameters + { maxDatagramFrameSize = ccMaxDatagramFrameSize + } serverConnection :: ServerConfig @@ -486,9 +491,10 @@ serverConnection ServerConfig{..} verInfo myAuthCIDs peerAuthCIDs = newConnection Server scParameters' verInfo myAuthCIDs peerAuthCIDs where - scParameters' = scParameters - { maxDatagramFrameSize = scMaxDatagramFrameSize - } + scParameters' = + scParameters + { maxDatagramFrameSize = scMaxDatagramFrameSize + } ---------------------------------------------------------------- diff --git a/Network/QUIC/Crypto/Nite.hs b/Network/QUIC/Crypto/Nite.hs --- a/Network/QUIC/Crypto/Nite.hs +++ b/Network/QUIC/Crypto/Nite.hs @@ -24,7 +24,6 @@ import Crypto.Cipher.ChaChaPoly1305 (aeadChacha20poly1305Init) import Crypto.Cipher.Types hiding (Cipher, IV) import Crypto.Error (maybeCryptoError) -import Data.IORef (IORef, newIORef, readIORef, writeIORef) import qualified Data.ByteArray as Byte (ByteArrayAccess (..), convert) import qualified Data.ByteString as BS import qualified Data.ByteString.Internal as BS diff --git a/Network/QUIC/Crypto/Utils.hs b/Network/QUIC/Crypto/Utils.hs --- a/Network/QUIC/Crypto/Utils.hs +++ b/Network/QUIC/Crypto/Utils.hs @@ -11,7 +11,6 @@ import qualified Data.ByteString as BS import qualified Data.ByteString.Short as Short import Network.TLS hiding (Version) -import Network.TLS.Extra.Cipher import Network.QUIC.Crypto.Nite import Network.QUIC.Crypto.Types diff --git a/Network/QUIC/Handshake.hs b/Network/QUIC/Handshake.hs --- a/Network/QUIC/Handshake.hs +++ b/Network/QUIC/Handshake.hs @@ -4,8 +4,8 @@ module Network.QUIC.Handshake where import qualified Control.Exception as E -import Data.List (intersect) import qualified Data.ByteString.Short as Short +import Data.List (intersect) import qualified Network.TLS as TLS import Network.TLS.QUIC @@ -107,11 +107,13 @@ qlogParamsSet conn (ccParameters conf, "local") -- fixme handshakeClient' conf' conn myAuthCIDs <$> getVersion conn <*> newHndStateRef where - conf' = conf - { ccParameters = (ccParameters conf) - { maxDatagramFrameSize = ccMaxDatagramFrameSize conf - } - } + conf' = + conf + { ccParameters = + (ccParameters conf) + { maxDatagramFrameSize = ccMaxDatagramFrameSize conf + } + } handshakeClient' :: ClientConfig -> Connection -> AuthCIDs -> Version -> IORef HndState -> IO () @@ -342,6 +344,10 @@ case myVers `intersect` peerVers of vers@(serverVer : _) | clientVer /= serverVer -> do + -- Before the keys below replace them: the client keeps + -- retransmitting in the version it started with until + -- our answer reaches it. + keepInitialKeys conn setVersionInfo conn $ VersionInfo serverVer vers dcid <- getClientDstCID conn initializeCoder conn InitialLevel $ initialSecrets serverVer dcid diff --git a/Network/QUIC/Packet/Decrypt.hs b/Network/QUIC/Packet/Decrypt.hs --- a/Network/QUIC/Packet/Decrypt.hs +++ b/Network/QUIC/Packet/Decrypt.hs @@ -21,9 +21,37 @@ ---------------------------------------------------------------- decryptCrypt :: Connection -> Crypt -> EncryptionLevel -> IO (Maybe Plain) -decryptCrypt conn Crypt{..} lvl = do - cipher <- getCipher conn lvl +decryptCrypt conn crypt lvl = do protector <- getProtector conn lvl + mplain <- decryptCryptWith conn crypt lvl protector $ getCoder conn lvl + case mplain of + Just _ -> return mplain + Nothing + | lvl == InitialLevel -> do + -- An Initial we cannot read may be one in the version the + -- peer addressed us in, from before we settled on a + -- compatible one of our own (RFC 9368). The peer goes on + -- retransmitting in that version until our answer reaches + -- it, and those carry the handshake just as well. Both the + -- header protection and the payload keys differ by version, + -- so the whole of it is tried again, not the payload alone. + mkeys <- getPrevInitialKeys conn + case mkeys of + Nothing -> return Nothing + Just (coder, protector') -> + decryptCryptWith conn crypt lvl protector' $ + \_ -> return coder + | otherwise -> return Nothing + +decryptCryptWith + :: Connection + -> Crypt + -> EncryptionLevel + -> Protector + -> (Bool -> IO Coder) + -> IO (Maybe Plain) +decryptCryptWith conn Crypt{..} lvl protector getCoder' = do + cipher <- getCipher conn lvl let proFlags = Flags (cryptPacket `BS.index` 0) sampleOffset = cryptPktNumOffset + 4 sampleLen = sampleLength cipher @@ -59,7 +87,7 @@ let keyPhase | lvl == RTT1Level = flags `testBit` 2 | otherwise = False - coder <- getCoder conn lvl keyPhase + coder <- getCoder' keyPhase siz <- decrypt coder (decryptBuf conn) ciphertext (AssDat header) pn let rrMask | lvl == RTT1Level = 0x18 diff --git a/Network/QUIC/Receiver.hs b/Network/QUIC/Receiver.hs --- a/Network/QUIC/Receiver.hs +++ b/Network/QUIC/Receiver.hs @@ -244,16 +244,29 @@ closeConnection conn StreamStateError emsg streamNotCreatedYet _ _ _ = return () --- | Opening a stream for a STREAM frame that found none, unless the stream --- was open once and has been closed since. 'guardStream' has already --- refused one of ours that we have not created yet. +-- | Opening a stream for a frame that found none, unless the stream was open +-- once and has been closed since. 'guardStream' has already refused one of +-- ours that we have not created yet. +-- +-- The application is not given the stream here; 'deliverStream' does that, +-- once the frame that opened it has been looked over. Handed the stream +-- first, the application could answer on it in the moment before a first +-- STREAM frame past the flow control limit closed the connection. An HTTP/3 +-- server did: it sent a response on a stream the peer had never opened, and +-- the peer called that a STREAM_STATE_ERROR, as RFC 9000 Sec 19.8 says to, +-- before our own FLOW_CONTROL_ERROR had reached it. openStream :: Connection -> StreamId -> IO (Maybe Stream) openStream conn sid | isInitiated conn sid = return Nothing | otherwise = do new <- claimPeerStream conn sid - if new then Just <$> createStream conn sid else return Nothing + if new then Just <$> addStream conn sid else return Nothing +-- | Handing a stream the peer opened to the application, if it is new to us. +deliverStream :: Connection -> Maybe Stream -> Stream -> IO () +deliverStream conn found strm = + when (isNothing found) $ putInput conn $ InpStream strm + processFrame :: Connection -> EncryptionLevel -> Frame -> IO () processFrame _ _ Padding{} = return () processFrame conn lvl Ping = do @@ -306,6 +319,7 @@ case mstrm of Nothing -> return () Just strm -> do + deliverStream conn mstrm0 strm onResetStreamReceived (connHooks conn) strm aerr -- Before the pseudo FIN below, so that whoever reads it can -- tell it was not a real one. @@ -394,6 +408,7 @@ conn FlowControlError "Flow control error for connection in 0-RTT" + deliverStream conn mstrm strm processFrame conn RTT1Level (StreamF sid _ [""] False) = do -- FLOW CONTROL: MAX_STREAMS: recv: rejecting if over my limit ok <- checkRxMaxStreams conn sid @@ -438,6 +453,7 @@ conn FlowControlError "Flow control error for connection in 1-RTT" + deliverStream conn mstrm strm processFrame conn lvl (MaxData n) = do when (lvl == InitialLevel || lvl == HandshakeLevel) $ closeConnection conn ProtocolViolation "MAX_DATA in Initial or Handshake" diff --git a/quic.cabal b/quic.cabal --- a/quic.cabal +++ b/quic.cabal @@ -1,6 +1,6 @@ cabal-version: 2.0 name: quic -version: 0.3.9 +version: 0.3.10 license: BSD3 license-file: LICENSE maintainer: kazu@iij.ad.jp diff --git a/test/ErrorSpec.hs b/test/ErrorSpec.hs --- a/test/ErrorSpec.hs +++ b/test/ErrorSpec.hs @@ -1,3 +1,5 @@ +{-# LANGUAGE OverloadedStrings #-} + module ErrorSpec where import Control.Concurrent @@ -23,7 +25,14 @@ threadDelay 500000 -- give enough time to the server return tid where - loop conn = forever $ void $ acceptStream conn + -- Answering on what it is given. A server that only accepts cannot + -- catch a stream handed to the application before the frame that opened + -- it was looked over -- the answer is what reaches the peer, for a + -- stream the peer never opened, and the peer calls that a + -- STREAM_STATE_ERROR before our FLOW_CONTROL_ERROR arrives. + loop conn = forever $ do + strm <- acceptStream conn + void $ forkIO $ sendStream strm "x" teardown :: ThreadId -> IO () teardown = killThread diff --git a/test/TransportError.hs b/test/TransportError.hs --- a/test/TransportError.hs +++ b/test/TransportError.hs @@ -340,9 +340,11 @@ ---------------------------------------------------------------- --- Stream 0 is not created internally. It is assumed that a server --- send CC without sending back Stream 0. If the server send back any --- data for Stream 0, `streamNotCreatedYet` throws an exception, sigh. +-- Stream 0, which the client has not opened. The server must answer the +-- offset with FLOW_CONTROL_ERROR and nothing else: anything it sends back on +-- stream 0 is, to a client that never opened it, a STREAM_STATE_ERROR +-- (RFC 9000 Sec 19.8), and the client closes the connection before the error +-- we are waiting for arrives. largeOffset :: EncryptionLevel -> Plain -> Plain largeOffset lvl plain | lvl == RTT1Level = plain{plainFrames = fake : plainFrames plain}