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 url to package definition #585

Merged
merged 3 commits into from
Nov 18, 2019
Merged

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Oct 16, 2019

Add information from where the package was installed.

related to #515

Follow up from #532 to focus on adding URL from where a software package was installed.


Follow up from previous discussions #532 (comment) and #532 (comment)

Last comment from #532 (comment):

webmat 16 days ago Member

I was initially misunderstanding the purpose of this field. Its description is not clear enough, and the example is misleading (Docker also has a registry of packages in Docker Hub).

Please remove the field from this PR, and reopen a PR with just this field.

When reopening the new PR, please adjust the following things to remove the ambiguity:

clarify the description, perhaps something like "Main URL of the software included in the package"
change the example so it's aligned with the other examples in the package field set. In other >words, this should be Golang's URL, which according to Homebrew, is simply "https://golang.org".

Another discussion point that I'd like to hash out: instead of partially nesting the URL field set here, I think we could use another pattern we've started using elsewhere, and name the field .reference. WDYT?

@webmat I didn't find a lot of context around using url.reference, but from the examples I saw it really looked like it was pointing to a page with a threat reference. Not sure this would translate to the package url here, so for now I went with adding url.original. Let's discuss this.

Add information from where the package was installed.

related to elastic#515
@ruflin
Copy link
Contributor

ruflin commented Oct 21, 2019

I think this will need a changelog entry.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Thanks for re-submitting, @simitt!

I'd like a few other opinions on the field naming:

  • package.url.original - I don't think we'll need to break down these URLs, so I'd prefer we find another name for it. Also, partially nesting a field set can be done if needed, but I'd rather avoid it if possible. I think it would be confusing.
  • package.homepage - I think this is a good name for this concept, as it avoids ambiguity with what kind of URL this is. It's meant to contain the homepage of the software in the package.
  • package.reference - Another good name. It's admittedly a bit less obvious for the "homepage" URLs, however I get the sense people will want to enrich many kinds of things with reference URLs. Reference URL for an attack tactic, reference URL for what a package contains, etc. So if other contributors agree, perhaps this is a pattern we can start establishing?

- name: url.original
level: extended
type: keyword
description: Main URL from where the package was installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this description is what initially caused the confusion. This is meant to capture the homepage of the software in the package, correct? Not where the package manager got the package.

I think something like this would be less ambiguous wrt package URLs:

Homepage of the software included in the package.

@@ -72,3 +72,9 @@

Use a short name, e.g. the license identifier from SPDX License List where possible (https://spdx.org/licenses/).
example: Apache License 2.0

- name: url.original
Copy link
Contributor

Choose a reason for hiding this comment

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

@simitt You're right that the only place in ECS where we use .reference is in the threat field set.

I was suggesting this name for the field, as in this case, it can be seen as a reference URL to learn more about the software in the package.

However perhaps we can align the description I'm suggesting in the my comment and name this field package.homepage. Either would be fine, but my new favourite is package.homepage :-)

I don't think we'll need to break down package URLs to look for suspicious things, so I'm not too fond of naming this package.url.original.

@simitt
Copy link
Contributor Author

simitt commented Oct 29, 2019

Giving this some more thoughts I am in favor of changing to package.url.reference, and making it clear in the description that this URL is e.g. the URL the package manager is pointing to or the URL of the packages project site, included in the package itself (as pointed out by @cwurm in #532 (comment))

Since the reference concept was already introduced recently I think it is more consistent to reuse here instead of introducing a new homepage term, basically describing the same thing.

@SergeyKleyman
Copy link

@simitt It might be helpful to understand the differences between these fields if you provide a couple of examples where fields are different. I would recommend examples based on packages installed by sysadmin (such as Debian packages) and packages coming with an application (such as the packages from Java's Maven repository).

@simitt
Copy link
Contributor Author

simitt commented Nov 11, 2019

I changed the name and description. @cwurm and @webmat could you have another look please.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

I love it as it is now, thanks for making the adjustment :-)

I agree with @SergeyKleyman we may want to expand a bit more on the two broad use cases, OS packages vs application/library packages. However I don't see this as a blocker for this PR.

This kind of clarification is difficult to fit in the current docs table. One thing I've had in mind for a while (captured in this ECS docs brainstorm) is to have another section per field set that is static asciidoc files -- in addition to the generated reference -- where we can expand on proper use of the field set, give fuller and more conversational example, include images or code/JSON snippets.

So with that in mind, I'd suggest we make progress and merge this now, as is. If there's a proposal on how to clarify OS package vs app package in the current doc format, I'd love to see this as a follow-up issue or PR. If not, this distinction is definitely something I'm noting down for later, when we address this new informal doc section I've described above.

I'll wait a bit for folks to respond to this, and I'm hoping to merge this mid next week at the latest.

Thanks @simitt and @SergeyKleyman :-)

@webmat
Copy link
Contributor

webmat commented Nov 14, 2019

I'll address the changelog conflict when merging

@webmat webmat merged commit 79b0b4a into elastic:master Nov 18, 2019
dcode pushed a commit to dcode/ecs that referenced this pull request Apr 15, 2020
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.

4 participants