Adding states and parameters for TLS 1.3 #280
Conversation
| @@ -30,7 +30,7 @@ import System.IO.Error (mkIOError, eofErrorType) | |||
| checkValid :: Context -> IO () | |||
| checkValid ctx = do | |||
| established <- ctxEstablished ctx | |||
| unless established $ throwIO ConnectionNotEstablished | |||
| when (established == NotEstablished) $ throwIO ConnectionNotEstablished | |||
| eofed <- ctxEOF ctx | |||
| when eofed $ throwIO $ mkIOError eofErrorType "data" Nothing Nothing | |||
There was a problem hiding this comment.
Does that mean sendData and recvData are possible in all 3 other states?
There was a problem hiding this comment.
For TLS 1.2 or earlier and clients of TLS 1.3, only Established and NotEstablished are used.
For servers of TLS 1.3, sendData can be used only on Established.
recvData can be used on either Established or EarlyDataAllowed.
There was a problem hiding this comment.
If you are concerned about this, let's come back to this issue after the entire merge process is finished.
There was a problem hiding this comment.
You can see the coming patches on https://github.com/kazu-yamamoto/hs-tls/tree/work13
There was a problem hiding this comment.
OK we can see checkValid later.
core/Network/TLS/Parameters.hs
Outdated
| | AcceptEarlyData Word32 | ||
| | SendEarlyData ByteString | ||
| deriving (Eq, Show) | ||
|
|
There was a problem hiding this comment.
This should be documented. I'm not sure to understand what the Word32 is for. If it's a length, then Int would be easier to use on external APIs. Also then I don't see difference between NoEarlyData and AcceptEarlyData 0.
There was a problem hiding this comment.
NoEarlyDatais deleted.AcceptEarlyData 0is used instead.AcceptEarlyData Word32is changed toAcceptEarlyData Int- Document is written.
There was a problem hiding this comment.
OK but if type is different for client and server, maybe this should be moved out of Supported to ClientParams / ServerParams directly.
There was a problem hiding this comment.
AcceptEarlyData is used in recvData which takes only Context.
So, if we take your approach, we need to add a new field to Context and copy the value from ServerParams to Context.
It it worth doing?
There was a problem hiding this comment.
Yes that looks useful. Because it would allow to track how much 0RTT data has been received and how much left is allowed. Maybe inside the EarlyDataAllowed constructor.
There was a problem hiding this comment.
Using EarlyDataAllowed is a really nice idea!
The patch to implement this idea is pushed.
core/Network/TLS/Types.hs
Outdated
| ++ ", ageAdd = " ++ show (ageAdd s) | ||
| ++ ", txrxTime = " ++ show (txrxTime s) | ||
| ++ " }" | ||
|
|
There was a problem hiding this comment.
This could implement showsPrec instead. Not long ago I was looking for an example and did one here: https://github.com/ocheron/cryptostore/blob/d6212dca8135c07b7dfa4f51b1c269d9a8bc5cb7/src/Crypto/Store/CMS/Info.hs#L194
There was a problem hiding this comment.
Nice idea!
Done.
core/Network/TLS/Types.hs
Outdated
| , sessionGroup :: Maybe Group | ||
| , sessionTicketInfo :: Maybe TLS13TicketInfo | ||
| , sessionALPN :: Maybe ByteString | ||
| , sessionMaxEarlyDataSize :: Word32 |
There was a problem hiding this comment.
Same here, I'm not sure Word32 is the best type on external API.
There was a problem hiding this comment.
Now Int is used.
There was a problem hiding this comment.
If Int is 32 bits, how should we treat valid values of 2^31 or larger?
There was a problem hiding this comment.
Is this validation code acceptable?
-- Word32 is used in TLS 1.3 protocol.
-- Int is used for API for Haskell TLS because it is natural.
-- If Int is 64 bits, users can specify bigger number than Word32.
-- If Int is 32 bits, 2^31 or larger may be converted into minus numbers.
safeNonNegative32 :: (Num a, Ord a) => a -> a
safeNonNegative32 x
| x <= 0 = 0
| otherwise = x
`min` fromIntegral (maxBound :: Int) -- Int is 32 bits
`min` fromIntegral (maxBound :: Word32) -- Int is 64 bitsThere was a problem hiding this comment.
I think fromIntegral (maxBound :: Word32) will be -1 with a 32-bit Int.
What about this instead:
safeNonNegative32 :: Int -> Word32
safeNonNegative32 x
| x <= 0 = 0
| finiteBitSize x <= 32 = fromIntegral x
| otherwise =
fromIntegral (x `min` fromIntegral (maxBound :: Word32))| , ageAdd :: Second -- NewSessionTicket.ticket_age_add | ||
| , txrxTime :: Millisecond -- serverSendTime or clientReceiveTime | ||
| , estimatedRTT :: IORef (Maybe Millisecond) | ||
| } deriving (Eq) |
There was a problem hiding this comment.
Isn't it an issue to expose this IORef? This will allow to change the value.
There was a problem hiding this comment.
RTT has not been estimated when TLS13TicketInfo is created.
So, we need to set it when RTT is estimated.
TLS13TicketInfo is an abstract data type. Its internal is not exported.
There was a problem hiding this comment.
OK but SessionData used to be fully serializable, i.e. the content could be written to a file. What are the recommendations to do this now? Can the IORef be simply skipped and reconstructed empty?
There was a problem hiding this comment.
- When writing, the
IORefshould be read and the value should be stored. - When reading, the value should be obtained and a new
IORefshould be created with the value.
There was a problem hiding this comment.
I tried to consider the possibility to remove estimatedRTT from SessionData.
But it results in introducing a new state manager, sigh.
There was a problem hiding this comment.
Issue is not only with estimatedRTT. If TLS13TicketInfo is opaque then it's not possible to serialize, unless dedicated primitives are provided.
core/Network/TLS/Session.hs
Outdated
| sessionResume :: SessionID -> IO (Maybe SessionData) | ||
| sessionResume :: SessionID -> IO (Maybe SessionData) | ||
| -- | used on server side to decide whether to resume a client session for TLS 1.3 0RTT | ||
| , sessionResumeOnlyOnce :: SessionID -> IO (Maybe SessionData) |
There was a problem hiding this comment.
The documentation should explain expectation about the implementation. i.e. not returning the SessionData again after this call.
There was a problem hiding this comment.
Thank you for your suggestion.
Done.
|
I think that I answered all questions. |
|
@ocheron What's necessary to approve this PR? |
|
Merged. Thank you for your review. |
This PR adds some states, parameters, APIs for TLS 1.3.
This does not break the current testing.