Add support for VOMS mapfile #1572

bbockelm · 2021-12-12T03:24:29Z

Add a mapfile for VOMS FQANs. This allows the VOMS plugin to map the contents of the VOMS extension to a name in the XrdSecentity.

This adds two new configuration parameters, voms.mapfile and voms.trace. Here's an example invocation:

voms.mapfile /etc/grid-security/voms-mapfile
voms.trace debug

Where the mapfile is in the format used by XrdLcmaps:

"/cms/Role=lcgadmin/Capability=NULL" lcgadmin
"/cms/*" cmsuser
"/des/Role=Analysis/Capability=NULL" des

At the debug level, things are pretty chatty - including dumping each line during parsing and each mapping invocation.

The mapfile is re-parsed every 30 seconds.

bbockelm · 2021-12-12T14:18:13Z

Oh! I should mention that the configuration parameters are of the variety "last time set wins", making it easier for administrators to provide an override file.

brianhlin · 2021-12-13T23:03:39Z

Addresses #1538

abh3

Good start but there are various things that really need to be changed. I likely did not find all of them but that's what iteration is all about. I also look at this as a way to address another problem you raised is how do we make this consistent across all protocols and do not require each one to specify the same thing using a different directive.

src/XrdOuc/XrdOucString.cc

src/XrdOuc/XrdOucString.hh

src/XrdVoms/XrdVomsMapfile.cc

src/XrdVoms/XrdVomsHttp.cc

src/XrdVoms/XrdVomsgsi.cc

src/XrdVoms/XrdVomsMapfile.cc

abh3 · 2022-01-08T00:57:56Z

@bbockelm Addressed your comments. I think we are converging.

If there's an entry found in the corresponding grid-mapfile, then that is considered "more specific" and overrides the "less specific" voms-mapfile entry. E.g., a mapping to `bbockelm` should be picked over a generic `cms`. To differentiate between an explicit mapping and the fallbacks generated for the plugins, we use the entity's extended attribute mechanism.

Instead of doing the reload of the mapfile in-line, add a new maintenance thread that will periodically reload the mapfile (if and only if a change is detected in the modification time of the mapfile).

This moves the mapfile invocation from being a "wrapper" around the existing XrdVoms plugins to be a core part of the functionality. Note this also changes the failure semantics from "fail quietly" to "fail process startup if misconfigured."

bbockelm · 2022-01-17T17:30:45Z

@abh3 - I believe the latest refactor has addressed all your comments.

src/XrdVoms/XrdVomsMapfile.hh

src/XrdVoms/XrdVomsMapfile.cc

abh3 · 2022-01-25T07:11:30Z

src/XrdHttp/XrdHttpSecurity.cc

@@ -164,6 +165,7 @@ XrdHttpProtocol::HandleGridMap(XrdLink* lp)
      TRACEI(DEBUG, " Mapping name: '" << SecEntity.moninfo << "' --> " << bufname);
      if (SecEntity.name) free(SecEntity.name);
      SecEntity.name = strdup(bufname);
+      SecEntity.eaAPI->Add("gridmap.name", bufname, true);


I see this consistently set but who is actually going to look at that? I thought the whole idea is that the entity.name would hold this value. So, I must be mistaken. Can you explain the reasoning here (I am sure it will be obvious once you do).

It's not obvious!

It's a bit of a problem of order of precedence. For the final value of entity.name, want the following order:

Explicit entries in the grid mapfile.

Explicit entries in the VOMS mapfile

Whatever the default behavior is for the GSI plugin as a whole (e.g., potentially an auto-generated name).

Unfortunately, the naive solution results in an ordering of (1), (3), then (2). Thus, we add any explicit usage of the grid mapfile to the entity so we can distinguish a positive entry from a default fallback.

I'm sure you looked at it until you fell asleep. Nonetheless, is there any way to fix this (i.e. adhere to the wanted order)? Now, let me understand this. The order basically says if the gridmap file has a map in it use it, if not use he one in the VOMS mapfile, and if it doesn't exists use the default. If that is the logic then swapping 2 and 3 should not make a difference because the default would be replaced by the VOMS mapping is it exists or the default would be kept. In the end, the same result. Now, please tell me it's not the way it works :-)

The VOMS plugin runs after the GSI authentication logic; the GSI authentication logic that generates the entity.name may be based on the mapfile or the default logic. What's added here allows one to differentiate those two cases.

We need to know the difference between the default and explicit mapping because, in one case (the default), the VOMS mapfile overrides the entity.name value. In the other case, the VOMS mapfile should keep the entity.name untouched.

OK, I now get it. However, couldn't we accomplish the same thing with a one bit flags to indicate the type of ID in the the name. Then Voms could do the right thing. That would be more straightforward than recording the full name as it looks like it going to be used for some other purpose that a simple "type" check. BTW we do have bit in the secentity structure. What do you think?

abh3

OK, I think we are down to one alternate proposal which you may or may not want to make and one that you agreed to but wasn't yet made. It's all in the new comments.

abh3 · 2022-01-27T06:59:54Z

src/XrdHttp/XrdHttpSecurity.cc

@@ -164,6 +165,7 @@ XrdHttpProtocol::HandleGridMap(XrdLink* lp)
      TRACEI(DEBUG, " Mapping name: '" << SecEntity.moninfo << "' --> " << bufname);
      if (SecEntity.name) free(SecEntity.name);
      SecEntity.name = strdup(bufname);
+      SecEntity.eaAPI->Add("gridmap.name", bufname, true);


I'm sure you looked at it until you fell asleep. Nonetheless, is there any way to fix this (i.e. adhere to the wanted order)? Now, let me understand this. The order basically says if the gridmap file has a map in it use it, if not use he one in the VOMS mapfile, and if it doesn't exists use the default. If that is the logic then swapping 2 and 3 should not make a difference because the default would be replaced by the VOMS mapping is it exists or the default would be kept. In the end, the same result. Now, please tell me it's not the way it works :-)

src/XrdVoms/XrdVomsMapfile.hh

Reduces some of the logical complexity; prevents log files filling up quicker on failure.

Decision was that this would be sufficiently infrequently used that we can just let the OS cleanup the maintenance thread.

bbockelm · 2022-01-29T19:22:24Z

Ok, I think I addressed the two items! Assuming that my comment about the order-of-logic between the GSI & VOMS plugins makes sense, i think it's ready to go.

bbockelm · 2022-02-05T15:14:39Z

@abh3 - quick ping on this for next week. I think all is ready (or, at least, ready for the next round of input!).

abh3

OK, one final comment on how to make the code less obtuse when it comes to deciding on how to handle the name filed. I'd like your comments. Everything else looks fine.

abh3 · 2022-02-08T05:17:11Z

src/XrdHttp/XrdHttpSecurity.cc

@@ -164,6 +165,7 @@ XrdHttpProtocol::HandleGridMap(XrdLink* lp)
      TRACEI(DEBUG, " Mapping name: '" << SecEntity.moninfo << "' --> " << bufname);
      if (SecEntity.name) free(SecEntity.name);
      SecEntity.name = strdup(bufname);
+      SecEntity.eaAPI->Add("gridmap.name", bufname, true);


OK, I now get it. However, couldn't we accomplish the same thing with a one bit flags to indicate the type of ID in the the name. Then Voms could do the right thing. That would be more straightforward than recording the full name as it looks like it going to be used for some other purpose that a simple "type" check. BTW we do have bit in the secentity structure. What do you think?

abh3

Missed handling when to reprocess the map file. I think we only need a comment to explain why ctime is used on not mtime.

OK, one final comment on how to make the code less obtuse when it comes to deciding on how to handle the name filed. I'd like your comments. Everything else looks fine.

src/XrdVoms/XrdVomsMapfile.cc

detecting file changes.

abh3 · 2022-02-09T02:13:53Z

Thanks for adding the comment. Now, what about using a type flag for the name instead of recording the actual name? I propose this because it would be very apparent what is being done here in terms of ordering the name mapping by using a type flag.

src/XrdVoms/XrdVomsMapfile.cc

abh3

Giuseppe found an apparent missing return statement.

src/XrdVoms/XrdVomsMapfile.cc

…this is also in the Entity name).

bbockelm · 2022-03-01T04:53:56Z

@abh3 - I think I have the last two items cleaned up. We now return nullptr where necessary and switch to recording a single bit (as 1) instead of a redundant name.

abh3 · 2022-03-01T05:48:59Z

Thank you Brian.

abh3 requested changes Jan 4, 2022

View reviewed changes

abh3 self-assigned this Jan 4, 2022

abh3 added the enhancement label Jan 4, 2022

bbockelm added 4 commits January 17, 2022 10:42

Add mapfile support as a separate class to the XrdVoms code.

62fa23c

Fixup: Add const fixes due to ABI change revert.

7b59dc1

Add initial README file covering the syntax and behavior of the mapfile.

fb22ea3

bbockelm force-pushed the vomsmapfile branch from 765dfea to f01530f Compare January 17, 2022 16:43

bbockelm added 2 commits January 17, 2022 11:26

Overhaul reload strategy to use a maintenance thread.

29fa30a

Instead of doing the reload of the mapfile in-line, add a new maintenance thread that will periodically reload the mapfile (if and only if a change is detected in the modification time of the mapfile).

bbockelm force-pushed the vomsmapfile branch from f01530f to f9931b4 Compare January 17, 2022 17:26

abh3 requested changes Jan 25, 2022

View reviewed changes

src/XrdVoms/XrdVomsMapfile.hh Show resolved Hide resolved

src/XrdVoms/XrdVomsMapfile.hh Outdated Show resolved Hide resolved

src/XrdVoms/XrdVomsMapfile.cc Outdated Show resolved Hide resolved

abh3 reviewed Jan 25, 2022

View reviewed changes

abh3 reviewed Jan 27, 2022

View reviewed changes

bbockelm added 2 commits January 29, 2022 13:09

Remove faster polling interval for failures.

b324a1a

Reduces some of the logical complexity; prevents log files filling up quicker on failure.

Remove shutdown capabilities for maintenance thread.

52a9600

Decision was that this would be sufficiently infrequently used that we can just let the OS cleanup the maintenance thread.

abh3 reviewed Feb 8, 2022

View reviewed changes

src/XrdVoms/XrdVomsMapfile.cc Show resolved Hide resolved

XrdVomsMapfile: Add a comment on why we use ctime over mtime for

5f0a255

detecting file changes.

dellaric reviewed Feb 11, 2022

View reviewed changes

src/XrdVoms/XrdVomsMapfile.cc Outdated Show resolved Hide resolved

abh3 requested changes Feb 12, 2022

View reviewed changes

src/XrdVoms/XrdVomsMapfile.cc Show resolved Hide resolved

bbockelm added 2 commits February 28, 2022 22:46

Return nullptr from function that won't ever hit return statement.

67d047b

Use gridmap.name as a flag - no need to preserve the actual name (as …

4ede2ba

…this is also in the Entity name).

abh3 merged commit 57d9a5a into xrootd:master Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for VOMS mapfile #1572

Add support for VOMS mapfile #1572

bbockelm commented Dec 12, 2021

bbockelm commented Dec 12, 2021

brianhlin commented Dec 13, 2021

abh3 left a comment

abh3 commented Jan 8, 2022

bbockelm commented Jan 17, 2022

abh3 Jan 25, 2022

bbockelm Jan 26, 2022

abh3 Jan 27, 2022

bbockelm Jan 29, 2022

abh3 Feb 8, 2022

abh3 left a comment

abh3 Jan 27, 2022

bbockelm commented Jan 29, 2022

bbockelm commented Feb 5, 2022

abh3 left a comment

abh3 Feb 8, 2022

abh3 left a comment

abh3 commented Feb 9, 2022

abh3 left a comment

bbockelm commented Mar 1, 2022

abh3 commented Mar 1, 2022

Add support for VOMS mapfile #1572

Add support for VOMS mapfile #1572

Conversation

bbockelm commented Dec 12, 2021

bbockelm commented Dec 12, 2021

brianhlin commented Dec 13, 2021

abh3 left a comment

Choose a reason for hiding this comment

abh3 commented Jan 8, 2022

bbockelm commented Jan 17, 2022

abh3 Jan 25, 2022

Choose a reason for hiding this comment

bbockelm Jan 26, 2022

Choose a reason for hiding this comment

abh3 Jan 27, 2022

Choose a reason for hiding this comment

bbockelm Jan 29, 2022

Choose a reason for hiding this comment

abh3 Feb 8, 2022

Choose a reason for hiding this comment

abh3 left a comment

Choose a reason for hiding this comment

abh3 Jan 27, 2022

Choose a reason for hiding this comment

bbockelm commented Jan 29, 2022

bbockelm commented Feb 5, 2022

abh3 left a comment

Choose a reason for hiding this comment

abh3 Feb 8, 2022

Choose a reason for hiding this comment

abh3 left a comment

Choose a reason for hiding this comment

abh3 commented Feb 9, 2022

abh3 left a comment

Choose a reason for hiding this comment

bbockelm commented Mar 1, 2022

abh3 commented Mar 1, 2022