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

Added dns.question.top_level_domain and dns.question.subdomain ecs field #14578

Merged
merged 6 commits into from
Dec 20, 2019
Merged

Added dns.question.top_level_domain and dns.question.subdomain ecs field #14578

merged 6 commits into from
Dec 20, 2019

Conversation

mbudge
Copy link
Contributor

@mbudge mbudge commented Nov 17, 2019

Added fields recently added to the elastic common schema to packeteat dns output.

Added the following fields to packetbeat

  • dns.question.subdomain
  • dns.question.top_level_domain

I tried adding unit tests for these fields but you might want to check that.

I also can't find the fields.yml file to define the exported packetbeat fields in the documentation.

Also not sure how to update the change log to document updates to packetbeat.

Here is an example of the code to get the top_level_domain and the subdomain.

`

                if eTLDPlusOne != "" && strings.Contains(eTLDPlusOne, ".") {
			topLevelDomain := strings.Join(strings.Split(eTLDPlusOne, ".")[1:], ".")
			qMapStr["top_level_domain"] = topLevelDomain

			subdomain := strings.Trim(strings.TrimRight(q.Name, eTLDPlusOne), ".")
			if subdomain != "" {
				qMapStr["subdomain"] = subdomain
			}
		}

`

The top_level_domain field removes the PlusOne to get the TLD (org, net, com etc)

The subdomain field is the full question name, minus the eTLDPlusOne/Registered_Domain with dots trimmed. If the value isn't an empty string then a subdomain is present.

Elastic common schema fields

Add dns.question.subdomain field #574
elastic/ecs#574

Added top_level_domain #572
elastic/ecs#572

Both the top_level_domain and subdomain are useful for security analytics (discussed in the linked ECS pull requests in more detail).

Added fields recently added to the elastic common schema to packeteat dns output.
@mbudge mbudge requested a review from a team as a code owner November 17, 2019 00:41
@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?

@mbudge
Copy link
Contributor Author

mbudge commented Nov 17, 2019

How do I fix this?

`
/go/src/beats/packetbeat$ mage fmt update
panic: failed determine libbeat dir location: failed to find github.com/elastic/beats/dev-tools/mage in the project's vendor

goroutine 1 [running]:
github.com/elastic/beats/dev-tools/mage.LibbeatDir(0xc0000b5e30, 0x1, 0x1, 0xc0000cad80, 0x34)
...../go/src/github.com/elastic/beats/dev-tools/mage/common.go:729 +0x1d7
github.com/elastic/beats/dev-tools/mage.init.ializers()
...../go/src/github.com/elastic/beats/dev-tools/mage/config.go:45 +0x443

`

@webmat
Copy link
Contributor

webmat commented Nov 18, 2019

jenkins, test this

@webmat
Copy link
Contributor

webmat commented Nov 18, 2019

Thanks @mbudge!

@andrewkroh or @adriansr can you guys take a look?

@mbudge
Copy link
Contributor Author

mbudge commented Nov 29, 2019

Please can someone help with this?

Thanks

@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@adriansr
Copy link
Contributor

Thanks for your contribution @mbudge !

I've pushed a commit with some test cases. Now it just needs a changelog entry, I'll leave that to you.

The file to edit is CHANGELOG.next.asciidoc, you need to a add a new line under Added -> Packetbeat. See 831c544 for an example.

@mbudge
Copy link
Contributor Author

mbudge commented Dec 6, 2019

Updated the change log.

@mbudge
Copy link
Contributor Author

mbudge commented Dec 9, 2019

Found a bug in the subdomain extract

https://play.golang.org/p/xydzQ0thVBg

fmt.Println(strings.TrimRight("news.joules.com", "joules.com"))

Fixes a bug where some subdomains where missing characters.
@mbudge
Copy link
Contributor Author

mbudge commented Dec 16, 2019

I found the code didn't handle domains like gov.ky because it threw an error.

The code only runs if the error contains "cannot derive eTLD+1 for domain", 1 dot and the top_level_domain string to the right of the dot isn't empty.

               else if strings.Contains(err.Error(), "cannot derive eTLD+1 for domain") && strings.Count(q.Name, ".") == 1 {
		// Handle publicsuffix.EffectiveTLDPlusOne eTLD+1 error with 1 dot in the domain.
		s := strings.Split(eTLDPlusOne, ".")
		if len(s) == 2 && s[1] != "" {
			qMapStr["top_level_domain"] = s[1]
		}
		qMapStr["registered_domain"] = q.Name
	}

Here's an example in Go playground https://play.golang.org/p/Kn3PQ70OGp0

…th 1 dot in the domain.

Changed eTLDPlusOne to q.Name 

// Handle publicsuffix.EffectiveTLDPlusOne eTLD+1 error with 1 dot in the domain.
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.

Here's my take on some utility methods for getting at the tld, etld, registered domain, and subdomain. https://play.golang.org/p/x0sUqRePN7-

One difference from what this does today is that no longer considers private suffixes in the PSL when getting the ETLD.

@@ -501,6 +501,23 @@ func addDNSToMapStr(m common.MapStr, dns *mkdns.Msg, authority bool, additional
// etld_plus_one should be removed for 8.0.0.
qMapStr["etld_plus_one"] = eTLDPlusOne
qMapStr["registered_domain"] = eTLDPlusOne

if eTLDPlusOne != "" && strings.Contains(eTLDPlusOne, ".") {
topLevelDomain := strings.Join(strings.Split(eTLDPlusOne, ".")[1:], ".")
Copy link
Member

Choose a reason for hiding this comment

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

Splitting and then re-joining seems inefficient. I think it could slice up the eTLDPlusOne string by finding the index of the first dot.

qMapStr["subdomain"] = subdomain
}
}
} else if strings.Contains(err.Error(), "cannot derive eTLD+1 for domain") && strings.Count(q.Name, ".") == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Matching error strings is really error prone because the error can change upstream without notice. Is there a way you can do this without matching the error string?

@mbudge
Copy link
Contributor Author

mbudge commented Dec 18, 2019

I've removed the string.contains eTLD+1 error as it doesn't matter what the error is. Also replaced strings.split with strings.indexbyte.

The helper functions would be good in a shared library so they can be accessed by all the beats agents. I've added the top_level_domain and registered_domain to all the ECS domain fields. It's only sub-domain which is dns only in ECS.

@andrewkroh
Copy link
Member

jenkins, test this

@andrewkroh andrewkroh merged commit bd1d277 into elastic:master Dec 20, 2019
@andrewkroh andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Mar 19, 2020
@andrewkroh andrewkroh added v7.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Mar 19, 2020
andrewkroh pushed a commit to andrewkroh/beats that referenced this pull request Mar 19, 2020
…elds (elastic#14578)

Added the following fields to packetbeat

    dns.question.subdomain
    dns.question.top_level_domain

Co-authored-by: Adrian Serrano <adrisr83@gmail.com>
(cherry picked from commit bd1d277)
andrewkroh added a commit that referenced this pull request Mar 20, 2020
…elds (#14578) (#17127)

Added the following fields to packetbeat

    dns.question.subdomain
    dns.question.top_level_domain

Co-authored-by: Adrian Serrano <adrisr83@gmail.com>
Co-authored-by: mbudge <mbudge1@gmail.com>

(cherry picked from commit bd1d277)
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.

5 participants