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

Use struct sockaddr_in6 for all addresses #1081

Merged
merged 1 commit into from
Oct 19, 2018
Merged

Conversation

i-rinat
Copy link
Contributor

@i-rinat i-rinat commented Oct 16, 2018

IPv4 addresses are represented as IPv4-mapped IPv6.

@krizhanovsky krizhanovsky requested review from vankoven and removed request for krizhanovsky October 16, 2018 21:20
Copy link
Contributor

@vankoven vankoven left a comment

Choose a reason for hiding this comment

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

Good, but small fixup is required.

* Valid HMAC for 24 bytes (first 2 bytes with AF_INET value, 14 zero bytes of
* IP address and 8 bytes with timestamp in little-endian order to match HMAC
* generated in TempestaFW code on x86):
* Valid HMAC for 36 bytes (2 bytes for AF_INET6, 26 zero bytes for the reset of
Copy link
Contributor

Choose a reason for hiding this comment

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

'for the reset' -> 'for the rest'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

tempesta_fw/t/unit/test_http_sticky.c Outdated Show resolved Hide resolved
@@ -404,7 +404,7 @@ tfw_listen_sock_start(TfwListenSock *ls)

inet_sk(sk)->freebind = 1;
sk->sk_reuse = 1;
r = ss_bind(sk, &addr->sa, tfw_addr_sa_len(addr));
r = ss_bind(sk, tfw_addr_sa(addr), tfw_addr_sa_len(addr));
Copy link
Contributor

Choose a reason for hiding this comment

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

The ss_bind() has different actions for AF_INET and AF_INET6 sockets. May be it worth to pass TfwAddr * as parameter for the ss_bind()? Same apply to ss_connect().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed ss_connect() and ss_bind() to accept const TfwAddr *.
ss_bind() now uses only inet6_bind(), but that should be fine, since inet6_bind() recognizes IPv4-mapped addresses and sets IPv4 source addresses just fine.

@@ -265,7 +265,7 @@ int prio0, prio1, prio3;
#define frang_dbg(fmt_msg, addr, ...) \
do { \
char abuf[TFW_ADDR_STR_BUF_SIZE] = {0}; \
tfw_addr_fmt_v6(&(addr)->v6.sin6_addr, 0, abuf); \
tfw_addr_fmt_v6(&(addr)->sin6_addr, 0, abuf); \
Copy link
Contributor

Choose a reason for hiding this comment

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

The macros is very close to the TFW_WARN_MOD_ADDR6() used few lines above excluding TFW_WARN/TFW_DBG difference. It seems pretty generic, shouldn't we put it to the log.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no obvious deduplication, I haven't found any other similar places. So I'd rather avoid moving it into log.h, at least in this PR.

Copy link
Contributor

@aleksostapenko aleksostapenko left a comment

Choose a reason for hiding this comment

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

Good to merge.

}
return 0;

if (tfw_addr_port(listener) != tfw_addr_port(server))
Copy link
Contributor

@aleksostapenko aleksostapenko Oct 18, 2018

Choose a reason for hiding this comment

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

It seems, that the same check with exit is repeated here and in the v4mapped block above. Perhaps it would be better to leave only one this check - before the v4 verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the check to the beginning of the function.

@i-rinat i-rinat force-pushed the ri-775-always-use-ipv6 branch 2 times, most recently from cba92f4 to b75daae Compare October 18, 2018 21:26
Drops everything from the TfwAddr union, making it essentially an alias for
struct sockaddr_in6. IPv4 addresses are converted to IPv4-mapped addresses
at the parsing stage.

Macros for log reporting with addresses of both IPv4 and IPv6 are unified.
They got a new parameter controlling whenever ports should be reported too.
@i-rinat
Copy link
Contributor Author

i-rinat commented Oct 18, 2018

Addressed (almost all of) comments. That lead to further changes:

  • ss_bind(), ss_connect(), and tfw_filter_block_ip() now use TfwAddr * for the address;
  • set of macros TFW_*_ADDR() and TFW_*_ADDR6() were combined into the set of TFW_*_ADDR(). Additionally, those macros now take an additional boolean parameter, controlling whenever a port is included into the string;
  • tfw_addr_fmt_v4 and tfw_addr_fmt_v6 were merged into tfw_addr_fmt().

Copy link
Contributor

@vankoven vankoven left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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.

3 participants