Skip to content

Conversation

@thecoop
Copy link
Member

@thecoop thecoop commented Dec 5, 2024

As part of removing 7.x transport versions, remove support for pre-7.2 token serialization

@thecoop thecoop added >refactoring :Security/Security Security issues without another label labels Dec 5, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Dec 5, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@thecoop thecoop force-pushed the remove-7.x-token-serialization branch from 97b4ebf to 5000837 Compare December 5, 2024 10:58
@slobodanadamovic slobodanadamovic self-requested a review December 5, 2024 11:08
Copy link
Contributor

@slobodanadamovic slobodanadamovic left a 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:

  1. KeyComputingRunnable class
  2. KeyAndCache class
  3. TokenKeys class
  4. synchronized void refreshMetadata(TokenMetadata metadata)
  5. private SecureString generateTokenKey() including KEY_BYTES constant
  6. private void submitUnbatchedTask(String source, ClusterStateUpdateTask task)
  7. private void installTokenMetadata(ClusterState state)
  8. void clearActiveKeyCache()
  9. 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).

@thecoop
Copy link
Member Author

thecoop commented Dec 6, 2024

@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)?

@slobodanadamovic
Copy link
Contributor

slobodanadamovic commented Dec 6, 2024

@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.

Copy link
Contributor

@slobodanadamovic slobodanadamovic left a 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.

@thecoop thecoop added the test-full-bwc Trigger full BWC version matrix tests label Dec 9, 2024
@thecoop thecoop merged commit ec66857 into elastic:main Dec 9, 2024
21 checks passed
@thecoop thecoop deleted the remove-7.x-token-serialization branch December 9, 2024 10:03
thecoop added a commit to thecoop/elasticsearch that referenced this pull request Dec 18, 2024
breskeby pushed a commit that referenced this pull request Dec 18, 2024
* Revert "Remove pre-7.2 token serialization support (#118057)" This reverts commit ec66857. * Add missing constant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Security/Security Security issues without another label Team:Security Meta label for security team test-full-bwc Trigger full BWC version matrix tests v9.0.0

3 participants