Skip to content
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

Clarifies scope of storage.stage and storage.read #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

slithy
Copy link

@slithy slithy commented Jun 15, 2023

No description provided.

@paulmillar
Copy link
Contributor

I'm not opposing the direction of this patch; however, it should be noted clearly that this change is breaking backwards compatibility.

The patch doesn't so much "clarify" the definition of storage.stage as rewrites it. Case in point: the document currently says that storage.stage ... is a superset of storage.read, which is not true with this patch.

IMHO a better approach would be to keep the storage.stage semantics as-is and introduce a new capability that has the desired semantics (authorises staging of file but does not authorise reading of that file).

I believe this would support the desired authorisation scenarios (at least, as I understand them) without breaking backwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch forgot to update line 18 (after the patch is applied), which includes the text

The WLCG Token Profile version described by this document is “1.0”.

This should be updated to say:

The WLCG Token Profile version described by this document is “1.1”.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are going to be a lot more changes in the next released version, which I believe is likely to be version 2.0. Update of the version history shouldn't be in this PR.

@slithy
Copy link
Author

slithy commented Jun 19, 2023

I'm not opposing the direction of this patch; however, it should be noted clearly that this change is breaking backwards compatibility.

OK, fair point.

I changed the version message to say, "v1.1 - Changes scope of storage.stage and storage.read capabilities" OK for you?

@msalle
Copy link

msalle commented Jun 19, 2023

I'm not opposing the direction of this patch; however, it should be noted clearly that this change is breaking backwards compatibility.

OK, fair point.

I changed the version message to say, "v1.1 - Changes scope of storage.stage and storage.read capabilities" OK for you?

I think a backwards incompatible change should require an increase of the major version number, so v2.0.
I would generally prefer if we can keep backwards compatibility, but it depends on whether we think anyone is actually using it already.

@paulmillar
Copy link
Contributor

Just to add my 2c-worth.

I'm less worried about the v1.1 vs v2.0 distinction since each token embeds the profile version the service needs in order to understand it: the wlcg.vers claim. Therefore, making a breaking change is "supported" with any version (where "supported" means that services will rejects newer tokens if they don't support the newer semantics).

I don't think there is much use of storage.stage. Non-backwards compatible changes are bad, but this one is probably the "least bad".

HOWEVER, this relies on the issuer being able to issue tokens with wlcg.vers claim of 1.1 if the token has a storage.stage AuthZ statement (i.e., with the new semantics).

Changing INDIGO-IAM so that it always issues tokens with wlcg.vers: 1.1 will cause all services to reject the new tokens -- at least, until they all upgrade. This is A Good Thing.

INDIGO-IAM could issue tokens with wlcg-vers: 1.0 for tokens without storage.stage and only wlcg-vers: 1.1 for tokens with storage.stage. This would give a broad backwards-compatible way of rolling out the new semantics.

However, I doubt IINDIGO-IAM already has the logic to support this kind of logic when issuing tokens.

@mpatrascoiu
Copy link

Hello all,

Jumping in on the discussion, I'm doubtful storage implementations also have the capability to reject tokens based on the wlcg.vers number. The way token versioning should be treated by storage entities (SEs + FTS) warrants a discussion on its own.

About the change itself, if I understand it correctly, storage.stage becomes bound to just the bringonline / prepare operation. So a client such as FTS, in order to perform "bringonline + transfer", it would need a token with storage.stage:/<path> + storage.read:/<path> scope.

I wonder how this plays with experiment policies. I'm thinking about what Martin (@bari12) mentioned, where ATLAS would only put a single scope inside the token.

For nearline data such as tape, the “stage+transfer” semantics are defined by the tape access protocol independently
from the authorization scheme. For XRootD, this means “prepare -s” (stage), “query prepare”, “abort prepare”, “evict”
and “copy”. For HTTP, the semantics are defined in the
[WLCG Tape REST API](https://cernbox.cern.ch/pdf-viewer/public/vLhBpHDdaXJSqwW/WLCG%20Tape%20REST%20API%20reference%20document.pdf).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to use the wlcg-storage/wlcg-tape-rest-api repository instead of CERNBox for the WLCG Tape REST API specification document.

@paulmillar
Copy link
Contributor

Hi @mpatrascoiu ,

Jumping in on the discussion, I'm doubtful storage implementations also have the capability to reject tokens based on the wlcg.vers number.

If dCache is configured to use the WLCG JWT profile then the wlcg.ver claim must be present and the value must be exactly 1.0. If the wlcg.ver claim is missing or the value is not 1.0 then the token is rejected.

The way token versioning should be treated by storage entities (SEs + FTS) warrants a discussion on its own

Do you see some ambiguity in the definition of the wlcg.ver claim, or do you see a potential problem with that definition?

@bari12
Copy link

bari12 commented Jul 18, 2023

I wonder how this plays with experiment policies. I'm thinking about what Martin (@bari12) mentioned, where ATLAS would only put a single scope inside the token.

Just to clarify, while we would prefer a single scope, multiple scopes with the same path, are not really a problem. Thus storage.stage:/<path> + storage.read:/<path> in your example, given that the two pathes are the same. The statement you are referring to is that we want to avoid tokens such as storage.stage:/<path1> + storage.stage:/<path2> + ... + storage.stage:/<pathN> simply because this makes it much harder to cache and efficiently re-serve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants