WLCG-AuthZ-WG / common-jwt-profile

A repo for the WLCG Common JWT profile document
3 stars 8 forks source link

Unclear how to handle backwards-compatible changes #43

Open paulmillar opened 1 year ago

paulmillar commented 1 year ago

Per page 7, the wlcg.ver, which is a required claim, denotes

the version of the WLCG token profile the relying party must understand to validate the token

The current version of the document is 1.0, thus by including the claim wlcg.ver with value 1.0, the token issuer ensures that the RP must understand the semantics of the included claims.

The document may be improved by introducing backwards compatible changes; for example, by simply fixing a simple typo. This new document would have a new version number (1.1, for example). There would then be two equivalent values for wlcg.ver claim: 1.0 and 1.1.

Under the current document definition, it is unclear which wlcg.ver claim value should be asserted.

paulmillar commented 1 year ago

There are (at least) a couple of ways of handling this:

  1. provide an extra variable to the document's version (e.g., 1.0.0),
  2. use an errata document to manage "fixes" and clarifications.

The first approach could identify the first two digits of the document's version as the wlcg.ver claim value; for example, the document with version 1.3.2 identifies semantics identified with wlcg.ver value 1.3. This document would be the third version that defines these semantics, with document version 1.3.0 and 1.3.1 identifying earlier attempts.

The second approach is more common across standardisation. Sometimes a "bis" version is released that includes errata.

msalle commented 1 year ago

I would probably prefer to have it defined in the doc that 1.X is backwards compatible with 1.0, and if we introduce something that breaks that, we'll go to V2.0 Furthermore, note that a client can request a version, see https://github.com/WLCG-AuthZ-WG/common-jwt-profile/blob/master/profile.md#requesting-token-versions Also a client should ignore things that don't correspond to the specific version: https://github.com/WLCG-AuthZ-WG/common-jwt-profile/blob/master/profile.md#claim-and-token-validation So I think most of it is already covered? A separate errata doc while staying on 1.0 seems a bit unpractical to me.

maarten-litmaath commented 9 months ago

Here are my takes at this time:

As a corollary, I think we should consider keeping 1.0 as the profile version while the changes stay backward-compatible with existing functionality. The document, however, would see versions 1.0.1 etc.

DrDaveD commented 8 months ago

It seems to me that the second statement here does not follow from the first:

The profile version ... A minor version increase from M.x to M.y indicates that functionality available in M.x still works the same way in M.y, while the latter would typically add new functionality that can just be ignored for previously existing use cases.

As a corollary, I think we should consider keeping 1.0 as the profile version while the changes stay backward-compatible with existing functionality. The document, however, would see versions 1.0.1 etc.

The first statement says that new, compatible functionality should increase the profile version from 1.0 to 1.1, but the second statement says it should stay at 1.0. So which is it?

If it's super-difficult to change the wlcg.ver, we could consider adding a new claim called wlcg.revision for backward-compatible changes, but really I think that implementations should be ignoring the version number or at minimum the minor (.x) portion unless they know about existing version numbers in which they need to behave differently.

DrDaveD commented 8 months ago

The document currently says:

Each client library implementation MUST know the versions it supports; if it encounters a token whose wlcg.ver value is not supported by the implementation, the token MUST be rejected as invalid.

Therefore we cannot change that value unless we want to introduce incompatible changes, because changing the document to say that clients need to ignore minor revision numbers is an incompatible change. So maybe a new wlcg.revision is needed for compatible changes.

maarten-litmaath commented 8 months ago

Hi @DrDaveD, the first statement you quoted does not say that new, compatible functionality should increase the profile version! Instead, it says what a minor version increase would imply.

The corollary rather refers to "any version increase may have significant consequences [...] should resort to that sparingly."

I agree implementations should at least ignore the minor version by default, which then would obviate the need for a wlcg.revision...

Since we are still in the early days of implementing all this, I think we can defend updating the v1.0 profile also with that requirement, while also keeping the profile on v1.0 for now, to allow time for clients and services to be upgraded as needed...

DrDaveD commented 8 months ago

the first statement you quoted does not say that new, compatible functionality should increase the profile version! Instead, it says what a minor version increase would imply.

Huh? It says that minor version increases indicate new, compatible functionality. So naturally one would assume that new, compatible functionality would increase the minor version. I guess it doesn't say it would have to, but that's the implication.

I agree implementations should at least ignore the minor version by default, which then would obviate the need for a wlcg.revision...

It would have been nice if the 1.0 profile said that, but that's not what it says, quite clearly.

Since we are still in the early days of implementing all this, I think we can defend updating the v1.0 profile also with that requirement, while also keeping the profile on v1.0 for now, to allow time for clients and services to be upgraded as needed...

I'm afraid it's too late for that, since the official, published profile clearly requires behavior incompatible to that. I think it would require a 2.0 version to make that change.

mambelli commented 8 months ago

Picking a bit from the previous comments. What about formalizing a MAJOR.MINOR.PATCH versioning schema for the document and saying that the profile uses only the MAJOR.MINOR versions?

msalle commented 8 months ago

I'm not sure I'm following, but if we add or change functionality, we should go to a higher version. If that is new optional functionality, I'd say that would be a minor version update, if it breaks backwards compatibility it's a major version. Implementation don't need to ignore the minor version, since they might want to use the new optional features that are indicated to be supported? Not sure we need patch versions, but those should not add or change functionality, only fixing typos, clarifying things etc.

DrDaveD commented 5 months ago

Right, they are not required to ignore the minor version, they are just required to accept any minor version and to ignore any claims or scopes they do not understand.