| Message ID | cover.1632240523.git.cdleonard@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tcp: Initial support for RFC5925 auth option | expand |
On Tue, 21 Sep 2021 19:14:43 +0300 Leonard Crestez wrote: > This is similar to TCP MD5 in functionality but it's sufficiently > different that wire formats are incompatible. Compared to TCP-MD5 more > algorithms are supported and multiple keys can be used on the same > connection but there is still no negotiation mechanism. Hopefully there will be some feedback / discussion, but even if everyone acks this you'll need to fix all the transient build failures, and kdoc warnings added - and repost. git rebase --exec='make' and scripts/kernel-doc are your allies.
On Tue, Sep 21, 2021 at 9:15 AM Leonard Crestez <cdleonard@gmail.com> wrote: > * Sequence Number Extension not implemented so connections will flap > every ~4G of traffic. Could you expand on this? What exactly do you mean by flap? Will the connection be terminated? I assume that depending on the initial sequence numbers the first flaps may occur well before 4G. Do you use a SNE of 0 in the hash computation, or do you just not include the SNE in it? Thanks, Francesco
On 9/22/21 11:23 PM, Francesco Ruggeri wrote: > On Tue, Sep 21, 2021 at 9:15 AM Leonard Crestez <cdleonard@gmail.com> wrote: >> * Sequence Number Extension not implemented so connections will flap >> every ~4G of traffic. > > Could you expand on this? > What exactly do you mean by flap? Will the connection be terminated? > I assume that depending on the initial sequence numbers the first flaps > may occur well before 4G. > Do you use a SNE of 0 in the hash computation, or do you just not include > the SNE in it? SNE is hardcoded to zero, with the logical consequence of incorrect signatures on sequence number wrapping. The SNE has to be included because otherwise all signatures would be invalid. You are correct that this can break much sooner than 4G of traffic, but still in the GB range on average. I didn't test the exact behavior (not clear how) but if signatures don't validate the connection will likely timeout. My plan is to use TCP_REPAIR to control sequence numbers and test this reliably in an isolated environment (not interop with a cisco VM or similar). I want to implement TCP_REPAIR support for TCP-AO anyway. It was skipped because the patch series is already quite large. -- Regards, Leonard
On 9/22/21 2:13 AM, Jakub Kicinski wrote: > On Tue, 21 Sep 2021 19:14:43 +0300 Leonard Crestez wrote: >> This is similar to TCP MD5 in functionality but it's sufficiently >> different that wire formats are incompatible. Compared to TCP-MD5 more >> algorithms are supported and multiple keys can be used on the same >> connection but there is still no negotiation mechanism. > > Hopefully there will be some feedback / discussion, but even if > everyone acks this you'll need to fix all the transient build > failures, and kdoc warnings added - and repost. > git rebase --exec='make' and scripts/kernel-doc are your allies. Hello, I already went through several round of testing with git rebase --exec='$test' but it seems I introduced a few new failures after several rounds of squashing fixes. I'll need to check kernel-doc comments for source files not referenced in documenation. Many of the patch splits were artificially created in order to ease review, for example "signing packets" doesn't do anything without also "hooking in the tcp stack". Some static functions will trigger warnings because they're unused until the next patch, not clear what the preferred solution would be here. I could remove the "static" marker until the next patch or reverse the order and have the initial "tcp integration" patches call crypto code that just returns an error and fills-in a signature of zeros. A large amount of the code is just selftests and much of it is not completely specific to TCP-AO. Maybe I could try to repost the parts that verify handling of timewait corners and resets in a variant that only handles "md5" and "unsigned"? I already tried posting my scapy implementation of TCP-AO and MD5 to scapy upstream because it is not specific to linux . -- Regards, Leonard
On Thu, 23 Sep 2021 10:49:53 +0300 Leonard Crestez wrote: > Many of the patch splits were artificially created in order to ease > review, for example "signing packets" doesn't do anything without also > "hooking in the tcp stack". Some static functions will trigger warnings > because they're unused until the next patch, not clear what the > preferred solution would be here. I could remove the "static" marker > until the next patch or reverse the order and have the initial "tcp > integration" patches call crypto code that just returns an error and > fills-in a signature of zeros. Ease of review is important, so although discouraged transient warnings are acceptable if the code is much easier to read that way. The problem here was that the build was also broken, but looking at it again I think you're just missing exports, please make sure to build test with IPV6 compiled as a module: ERROR: modpost: "tcp_authopt_hash" [net/ipv6/ipv6.ko] undefined! ERROR: modpost: "__tcp_authopt_select_key" [net/ipv6/ipv6.ko] undefined!
On 9/23/21 1:38 AM, Leonard Crestez wrote: > On 9/22/21 11:23 PM, Francesco Ruggeri wrote: >> On Tue, Sep 21, 2021 at 9:15 AM Leonard Crestez <cdleonard@gmail.com> >> wrote: >>> * Sequence Number Extension not implemented so connections will flap >>> every ~4G of traffic. >> >> Could you expand on this? >> What exactly do you mean by flap? Will the connection be terminated? >> I assume that depending on the initial sequence numbers the first flaps >> may occur well before 4G. >> Do you use a SNE of 0 in the hash computation, or do you just not include >> the SNE in it? > > SNE is hardcoded to zero, with the logical consequence of incorrect > signatures on sequence number wrapping. The SNE has to be included > because otherwise all signatures would be invalid. > > You are correct that this can break much sooner than 4G of traffic, but > still in the GB range on average. I didn't test the exact behavior (not > clear how) but if signatures don't validate the connection will likely > timeout. > This is for BGP and LDP connections. What's the expected frequency of rollover for large FIBs? Seems like it could be fairly often.
On 9/25/21 4:35 AM, David Ahern wrote: > On 9/23/21 1:38 AM, Leonard Crestez wrote: >> On 9/22/21 11:23 PM, Francesco Ruggeri wrote: >>> On Tue, Sep 21, 2021 at 9:15 AM Leonard Crestez <cdleonard@gmail.com> >>> wrote: >>>> * Sequence Number Extension not implemented so connections will flap >>>> every ~4G of traffic. >>> >>> Could you expand on this? >>> What exactly do you mean by flap? Will the connection be terminated? >>> I assume that depending on the initial sequence numbers the first flaps >>> may occur well before 4G. >>> Do you use a SNE of 0 in the hash computation, or do you just not include >>> the SNE in it? >> >> SNE is hardcoded to zero, with the logical consequence of incorrect >> signatures on sequence number wrapping. The SNE has to be included >> because otherwise all signatures would be invalid. >> >> You are correct that this can break much sooner than 4G of traffic, but >> still in the GB range on average. I didn't test the exact behavior (not >> clear how) but if signatures don't validate the connection will likely >> timeout. >> > > This is for BGP and LDP connections. What's the expected frequency of > rollover for large FIBs? Seems like it could be fairly often. Implementing SNE is obviously required for standard conformance, I'm not claiming it is not needed. I will include this in a future version. I skipped it because it has very few interactions with the rest of the code so it can be implemented separately. Many tests can pass just fine ignoring SNE. -- Regards, Leonard
On 9/23/21 4:58 PM, Jakub Kicinski wrote: > On Thu, 23 Sep 2021 10:49:53 +0300 Leonard Crestez wrote: >> Many of the patch splits were artificially created in order to ease >> review, for example "signing packets" doesn't do anything without also >> "hooking in the tcp stack". Some static functions will trigger warnings >> because they're unused until the next patch, not clear what the >> preferred solution would be here. I could remove the "static" marker >> until the next patch or reverse the order and have the initial "tcp >> integration" patches call crypto code that just returns an error and >> fills-in a signature of zeros. > > Ease of review is important, so although discouraged transient warnings > are acceptable if the code is much easier to read that way. The problem > here was that the build was also broken, but looking at it again I > think you're just missing exports, please make sure to build test with > IPV6 compiled as a module: > > ERROR: modpost: "tcp_authopt_hash" [net/ipv6/ipv6.ko] undefined! > ERROR: modpost: "__tcp_authopt_select_key" [net/ipv6/ipv6.ko] undefined! The kernel build robot sent me an email regarding IPv6=m last time, I fixed that issue but introduced another. I check for IPv6=m specifically but only did "make net/ipv4/ net/ipv6/" and missed the error. I went through the series and solved checkpatch, kernel-doc and compilation issues in a more systematic fashion, I will repost later. -- Regards, Leonard