5159 ipsec_libssl_setup.c loads libcrypto

Review Request #966 — Created March 20, 2018 and submitted

citrus
illumos-gate
master
5159
9fd96de...
general

5159 ipsec_libssl_setup.c loads libcrypto

Work by Jason King.

See https://illumos.org/rb/r/966/bugs/5159/ for details of testing done.

  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
citrus
cjr
  1. 
      
  2. (I've been working on LDAP servers since the 1990s.)
    
    Although you're trying to follow LDAP RFCs for formatting the DN (good!) it looks like you're outputting the RDNs backwards. LDAP DNs are "little endian", so you should reverse the loop.
    
    I would also separate the RDNs with a plain "," and lose the extra space.
    
    IOW currently you'd output this DN:
    
    c=US, o=illumos, cn=server
    
    but this would be correct:
    
    cn=server,o=illumos,c=US
    
    Having the wrong order of RDNs is a significant problem, but easy to fix.
    1. You're right, in an LDAP context RFC 4514 explicitly requires that the RDNs are output in reverse order. However, in the X.509 world, the output is mixed in practice* and I'm not sure where (if?) the DN order is defined. I understand that the author of this change used RFC 4514 as a reference for the escaping etc., since there seems to be nothing better, but wasn't following it to the letter since this is not LDAP.

      * Tools such as openssl and Mozilla NSS libraries output C first; Java's keytool puts C last. Viewing certificates in a web browser gives mixed results - many now just list the individual RDNs, some in forward and some in reverse order.

      Since the intention of this change is just to remove the dependency on OpenSSL, I feel that the order of the output from print_asn1_name() should not change from what it was previously. Perhaps open a new bug to handle that and get feedback from those who would be directly affected (predominantly those working on IKEv2 I imagine)?

    2. X.500 (and thus X.509) describes a DN as an ordered sequence of RDNs. They never decribed a text representation, so this did result in various tools formatting them in various wild and wacky ways (@c=GB@cn=Chris or /cn=Chris/c=GB/ or ... well you get the idea!)

      LDAP did formally specify the way to write down the RDNs in a particular, consistent, and unambiguous order all the way back in 1993, so IMO there's no excuse for anything generating anything different any more.

      As this is just for debugging, can't we just fix the format? Or at least note somewhere that it is LDAP except for being written big-endian and reversed?

    3. LDAP did formally specify the way to write down the RDNs in a particular, consistent, and unambiguous order all the way back in 1993, so IMO there's no excuse for anything generating anything different any more.
      I agree but the old implementation outputs it like this (despite the openssl call being passed ASN1_STRFLGS_RFC2253) and this change is about removing the dependency on OpenSSL while aiming not to break any consumers. Let's open a new issue about re-ordering and removing the spaces.

    4. The concensus appears to be to keep displaying these DNs in the current order such that DNs based on DNS domain names continue to appear in order, for example dc=example,dc=com.
      I'm dropping this issue from this change, if you wish, please open a new issue for reversing the display order at https://illumos.org/issues and begin discussion on the illumos developer's list.

  3. 
      
gwr
  1. 
      
    1. It really made the code significantly simpler. I do have a change sitting around that pulls the custr_ stuff into it's own library. Talking a bit internally, I could probably get it into illumos-joyent in the next few weeks (now that things have calmed down) and get it upstreamed into illumos-gate after that. It'd still be considered a 'private' library (i.e. in-gate use only) for the time being.

      That'd remove the libcmdutils dependency, replacing it with a dependency on the new libcustr.so (which itself would only have the usual libc dependency). Would that address your concerns?

    2. I'm not all that concerned (depending on the impact) but wanted to make sure people had thought about the "drags in everything" problem.
      What links with libkmf? (what's the impact?)

    3. With jbk's integration of libcustr into gate, I'm changing to use this in place of libcmdutils as it does not pull in the additional dependencies that libcmdutils does.

  2. The custr... stuff is cool and all, but is it worth dragging in libcmdutils here to get it?

    Did you consider making a pass over the RDNs here
    and estimating the output buffer space required?
    Then you could allocate that buffer here and
    pass it as (... buf, len) to the other functions
    in here to fill that in. Better?

    1. The first draft of the change that I saw did something like that but was reworked in favour of custr since it simplified things. libcmdutils is tiny, is it really a problem to use it for libkmf?

    2. I'm not sure. I think I'm sensitive about the issue after runninging into https://illumos.org/issues/3087 again recently.
      I see libcmdutils does also drag in libavl and libnvpair, and I guess this is how we gradually get to "pulls in everything":)

  3. 
      
igork
  1. Ship It!
  2. 
      
citrus
citrus
citrus
citrus
tsoome
  1. Ship It!
  2. 
      
citrus
Review request changed

Status: Closed (submitted)

Loading...