Skip to content

Rename user.username to user.name for SetSecurityUserProcessor #51799

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

Open
ywangd opened this issue Feb 3, 2020 · 8 comments
Open

Rename user.username to user.name for SetSecurityUserProcessor #51799

ywangd opened this issue Feb 3, 2020 · 8 comments
Assignees
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement :Security/Security Security issues without another label Team:Data Management Meta label for data/management team Team:Security Meta label for security team v9.1.0

Comments

@ywangd
Copy link
Member

ywangd commented Feb 3, 2020

The SetSecurityUserProcessor auguments ingested documents by adding an user object, where the username field does not conform to the recommended naming convention (name) described in ECS. The purpose of this issue is to faciliate discussion on whether we should rename this field.

PS: there are a few other fields under the user object. They are either confirm to ECS (email, full_name) or have no definition in ECS, i.e. custom fields (metadata, roles etc). Both situations are valid per ECS spec.

@ywangd ywangd added discuss :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP :Security/Security Security issues without another label labels Feb 3, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Security)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Ingest)

@ywangd
Copy link
Member Author

ywangd commented Mar 18, 2020

Summary from team discussion:

  • ECS is applicable in this case
  • Renaming is a breaking change, hence it should be in 8.0 instead of minors.
  • We wanna provide a upgrade/deprecation path of 7.x. The suggested approach is to make the name configurable in 7.x. The config option maintains the existing behaviour, but issues deprecation warnings if not configured to conform 8.0 behaviour.
    • GeoIp and userAgent processors used the same approach to deal with similar renaming issues.

@ywangd
Copy link
Member Author

ywangd commented Mar 24, 2020

@albertzaharovits Could you please review my following analysis and proposed solution? I have some new findings which lead to a slightly different approach. Thanks!

The name of the top level field used to store authentication information is configurable. The example in the doc configures it to be "user". However, in theory, this field can be called anything.

For ESC to be really applicable here, the parent field must be named as "user". If it is not, there is no point to apply ECS to the sub-fields, since ESC's choice of using "name" instead of "username" is to follow the Avoid repetition guideline. In addition, we could even argue that when the parent field is not user, say it is auth, it is more desirable for the sub-field to be called username instead of name for clarity.

Therefore my plan is as follows:

  • Keep it possible for this field to be username even in 8.0
  • Add a new boolean option to the processor configuration. If it is true and the parent field is user, the username field will be saved as name. Otherwise, it is saved as username. (see changed behaviour below)
  • The boolean option defaults to true in 8.0 and false in 7.x (for backwards compatibility and will issue warnings when applicable)

@ywangd
Copy link
Member Author

ywangd commented Mar 24, 2020

Per Tim's comment, I am tightening the behaviour so that:
If the new compliance option is set, it will also require the field be named as name. Otherwise it is a validation error.

@albertzaharovits
Copy link
Contributor

The name of the top level field used to store authentication information is configurable. The example in the doc configures it to be "user". However, in theory, this field can be called anything.
For ESC to be really applicable here, the parent field must be named as "user". If it is not, there is no point to apply ECS to the sub-fields, since ESC's choice of using "name" instead of "username" is to follow the Avoid repetition guideline.

In the light of this, maybe we should nest all the user properties under a user object field, so the administrator gets to configure <target_field>.user.name

Maybe it's better to continue this discussion under #54051 (review)

@rjernst rjernst added Team:Data Management Meta label for data/management team Team:Security Meta label for security team labels May 4, 2020
@arteam arteam added v8.1.0 and removed v8.0.0 labels Jan 12, 2022
@mark-vieira mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement :Security/Security Security issues without another label Team:Data Management Meta label for data/management team Team:Security Meta label for security team v9.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.