@abh3 requested changes on this pull request.

So, I see your predicament. Indeed, this is the only way to handle these kinds of certs. It's just unfortunate that ATLAS has succumbed to the Wild West when it comes to this. Seems like they could use more discipline to avoid shoving a large part of GSI into the http world. That said, I'd like to set the stage for easier migration to one TLS implementation and unfortunately you landed in the middle of that. So, what I am proposing 1) Move the GetVOMSDData() and InitSecurity() into a separate file (say, XrdHttpSecurity.cc -- if it makes more sense to you XrdHttpProtocolSecurity.cc though that's way too long), 2) consider renaming GetVOMSData() for the obvious reasons. 3) move all the includes that are needed by the code there and remove them from where they are not needed, 4) use XrdTlsPeerCerts.hh to pass around certs (i.e. don't pass around the ssl object), and 5) address my concern about the gridmap/secxtractor conflict. I know that much of this might seem like window dressing but it really does help to ease migration and maintainability. As it is, XrdHttpProtocol.cc does way too much. Anyway, the details are in the comments.

In src/XrdHttp/XrdHttpProtocol.cc:

> @@ -1778,6 +1755,14 @@ int XrdHttpProtocol::Configure(char *parms, XrdProtocol_Config * pi) {
 /******************************************************************************/
 
 bool XrdHttpProtocol::InitSecurity() {
+#ifdef HAVE_XRDCRYPTO
+  // Borrow the initialization of XrdCryptossl, in order to share the
+  // OpenSSL threading bits
+  if (!(myCryptoFactory = XrdCryptoFactory::GetCryptoFactory("ssl"))) {
+    eDest.Say("Error instantiating crypto factory ssl", "");
+    return false;
+  }
+#endif
 

As I will recommend moving the whole GetVOMSData() method to a different file so it remains totally isolated, it would make sense to move InitSecurity() to that file as well. Feel free to rename GetVOMSData() to something less confusing.

In src/XrdHttp/XrdHttpProtocol.cc:

> -        
-        mape = servGMap->dn2user(SecEntity.moninfo, bufname, sizeof(bufname), 0);
-        if ( !mape ) {
-          TRACEI(DEBUG, " Mapping name: " << SecEntity.moninfo << " --> " << bufname);
-          if (SecEntity.name) free(SecEntity.name);
-          SecEntity.name = strdup(bufname);
-        }
-        else {
-          TRACEI(ALL, " Mapping name: " << SecEntity.moninfo << " Failed. err: " << mape);
-        }
-      }
-      
+  if (!dn) {
+    chain.Cleanup();
+    return 0;
+  }

So, this says that if gridmap failed it won't bother calling the secxtrator even if it has been defined. Now, the old code did call the secxtrator whether or not gridmap returned anything. While that might not make complete sense it still seems like the correct thing to do.

In src/XrdCrypto/XrdCryptosslAux.cc:

> @@ -43,6 +43,7 @@
 #include "XrdCrypto/XrdCryptosslX509.hh"
 #include "XrdCrypto/XrdCryptosslTrace.hh"
 #include <openssl/pem.h>
+#include <openssl/ssl.h>

See comment in GetVOMSData() on how to eliminate this include. Crypto is pretty clean of this dependency and it would be good to keep it that way.

In src/XrdCrypto/XrdCryptoX509Chain.cc:

> @@ -308,6 +308,8 @@ void XrdCryptoX509Chain::PushBack(XrdCryptoX509 *c)
          end->SetNext(nc);
       end = nc;
       size++;
+   } else if (c) {
+      delete c;
    }

Good catch.

In src/XrdCrypto/XrdCryptosslAux.cc:

> @@ -376,6 +377,63 @@ int XrdCryptosslX509ChainToFile(XrdCryptoX509Chain *ch, const char *fn)
    return 0;
 }
 
+//____________________________________________________________________________
+int XrdCryptosslX509ParseStack(void* ssl_conn, XrdCryptoX509Chain *chain)
+{
+   EPNAME("X509ParseStack");
+   SSL* ssl = (SSL*) ssl_conn;

See my comment in GetVOMSData() to avoid passing the ssl object around.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

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