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

Check Security Roles in Deprecation Info API #49212

Closed
wants to merge 2 commits into from

Conversation

gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Nov 16, 2019

This PR adds infrastructure to allow Roles to be checked in the
Deprecation Info API. Roles are retrieved via the existing Get Roles API
for roles defined via API, or by new infrastructure which retrieves the
file-based roles from each node.

Each role deprecation check is run under two different circumstances:
Once against the list of roles defined via the API, which are shared
across the cluster, and again against the list of roles retrieved from
each node. The deprecation issues produced by running against the
cluster-wide roles are merged into the list of Cluster-level issues,
whereas the deprecation issues produced by running against the
node-specific roles are deduplicated using a similar process to the
existing node-level deprecation checks and merged into the list of
node-level issues.

Note that this means there is no change in the format of the response
from the Deprecation Info API.

This PR also includes one deprecation check as an example. This
check may have to be removed prior to merging if the replacement role
is not yet ready.

Closes #47714
Relates #47333

This PR adds infrastructure to allow Roles to be checked in the
Deprecation Info API. Roles are retrieved via the existing Get Roles API
for roles defined via API, or by new infrastructure which retrieves the
file-based roles from each node.

Each role deprecation check is run under two different circumstances:
Once against the list of roles defined via the API, which are shared
across the cluster, and again against the list of roles retrieved from
each node. The deprecation issues produced by running against the
cluster-wide roles are merged into the list of Cluster-level issues,
whereas the deprecation issues produced by running against the
node-specific roles are deduplicated using a similar process to the
existing node-level deprecation checks and merged into the list of
node-level issues.

Note that this means there is *no change* in the format of the response
from the Deprecation Info API.

This PR also includes one deprecation check as an example.
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

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

@gwbrown
Copy link
Contributor Author

gwbrown commented Nov 16, 2019

@dakrone, I asked for your review because you've reviewed similar functionality changes before.
@tvernum, I asked for your review because this needs a review from someone on the Security team - feel free to hand this off to another member of the team if you want.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I am marking this as request changes even though I haven't reviewed the code to clearly communicate that I am uncertain about this direction.

I am concerned that we are adding entirely too much machinery here (more transport actions). I understand the tradeoffs in trying to surface this through the deprecation logs, but there's also a tradeoff to avoid adding 600+ lines of code every time that we need to surface something like this in the deprecation API that can't be collected via existing APIs. This is why I propose the general mechanism in #46106. Is deprecating roles going to be that common (my expectation is that it isn't), that it's worth carrying this machinery from major release to major release?

@gwbrown
Copy link
Contributor Author

gwbrown commented Nov 18, 2019

I discussed this briefly at EAH with @tvernum, who mentioned that the new transport action here might be useful for other purposes as well, such as making these file-based roles more easily visible for troubleshooting purposes. I understand the concern about adding code that needs to be maintained, but I don't think the new transport action will be single-purpose long-term (someone on the Security team please correct me if this is wrong!).

I agree we shouldn't add infrastructure or a ton of code every time there's something we can't check for the in the Deprecation Info API, but I'm weighting this heavier than usual considering that a broken security configuration can seriously ruin your day, so I'd like to make sure that we can give administrators the most confidence we can.

That said, I'm open to handling this through the more generic infrastructure described in #46106, but I think in order to do the right thing for our users, that needs to include ensuring very clear deprecation log messages, making sure we can show the contents of that index in the Migration Assistant UI, and documenting when the logs are triggered so that users can have a high confidence in their security configuration going into the upgrade. If we have a plan to do that, I'll be happy to remove the file-based role handling (and the associated transport action, which makes up the bulk of the diff) from this PR.

@dakrone dakrone removed their request for review January 9, 2020 22:53
@polyfractal polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020
@bpintea bpintea added v7.8.0 and removed v7.7.0 labels Mar 25, 2020
@rjernst rjernst added Team:Data Management Meta label for data/management team Team:Security Meta label for security team labels May 4, 2020
@martijnvg
Copy link
Member

@gwbrown Do you have plans to revive this pr? I think the reason why you added team-discuss label is the same?

@gwbrown
Copy link
Contributor Author

gwbrown commented Aug 20, 2020

@martijnvg I would be happy to revive this PR, but there's an open question of whether this is something we want to do (see previous discussion). Since the last time there was movement on this PR, #58924 has gone up to add the capability to index deprecation logs, which may make this PR unnecessary.

Last I knew, core/features was going to discuss with security to figure out whether the approach in this PR is worth it, but that seems to have (understandably, there's lot going on right now that's much more important than this) fallen by the wayside. At this point, I think it might be best to close this PR and reopen/open a new one if/when we decide to do this.

@andreidan andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020
@danhermann
Copy link
Contributor

I'm going to close this since deprecation logs are now indexed. We can re-open later if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Data Management Meta label for data/management team Team:Security Meta label for security team team-discuss v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Deprecation Info API to check for deprecated security configuration