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 support for Public Suffix #38

Merged
merged 5 commits into from
Feb 17, 2022

Conversation

juliusrickert
Copy link
Contributor

Add fields based on the public suffix list (PSL):

  • public suffix
  • public suffix + 1 label

Useful for aggregation.

@juliusrickert
Copy link
Contributor Author

@dmachard What do you think about this?

I am mostly interested in aggregating by domain, so by public suffix + 1 label. (I know, there are cases for which this does not work.)

@dmachard
Copy link
Owner

dmachard commented Feb 6, 2022

Thank again, any PR will be apreciated.

Just to be sure, have you take a look to the User feature privacy ?

If my understand is correct, you can have acheived what you need by enable the minimaze-name
for example xx.mail.google.com wil be replaced by google.com so on the top domain metrics you have only tld+1label

subprocessors:
  user-privacy:
    minimaze-qname: true

On another hand, your implemention is perphap better, have you make some bench regarding the publicsuffix library ?

@juliusrickert
Copy link
Contributor Author

I did look at the user privacy feature but it doesn't work as intended for domains like .co.uk.

I have not conducted a performance test yet but I'm pretty certain that the impact would be negligible.

Add fields based on the public suffix list (PSL).
Useful for aggregation.
@juliusrickert
Copy link
Contributor Author

have you make some bench regarding the publicsuffix library ?

I ran the BenchmarkPublicSuffix.
I got ~50k ops/s on a single logical CPU on my dated Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz.

Performance-wise, this should be absolutely negligible.

I moved the the computation of the PSL-related fields to the dnstap and dnssniffer collectors.
EffectiveTLDPlusOne is not populated if QNAME minimisation ist requested.

@dmachard
Copy link
Owner

dmachard commented Feb 9, 2022

I did look at the user privacy feature but it doesn't work as intended for domains like .co.uk.

You're right, the qname minimization implementation is very basic.

Thanks for feedback regarding performance.

@dmachard
Copy link
Owner

dmachard commented Feb 9, 2022

Before to validate the merge request, could you add some documentations regarding the new directives qnamepublicsuffix and qnameeffectivetldplusone ?

Thanks again :)

@juliusrickert
Copy link
Contributor Author

could you add some documentations regarding the new directives qnamepublicsuffix and qnameeffectivetldplusone ?

I didn't add these directives. Why did you choose to write your own templating instead of using text/template and just passing the dnsutils.DnsMessage? :)

Neither did I add support for the tail collector.

@dmachard
Copy link
Owner

In fact you did, in the file dnsutils/message.go line 218 to 221

case "qnamepublicsuffix":
    s.WriteString(dm.DNS.QnamePublicSuffix)
case "qnameeffectivetldplusone":
    s.WriteString(dm.DNS.QnameEffectiveTLDPlusOne)

This part in the code is used to have a custom text output line.

Regarding text/template, I will take a look thanks.

@juliusrickert
Copy link
Contributor Author

Hey @dmachard, I've added documentation regarding the newly added directives :)

@dmachard dmachard merged commit 1f689ba into dmachard:main Feb 17, 2022
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.

2 participants