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

Specify MAC address format like this example 00-00-5E-00-53-23 #456

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

ypid-geberit
Copy link
Contributor

@ypid-geberit ypid-geberit commented May 16, 2019

Reasons for choosing this format:

It is not yet clear if we should prefer upper or lowercase. I just went with uppercase for now because that is what the references above use.

Reason for choosing 00-00-5E-00-53-23:

It is in the range of unicast MAC addresses reserved for documentation as specified by: https://tools.ietf.org/html/rfc7042#section-2.1.2

Note that I did not update generated files because my development machine is not fully setup. Maybe you can update them real quick before merging? Thanks.

Edit: I moved the following from a comment below to here to have it in this overview comment:

It is also my observation that : are more commonly used and I am definitely also fine with going with those. When we setup our Logstash config for log parsing, we had to decide on a MAC address format (enforced by Logstash filters) and went with - for the above reasons. Here are some arguments for both of them (by no means comprehensive):

Pro :

  • Linux (and lowercase)
  • Android (and lowercase)

Pro -

@ruflin ruflin requested a review from webmat May 16, 2019 11:01
@webmat
Copy link
Contributor

webmat commented May 23, 2019

@ypid-geberit Thanks for the PR. No problem with the generated files, happy to pick this up and run the generator all the way.

The criteria I would like to use to determine the format, is ease of use for the end user. Aka make copy/paste easier ;-)

My understanding is that most tooling out there (monitoring, asset management, etc) use colon, rather than dash as the separator. For this reason I'm thinking colons would make more sense. I'd like to understand a bit more on why you favour the dashes over the colon, though.

As for upper/lowercase my criteria would also be the same, so I agree uppercase makes sense. However I'm also considering making this field case insensitive, but this will be more involved in how we document this, updating the sample ES templates etc. So out of scope for your PR :-)

So we can decide between - or : here, potentially adjust the example, but leave the capitalization underspecified for now. We'll tackle that part later.

@ebeahan
Copy link
Member

ebeahan commented Jul 1, 2020

I like the idea of including the example mac address from the reserved range, however I'm in favor of using : over - in the documentation. I agree with @webmat that most tooling seems to use the colon-separate notation.

@ebeahan ebeahan self-assigned this Jul 2, 2020
@ebeahan ebeahan added the ready Issues we'd like to address in the future. label Jul 2, 2020
@webmat webmat added the 1.9.0 label Nov 3, 2020
@sgryphon
Copy link

The documentation change is fine. Should use "-", as that is what is specified in the standards (irrespective of whether some other systems don't follow them).

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.

In addition to updating the examples, the descriptions should be updated to specify the format. For example RFC7042 describes the notation as:

Each octet (that is, 8-bit byte) is represented by two [uppercase] hexadecimal digits giving the value of the octet as an unsigned integer. Successive octets are separated by a hyphen.

I think we need to decide on a format so that implementations can start converging on a format. I don't have a strong opinion either way. But if there is an existing notation convention then I tend to think ECS should follow it.

schemas/client.yml Show resolved Hide resolved
@ebeahan
Copy link
Member

ebeahan commented Jan 22, 2021

I think we need to decide on a format so that implementations can start converging on a format. I don't have a strong opinion either way. But if there is an existing notation convention then I tend to think ECS should follow

@andrewkroh I wonder if we begin with introducing a recommendation in the field's description to normalize to the hyphen, with the intent to revisit and add firmer guidance in the future?

Outside of colon vs. hyphen, there are implementations using periods, EUI-48 vs. EUI-64, etc. Given many, many sources won't align with the notation convention and users will already be populating this field with a variety of formats, I'd prefer to hold off on introducing firm guidance.

//	00:00:5e:00:53:01
//	02:00:5e:10:00:00:00:01
//	00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01
//	00-00-5e-00-53-01
//	02-00-5e-10-00-00-00-01
//	00-00-00-00-fe-80-00-00-00-00-00-00-02-00-5e-10-00-00-00-01
//	0000.5e00.5301
//	0200.5e10.0000.0001
//	0000.0000.fe80.0000.0000.0000.0200.5e10.0000.0001

@ypid-geberit
Copy link
Contributor Author

ypid-geberit commented Jan 25, 2021

In addition to updating the examples, the descriptions should be updated to specify the format.

Done that.

I think we need to decide on a format so that implementations can start converging on a format. I don't have a strong opinion either way. But if there is an existing notation convention then I tend to think ECS should follow it.

I agree. There are two competing standards. Lets just go with one of them. Better than having implementers choose one at random.

Outside of colon vs. hyphen, there are implementations using periods, EUI-48 vs. EUI-64, etc. Given many, many sources won't align with the notation convention and users will already be populating this field with a variety of formats, I'd prefer to hold off on introducing firm guidance.

They need to update than when they want to comply with the next ECS version than. I see no big issue there.

I wonder if we begin with introducing a recommendation in the field's description to normalize to the hyphen, with the intent to revisit and add firmer guidance in the future?

So a soft migration? Or does this have other benefits?

@ebeahan ebeahan removed the ready Issues we'd like to address in the future. label Jan 26, 2021
@ebeahan
Copy link
Member

ebeahan commented Feb 1, 2021

So a soft migration? Or does this have other benefits?

Yes, the idea would be to introduce a recommended convention in a minor release and firm up the guidance on adopting later on.

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

@ypid-geberit Thanks for the iteration! I think we're very close; I have just one suggestion in the description's updated wording.

I'd be happy to add the generated files and get CI to ✅.

schemas/client.yml Outdated Show resolved Hide resolved
@ypid-geberit ypid-geberit force-pushed the feature/mac_address_format branch 2 times, most recently from 0d4a296 to a388792 Compare February 11, 2021 09:03
Reasons for choosing this format:

* IEEE 802 uses this format: https://en.wikipedia.org/wiki/MAC_address#Notational_conventions
* IETF uses this format: https://tools.ietf.org/html/rfc7042

It is not yet clear if we should prefer upper or lowercase. I just went
with uppercase for now because that is what the references above use.

Reason for choosing 00-00-5E-00-53-23:

It is in the range of unicast MAC addresses reserved for documentation
as specified by: https://tools.ietf.org/html/rfc7042#section-2.1.2
@ypid-geberit
Copy link
Contributor Author

Works for me, done.

@ebeahan
Copy link
Member

ebeahan commented Feb 11, 2021

@elasticmachine, run elasticsearch-ci/docs

@ebeahan
Copy link
Member

ebeahan commented Feb 11, 2021

@elasticmachine, run elasticsearch-ci/docs

@ebeahan ebeahan merged commit 8d1f638 into elastic:master Feb 11, 2021
ebeahan pushed a commit to ebeahan/ecs that referenced this pull request Feb 11, 2021
Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
# Conflicts:
#	experimental/generated/csv/fields.csv
#	generated/csv/fields.csv
ebeahan added a commit that referenced this pull request Feb 11, 2021
Co-authored-by: Robin Schneider <36660054+ypid-geberit@users.noreply.github.com>
@ypid-geberit
Copy link
Contributor Author

Thanks 👍🏾

dseeley added a commit to dseeley/ecs that referenced this pull request May 5, 2021
* bumping version for 1.x release branch (elastic#921)

* [1.x] add related.hosts (elastic#913) (elastic#924)

* [1.x][DOCS] Fixes SIEM links (elastic#936)

* [1.x] Consolidate field-details doc template (elastic#897) (elastic#946)

* Add http.[request|response].mime_type (elastic#944) (elastic#949)

* [1.x] Cut 1.6 Changelog (elastic#933) (elastic#952) (elastic#953)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Add threat.technique.subtechnique (elastic#951) (elastic#956)

Co-authored-by: Ross Wolf <31489089+rw-access@users.noreply.github.com>

* [1.x] Nest as for foreign reuse (elastic#960) (elastic#962)

* [1.x] Remove `expected_event_types` from protocol (elastic#964) (elastic#965)

* [1.x] Expand definitions of source and destination field sets (elastic#967) (elastic#973)

* [1.x] Introduce `--strict` flag (elastic#937) (elastic#975)

* [1.x] Add example value composite type checking (elastic#966) (elastic#976)

* Add example value composite type checking (elastic#966)
* generate csv artifact

* [1.x] Add event category configuration (elastic#963) (elastic#977)

* [1.x] Add normalizer multi-field capability (elastic#971) (elastic#978)

Co-authored-by: Eric Beahan <ebeahan@gmail.com>

Co-authored-by: Madison Caldwell <madison.rey.caldwell@gmail.com>

* [1.x] Add mapping network event guidance doc (elastic#969) (elastic#983)

* [1.x] Removing unneeded link under `Additional Information` (elastic#984) (elastic#985)

* [1.x] Add discrete attribute to field details page headers (elastic#989) (elastic#990)

* [1.x] Uniformity across domain name breakdown fields (elastic#981) (elastic#994)

Co-authored-by: Mathieu Martin <webmat@gmail.com>

* Add --oss flag to the ECS generator script (elastic#991) (elastic#995)

* Add network directions ingress and egress (elastic#945) (elastic#997)

* Mention ECS Mapper in the main documentation (elastic#987) (elastic#1000)

Co-authored-by: Dan Roscigno <dan@roscigno.com>

* [1.x] Introduce experimental artifacts (elastic#993) (elastic#1001)

Co-authored-by: Mathieu Martin <webmat@gmail.com>

* Bump version to 1.8.0-dev in branch 1.x (elastic#1011)

* Cut 1.7 changelog (elastic#1010) (elastic#1012)

* [1.x] Clarify that file extension should exclude the dot. (elastic#1016) (elastic#1020)

* [1.x] Add usage docs section (elastic#988) (elastic#1024)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] feat: include alias path when generating template (elastic#877) (elastic#1035)

Co-authored-by: Richard Gomez <32133502+rgmz@users.noreply.github.com>

* [1.x] Add support for `scaling_factor` in the generator (elastic#1042) (elastic#1055)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Add fallback for constant_keyword (elastic#1046) (elastic#1056)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Add wildcard type support to go code generator (elastic#1050) (elastic#1057)

* add wildcard type support

* also add version and constant_keyword

* changelog

* [1.x] New default make task that generates main and experimental artifacts. (elastic#1041) (elastic#1060)

Also changing the order of the 'generate' task: it now starts with the new generator, then runs the legacy scripts.

* [1.x] Change the index pattern in the sample template. (elastic#1048) (elastic#1068)

* [1.x] Prepare link to Logs docs changing with the 7.10 release in "getting-started" (elastic#1073) (elastic#1079)

Co-authored-by: EamonnTP <Eamonn.Smith@elastic.co>

* [1.x] Prepare link to Logs docs changing with the 7.10 release in "products-solutions" page (elastic#1074) (elastic#1083)

Co-authored-by: EamonnTP <Eamonn.Smith@elastic.co>

* [1.x] Add event.category session. (elastic#1049) (elastic#1093)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Add event.category registry (elastic#1040) (elastic#1094)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Add --ref support for experimental artifacts (elastic#1063) (elastic#1101)

Co-authored-by: Mathieu Martin <webmat@gmail.com>

* [1.x] Remove experimental event.original definition (elastic#1053) (elastic#1104)

* [1.x] Add missing `process.thread.name` to experimental definitions (elastic#1103) (elastic#1106)

* [1.x] Remove index parameter for wildcard fields (elastic#1115) (elastic#1119)

* [1.x] Add dns.answer object into experimental schema (elastic#1118) (elastic#1121)

* [1.x] Clarify x509 definition guidance for network events with only one cert (elastic#1114) (elastic#1123)

* [1.x] Indicate when artifacts include experimental changes (elastic#1117) (elastic#1125)

* [1.x] Add os.type field, with list of allowed values (elastic#1111) (elastic#1130)

* [1.x] Add support for constant_keyword's 'value' parameter (elastic#1112) (elastic#1132)

* [1.x] Beta label support (elastic#1051) (elastic#1133)

Co-authored-by: Mathieu Martin <webmat@gmail.com>

* [1.x] Backport elastic#1134 and elastic#1135 (elastic#1136)

* Remove temporary ifeval in "getting started" page, add link to Metrics docs (elastic#1134)
* Remove temporary ifeval from products page, add link to Metrics (elastic#1135)

* Two small documentation backports (elastic#1149)

* Remove an incorrect `event.type` from the 'converting' page (elastic#1146)
* Mention Logstash support for ECS in the 'products' page (elastic#1147)

* [1.x] Reinforce the exclusion of the leading dot from url.extension (elastic#1151) (elastic#1152)

* [1.x] Make all fields linkable directly via an HTML ID (elastic#1148) (elastic#1154)

* [1.x] Tracing fields should be at the root (elastic#1165)

* Add notice to the tracing field set, about not nesting field names. (elastic#1162)
* Tracing fields should be at top level in Beats artifact (elastic#1164)

* [1.x] Usage of brackets for a URL containing IPv6 address (elastic#1131) (elastic#1168)

* [1.x] 6.x index template data type fallback (elastic#1171) (elastic#1172)

* [1.x] Apply RFC 0007 stage 3 changes - multi-user (elastic#1066) (elastic#1175)

Conflict: deleted file rfcs/text/0007-multiple-users.md as RFCs are not backported to version branches.

* [1.x] Handle `error.stack_trace` case for ES 6.x template (elastic#1176) (elastic#1177)

* [1.x] Add composable index templates artifacts (elastic#1156) (elastic#1179)

* [1.x] Move _meta section back inside mappings, in legacy templates. (elastic#1186) (elastic#1187)

Backports the following commits to 1.x:

* Move _meta section back inside mappings, in legacy templates. (elastic#1186) 

This fixes an issue introduced by elastic#1156, discovered in elastic#1180. Composable templates support `_meta` at the template's root, but legacy templates don't. So we're just putting it back inside the mappings for legacy templates.

This also fixes missing updates to the component template, after the introduction of wildcard in elastic#1098.

* [1.x] Apply the RFC 0005 stage 2 (host metrics) changes in the experimental artifacts (elastic#1159) (elastic#1184)

Co-authored-by: Mathieu Martin <mathieu.martin@elastic.co>

* [1.x] Stage 3 changes for wildcard RFC 0001 (elastic#1098) (elastic#1183)

* [1.x] Conditional handling in es_template.template_settings (elastic#1191) (elastic#1192)

* [1.x] Artifacts docs page (elastic#1189) (elastic#1195)

* [1.x] Remove beta warning label from categorization fields docs (elastic#1067) (elastic#1196)

* [1.x] Correct wording of `event.reference` description (elastic#1181) (elastic#1197)

* Bump version to 1.9.0-dev in branch 1.x (elastic#1198)

* [1.x] Cut 1.8 FF changelog.next.md elastic#1199 (elastic#1201)

* Merge custom and core multi_fields arrays (elastic#982) (elastic#1213)

Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>

* [1.x] Stage 2 changes for RFC 0009 - data_stream fields (elastic#1215) (elastic#1222)

* [1.x] add http.request.id (elastic#1208) (elastic#1223)

Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
Co-authored-by: Gil Raphaelli <gil@elastic.co>

* [1.x] add cloud.service.name (elastic#1204) (elastic#1224)

* add cloud.platform

* expand cloud.platform description

* move to cloud.service.name

Co-authored-by: Gil Raphaelli <gil@elastic.co>

* [1.x] Add ssdeep hash (elastic#1169) (elastic#1227)

Co-authored-by: Andrew Stucki <andrew.stucki@elastic.co>

* [CI] Switch to GitHub actions (elastic#1236) (elastic#1245)

Co-authored-by: Eric Beahan <ebeahan@gmail.com>

Co-authored-by: Andrew Stucki <andrew.stucki@elastic.co>

* Revert wildcard adoption back to experimental stage (elastic#1235) (elastic#1243)

* Add scaled_float type to go generator (elastic#1250) (elastic#1251)

* add scaled_float

* changelog

* Add categorization fields usage docs (elastic#1242) (elastic#1257)

* add time_zone, postal_code, and continent_code (elastic#1229) (elastic#1258)

* Specify MAC address format (elastic#456) (elastic#1260)

Co-authored-by: Robin Schneider <36660054+ypid-geberit@users.noreply.github.com>

* finalize 1.8.0 changelog (elastic#1262) (elastic#1265)

* Add additional host fields (elastic#1248) (elastic#1267)

Co-authored-by: kaiyan-sheng <kaiyan.sheng@elastic.co>

* Stage 1 changes for RFC 0014 - extend pe fields (elastic#1256) (elastic#1270)

* Add 2 fields to code_signature (elastic#1269) (elastic#1272)

Co-authored-by: Yamin Tian <56367679+Trinity2019@users.noreply.github.com>

* Stage 3 changes for RFC 0007 - remove beta attribute (elastic#1271) (elastic#1273)

* Stage 1 experimental changes for RFC 0008 - threat.indicator fields (elastic#1268) (elastic#1274)

* Stage 1 changes for RFC 0015 - add elf fieldset (elastic#1261) (elastic#1275)

* Cut 1.9 FF CHANGELOG.next.md (elastic#1277)

* lock go version in actions (elastic#1283) (elastic#1290)

* Bump jinja2 from 2.11.2 to 2.11.3 in /scripts (elastic#1310) (elastic#1320)

* Bump jinja2 from 2.11.2 to 2.11.3 in /scripts

* Bump pyyaml from 5.3b1 to 5.4 in /scripts (elastic#1318) (elastic#1325)

Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Adjust terminology - change whitelist to allowlist (elastic#1315) (elastic#1331)

Co-authored-by: Dominic Page <11043991+djptek@users.noreply.github.com>

* Remove -dev label from 1.9 version (elastic#1329)

* remove -dev label from 1.9 version

* generate artifacts

* removing rules artifacts

* Cut 1.9 changelog (elastic#1328)

* move 1.9 changes to changelog

* add 1.9 release changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants