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

Pass Authorization header to the bridge via opaque info #581

Closed
wants to merge 3 commits into from

Conversation

bbockelm
Copy link
Collaborator

This translates the Authorization HTTP header into the authz opaque info.

It allows for the ACC layer to do things like authorize requests based on tokens, analogously to how ALICE token-based auth has always worked.

This allows any ACC plugins to utilize the contents of the authorization
header, providing a mechanism for request-level authorization that is
more common on the web.
@bbockelm
Copy link
Collaborator Author

@ffurano - any thoughts on this one?

@bbockelm
Copy link
Collaborator Author

@ffurano ping!

@ffurano
Copy link
Contributor

ffurano commented Sep 22, 2017

This fix adds the content of the authorization header (the standard one) to the opaque information string that is consumed only internally by the Xrd plugins, and hence Acc plugins.
Not that I like it too much, as I see it as a workaround for the absence of a suitable field in XrdSecEntity.

Wouldn't it be possible to append this opaque string to the "Protocol specific endorsements" field instead ? The Acc plugins do have access to this.

My fear is that we may not be able to exclude any present or future interaction with other existing Ofs/Oss plugin implementations that happen to use the "authz=" info, or interactions with clients that already provide an "authz=" opaque token.

@bbockelm
Copy link
Collaborator Author

Hi @ffurano -

I picked opaque information instead of wedging it into the XrdSecEntity for two reasons:

  1. I believe XrdSecEntity reflects information about the login session while the opaque information relates to the current request. The Authorization header belongs to the request, not the session (and the session is indeed already established by time it is here). It's perfectly valid (in fact, foreseen) that the Authorization header may only contain sufficient authorization for a specific narrow operation.
  2. When I was comparing to two other similar approaches (looking at the ALICE token plugin and @abh3's suggestion that this could be passed through by xrootd's opaque info but encrypted using the session key), they also use the opaque info route.

I think it's good to standardize on a consistent key name of the opaque information (similar to how HTTP utilizes the Authorization header for a multitude of authorization schemes) across multiple implementations; here, I picked authz on purpose because it appears to be what ALICE uses for their token format.

@ffurano
Copy link
Contributor

ffurano commented Sep 22, 2017

I see the issue, which is linked to too little information being passed through the authorization, i.e. one has to choose among the SecEntity and the URL where to put this kind of info. I would welcome improvements to this (maybe xrootd5?)

This PR silently appends an authz keyvalue to the internal opaque info, and I think that this is delicate business. What if the opaque info already had such a token?

Since I'd like to avoid philosophy, would you mind adding a log line to the code that prints (at DEBUG level) something like "I am silently adding authz= to the opaque info" ?
At least, if our destiny is to have issues with this, it might be understood from a log file.

@bbockelm
Copy link
Collaborator Author

It should be possible to parse out the opaque info and then decide whether we want to override authz.

So, I see three approaches:

  1. Parse opaque info, check for an existing authz header, and then decide to overwrite.
  2. Parse opaque info, check for an existing authz header, and then decide to keep it.
  3. Do not parse and simply add a debug line as you suggest above.

There are, of course, CPU-time and memory-allocation costs to (1) and (2). I'm fine with any option. Which would you prefer?

@ffurano
Copy link
Contributor

ffurano commented Sep 25, 2017

Hi Brian,
my preference is 3) as it's unclear if anyone will ever encounter this, and making every request pay one additional parsing might be undesirable. If it happens we'll have a clue from the log.
IMO the best solution would be to review some Xrd data structures, in the long run.

@bbockelm
Copy link
Collaborator Author

Ok - @ffurano, this is done!

@abh3
Copy link
Member

abh3 commented Sep 26, 2017

It would seem to me that a more systematic change would be better here to avoid future conflicts. Since autz= is already in use then the authorization header shouldn't reuse that cgi element. It would seem that a better implementation would be to use a different cgi element (say 'auth-') to pass the header via cgi and then the XrdAcc plugin developed to handle it. If the idea is that the ALICE plugin would be used, then that plugin should have been changed to handle the new element in addition to the original authz element. Yes, that would mean changing the ALICE plugn but at least it would be consistent and understandable.

Before we merge this request I'd like to get additional comments as this is a potentially conflicting future sore point.

@ffurano
Copy link
Contributor

ffurano commented Sep 26, 2017

As a followup to the chat we just had, I'd like to implement something more general that is able to copy arbitrary headers into arbitrary CGI entries in the URL, defined in the config file.
So, for me, Brian's pull req can even be accepted for the time being if it helps Brian's other developments (I leave this little decision to Andy), and I will implement the more generic approach in 7-10 days (I am taking a few days off)

@bbockelm
Copy link
Collaborator Author

Hi -

To me, the important point is the opaque key is reused. I.e., it's the same key whether the authorization comes from the XrdHttp plugin or straight through plain-old xrdcp. I think it's a distinct disadvantage if the ACC plugin needs different logic depending on which protocol the authorization comes from -- it is a layering violation.

That said, given this is mostly internal details, I can make a change just to keep this project moving forward. I'm not sure a - is a good idea in the key name - how about simply authorization?

Brian

@abh3
Copy link
Member

abh3 commented Sep 26, 2017

I think that Fabrizio and I came up with a reasonable solution here. He will implement a generalized directive that will allow you to promote any header to CGI. It would be something like
http.header2cgi


That way you can have your cake and we can eat it too!

@ffurano
Copy link
Contributor

ffurano commented Oct 6, 2017

This PR has been obsoleted by #597. Thank you Brian for providing a working example of what was needed!

@ffurano ffurano closed this Oct 6, 2017
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.

3 participants