Add UDP encapsulation of ESP in IPv6

[kiwi]

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 <[e-mail]>
Co-authored-by: Xavier Beaudouin <[e-mail]>
Sponsored-by: Stormshield
Sponsored-by: Wiktel
Sponsored-by: Klara Inc.

[tuexen]

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?

[kiwi]

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.

[kiwi]

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

[markj]

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.

[kiwi]

In D42526#976026, @markj wrote:

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 ?

[kiwi]

This update fix the styles on switch / case as requested

[markj]

In D42526#977356, @xavier.beaudouin_klarasystems.com wrote:

In D42526#976026, @markj wrote:

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]

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 ()

[kiwi]

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.

[markj]

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?

[kiwi]

• 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

[kiwi]

Fixed some suggested changes.

[tuexen]

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.

[markj]

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.

[kiwi]

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?

[kiwi]

sys/netipsec/udpencap.c

387

Will fix

[kiwi]

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.

[markj]

In D42526#983008, @xavier.beaudouin_klarasystems.com wrote:

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.

[kiwi]

sys/netipsec/udpencap.c

392

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

[kiwi]

sys/netinet6/udp6_usrreq.c

177

Sorry

sys/netipsec/key.c

5939

Forgotten this one... sorry;

[kiwi]

Style fix for now

[kiwi]

Added KASSERT() requested

[allanjude]

Committed as 80044c785cb040a2cf73779d23f9e1e81a00c6c3