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.

Sign up for GitHub

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

Jump to bottom

Add attested user and groups to process #2050

Merged
merged 6 commits into from
Sep 20, 2022
Merged

Add attested user and groups to process #2050

merged 6 commits into from
Sep 20, 2022

Conversation

daniel-almeida
Copy link
Contributor

@daniel-almeida daniel-almeida commented Sep 13, 2022 •
edited
Loading

  • Have you signed the contributor license agreement? yes
  • Have you followed the contributor guidelines? yes
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process? yes
  • If submitting code/script changes, have you verified all tests pass locally using make test? N/A
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? yes
  • Is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed. yes
  • Have you added an entry to the CHANGELOG.next.md? yes

mitodrummer
mitodrummer reviewed Sep 13, 2022
View reviewed changes
mitodrummer
mitodrummer reviewed Sep 13, 2022
View reviewed changes
mitodrummer
mitodrummer reviewed Sep 13, 2022
View reviewed changes
schemas/user.yml Outdated
@@ -54,6 +54,10 @@
as: real_user
short_override: The real user (ruid). Identifies the real owner of the process.
beta: Reusing the `user` fields in this location is currently considered beta.
- at: process
as: attested_user
short_override: The attested user (auid). Identifies the attested used associated with the process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, i know there isn't a ton of space for descriptions on nested fields. "short only", but I wonder if we can explain what attested means. e.g via 2factor mechanisms etc...

Copy link

Choose a reason for hiding this comment

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

"externally" attested is the key aspect ie they have done something to prove to an external source (e.g. pw & 2nd factor) that they are who claim to be and authorized to obtain the role/identity stated here.

@mitodrummer
Copy link
Contributor

This works good as a nested field. When this gets merged and we update endpoint-package, we'll need to decide what nesting this attested_user lives at (mapping wise). Assuming it will only be attested once at the entry_leader process, it probably makes sense to only add a mapping for process.entry_leader.attested_user and process.entry_leader.attested_group. Not only where it's nested, but which fields in the user fieldset will be mapped. e.g email, username etc...

@daniel-almeida
Copy link
Contributor Author

Not only where it's nested, but which fields in the user fieldset will be mapped. e.g email, username etc...

@mitodrummer , we're inclined to use this mapping:

k8s username -> user.name
k8s user id -> user.id
k8s group names -> group.name

@qcorporation qcorporation mentioned this pull request Sep 14, 2022
Closed
@daniel-almeida daniel-almeida changed the title Add attested user and group to process Add attested user and groups to process Sep 19, 2022
@daniel-almeida daniel-almeida marked this pull request as ready for review September 19, 2022 02:21
@daniel-almeida daniel-almeida requested a review from a team as a code owner September 19, 2022 02:21
@daniel-almeida daniel-almeida force-pushed the add-process-attested-user branch from 1fa66c1 to e5a4e50 Compare September 19, 2022 02:34
mitodrummer
mitodrummer approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@mitodrummer mitodrummer left a comment

Choose a reason for hiding this comment

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

LGTM!

@mitodrummer mitodrummer requested a review from kgeller September 19, 2022 20:33
@mitodrummer mitodrummer requested a review from ebeahan September 19, 2022 20:39
m-sample
m-sample approved these changes Sep 19, 2022
View reviewed changes
Copy link

@m-sample m-sample left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work & thanks @daniel-almeida.

We can review if we need to support more than one externally attested identity per event before this leaves beta.

@kgeller
Copy link
Contributor

kgeller commented Sep 20, 2022

@daniel-almeida I think these changes look good, I just have one question. Can we get these added to the subset file, or is there is a reason that was omitted?

example: process.supplemental_groups

@daniel-almeida
Copy link
Contributor Author

That was probably oversight on my part, @kgeller . I'll look into it.

kgeller
kgeller approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@kgeller kgeller left a comment

Choose a reason for hiding this comment

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

LGTM

@mitodrummer mitodrummer merged commit 266cf6a into main Sep 20, 2022
@kgeller kgeller mentioned this pull request Sep 20, 2022
Merged
kgeller pushed a commit to kgeller/ecs that referenced this pull request Sep 20, 2022
@daniel-almeida @kgeller
Add attested user and groups to process (elastic#2050)
e16984c 
* Add attested user and group to process

* Drop auid, make attested groups an array, and clarify that attestation is based on an external source

* Update short description and add process.attested_user and attested_groups to CHANGELOG

* subset file updated for process.entry_leader.attested_user and attested_groups

* updates from make

Co-authored-by: Karl Godard <[email protected]>
Co-authored-by: Kylie Geller <[email protected]>
# Conflicts:
#	experimental/generated/csv/fields.csv
#	generated/csv/fields.csv
kgeller added a commit that referenced this pull request Sep 20, 2022
@kgeller
Add attested user and groups to process (#2050) (#2054)
1f9a947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgeller kgeller kgeller approved these changes

@mitodrummer mitodrummer mitodrummer approved these changes

@m-sample m-sample m-sample approved these changes

@ebeahan ebeahan Awaiting requested review from ebeahan

@andrewkroh andrewkroh Awaiting requested review from andrewkroh

Assignees
No one assigned
Labels
8.5.0 RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@daniel-almeida @mitodrummer @kgeller @andrewkroh @m-sample