From 6044af7fd249160b5631cdaf76617c5b7262d99f Mon Sep 17 00:00:00 2001 From: Peter Ansell Date: Wed, 27 May 2020 08:44:33 +1000 Subject: [PATCH 1/7] issue #18709 : Add minimum cache TTL for successful DNS responses Signed-off-by: Peter Ansell --- libbeat/processors/dns/cache.go | 22 ++++++++++++++++------ libbeat/processors/dns/config.go | 9 ++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/libbeat/processors/dns/cache.go b/libbeat/processors/dns/cache.go index 6bd6b373db9..6db83666d30 100644 --- a/libbeat/processors/dns/cache.go +++ b/libbeat/processors/dns/cache.go @@ -37,6 +37,7 @@ type ptrCache struct { sync.RWMutex data map[string]ptrRecord maxSize int + minSuccessTTL time.Duration } func (c *ptrCache) set(now time.Time, key string, ptr *PTR) { @@ -49,7 +50,7 @@ func (c *ptrCache) set(now time.Time, key string, ptr *PTR) { c.data[key] = ptrRecord{ host: ptr.Host, - expires: now.Add(time.Duration(ptr.TTL) * time.Second), + expires: now.Add(maxDuration(time.Duration(ptr.TTL), c.minSuccessTTL) * time.Second), } } @@ -135,11 +136,12 @@ func (ce *cachedError) Cause() error { return ce.err } // reverse DNS queries. It caches the results of queries regardless of their // outcome (success or failure). type PTRLookupCache struct { - success *ptrCache - failure *failureCache - failureTTL time.Duration - resolver PTRResolver - stats cacheStats + success *ptrCache + minSuccessTTL time.Duration + failure *failureCache + failureTTL time.Duration + resolver PTRResolver + stats cacheStats } type cacheStats struct { @@ -157,6 +159,7 @@ func NewPTRLookupCache(reg *monitoring.Registry, conf CacheConfig, resolver PTRR success: &ptrCache{ data: make(map[string]ptrRecord, conf.SuccessCache.InitialCapacity), maxSize: conf.SuccessCache.MaxCapacity, + minSuccessTTL: conf.SuccessCache.MinTTL, }, failure: &failureCache{ data: make(map[string]failureRecord, conf.FailureCache.InitialCapacity), @@ -208,3 +211,10 @@ func max(a, b int) int { } return b } + +func maxDuration(a time.Duration, b time.Duration) time.Duration { + if a >= b { + return a + } + return b +} diff --git a/libbeat/processors/dns/config.go b/libbeat/processors/dns/config.go index ae447a20c72..a80919953b3 100644 --- a/libbeat/processors/dns/config.go +++ b/libbeat/processors/dns/config.go @@ -84,9 +84,12 @@ type CacheConfig struct { // CacheSettings define the caching behavior for an individual cache. type CacheSettings struct { // TTL value for items in cache. Not used for success because we use TTL - // from the DNS record. + // from the DNS record or the minimum configured TTL. TTL time.Duration `config:"ttl"` + // Minimum TTL value for successful DNS responses. + MinTTL time.Duration `config:"ttl.min" validate:"min=1"` + // Initial capacity. How much space is allocated at initialization. InitialCapacity int `config:"capacity.initial" validate:"min=0"` @@ -122,6 +125,9 @@ func (c *Config) Validate() error { // Validate validates the data contained in the CacheConfig. func (c *CacheConfig) Validate() error { + if c.SuccessCache.MinTTL <= 0 { + return errors.Errorf("success_cache.ttl.min must be > 0") + } if c.FailureCache.TTL <= 0 { return errors.Errorf("failure_cache.ttl must be > 0") } @@ -146,6 +152,7 @@ func (c *CacheConfig) Validate() error { var defaultConfig = Config{ CacheConfig: CacheConfig{ SuccessCache: CacheSettings{ + MinTTL: time.Minute, InitialCapacity: 1000, MaxCapacity: 10000, }, From f560dc4b90307d1246e74ac1ba654e28893f89fd Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Thu, 16 Jul 2020 11:31:04 +0200 Subject: [PATCH 2/7] Fix cache set and add unit test --- libbeat/processors/dns/cache.go | 10 +++++----- libbeat/processors/dns/cache_test.go | 25 +++++++++++++++++++++---- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/libbeat/processors/dns/cache.go b/libbeat/processors/dns/cache.go index 6db83666d30..2eb4d659fa5 100644 --- a/libbeat/processors/dns/cache.go +++ b/libbeat/processors/dns/cache.go @@ -35,8 +35,8 @@ func (r ptrRecord) IsExpired(now time.Time) bool { type ptrCache struct { sync.RWMutex - data map[string]ptrRecord - maxSize int + data map[string]ptrRecord + maxSize int minSuccessTTL time.Duration } @@ -50,7 +50,7 @@ func (c *ptrCache) set(now time.Time, key string, ptr *PTR) { c.data[key] = ptrRecord{ host: ptr.Host, - expires: now.Add(maxDuration(time.Duration(ptr.TTL), c.minSuccessTTL) * time.Second), + expires: now.Add(maxDuration(time.Duration(ptr.TTL)*time.Second, c.minSuccessTTL)), } } @@ -157,8 +157,8 @@ func NewPTRLookupCache(reg *monitoring.Registry, conf CacheConfig, resolver PTRR c := &PTRLookupCache{ success: &ptrCache{ - data: make(map[string]ptrRecord, conf.SuccessCache.InitialCapacity), - maxSize: conf.SuccessCache.MaxCapacity, + data: make(map[string]ptrRecord, conf.SuccessCache.InitialCapacity), + maxSize: conf.SuccessCache.MaxCapacity, minSuccessTTL: conf.SuccessCache.MinTTL, }, failure: &failureCache{ diff --git a/libbeat/processors/dns/cache_test.go b/libbeat/processors/dns/cache_test.go index d64dbd460b4..51729e4e000 100644 --- a/libbeat/processors/dns/cache_test.go +++ b/libbeat/processors/dns/cache_test.go @@ -19,8 +19,8 @@ package dns import ( "io" - "strings" "testing" + "time" "github.com/stretchr/testify/assert" @@ -30,12 +30,14 @@ import ( type stubResolver struct{} func (r *stubResolver) LookupPTR(ip string) (*PTR, error) { - if ip == gatewayIP { + switch ip { + case gatewayIP: return &PTR{Host: gatewayName, TTL: gatewayTTL}, nil - } else if strings.HasSuffix(ip, "11") { + case gatewayIP + "1": return nil, io.ErrUnexpectedEOF + case gatewayIP + "2": + return &PTR{Host: gatewayName, TTL: 0}, nil } - return nil, &dnsError{"fake lookup returned NXDOMAIN"} } @@ -98,4 +100,19 @@ func TestCache(t *testing.T) { assert.EqualValues(t, 3, c.stats.Hit.Get()) assert.EqualValues(t, 3, c.stats.Miss.Get()) // Cache miss. } + + // Cache returned TTL=0 with MinTTL. + ptr, err = c.LookupPTR(gatewayIP + "2") + if assert.NoError(t, err) { + assert.EqualValues(t, gatewayName, ptr.Host) + // TTL counts down while in cache. + assert.EqualValues(t, 0, ptr.TTL) + assert.EqualValues(t, 3, c.stats.Hit.Get()) + assert.EqualValues(t, 4, c.stats.Miss.Get()) + + minTTL := defaultConfig.CacheConfig.SuccessCache.MinTTL + expectedExpire := time.Now().Add(minTTL).Unix() + gotExpire := c.success.data[gatewayIP+"2"].expires.Unix() + assert.InDelta(t, expectedExpire, gotExpire, 1) + } } From 7b14f821b64349add975494f709b412e89911ac6 Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Fri, 17 Jul 2020 09:38:39 +0200 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Andrew Kroh --- libbeat/processors/dns/cache.go | 2 +- libbeat/processors/dns/cache_test.go | 1 - libbeat/processors/dns/config.go | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/libbeat/processors/dns/cache.go b/libbeat/processors/dns/cache.go index 2eb4d659fa5..3612c8aec46 100644 --- a/libbeat/processors/dns/cache.go +++ b/libbeat/processors/dns/cache.go @@ -212,7 +212,7 @@ func max(a, b int) int { return b } -func maxDuration(a time.Duration, b time.Duration) time.Duration { +func maxDuration(a, b time.Duration) time.Duration { if a >= b { return a } diff --git a/libbeat/processors/dns/cache_test.go b/libbeat/processors/dns/cache_test.go index 51729e4e000..6d9527237ec 100644 --- a/libbeat/processors/dns/cache_test.go +++ b/libbeat/processors/dns/cache_test.go @@ -105,7 +105,6 @@ func TestCache(t *testing.T) { ptr, err = c.LookupPTR(gatewayIP + "2") if assert.NoError(t, err) { assert.EqualValues(t, gatewayName, ptr.Host) - // TTL counts down while in cache. assert.EqualValues(t, 0, ptr.TTL) assert.EqualValues(t, 3, c.stats.Hit.Get()) assert.EqualValues(t, 4, c.stats.Miss.Get()) diff --git a/libbeat/processors/dns/config.go b/libbeat/processors/dns/config.go index a80919953b3..0a1dff61a72 100644 --- a/libbeat/processors/dns/config.go +++ b/libbeat/processors/dns/config.go @@ -84,11 +84,11 @@ type CacheConfig struct { // CacheSettings define the caching behavior for an individual cache. type CacheSettings struct { // TTL value for items in cache. Not used for success because we use TTL - // from the DNS record or the minimum configured TTL. + // from the DNS record. TTL time.Duration `config:"ttl"` // Minimum TTL value for successful DNS responses. - MinTTL time.Duration `config:"ttl.min" validate:"min=1"` + MinTTL time.Duration `config:"min_ttl" validate:"min=1"` // Initial capacity. How much space is allocated at initialization. InitialCapacity int `config:"capacity.initial" validate:"min=0"` From e199f01b64d004616a98c81ace5133fa0add1b6f Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Fri, 17 Jul 2020 09:48:19 +0200 Subject: [PATCH 4/7] Add CHANGELOG and doc entries --- CHANGELOG.next.asciidoc | 1 + libbeat/processors/dns/docs/dns.asciidoc | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 53123ca30f5..d2886fe5397 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -341,6 +341,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Add the `overwrite_keys` configuration option to the dissect processor. {pull}19464[19464] - Add support to trim captured values in the dissect processor. {pull}19464[19464] - Added the `max_cached_sessions` option to the script processor. {pull}19562[19562] +- Add minimum cache TTL for successful DNS responses. {pull}18986[18986] *Auditbeat* diff --git a/libbeat/processors/dns/docs/dns.asciidoc b/libbeat/processors/dns/docs/dns.asciidoc index b75fb8bf87a..10063a0ca2e 100644 --- a/libbeat/processors/dns/docs/dns.asciidoc +++ b/libbeat/processors/dns/docs/dns.asciidoc @@ -51,6 +51,7 @@ processors: success_cache: capacity.initial: 1000 capacity.max: 10000 + min_ttl: 1m failure_cache: capacity.initial: 1000 capacity.max: 10000 @@ -80,6 +81,9 @@ the memory for this number of items. Default value is `1000`. cache can hold. When the maximum capacity is reached a random item is evicted. Default value is `10000`. +`success_cache.min_ttl`:: The duration of the minimum alternative cache TTL for successful DNS responses. Ensures that `TTL=0` successful reverse DNS responses can be cached. +Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". Default value is `1m`. + `failure_cache.capacity.initial`:: The initial number of items that the failure cache will be allocated to hold. When initialized the processor will allocate the memory for this number of items. Default value is `1000`. From 61a2a6968c16682453501913e310e9b3656c5f2a Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Fri, 17 Jul 2020 10:03:39 +0200 Subject: [PATCH 5/7] Override ptr.TTL with minTTL and cleanup unused code --- libbeat/processors/dns/cache.go | 24 +++++++++--------------- libbeat/processors/dns/cache_test.go | 17 ++++++++++++++--- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/libbeat/processors/dns/cache.go b/libbeat/processors/dns/cache.go index 3612c8aec46..d8be672d920 100644 --- a/libbeat/processors/dns/cache.go +++ b/libbeat/processors/dns/cache.go @@ -50,7 +50,7 @@ func (c *ptrCache) set(now time.Time, key string, ptr *PTR) { c.data[key] = ptrRecord{ host: ptr.Host, - expires: now.Add(maxDuration(time.Duration(ptr.TTL)*time.Second, c.minSuccessTTL)), + expires: now.Add(time.Duration(ptr.TTL) * time.Second), } } @@ -136,12 +136,10 @@ func (ce *cachedError) Cause() error { return ce.err } // reverse DNS queries. It caches the results of queries regardless of their // outcome (success or failure). type PTRLookupCache struct { - success *ptrCache - minSuccessTTL time.Duration - failure *failureCache - failureTTL time.Duration - resolver PTRResolver - stats cacheStats + success *ptrCache + failure *failureCache + resolver PTRResolver + stats cacheStats } type cacheStats struct { @@ -201,18 +199,14 @@ func (c PTRLookupCache) LookupPTR(ip string) (*PTR, error) { return nil, err } + // We set the ptr.TTL to the minimum TTL in case it is less than that. + ptr.TTL = max(ptr.TTL, uint32(c.success.minSuccessTTL/time.Second)) + c.success.set(now, ip, ptr) return ptr, nil } -func max(a, b int) int { - if a >= b { - return a - } - return b -} - -func maxDuration(a, b time.Duration) time.Duration { +func max(a, b uint32) uint32 { if a >= b { return a } diff --git a/libbeat/processors/dns/cache_test.go b/libbeat/processors/dns/cache_test.go index 6d9527237ec..2242d1527c0 100644 --- a/libbeat/processors/dns/cache_test.go +++ b/libbeat/processors/dns/cache_test.go @@ -101,17 +101,28 @@ func TestCache(t *testing.T) { assert.EqualValues(t, 3, c.stats.Miss.Get()) // Cache miss. } - // Cache returned TTL=0 with MinTTL. + minTTL := defaultConfig.CacheConfig.SuccessCache.MinTTL + // Initial success returned TTL=0 with MinTTL. ptr, err = c.LookupPTR(gatewayIP + "2") if assert.NoError(t, err) { assert.EqualValues(t, gatewayName, ptr.Host) - assert.EqualValues(t, 0, ptr.TTL) + + assert.EqualValues(t, minTTL/time.Second, ptr.TTL) assert.EqualValues(t, 3, c.stats.Hit.Get()) assert.EqualValues(t, 4, c.stats.Miss.Get()) - minTTL := defaultConfig.CacheConfig.SuccessCache.MinTTL expectedExpire := time.Now().Add(minTTL).Unix() gotExpire := c.success.data[gatewayIP+"2"].expires.Unix() assert.InDelta(t, expectedExpire, gotExpire, 1) } + + // Cached success from a previous TTL=0 response. + ptr, err = c.LookupPTR(gatewayIP + "2") + if assert.NoError(t, err) { + assert.EqualValues(t, gatewayName, ptr.Host) + // TTL counts down while in cache. + assert.InDelta(t, minTTL/time.Second, ptr.TTL, 1) + assert.EqualValues(t, 4, c.stats.Hit.Get()) + assert.EqualValues(t, 4, c.stats.Miss.Get()) + } } From 4fc6e1f0e0716ba6b8fd50fd6ff399fb73f897a1 Mon Sep 17 00:00:00 2001 From: Peter Ansell Date: Sat, 18 Jul 2020 13:46:08 +1000 Subject: [PATCH 6/7] issue #18709 : Change error message to match config item change Signed-off-by: Peter Ansell --- libbeat/processors/dns/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libbeat/processors/dns/config.go b/libbeat/processors/dns/config.go index 0a1dff61a72..2c2bcedda24 100644 --- a/libbeat/processors/dns/config.go +++ b/libbeat/processors/dns/config.go @@ -126,7 +126,7 @@ func (c *Config) Validate() error { // Validate validates the data contained in the CacheConfig. func (c *CacheConfig) Validate() error { if c.SuccessCache.MinTTL <= 0 { - return errors.Errorf("success_cache.ttl.min must be > 0") + return errors.Errorf("success_cache.min_ttl must be > 0") } if c.FailureCache.TTL <= 0 { return errors.Errorf("failure_cache.ttl must be > 0") From 09d146439f6f9f5a46e4581750c6a24f5a04e496 Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Tue, 21 Jul 2020 13:53:18 +0200 Subject: [PATCH 7/7] Reorder CHANGELOG.next.asciidoc --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 7d14fc1d861..01b54926519 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -348,8 +348,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Add the `overwrite_keys` configuration option to the dissect processor. {pull}19464[19464] - Add support to trim captured values in the dissect processor. {pull}19464[19464] - Added the `max_cached_sessions` option to the script processor. {pull}19562[19562] -- Add minimum cache TTL for successful DNS responses. {pull}18986[18986] - Add support for DNS over TLS for the dns_processor. {pull}19321[19321] +- Add minimum cache TTL for successful DNS responses. {pull}18986[18986] *Auditbeat*