Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IPv6 support to fwknop #285

Open
wants to merge 78 commits into
base: master
Choose a base branch
from

Conversation

khorben
Copy link
Contributor

@khorben khorben commented Aug 20, 2018

This branch adds complete support for IPv6 on fwknop. It still has a few limitations though:

  • fwknop must be run with the -6 command-line parameter
  • when using IPTables, fwknop supports either IPv4 or IPv6 but not both at the same time
  • IPv6 is supported in UDP, TCP, and HTTP modes (both client and server)
    This should close Add full IPv6 support to fwknop #1.

The remote code seems to be independent from the fwknop project though.
Until it will be capable to return IPv6 addresses, in itself this will
remain irrelevant for the purpose of adding IPv6 support to fwknop.

On another hand, it does help us introduce definitions and update
headers to actually support IPv6.
Again, this depends on the remote host to be actually supporting IPv6.
It is now called is_valid_ip_addr() and expects an additional parameter
for the address family.
This greatly loosens the check for a valid IPv4/IPv6 there - but it is
redundant anyway, since there is always a call to is_valid_ip_addr().
This alone should allow interacting with IPv4 firewalling rules over
IPv6, for these two protocols.
This will allow porting the raw ICMP code to IPv6.
I believe it should be more portable this way, since AF_INET is required
to be present in <sys/socket.h> in POSIX.
This should help with portability for the protocol family eventually.
This should help with portability for the protocol family eventually.
This should eventually help with portability to IPv6.
The correct syntax is the comma (",") instead.
The problem looks real, fix looks good to me, but have not been able to
reproduce the actual issue so far :(
@mrash
Copy link
Owner

mrash commented Aug 22, 2018

Wow, tons of work. I have a local ipv6 branch to track this. I'm going to add some test suite support for IPv6 so I can work into what you've done, and then push this branch to github so we can collaborate. Let's target the next release of fwknop to merge your changes into master.

@mrash mrash self-assigned this Aug 22, 2018
@securitygeneration
Copy link

That's indeed some epic work. Well done @khorben! Now if only the world could finally move to IPv6 ;)

@khorben
Copy link
Contributor Author

khorben commented Aug 29, 2018

Thank you :)

@khorben
Copy link
Contributor Author

khorben commented Aug 29, 2018

This was sponsored by Asahi Net, Inc. by the way, https://asahi-net.co.jp/en/corporate/ (as mentioned in #1)

@4mig4
Copy link

4mig4 commented Aug 30, 2018

Great work Pierre thanks a lot , much appreciated.

@mrash
Copy link
Owner

mrash commented Sep 2, 2018

Diving into this finally, sorry for the delay. I've opened a new issue based on a start on IPv6 test suite support.

@mrash
Copy link
Owner

mrash commented Oct 15, 2018

Targeting this work for the next major release of fwknop.

Copy link

@bastien-roucaries bastien-roucaries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the size of ipv6 scoped litteral could be more than 40....

Copy link

@bastien-roucaries bastien-roucaries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please instead of MAX_IP46_STR_LENGTH use INET6_ADDRSTRLEN

While you are right that longtest IPv6 address takes 39 bytes, with IPv4 tunneling, the longest form can be 45 bytes:

Copy link

@bastien-roucaries bastien-roucaries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should change and use constant

@@ -87,8 +87,8 @@ typedef struct fko_cli_options
int no_save_args;
int use_hmac;
char spa_server_str[MAX_SERVER_STR_LEN]; /* may be a hostname */
char allow_ip_str[MAX_IPV4_STR_LEN];
char spoof_ip_src_str[MAX_IPV4_STR_LEN];
char allow_ip_str[MAX_IPV46_STR_LEN];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use INET6_ADDRSTRLEN
While you are right that longtest IPv6 address takes 39 bytes, with IPv4 tunneling, the longest form can be 45 bytes:

char allow_ip_str[MAX_IPV4_STR_LEN];
char spoof_ip_src_str[MAX_IPV4_STR_LEN];
char allow_ip_str[MAX_IPV46_STR_LEN];
char spoof_ip_src_str[MAX_IPV46_STR_LEN];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use INET6_ADDRSTRLEN

@@ -59,6 +59,12 @@
#define MAX_IPV4_STR_LEN 16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INET4_ADDRSTRLEN

@@ -59,6 +59,12 @@
#define MAX_IPV4_STR_LEN 16
#define MIN_IPV4_STR_LEN 7

#define MAX_IPV46_STR_LEN 40

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INET6_ADDRSTRLEN

#define MAX_IPV46_STR_LEN 40
#define MIN_IPV46_STR_LEN 3

#define MAX_IPV6_STR_LEN 40

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INET6_ADDRSTRLEN

Copy link

@bastien-roucaries bastien-roucaries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with this one

int o1, o2, o3, o4, got_resp=0, i=0;
char *ndx, resp[MAX_IPV4_STR_LEN+1] = {0};
int got_resp=0, error;
char resp[MAX_IPV4_STR_LEN+1] = {0};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_IPV46_STR_LEN ?

for (rp = result; rp != NULL; rp = rp->ai_next) {
/* the canonical value is in the first structure returned */
strlcpy(options->allow_ip_str,
rp->ai_canonname, sizeof(options->allow_ip_str));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why getting last entry ?

@bam80
Copy link

bam80 commented May 31, 2023

Why it wasn't merged still?

@khorben
Copy link
Contributor Author

khorben commented May 31, 2023

I do not know. The reviewer (@bastien-roucaries) seems to have approved the changes 3 years ago; on my side I haven't had the opportunity to comment on the proposed changes.
Maybe the remaining limitations are considered unsuitable for merge into master; @mrash?

@bam80
Copy link

bam80 commented Jun 6, 2023

@mrash Could you comment please?
Without IPv6 support, it looks mostly as abandon-ware these days, so it makes sense to clarify the project status..

@damienstuart @bastien-roucaries
#344 #309

@mrash
Copy link
Owner

mrash commented Jun 6, 2023 via email

@bam80
Copy link

bam80 commented Jun 6, 2023

Thank you for the update Michael.

First, if we can get nft support into fwknop, then v6 support becomes a lot easier.

Yeah, that is the other blocker: #107.
Without it we can't even run the daemon on recent popular router firmware:
#352
https://forum.openwrt.org/t/fwknop-on-x86-64-22-03-2/143855
So should be probably addressed first indeed.

There seems to be a lot of work ahead but I hope we will get there eventually!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add full IPv6 support to fwknop
6 participants