 |
 |
@abh3 requested changes on this pull request.
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.
In src/XrdOuc/XrdOucString.cc:
> @@ -257,7 +257,7 @@ int XrdOucString::form(XrdOucString &str, const char *fmt, ...) #endif //______________________________________________________________________________ -int XrdOucString::find(const char c, int start, bool forward) +int XrdOucString::find(const char c, int start, bool forward) const
I know this should have been done earlier. However, changing a method qualifier breaks ABI. So, this is a no-no and will have to wait for R6.
In src/XrdOuc/XrdOucString.hh:
> char &operator[](int j); - int find(const char c, int start = 0, bool forward = 1); - int find(const char *s, int start = 0); - int find(XrdOucString s, int start = 0); - int rfind(const char c, int start = STR_NPOS) + int find(const char c, int start = 0, bool forward = 1) const; + int find(const char *s, int start = 0) const; + int find(XrdOucString s, int start = 0) const; + int rfind(const char c, int start = STR_NPOS) const
All of these changes break ABI. Please revert them.
In src/XrdVoms/XrdVomsMapfile.cc:
> +int
+XrdVomsMapfile::GetSecData(XrdLink * lnk, XrdSecEntity &entity, SSL *ssl)
+{
+ if (!m_xrdvoms) return -1;
+
+ auto retval = m_xrdvoms->GetSecData(lnk, entity, ssl);
+ if (retval) return retval;
+
+ return Apply(entity);
+}
+
+
+int
+XrdVomsMapfile::Apply(XrdSecEntity &entity)
+{
+ Reconfigure();
This is far too brutish. This really should be a) independently timer driven with the typical interval of 5-10 minutes, b) the test at each interval should check if the mapfile was actually modified, and c) only then should it reparse the file. Let's do this the right way.
In src/XrdVoms/XrdVomsMapfile.cc:
> + }
+ entries->emplace_back(entry);
+ }
+ }
+ m_entries = entries;
+ return true;
+}
+
+
+bool
+XrdVomsMapfile::Reconfigure() {
+ auto now = time(NULL);
+ auto retval = true;
+ std::stringstream ss;
+ ss << "Last update " << m_last_update.load(std::memory_order_relaxed) << ", " << now;
+ m_edest->Log(LogMask::Debug, "VOMS Mapfile", ss.str().c_str());
As per my previous comment in Apply(), this really has to be reworked to be a more reasonable implementation instead of a brute force implementation.
In src/XrdVoms/XrdVomsMapfile.cc:
> +}
+
+
+XrdVomsMapfile *
+XrdVomsMapfile::Configure(XrdSysError *erp, XrdHttpSecXtractor *xtractor)
+{
+ if (tried_configure) {
+ auto result = mapper.get();
+ if (result) {
+ result->SetExtractor(xtractor);
+ result->SetErrorStream(erp);
+ }
+ return result;
+ }
+
+ tried_configure = true;
Why do we need this at all. The assumption, code-wise, is that this will be called during initialization and hence will be called only once. So, it's very confusing why this code exists here at all. Is there something we don't know about?
In src/XrdVoms/XrdVomsHttp.cc:
> @@ -142,7 +143,9 @@ XrdHttpSecXtractor *XrdHttpGetSecXtractor(XrdHttpSecXtractorArgs) // Now return the interface object // - return (XrdHttpSecXtractor *)new XrdVomsHttp(eDest, *vomsFun); + auto base = static_cast<XrdHttpSecXtractor *>(new XrdVomsHttp(eDest, *vomsFun)); + auto wrapper = static_cast<XrdHttpSecXtractor *>(XrdVomsMapfile::Configure(eDest, base)); + return wrapper ? wrapper : base;
I have a problem with this a) it totally ignores any errors that may occur during configuration and this has always been a site complaint about silent errors that should be fatal, and b) this could have been done in a much more straightforward way by simply inserting a test for mapping in XrdVomsHttp::GetSecData() instead using an indirect wrapper. Though, frankly, I would do it the other way around and pass a mapper to the voms function itself (or a null pointer) so that this works for http as well as xrootd. Then we have a chance to make it consistent across all protocols.
> @@ -57,7 +59,11 @@ int XrdSecgsiVOMSFun(XrdSecEntity &ent)
{
// Make sure we were initialized. If so, invoke the function and return result.
//
- return (vomsFun ? vomsFun->VOMSFun(ent) : -1);
+ int retval = (vomsFun ? vomsFun->VOMSFun(ent) : -1);
+ if (retval == -1) {return retval;}
+
+ auto mapfile = XrdVomsMapfile::Get();
+ return mapfile ? mapfile->Apply(ent) : retval;
As per my previous comments, we really don't need a change here. The mapping should be part of the core VomsFun and the interface to the xroot-side initialization should pretty much do what the http side does. That way it's all in one place and you clear can see it's part of the core logic. Plus it guarantees that the map will be applied if the "voms.mapfile" directive is in the config file irrespective of the protocol.
In src/XrdVoms/XrdVomsMapfile.cc:
> +XrdVomsMapfile::Reconfigure() {
+ auto now = time(NULL);
+ auto retval = true;
+ std::stringstream ss;
+ ss << "Last update " << m_last_update.load(std::memory_order_relaxed) << ", " << now;
+ m_edest->Log(LogMask::Debug, "VOMS Mapfile", ss.str().c_str());
+ if (now > m_last_update.load(std::memory_order_relaxed) + 30) {
+ retval = ParseMapfile(m_mapfile);
+ m_last_update.store(now, std::memory_order_relaxed);
+ }
+ return retval;
+}
+
+
+bool
+XrdVomsMapfile::ParseLine(const std::string &line, std::vector<std::string> &entry, std::string &target)
We will need documentation on the format of the mapfile as well as the rules that are used for the actual mapping. Can you provide such a document so that it can be included in the security reference.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1