- Notifications
You must be signed in to change notification settings - Fork 25.6k
Remove pre-7.2 token serialization support #118057
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
Conversation
| Pinging @elastic/es-security (Team:Security) |
97b4ebf to 5000837 Compare There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thecoop Thanks for doing this! The changes look good, but I think we still have couple of things to remove:
Namely, we should nuke all classes and methods related to pre-7.2 tokens, that includes:
KeyComputingRunnableclassKeyAndCacheclassTokenKeysclasssynchronized void refreshMetadata(TokenMetadata metadata)private SecureString generateTokenKey()includingKEY_BYTESconstantprivate void submitUnbatchedTask(String source, ClusterStateUpdateTask task)private void installTokenMetadata(ClusterState state)void clearActiveKeyCache()- and potentially others I may have missed to name
Further, we should also try and remove all references to TokenMetadata. Unfortunately, we cannot completely remove the class due to the BWC, as the cluster state can still hold the encryption keys and need to know how to deserialize it. I think it would be sufficient to simply not depend or require that encryption keys in anyway need to be in the cluster state.
In order to be able to remove it completely, we would need to run a migration and delete these keys (token metadata) from the cluster state. I'm mostly writing this for posterity, but no need to address this. I think migration would be out-of-scope for this PR and should be done in a followup (which I can own).
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java Outdated Show resolved Hide resolved
| @slobodanadamovic do you want those extra removals to be done here, or are you going to manage that later (so this PR is just the transport version removal)? |
Makes sense to keep this PR scoped to the transport version removal only, and I will address the removal of dead code in a followup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 (good to merge on green CI)
Edit: Optional: It might make sense for this change to to run through full BWC matrix by setting test-full-bwc label.
This reverts commit ec66857.
As part of removing 7.x transport versions, remove support for pre-7.2 token serialization