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

[XrdSecztn] Add security library option to configure token validation… #1921

Closed
wants to merge 1 commit into from

Conversation

apeters1971
Copy link
Contributor

Add ZTN security library option to configure token validation policy using new configuration entry:
-validation {required|optional|ignore}'
The default policy is required and does not need to be specified.

Example 1: don't validate tokens at all
sec.protocol ztn -validation ignored

Example 2: validate token but don't fail if validation failed
sec.protocol ztn -validation optional

@bbockelm
Copy link
Contributor

@apeters1971 - would it be possible to also specify the policy outside of a flag?

sec.protocol ztn -validation ignored

does not compose across multiple configuration files while

sec.protocol ztn
ztn.validation ignored

does (as the second line could be part of a separate drop-in config file in an RPM).

@apeters1971
Copy link
Contributor Author

Hi Brian, I checked. i cannot do this so easily, because the plug-ins are only called with the parameters on the line of the security library. This is something which has to be changed 'on top' of the library, since the configuration file is not parsed inside security plugins.

@xrootd-dev
Copy link

xrootd-dev commented Feb 21, 2023 via email

@apeters1971
Copy link
Contributor Author

To say "No tokenlib" does not work, because they are needed and stacked.

SciTokens->[ALICE Tokens]->EosTokens

All works via HTTP, not via ZTN. I don't want to rule out to use SciTokens via ZTN.

I just need this simple switch as suggested in the pull request. Brians comment is about nested config files, but this is already not possible by design for security plugin configuration and I don't need it.

@bbockelm
Copy link
Contributor

@apeters1971 - I'll defer to you on this but do note that new XRootD 5 features allow you to pull the configuration from a global. See: https://github.com/xrootd/xrootd/blob/master/src/XrdSciTokens/XrdSciTokensAccess.cc#L907-L916; it was a similar setup where the plugin doesn't get the config file.

Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

Two questions before this goes in. The first in a comment in the code about "optional". The second is why do you think that one cannot say "-tokenlib none" as a parameter to ztn. The ztn protocol is not responsible for loading the SciToken plugin for authorization purposes. This is done via the authlib directive. However, it needs that plugin it is in order to validate the token. If no validation is desired there is no reason to load that plugin as it will never be called. Right?

What we have now is an oddity that when you say you don't want validation ztn still requires that the plugin that validates the token be present even though it will never use it. I hope you see the inconsistency here.

return 0;
}
if (strcmp(val, "ignore")) validation = 0;
else if (strcmp(val, "optional")) validation = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Since this has to be documented, the meaning of optional seems not to do anything worthwhile in the code. So, either I am missing something or optional seems irrelevant. Can you explain?

@apeters1971
Copy link
Contributor Author

I will go for tokenlib "none". That is the easiest indeed!

@apeters1971
Copy link
Contributor Author

I will create one for option '-tokenlib none' to disable token validation

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

4 participants