Hi Frank,
Thank you for your feedback. I'll make the changes as per your suggestions
and resubmit the patch later. My apologies for not realizing that you were
using a formatting check tool. I installed Uncrustify, and it worked
perfectly. :-)
I also wanted to ask about the automated tests on GitHub. Should I fork the
project and submit the patch there, or should I submit it directly to the
main project? What is the recommended approach?
Looking forward to your guidance.
Best regards,
Rafael
On Fri, Feb 21, 2025 at 8:58 AM Frank Lichtenheld <fr...@lichtenheld.com>
wrote:
> On Thu, Feb 20, 2025 at 04:48:27PM -0300, Rafael Gava wrote:
> > I believe this feature will be beneficial for many OpenVPN users,
> improving
> > automation and simplifying VPN gateway configurations. Please find the
> > patch inline for review and feedback.
> >
>
> Thanks for your submission. Some general notes about the patch.
> (Not a review of the actual change but more about the format of
> submission)
>
> > ----
> > From fbc7045d652b5f11e2aa043aa2af9ca14f36b604 Mon Sep 17 00:00:00 2001
> > From: Rafael Gava <gava...@gmail.com>
> > Date: Thu, 20 Feb 2025 19:19:39 +0000
> > Subject: [PATCH] Added the localhost token to the client-nat network
> option,
> > enabling the application to dynamically use the client IP provided by
> the
> > server.
>
> The subject line should be concise and make sense on its own. The rest
> of the commit message should be the longer description. You can achieve
> this
> by adding an empty line after the first one.
>
> > Signed-off-by: Rafael Gava <gava...@gmail.com>
> > ---
> > src/openvpn/clinat.c | 47 +++++++++++++++++++++++++++++++++++++++----
> > src/openvpn/clinat.h | 3 +++
> > src/openvpn/init.c | 2 ++
> > src/openvpn/options.c | 1 +
> > 4 files changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/openvpn/clinat.c b/src/openvpn/clinat.c
> > index 2d3b359f..1c79b20d 100644
> > --- a/src/openvpn/clinat.c
> > +++ b/src/openvpn/clinat.c
> > @@ -127,12 +127,19 @@ add_client_nat_to_option_list(struct
> > client_nat_option_list *dest,
> > return;
> > }
> >
> > - e.network = getaddr(0, network, 0, &ok, NULL);
> > - if (!ok)
> > + if (network && !strcmp(network, "localhost"))
> > {
> > - msg(msglevel, "client-nat: bad network: %s", network);
> > - return;
> > + msg (M_INFO, "*** client-nat localhost detected...");
> > + e.network = 0xFFFFFFFF;
> > + } else {
>
> This is not in line with our usual formatting. We always format
> like this
>
> }
> else
> {
>
> There is an uncrustify config in dev-tools/uncrustify.conf that can
> help to check the formatting. Or you submit the patch in Github or
> Gerrit which can do automated checks of the formatting.
>
> > + e.network = getaddr(0, network, 0, &ok, NULL);
> > + if (!ok)
> > + {
> > + msg(msglevel, "client-nat: bad network: %s", network);
> > + return;
> > + }
> > }
> > +
> > e.netmask = getaddr(0, netmask, 0, &ok, NULL);
> > if (!ok)
> > {
> > @@ -274,3 +281,35 @@ client_nat_transform(const struct
> > client_nat_option_list *list,
> > }
> > }
> > }
> > +
> > +/*
> > +* Replaces the localhost token with the IP received from OpenVPN
> > +*/
> > +bool
> > +update_localhost_nat(struct client_nat_option_list *dest, in_addr_t
> > local_ip)
> > +{
> > + int i;
> > + bool ret = false;
> > +
> > + if (!dest) {
> > + return ret;
> > + }
> > +
> > + for (i=0; i <= dest->n; i++)
> > + {
> > + struct client_nat_entry *nat_entry = &dest->entries[i];
> > + if (nat_entry && nat_entry->network == 0xFFFFFFFF)
> > + {
> > + struct in_addr addr;
> > +
> > + nat_entry->network = ntohl(local_ip);
> > + addr.s_addr = nat_entry->network;
> > + char *dot_ip = inet_ntoa(addr);
> > +
> > + msg (M_INFO, "CNAT - Updating NAT table from localhost to:
> > %s", dot_ip);
> > + ret = true;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > diff --git a/src/openvpn/clinat.h b/src/openvpn/clinat.h
> > index 94141f51..06afa3b4 100644
> > --- a/src/openvpn/clinat.h
> > +++ b/src/openvpn/clinat.h
> > @@ -64,4 +64,7 @@ void client_nat_transform(const struct
> > client_nat_option_list *list,
> > struct buffer *ipbuf,
> > const int direction);
> >
> > +bool update_localhost_nat(struct client_nat_option_list *dest, in_addr_t
> > local_ip);
> > +
> > +
> > #endif /* if !defined(CLINAT_H) */
> > diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> > index b57e5f8a..dadc10dc 100644
> > --- a/src/openvpn/init.c
> > +++ b/src/openvpn/init.c
> > @@ -2052,6 +2052,8 @@ do_open_tun(struct context *c, int *error_flags)
> > *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS);
> > }
> >
> > + update_localhost_nat(c->options.client_nat,
> c->c1.tuntap->local);
> > +
> > ret = true;
> > static_context = c;
> > }
> > diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> > index 6b2dfa58..f7161931 100644
> > --- a/src/openvpn/options.c
> > +++ b/src/openvpn/options.c
> > @@ -254,6 +254,7 @@ static const char usage_message[] =
> > " (Server) Instead of forwarding IPv6 packets
> send\n"
> > " ICMPv6 host unreachable packets to the
> client.\n"
> > "--client-nat snat|dnat network netmask alias : on client add 1-to-1
> > NAT rule.\n"
> > + " set the network to 'localhost' to use the client
> ip
> > received from the server.\n"
>
> Please also include an update to the documentation in
> doc/man-sections/client-options.rst
>
> > "--push-peer-info : (client only) push client info to server.\n"
> > "--setenv name value : Set a custom environmental variable to pass
> to
> > script.\n"
> > "--setenv FORWARD_COMPATIBLE 1 : Relax config file syntax checking
> to
> > allow\n"
> > --
> > 2.39.5
>
> Regards,
> --
> Frank Lichtenheld
>
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
