Refactor security uses of Version.transportVersion

[thecoop]

Refactor security-related uses of Version.transportVersion for serialisation to not use that.

Affected classes:

• ClientHelper
• TokenMetadata
• TokenService

[elasticsearchmachine]

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

[thecoop]

For ClientHelper, this is using transportVersion for maybeRewriteAuthenticationHeadersForVersion, based on the node minversion.

[thecoop]

Tokens have an associated transportversion - what's this for?

[gwbrown]

Tokens have an associated transportversion - what's this for?

I started looking for the answer to this question and found that the code was originally written to use Version and changed to use TransportVersion in this PR by @pgomulka without review from the Security team. So I don't actually know why tokens are using TransportVersion instead of Version - maybe @pgomulka can comment?

The same is true of ClientHelper - it was changed in the same PR, it appears without input from the Security team.

[pgomulka]

#93089 was part of bigger refactoring #93098 where simple use cases of Version being used for de/serialisation can be replaced with a TransportVersion. There is no change of behaviour as Version have an asossiated TransportVersion. We also discussed this offline with @albertzaharovits (cc)

@TheCop is now working on a split between Version and TransportVersion, so it is easier to upgrade versions.

I suspect @thecoop is asking if these affected classes could be rewriten without the Version/TransportVersion at all? Can you confirm/clarify?

[gwbrown]

Thanks for the clarification @pgomulka, I appear to have misinterpreted the ask pretty badly here, my apologies. That said, it is still unclear to me exactly what the request here is, though the suggestion above sounds like a reasonable interpretation to me.

Assuming that's true, what are our specific goals in getting rid of the TransportVersion usage? While I'm not extensively familiar with the tokens code, from some internal discussions and poking through the code it looks like this version is primarily used to translate authentication objects between versions. If not Version or TransportVersion, how should we be varying behavior based on e.g. creation version of an object or the mixed cluster version?

It's also stored in documents (though not serialized as in writeTo, just as an integer field in a JSON doc), so we'll need some way to translate from old school integer version IDs to whatever we should be using.

For ClientHelper, this is using transportVersion for maybeRewriteAuthenticationHeadersForVersion, based on the node minversion. This is not a safe thing to do.

Could you expand on that please?

[thecoop]

So the aim here is that we are separating Version and TransportVersion, to make the transport version (aiming specifically at upcoming cloud deployments) completely separate to on-prem node versions. The Version.transportVersion field is a link between those two classes, which makes upgrading/modifying Version or TransportVersion on version upgrades & new transport versions much more complicated than it should be.

We are aiming to remove the transportVersion field. It is used in various places in the security code to go from 'minimum active node version' to 'serialisation version'. The question here is, is there a way to completely separate the serialisation version & node version in this code, to remove the transportVersion field?

[thecoop]

For ClientHelper, this is using transportVersion for maybeRewriteAuthenticationHeadersForVersion, based on the node minversion. This is not a safe thing to do.

Could you expand on that please?

Apologies, this was a misunderstanding on my part. I've edited it.

[tvernum]

Let's restrict ourselves to 1 example for now so that we can talk specific rather than making general statements. Once we've work out that 1 example we can see if the answer applies to the other case.

I've picked ClientHelper, since it's been mentioned frequently in this issue.

The question here is, is there a way to completely separate the serialisation version & node version in this code, to remove the transportVersion field?

The code is attempting to determine What is the maximum TransportVersion that can be safely understood by all nodes in the cluster? so that it can (if necessary) re-serialize a piece of data (the Authentication object) into a backwards compatible format.

If we are removing the transportVersion field from the node Version, then how do we know what TransportVersion the cluster (as a whole) can work in?

[pgomulka]

to add an extra bit of colour - the idea of removing TransportVersion (and Version) from Node comes from #93822
The TransportVersion which is meant ot replace Version in the transport protocol contexts.
However Version is also used in non transport protocol use cases, like

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ClientHelper.java

Line 161 in da01691

if (authentication != null && authentication.getEffectiveSubject().getTransportVersion().after(minNodeVersion)) {

I like to think about those as 'version specific feature flags'
The initial idea was simply replace Version with TransportVersion on DiscoveryNode #93818 but during the review it was concluded that TransportVersion should be a property of a transport connection and limited to the transport protocol use case. I wonder if @DaveCTurner could give us more context?

I personally wouldn't mind keep on using TransportVersion for those 'version specific feature flags' as well as for transport protocol serialisation. The TransportVersion name might be a bit unfortunate in this case

some more context on /ES-5599

[DaveCTurner]

If we are removing the transportVersion field from the node Version, then how do we know what TransportVersion the cluster (as a whole) can work in?

We don't, and the whole point is that we don't want to know this. If we did, we would need to implement a version barrier based on the transport version, preventing from joining any nodes that speak transport versions older than the barrier version.

Or, put differently, the barrier version is the earliest transport version that the current node supports. We shouldn't ever see any nodes speaking earlier versions. In these contexts where we're using transport serialization for non-transport purposes, can we use the min supported transport version?

The code is attempting to determine What is the maximum TransportVersion that can be safely understood by all nodes in the cluster?

There is no need for us to find the maximum here. Any universally-understood version will do.