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 minimum cache TTL for successful DNS responses #18986

Merged
merged 12 commits into from
Aug 10, 2020

Conversation

ansell
Copy link
Contributor

@ansell ansell commented Jun 4, 2020

What does this PR do?

An enhancement to add a minimum alternative cache TTL to the libbeat dns processor for successful DNS responses. This ensures that TTL=0 successful reverse DNS responses can be cached to avoid sending the same reverse DNS request again within a short period of time.

Why is it important?

The libbeat dns processor is used as a reverse DNS annotator for auditbeat events. Some of these IP addresses respond to reverse DNS requests with TTL=0 in the responses. These were causing load issues for my systems when I had the reverse DNS processor enabled for auditbeat.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • Sign Elastic CLA

How to test this PR locally

The auditbeat system/socket module that I had the issue with is not open-sourced, so I have not been able to test its completeness myself. I am submitting this in good faith that it can be completed by someone with access to that module for testing purposes, or who is experienced enough to add the necessary tests here.

Related issues

Use cases

This will add a cache for successful DNS responses that will not always correspond to the upstream DNS TTL value. This is acceptable as it avoids a possible denial-of-service for auditbeat enabled systems, and it is configurable by users.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 4, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 4, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [andrewkroh commented: run tests]

  • Start Time: 2020-08-02T17:21:46.199+0000

  • Duration: 87 min 49 sec

Test stats 🧪

Test Results
Failed 0
Passed 14465
Skipped 1373
Total 15838

@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 5, 2020
@marc-gr
Copy link
Contributor

marc-gr commented Jul 15, 2020

Hello @ansell and thanks for opening this PR!

Could you rebase this from master and add an entry in the CHANGELOG.next.asciidoc file? Or if you prefer we take over from this point please say so! Whatever works best for you

@marc-gr
Copy link
Contributor

marc-gr commented Jul 15, 2020

jenkins run tests

@ansell ansell force-pushed the issue-18709-auditbeat-dns-cache branch from b736d97 to 6044af7 Compare July 16, 2020 08:36
@ansell
Copy link
Contributor Author

ansell commented Jul 16, 2020

@marc-gr I followed the basic instructions that @andrewkroh mentioned on the linked issue but I wasn't able to get the tests to work locally to verify my approach was appropriate.

I would prefer if you took over at this point. I have rebased onto the current master and pushed again.

@marc-gr
Copy link
Contributor

marc-gr commented Jul 16, 2020

@marc-gr I followed the basic instructions that @andrewkroh mentioned on the linked issue but I wasn't able to get the tests to work locally to verify my approach was appropriate.

I would prefer if you took over at this point. I have rebased onto the current master and pushed again.

No problem!

@marc-gr
Copy link
Contributor

marc-gr commented Jul 16, 2020

jenkins run tests

@marc-gr
Copy link
Contributor

marc-gr commented Jul 16, 2020

I fixed the set method and added a unit test. Please @andrewkroh take a look in case I missed something when you have a minute.

@marc-gr marc-gr requested a review from andrewkroh July 16, 2020 09:32
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I have requested a change to ensure the TTL returned by the PTRLookupCache consistently reflects the cache TTL rather than the DNS response TTL.

Additionally this needs

  • Changelog entry
  • Doc update to include the new parameter

libbeat/processors/dns/config.go Outdated Show resolved Hide resolved
libbeat/processors/dns/config.go Outdated Show resolved Hide resolved
libbeat/processors/dns/cache_test.go Outdated Show resolved Hide resolved
libbeat/processors/dns/cache_test.go Outdated Show resolved Hide resolved
libbeat/processors/dns/cache.go Outdated Show resolved Hide resolved
@marc-gr
Copy link
Contributor

marc-gr commented Jul 17, 2020

jenkins run tests

@marc-gr marc-gr requested a review from andrewkroh July 17, 2020 08:24
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ansell and @marc-gr .

@ansell
Copy link
Contributor Author

ansell commented Jul 18, 2020

I made one small change to make the error message match the configuration item it references.

@marc-gr
Copy link
Contributor

marc-gr commented Jul 20, 2020

jenkins run tests

@marc-gr
Copy link
Contributor

marc-gr commented Jul 21, 2020

jenkins run tests

@andrewkroh
Copy link
Member

run tests

@andrewkroh andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Aug 10, 2020
@andrewkroh andrewkroh merged commit 72da5a6 into elastic:master Aug 10, 2020
@andrewkroh andrewkroh changed the title issue #18709 : Add minimum cache TTL for successful DNS responses Add minimum cache TTL for successful DNS responses Aug 10, 2020
andrewkroh pushed a commit to andrewkroh/beats that referenced this pull request Aug 10, 2020
An enhancement to add a minimum alternative cache TTL to the libbeat dns processor for successful DNS responses. This ensures that TTL=0 successful reverse DNS responses can be cached to avoid sending the same reverse DNS request again within a short period of time.

The libbeat dns processor is used as a reverse DNS annotator for auditbeat events. Some of these IP addresses respond to reverse DNS requests with TTL=0 in the responses. These were causing load issues for my systems when I had the reverse DNS processor enabled for auditbeat.

The new settings is `success_cache.min_ttl`.

Closes elastic#18709

Signed-off-by: Peter Ansell <[email protected]>
Co-authored-by: Marc Guasch <[email protected]>
(cherry picked from commit 72da5a6)
@andrewkroh andrewkroh added v7.10.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 10, 2020
andrewkroh added a commit that referenced this pull request Aug 11, 2020
…esponses (#20525)

* Add success cache minimum TTL for DNS responses (#18986)

An enhancement to add a minimum alternative cache TTL to the libbeat dns processor for successful DNS responses. This ensures that TTL=0 successful reverse DNS responses can be cached to avoid sending the same reverse DNS request again within a short period of time.

The libbeat dns processor is used as a reverse DNS annotator for auditbeat events. Some of these IP addresses respond to reverse DNS requests with TTL=0 in the responses. These were causing load issues for my systems when I had the reverse DNS processor enabled for auditbeat.

The new settings is `success_cache.min_ttl`.

Closes #18709

Signed-off-by: Peter Ansell <[email protected]>
Co-authored-by: Marc Guasch <[email protected]>
(cherry picked from commit 72da5a6)

* Update CHANGELOG.next.asciidoc

Co-authored-by: Peter Ansell <[email protected]>
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
An enhancement to add a minimum alternative cache TTL to the libbeat dns processor for successful DNS responses. This ensures that TTL=0 successful reverse DNS responses can be cached to avoid sending the same reverse DNS request again within a short period of time.

The libbeat dns processor is used as a reverse DNS annotator for auditbeat events. Some of these IP addresses respond to reverse DNS requests with TTL=0 in the responses. These were causing load issues for my systems when I had the reverse DNS processor enabled for auditbeat.

The new settings is `success_cache.min_ttl`.

Closes elastic#18709

Signed-off-by: Peter Ansell <[email protected]>
Co-authored-by: Marc Guasch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a minimum TTL to auditbeat reverse DNS resolution
5 participants