)]}' { "commit": "484c3340e09fe99ddce41cb3ec7ea3fba6457bd6", "tree": "78d1bd9c4f71beb05c1dea5321b236951ff4c049", "parents": [ "090256ac02ee25fccf916b73c64cd81d08f2f574" ], "author": { "name": "David Benjamin", "email": "davidben@google.com", "time": "Fri Nov 22 00:38:38 2024 -0500" }, "committer": { "name": "Boringssl LUCI CQ", "email": "boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com", "time": "Thu Dec 05 18:45:44 2024 +0000" }, "message": "Rework how DTLS ACKs and retransmits are flushed\n\nThis CL addresses a number of issues:\n\n1. If an ACK or retransmit hit SSL_ERROR_WANT_WRITE, we do not provide a\n way for the caller to retry it.\n\n2. If the retransmit timer fires too many times, we do not track the\n fatal error. Instead, we return an error out of the\n DTLSv1_handle_timeout and leave the timeout unhandled. If the caller\n does not tear down the connection in response to\n DTLSv1_handle_timeout failing, they may register a timeout for 0s,\n wake up again and loop forever.\n\n This happened in https://crbug.com/42224241. I filed\n https://crbug.com/42290595 to track fixing this API wart.\n\n3. When the DTLS open_app_data hook needs to retransmit or send an ACK,\n we write immediately in that callback, but the eventual aim was to\n provide an in-place encrypt/decrypt API, so those hooks should not\n perform I/O.\n\n DTLS 1.3 adds a lot more cases where we\u0027ll want to do this:\n\n - We might read a message and then want to ACK on a timer\n - We might read a message and then want to ACK immediately\n - We might read a past retransmit and then want to retransmit\n the next flight\n - We might read an ACK, clear the current flight, and then want to\n send a queued up KeyUpdate that was waiting on it.\n\nTo address all these, this CL rearranges things slightly. Now the DTLS\nrecord layer keeps track of whether it owes the peer an ACK and a\nflight. If so, the flush() operation will try to write those out. If it\nfails, it keeps track of how far it got. If it succeeds, it clears those\nflags.\n\nVarious operations can set those flags, notably the timeouts. This means\nthe actual \"handle timeout\" part of DTLSv1_handle_timeout is\ninfallible. Handling a timeout means cashing the timeout expiry in for\nsetting the flag.\n\nFrom there, we drive flush() to completion where we need to. For now,\nI\u0027ve added it to the handshake and SSL_read(), though I\u0027m not sure if\nI\u0027ve gotten exactly the right spots. (Should we also flush on\nSSL_write?) We could also go further and have flush() drive\nhandle_timeout(), and then handle_timeout() can be deprecated\naltogether. The model is just \"you must retry this operation after the\ntimeout, in addition to the SSL_ERROR_WANT_WHATEVER you got\". That\u0027s\nquite attractive, but for now I haven\u0027t quite gone that far.\n\nAnother complication is that flush() currently behaves very differently\nbetween TLS and DTLS. In DTLS, I had to add an explicit finish_flight()\nto queue up the flight for sending. If you leave things in there, it\u0027s\nno big deal because everything in DTLS is unordered. In TLS, everything\nis ordered, so once we\u0027ve encrypted records, we have to flush them\nregardless. To that end, in TLS we generally like to flush things\nimplicitly on write, because that\u0027s when the caller is expecting I/O\nanyway, and we can concatenate the, say, encrypted KeyUpdate with the\nencrypted application data. Not sure what\u0027s best to do there. For now\nI\u0027ve just gated flush() calls on SSL_is_dtls().\n\nI\u0027m sure we\u0027ll find this also isn\u0027t quite right and rearrange it again\nlater, but hopefully this works a bit better than what we had before.\n\nBug: 42290594, 376718254\nFixed: 42290595\nChange-Id: I4d9b5c753530b514a1bc5e4e69ddb25dfc8c1d06\nReviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73527\nReviewed-by: Nick Harper \u003cnharper@chromium.org\u003e\nCommit-Queue: David Benjamin \u003cdavidben@google.com\u003e\n", "tree_diff": [ { "type": "modify", "old_id": "17b5d0ee151e02b733ebf0bb47f0c448798d6f09", "old_mode": 33188, "old_path": "include/openssl/ssl.h", "new_id": "e7c57c7082d506756b1586ba025d3a35632af155", "new_mode": 33188, "new_path": "include/openssl/ssl.h" }, { "type": "modify", "old_id": "838c21b53fba7022cade1c7aa12ae50443c23c9e", "old_mode": 33188, "old_path": "ssl/d1_both.cc", "new_id": "db51c9a36c373bf5c752d8ce74a92d27d9f93d6f", "new_mode": 33188, "new_path": "ssl/d1_both.cc" }, { "type": "modify", "old_id": "520bde3e0ecf2040962773e5d16f6eb25e8371e6", "old_mode": 33188, "old_path": "ssl/d1_lib.cc", "new_id": "5cca499bb4f10f9d12f5d203ae4af740757e4ae9", "new_mode": 33188, "new_path": "ssl/d1_lib.cc" }, { "type": "modify", "old_id": "7bdeca18c9116e4c99ce8150fe8bcb3f69cb9005", "old_mode": 33188, "old_path": "ssl/d1_pkt.cc", "new_id": "b4d4c678278d6be94da409f53f188e1eae992d07", "new_mode": 33188, "new_path": "ssl/d1_pkt.cc" }, { "type": "modify", "old_id": "54bac2fc337de8d5f12ae24e13b41ebc3db75a87", "old_mode": 33188, "old_path": "ssl/dtls_method.cc", "new_id": "de600bb08979482890da1b7e9beef4ea6aa30fc1", "new_mode": 33188, "new_path": "ssl/dtls_method.cc" }, { "type": "modify", "old_id": "968fe1b6649f91cc99f7b1246027ab85ec4518aa", "old_mode": 33188, "old_path": "ssl/handshake.cc", "new_id": "0b5db42ac07d271ef8e455fd3be4d9a7f0328458", "new_mode": 33188, "new_path": "ssl/handshake.cc" }, { "type": "modify", "old_id": "9721cb1fb36b48b418390d12e24015ac489e40e1", "old_mode": 33188, "old_path": "ssl/internal.h", "new_id": "463852b4893203378a79cc3d69736eedeeabdd36", "new_mode": 33188, "new_path": "ssl/internal.h" }, { "type": "modify", "old_id": "f456ad781f0f577ba02c70af937ecdc74bf6e535", "old_mode": 33188, "old_path": "ssl/s3_both.cc", "new_id": "82671c02930dd5f4345e848fe0a5b2bc2ae9e4b2", "new_mode": 33188, "new_path": "ssl/s3_both.cc" }, { "type": "modify", "old_id": "f7d0263f9323a9dfb8b509c7347476c32999b419", "old_mode": 33188, "old_path": "ssl/ssl_lib.cc", "new_id": "379e628dab26c025c9ecb9541eb95f7095655032", "new_mode": 33188, "new_path": "ssl/ssl_lib.cc" }, { "type": "modify", "old_id": "a47660d61f36df07d6f314f64e1508e91ba91eb1", "old_mode": 33188, "old_path": "ssl/ssl_test.cc", "new_id": "055015fd632b54fe008f114b04973c1349684cf3", "new_mode": 33188, "new_path": "ssl/ssl_test.cc" }, { "type": "modify", "old_id": "1c9859afee5641f196c55a396d3aca3d07a6a7b4", "old_mode": 33188, "old_path": "ssl/test/async_bio.cc", "new_id": "a2967159cc7bc2f13a663a896f1d4dea888957a9", "new_mode": 33188, "new_path": "ssl/test/async_bio.cc" }, { "type": "modify", "old_id": "9974139d954a30f2a5f30a842f37449ca1a44992", "old_mode": 33188, "old_path": "ssl/test/async_bio.h", "new_id": "7ab764e3890e5b09d8c8fe20351296ea6d870f72", "new_mode": 33188, "new_path": "ssl/test/async_bio.h" }, { "type": "modify", "old_id": "5a17c5cc7f0eaf3eafb4dadc746c2649ec4e9e07", "old_mode": 33188, "old_path": "ssl/test/bssl_shim.cc", "new_id": "aada04fcc8688a9f9e3d90f58b82659f06ddffda", "new_mode": 33188, "new_path": "ssl/test/bssl_shim.cc" }, { "type": "modify", "old_id": "d5e43e7fd6d79e3a5b8d5a3f403b6e276131347e", "old_mode": 33188, "old_path": "ssl/test/handshake_util.cc", "new_id": "f2c58a646d3c3ab1ca3e4fa753a29f7d817305df", "new_mode": 33188, "new_path": "ssl/test/handshake_util.cc" }, { "type": "modify", "old_id": "54bb20c69f8900a18b4d79be3ce2be966078eebb", "old_mode": 33188, "old_path": "ssl/tls13_both.cc", "new_id": "e0d5ad0ba7697b3ecc904bbf2b8a6f212059b351", "new_mode": 33188, "new_path": "ssl/tls13_both.cc" }, { "type": "modify", "old_id": "b783b0aef9613ac1cfa071c153435ab00bac0084", "old_mode": 33188, "old_path": "ssl/tls13_server.cc", "new_id": "8b6683eebee17dc41c4a1df58951e0dd2ca2fb28", "new_mode": 33188, "new_path": "ssl/tls13_server.cc" }, { "type": "modify", "old_id": "0861d8687d5f0affdae930247a6be61aac213142", "old_mode": 33188, "old_path": "ssl/tls_method.cc", "new_id": "33c3a7a08109decc55cfc248127623f9c3d72568", "new_mode": 33188, "new_path": "ssl/tls_method.cc" } ] }