Page MenuHomeFreeBSD

Add UDP encapsulation of ESP in IPv6
ClosedPublic

Authored by xavier.beaudouin_klarasystems.com on Nov 9 2023, 9:01 AM.
Tags
None
Referenced Files
F103426014: D42526.id131607.diff
Sun, Nov 24, 9:10 PM
Unknown Object (File)
Fri, Nov 15, 3:31 AM
Unknown Object (File)
Thu, Nov 14, 1:21 AM
Unknown Object (File)
Oct 8 2024, 10:14 PM
Unknown Object (File)
Oct 4 2024, 4:50 PM
Unknown Object (File)
Oct 4 2024, 4:02 PM
Unknown Object (File)
Oct 2 2024, 7:30 AM
Unknown Object (File)
Oct 2 2024, 6:23 AM

Details

Reviewers
markj
ae
tuexen
Group Reviewers
transport
Summary

This patch provides UDP encapsulation of ESP packets over IPv6.
It mostly consist of porting IPv4 code to IPv6 and adding support of IPv6 in udpencap.c
As required by the RFC and unlike in IPv4 encapsulation, UDP checksums are calculated

Test Plan for this change:
After enabling UDP encapsulation with NAT66, verify pings are correctly
encapsulated in UDP, as well as other data (ssh, netcat thru IPSec Tunnel).

Authored-by: Aurelien Cazuc <aurelien.cazuc.external@stormshield.eu>
Co-authored-by: Xavier Beaudouin <xavier.beaudouin@klarasystems.com>
Sponsored-by: Stormshield
Sponsored-by: Wiktel
Sponsored-by: Klara Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55318
Build 52207: arc lint + arc unit

Event Timeline

allanjude edited the summary of this revision. (Show Details)
tuexen added inline comments.
sys/netinet6/udp6_usrreq.c
662

This function is not used anywhere. So why do you define it? Later down, .pr_ctloutput is set to udp_ctloutput. I guess you want to tweak udp_ctloutput to do the right thing for handling UDP_ENCAP for IPv6. Or am I missing something?

sys/netinet6/udp6_usrreq.c
662

Hi !
You are totally right, this code is not used mostly because I have worked on old code.
I am currently working to fix that.
Testing the code if it is correctly working and will fix this soon.

Removed unused function in sys/netinet6/udp6_usrreq.c, added IPv6
support for SOPT_SET and SOPT_GET in sys/netinet/udp_usrreq.c

This revision is now accepted and ready to land.Nov 28 2023, 2:27 PM

There are some style bugs in the patch. Could you please run tools/build/checkstyle9.pl on the patch file and fix the problems that it reports? In particular, case statements should be at the same indentation level as the switch, and fixing that will fix some of the complaints about lines being too long.

There are some style bugs in the patch. Could you please run tools/build/checkstyle9.pl on the patch file and fix the problems that it reports? In particular, case statements should be at the same indentation level as the switch, and fixing that will fix some of the complaints about lines being too long.

You mean in sys/netinet/udp_usrreq.c ?

This update fix the styles on switch / case as requested

This revision now requires review to proceed.Dec 1 2023, 10:21 AM

There are some style bugs in the patch. Could you please run tools/build/checkstyle9.pl on the patch file and fix the problems that it reports? In particular, case statements should be at the same indentation level as the switch, and fixing that will fix some of the complaints about lines being too long.

You mean in sys/netinet/udp_usrreq.c ?

Sorry, I missed your question last week. I mean, run checkstyle9.pl on a file containing the patch, not just the changes to udp_usrreq.c.

sys/netinet/udp_usrreq.c
908–909

The break statement does nothing here.

There is a mix of styles here: some error paths return an error number, others set error equal to the error number, then break;. We should be consistent. Most of the code in this function does the latter, so the new code should do that as well.

972–973

Same comments as above.

sys/netinet6/udp6_usrreq.c
185

Rather than checking IPSEC_ENABLED(ipv6) twice, it'd be cleaner to structure the code like this:

if (IPSEC_ENABLED(ipv6)) {
    if (IPSEC_CHECK_POLICY(...) != 0) {
        ...
    }
    if ((up->u_flags & UF_ESPINUDP) != 0 &&
        UDPENCAP_INPUT(...) != 0) {
            return (0);
    }
}
sys/netipsec/key.c
5877

This line is too long.

5882

The style here is wrong, the opening brace should be on the previous line.

5886

The indentation here is wrong. checkstyle9.pl should complain about this.

sys/netipsec/udpencap.c
262

What if there is an IPv6 extension header following the base header? We will end up inserting the UDP header in between, which seems wrong. Is there some guarantee that this won't happen?

371

There should be an empty line after variable declarations.

kib added inline comments.
sys/netipsec/key.c
5877

Why do you use literal '16' there?

sys/netipsec/udpencap.c
148

There should be a blank line after the locals declarations.

210

A blank line should be added before the block comment

233

There too

349

extra ()

356

extra ()

sys/netinet6/udp6_usrreq.c
185

Good Idea.

sys/netipsec/udpencap.c
210

Will add it, but this comment was not from me.

233

Will do.

sys/netipsec/udpencap.c
262

@tuexen can you say whether I'm missing something here? Is there some reason we can simply not worry about extension headers here?

xavier.beaudouin_klarasystems.com marked 11 inline comments as done.
  • Fix some style bugs as requested (line too long / space before comment etc...)
  • Make structure clearer on line 179 of udp6_usrreq.c
  • Remove litteral
  • Remove unecessary break
sys/netipsec/udpencap.c
262

@markj Good point. I think the UDP header should be placed right before the ESP header, not after the IPv6 header. There can be extension headers between the IPv6 header and the ESP header.

From RFC 4303:

In the IPv6 context, ESP is viewed as an end-to-end payload, and thus
should appear after hop-by-hop, routing, and fragmentation extension
headers. Destination options extension header(s) could appear
before, after, or both before and after the ESP header depending on
the semantics desired. However, because ESP protects only fields
after the ESP header, it generally will be desirable to place the
destination options header(s) after the ESP header. The following
diagram illustrates ESP transport mode positioning for a typical IPv6
packet.

sys/netinet/udp_usrreq.c
912
sys/netipsec/ipsec_output.c
938

This looks like it should instead be

#if defined(INET) || defined(INET6)
sys/netipsec/key.c
5943

The style here is still wrong. Please make sure to address the issues that checkstyle9.pl reports.

sys/netipsec/udpencap.c
262

Looking closer, I think the current implementation always puts the ESP header immediately after the base header: ipsec(4|6)_perform_request() pass the offset of the "next header" field to esp_output(), which inserts its own header before the original next header. Subsequently the IP layer may insert extension headers, but that happens only after UDP encapsulation is done.

So, I think the code is ok, it should just assert that ip6->ip6_nxt == IPPROTO_ESP, i.e., I'd like to see a KASSERT() to that effect.

387

There should be an empty line after variable declarations. The same issue exists in a few places in the diff.

xavier.beaudouin_klarasystems.com added inline comments.
sys/netipsec/key.c
5886

It complains in lots of part of this file...

5943

Same as previous comment.

sys/netipsec/udpencap.c
262

@markj Do I have to add something to this code?

xavier.beaudouin_klarasystems.com added inline comments.
sys/netipsec/udpencap.c
387

Will fix

Fixed some style issues. On some files checkstyle9.pl is litterally yelling on
allmost all lines. Sorry to have tried to use same incorrect style of the file.
Added suggested #if defined on ipsec_output.c.

Fixed some style issues. On some files checkstyle9.pl is litterally yelling on
allmost all lines. Sorry to have tried to use same incorrect style of the file.
Added suggested #if defined on ipsec_output.c.

Are you running checkstyle9.pl on the patch file or on the source files? When I try it, it complains mostly about some lines being too long. Let's just ignore those. There are two other nits, which I noted in the review.

sys/netinet6/udp6_usrreq.c
177
sys/netipsec/key.c
5939

The opening brace should be on the previous line.

sys/netipsec/udpencap.c
262

Please add a KASSERT which verifies that ip6->ip6_nxt is equal to IPPROTO_ESP.

392

Continuing lines should be indented by four spaces, not one.

sys/netipsec/udpencap.c
392

Okay but checkstyle9pl is complaining about 80 col. So... is this an exception or not?

xavier.beaudouin_klarasystems.com added inline comments.
sys/netinet6/udp6_usrreq.c
177

Sorry

sys/netipsec/key.c
5939

Forgotten this one... sorry;

This revision is now accepted and ready to land.Jan 11 2024, 3:20 PM