Improve BWC for persisted authentication headers#83913
Improve BWC for persisted authentication headers#83913ywangd merged 8 commits intoelastic:masterfrom
Conversation
|
Hi @ywangd, I've created a changelog YAML for you. |
| } | ||
| if (headers.isEmpty() == false) { | ||
| builder.setHeaders(filterSecurityHeaders(headers)); | ||
| builder.setHeaders(ClientHelper.getPersistableSafeSecurityHeadersForVersion(headers, minNodeVersion)); |
There was a problem hiding this comment.
Ideally, authentication headers are already filtered and converted to minNodeVersion when persisted as part of a task configuration. When they are retrieved and used, there is no need to filter and convert them again. But it is safe to do it again with very little cost. It may also help headers that are persisted before this change.
|
Pinging @elastic/es-security (Team:Security) |
| try { | ||
| final Authentication authentication = authenticationReader.apply(authenticationHeaderKey); | ||
| if (authentication != null && authentication.getVersion().after(minNodeVersion)) { | ||
| return authentication.maybeRewriteForOlderVersion(minNodeVersion).encode(); |
There was a problem hiding this comment.
This works for now. In a future PR, I intend to review how Authentication is constructed, serialised and downgraded.
|
@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample |
| * and rewrite them using minNodeVersion so that they are safe to be persisted as index data | ||
| * and loaded by all nodes in the cluster. | ||
| */ | ||
| public static Map<String, String> getPersistableSafeSecurityHeadersForVersion(ThreadContext threadContext, Version minNodeVersion) { |
There was a problem hiding this comment.
I think this is trappy.
It assumes that the caller knows what the right version is, which means we're expecting every caller to have the cluster service and get the minimum node version, with the possible exception of accessing a remote cluster.
That means that over time we will end up with callers who just pass in Version.CURRENT because they don't understand the implications.
I think we should pass in the ClusterState instead.
There was a problem hiding this comment.
Good point, thanks! I have updated the PR accordingly.
albertzaharovits
left a comment
There was a problem hiding this comment.
LGTM
I haven't thought it through the whole lifecycle of the Authentication headers.
The logic to maybe downgrade the Authentication headers, to the smallest cluster node version, whenever they're stored for some task, where today they are copied as is, is simple and effective.
|
@tvernum Do you want to take another look? I'd like to merge this soon since related tests are failing on CI daily. |
Authentication headers are persisted as part of a task definition including ML jobs, CCR following etc. The persistence process store them into either an index or the cluster state. In both cases, the headers are retrieved from ThreadContext as a string which is the serialised form of the Authentication object. This string is always serialised with the node's version. The problem is: In a mixed cluster, the task can be created in a newer node and persisted into an index but then needs to be loaded by a older node. The older node does not understand the newer format of the serialised Authentication object and hence error out on reading it. This PR adds additional logic in places where the headers are persisted. It compares the Authentication version with minNodeVersion and rewrites it if the minNodeVersion is older. Since we already filter security headers in places where headers are persisted, the new logic is hooked into the same places and essentially another enhancement on how to handle security headers for persisted tasks. Resolves: elastic#83567
Authentication headers are persisted as part of a task definition including ML jobs, CCR following etc. The persistence process store them into either an index or the cluster state. In both cases, the headers are retrieved from ThreadContext as a string which is the serialised form of the Authentication object. This string is always serialised with the node's version.
The problem is: In a mixed cluster, the task can be created in a newer node and persisted into an index but then needs to be loaded by a older node. The older node does not understand the newer format of the serialised Authentication object and hence error out on reading it.
This PR adds additional logic in places where the headers are persisted. It compares the Authentication version with minNodeVersion and rewrites it if the minNodeVersion is older. Since we already filter security headers in places where headers are persisted, the new logic is hooked into the same places and essentially another enhancement on how to handle security headers for persisted tasks.
Resolves: #83567