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.

Sign up for GitHub

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

Jump to bottom

DNSTAP logging in Unbound-1.11.0 #365

Closed
iruzanov opened this issue Dec 8, 2020 · 5 comments · Fixed by #367
Closed

DNSTAP logging in Unbound-1.11.0 #365

iruzanov opened this issue Dec 8, 2020 · 5 comments · Fixed by #367

Comments

@iruzanov
Copy link

iruzanov commented Dec 8, 2020

Hello, Wouter!

As i said earlier i have made patches set for Unbound-1.11.0 to log over DNSTAP both addresses - source (from client) address and destination (local or service) one. My core idea is to introduce NSD-like structure called unbound_socket and use it in comm_points of netevent code. So please look in my patches, i hope they will be reasonable and secure for application.
And only one TODO that i have to do in future - right now i can log only the front side, i.e. client requests and responses. Upstream side is currently logged as 0.0.0.0 local address. Its a question of some rework of outside_network code.

All the patches are in attached tarball.

Thank you in advance!
patches.tar.gz
@wcawijngaards
Copy link
Member

You have to use freeaddrinfo for the free of list->socket->addr at listen_dnsport.c 1544.
I would prefer a couple of mallocs of the socket struct to be calloc, so that it is zeroed.
The malloc and free in the daemon worker handle routine to make the dst addr is cumbersome and it appear not really needed, it could put a struct sockaddr_storage on the stack there and pass that to the routine that fills the structure (now mk_local_addr that mallocs). That would be faster and a lot of dnstap users value their speed.

Other than those initial review comments, it looks okay! I can make those changes, or you can if you feel like it, and then it needs a bit more time from me to get a more detailed review. Thanks for the re-submit of this with the bugs fixed that you found.

@iruzanov
Copy link
Author

iruzanov commented Dec 8, 2020

Ohh, yes! It was my stupid mistake about freeaddrinfo().
And i agree with you about malloc(), - it was my blind paranoia to fetch new memory regions for every step in program :)) So it is a question of good program optimizing. Thank you for the valuable clarifications! Could you please to make the changes that you found by yourself? And then when you are done with detailed research i would like to download the final code to find out all the my mistakes ;) Also it would be great to get binary package with patched code of Unbound-1.11.0

wcawijngaards added a commit that referenced this issue Dec 9, 2020
@wcawijngaards
Import the patches from the files in the tarball in
bdfa65c 
issue #365 https://github.com/NLnetLabs/unbound/files/5659923/patches.tar.gz
from iruzanov.  The merge conflicts are fixed, but no changes are made
to the patched code.
@wcawijngaards wcawijngaards mentioned this issue Dec 9, 2020
Merged
@wcawijngaards
Copy link
Member

I have created a branch and pull request at #367 . There I imported the patches you linked in the tar.gz file, and then made the changes and some merge fixes and bug fixes. You can review the code there.

A tarball with the code repository of today with the state of that code now is here https://nlnetlabs.nl/~wouter/unbound-1.13.1_20201209.tar.gz

The diff for the updated patches can be downloaded in this link https://patch-diff.githubusercontent.com/raw/NLnetLabs/unbound/pull/367.diff

@wcawijngaards wcawijngaards linked a pull request Dec 9, 2020 that will close this issue
DNSTAP log local address #367
Merged
@iruzanov
Copy link
Author

iruzanov commented Dec 9, 2020

Great work, Wouter!

And i looked into my patches for your changes and understood that mk_local_addr() was just like a ballast in the code :)) I completely forgot that we can pass repinfo->c->socket->addr directly to dt_msg_send_client_* functions. And also there are more accurate callocs/free in listen_dnsport.c source code. Looks great! Thank you very much!
So now i can download fresh tarball of Unbound!

Also i have made the same patches for NSD-4.2.2. There is more simplier code. And later i will send you the patchset to ask you to review the code for its correctness and optimality.

wcawijngaards added a commit that referenced this issue Feb 25, 2021
@wcawijngaards
Changelog note for #365, #367 and #368.
6c46438 
- Merge PR #367 : DNSTAP log local address.  With code from PR #365
  and fixes #368 : dnstap does not log the DNS message ID for
  FORWARDER_QUERY.
@wcawijngaards
Copy link
Member

Applied your contributed code with review from my colleague @gthess from the linked pull request. Thanks for the contribution!

jedisct1 added a commit to jedisct1/unbound that referenced this issue Feb 26, 2021
@jedisct1
Merge remote-tracking branch 'nlnet/master'
5b3e626 
* nlnet/master: (103 commits)
  - Fix: Resolve interface names on control-interface too.
  - Fix for NLnetLabs#367: rc_ports don't have ub_sock; skip cleaning up.
  - Fix to allow rpz with wildcard that applies to all TLDs at once.
  Changelog note for NLnetLabs#365, NLnetLabs#367 and NLnetLabs#368. - Merge PR NLnetLabs#367 : DNSTAP log local address.  With code from PR NLnetLabs#365   and fixes NLnetLabs#368 : dnstap does not log the DNS message ID for   FORWARDER_QUERY.
  Fix comment item.
  Fix to use a simple pointer in the call of make_sock and make_sock_port.
  - spelling fix in header.
  - Fix unit test for added ulimit checks.
  - Fix function documentation.
  - On startup of unbound it checks if rlimits on memory size look   sufficient for the configured cache size, and logs warning if not.
  - ipsecmod: Better logging for detecting a cycle when attaching the   A/AAAA subquery.
  - Fix NLnetLabs#384: (1) A minor request to improve the log (2) A minor bug in   one log message.
  - Fix for zonemd, do not reject insecure result from trust anchor   validation step in dnssec chain of trust.
  - Fix for zonemd, that domain-insecure zones work without dnssec.
  Spelling fix.
  - Fix for zonemd, that nxdomain for the chain of trust is allowed   for island zones, it is treates as an insecure zone for verification.
  - Fix NLnetLabs#431: Squelch permission denied errors for tcp connect
  - rpz skip nsec3param records, and nicer log for unsupported actions.
  - Fix NLnetLabs#429: rpz: url: with https: broken (regression in 1.13.1).
  - Fix doxygen and pydoc warnings.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

DNSTAP log local address NLnetLabs/unbound
2 participants
@wcawijngaards @iruzanov