389-ds-base

Clone
Source Code
GIT

#49613 Issue 49109 - nsDS5ReplicaTransportInfo should accept StartTLS as an option
Closed 3 years ago by spichugi. Opened 6 years ago by spichugi.
spichugi/389-ds-base transport_tls_rename  into  master

file modified
+59 -4 
@@ -33,6 +33,11 @@ 
 

      m1 = topo_m2.ms['master1']
 

      m2 = topo_m2.ms['master2']
 

  

+     if ds_is_older('1.4.0.6'):
 

+         transport = 'SSL'
 

+     else:
 

+         transport = 'LDAPS'
 

+ 

      # Create the certmap before we restart for enable_tls
 

      cm_m1 = CertmapLegacy(m1)
 

      cm_m2 = CertmapLegacy(m2)
 
@@ -65,8 +70,8 @@ 
 

  

      agmt_m1.replace_many(
 

          ('nsDS5ReplicaBindMethod', 'SSLCLIENTAUTH'),
 

-         ('nsDS5ReplicaTransportInfo', 'SSL'),
 

-         ('nsDS5ReplicaPort', '%s' % m2.sslport),
 

+         ('nsDS5ReplicaTransportInfo', transport),
 

+         ('nsDS5ReplicaPort', str(m2.sslport)),
 

      )
 

      agmt_m1.remove_all('nsDS5ReplicaBindDN')
 

  
@@ -75,8 +80,8 @@ 
 

  

      agmt_m2.replace_many(
 

          ('nsDS5ReplicaBindMethod', 'SSLCLIENTAUTH'),
 

-         ('nsDS5ReplicaTransportInfo', 'SSL'),
 

-         ('nsDS5ReplicaPort', '%s' % m1.sslport),
 

+         ('nsDS5ReplicaTransportInfo', transport),
 

+         ('nsDS5ReplicaPort', str(m1.sslport)),
 

      )
 

      agmt_m2.remove_all('nsDS5ReplicaBindDN')
 

  
@@ -85,6 +90,56 @@ 
 

      return topo_m2
 

  

  

+ def test_ssl_transport(tls_client_auth):
 

+     """Test different combinations for nsDS5ReplicaTransportInfo values
 

+ 

+     :id: 922d16f8-662a-4915-a39e-0aecd7c8e6e2
 

+     :setup: Two master replication, enabled TLS client auth
 

+     :steps:
 

+         1. Set nsDS5ReplicaTransportInfoCheck: SSL or StartTLS or TLS
 

+         2. Restart the instance
 

+         3. Check that replication works
 

+         4. Set nsDS5ReplicaTransportInfoCheck: LDAPS back
 

+     :expectedresults:
 

+         1. Success
 

+         2. Success
 

+         3. Replication works
 

+         4. Success
 

+     """
 

+ 

+     m1 = tls_client_auth.ms['master1']
 

+     m2 = tls_client_auth.ms['master2']
 

+     repl = ReplicationManager(DEFAULT_SUFFIX)
 

+     replica_m1 = Replicas(m1).get(DEFAULT_SUFFIX)
 

+     replica_m2 = Replicas(m2).get(DEFAULT_SUFFIX)
 

+     agmt_m1 = replica_m1.get_agreements().list()[0]
 

+     agmt_m2 = replica_m2.get_agreements().list()[0]
 

+ 

+     if ds_is_older('1.4.0.6'):
 

+         check_list = (('TLS', False),)
 

+     else:
 

+         check_list = (('SSL', True), ('StartTLS', False), ('TLS', False))
 

+ 

+     for transport, secure_port in check_list:
 

+         agmt_m1.replace_many(('nsDS5ReplicaTransportInfo', transport),
 

+                              ('nsDS5ReplicaPort', '{}'.format(m2.port if not secure_port else m2.sslport)))
 

+         agmt_m2.replace_many(('nsDS5ReplicaTransportInfo', transport),
 

+                              ('nsDS5ReplicaPort', '{}'.format(m1.port if not secure_port else m1.sslport)))
 

+         repl.test_replication_topology(tls_client_auth)
 

+ 

+     if ds_is_older('1.4.0.6'):
 

+         agmt_m1.replace_many(('nsDS5ReplicaTransportInfo', 'SSL'),
 

+                              ('nsDS5ReplicaPort', str(m2.sslport)))
 

+         agmt_m2.replace_many(('nsDS5ReplicaTransportInfo', 'SSL'),
 

+                              ('nsDS5ReplicaPort', str(m1.sslport)))
 

+     else:
 

+         agmt_m1.replace_many(('nsDS5ReplicaTransportInfo', 'LDAPS'),
 

+                              ('nsDS5ReplicaPort', str(m2.sslport)))
 

+         agmt_m2.replace_many(('nsDS5ReplicaTransportInfo', 'LDAPS'),
 

+                              ('nsDS5ReplicaPort', str(m1.sslport)))
 

+     repl.test_replication_topology(tls_client_auth)
 

+ 

+ 

  def test_extract_pemfiles(tls_client_auth):
 

      """Test TLS client authentication between two masters operates
 

      as expected with 'on' and 'off' options of nsslapd-extract-pemfiles

file modified
+12 -6 
@@ -74,6 +74,8 @@ 
 

  #define DNA_PROT_LDAP "LDAP"
 

  #define DNA_PROT_TLS "TLS"
 

  #define DNA_PROT_SSL "SSL"
 

+ #define DNA_PROT_LDAPS "LDAPS"
 

+ #define DNA_PROT_STARTTLS "StartTLS"

Tiny nitpick but I think this should be LDAP+StartTLS instead of StartTLS. But that's just my opinion :) 

  

  /* For transferred ranges */
 

  #define DNA_NEXT_RANGE "dnaNextRange"
 
@@ -1901,8 +1903,10 @@ 
 

                      }
 

                      if (strcasecmp(server->remote_bind_method, DNA_METHOD_SSL) == 0) {
 

                          /* requires a bind DN */
 

-                         if (strcasecmp(server->remote_conn_prot, DNA_PROT_SSL) != 0 &&
 

-                             strcasecmp(server->remote_conn_prot, DNA_PROT_TLS) != 0) {
 

+                         if ((strcasecmp(server->remote_conn_prot, DNA_PROT_SSL) != 0 ||
 

+                              strcasecmp(server->remote_conn_prot, DNA_PROT_LDAPS) != 0) &&
 

+                             (strcasecmp(server->remote_conn_prot, DNA_PROT_TLS) != 0 ||
 

+                              strcasecmp(server->remote_conn_prot, DNA_PROT_STARTTLS) != 0)) {
 

                              reason = "bind method (SSL) requires either SSL or TLS connection "
 

                                       "protocol.";
 

                              err = 1;
 
@@ -3065,9 +3069,9 @@ 
 

          *port = slapi_entry_attr_get_int(entries[0], DNA_REPL_PORT);
 

  

          /* Check if we should use SSL */
 

-         if (transport && (strcasecmp(transport, "SSL") == 0)) {
 

+         if (transport && (strcasecmp(transport, DNA_PROT_SSL) == 0 || strcasecmp(transport, DNA_PROT_LDAPS) == 0)) {
 

              *is_ssl = 1;
 

-         } else if (transport && (strcasecmp(transport, "TLS") == 0)) {
 

+         } else if (transport && (strcasecmp(transport, DNA_PROT_TLS) == 0 || strcasecmp(transport, DNA_PROT_STARTTLS) == 0))  {
 

              *is_ssl = 2;
 

          } else {
 

              *is_ssl = 0;
 
@@ -3156,9 +3160,11 @@ 
 

          ;    /* just use it directly */
 

      }
 

  

-     if (server->remote_conn_prot && strcasecmp(server->remote_conn_prot, DNA_PROT_SSL) == 0) {
 

+     if (server->remote_conn_prot && (strcasecmp(server->remote_conn_prot, DNA_PROT_SSL) == 0 ||
 

+         strcasecmp(server->remote_conn_prot, DNA_PROT_LDAPS) == 0 )) {
 

          *is_ssl = 1;
 

-     } else if (server->remote_conn_prot && strcasecmp(server->remote_conn_prot, DNA_PROT_TLS) == 0) {
 

+     } else if (server->remote_conn_prot && (strcasecmp(server->remote_conn_prot, DNA_PROT_TLS) == 0 ||
 

+                strcasecmp(server->remote_conn_prot, DNA_PROT_STARTTLS) == 0 )) {
 

          *is_ssl = 2;
 

      } else {
 

          *is_ssl = 0;

file modified
+2 -2 
@@ -360,8 +360,8 @@ 
 

  /* In repl5_agmt.c */
 

  typedef struct repl5agmt Repl_Agmt;
 

  

- #define TRANSPORT_FLAG_SSL 1
 

- #define TRANSPORT_FLAG_TLS 2
 

+ #define TRANSPORT_FLAG_LDAPS 1
 

+ #define TRANSPORT_FLAG_STARTTLS 2
 

  #define BINDMETHOD_SIMPLE_AUTH 1
 

  #define BINDMETHOD_SSL_CLIENTAUTH 2
 

  #define BINDMETHOD_SASL_GSSAPI 3

file modified
+8 -8 
@@ -73,7 +73,7 @@ 
 

  {
 

      char *hostname;                        /* remote hostname */
 

      int64_t port;                          /* port of remote server */
 

-     uint32_t transport_flags;              /* SSL, TLS, etc. */
 

+     uint32_t transport_flags;              /* LDAPS, StartTLS, etc. */
 

      char *binddn;                          /* DN to bind as */
 

      struct berval *creds;                  /* Password, or certificate */
 

      int64_t bindmethod;                    /* Bind method - simple, SSL */
 
@@ -148,7 +148,7 @@ 
 

  cn
 

  nsds5ReplicaHost - hostname
 

  nsds5ReplicaPort - port number
 

- nsds5ReplicaTransportInfo - "SSL", "startTLS", or may be absent;
 

+ nsds5ReplicaTransportInfo - "LDAPS", "StartTLS", or may be absent ("SSL" and "TLS" values will be deprecated later)
 

  nsds5ReplicaBindDN
 

  nsds5ReplicaCredentials
 

  nsds5ReplicaBindMethod - "SIMPLE" or "SSLCLIENTAUTH".
 
@@ -207,7 +207,7 @@ 
 

      if ((0 == ra->transport_flags) && (BINDMETHOD_SSL_CLIENTAUTH == ra->bindmethod)) {
 

          slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "agmt_is_valid - Replication agreement \"%s\" "
 

                                                         " is malformed: cannot use SSLCLIENTAUTH if using plain LDAP - please "
 

-                                                        "change %s to SSL or TLS before changing %s to use SSLCLIENTAUTH\n",
 

+                                                        "change %s to LDAPS or StartTLS before changing %s to use SSLCLIENTAUTH\n",
 

                        slapi_sdn_get_dn(ra->dn), type_nsds5TransportInfo, type_nsds5ReplicaBindMethod);
 

          return_value = 0;
 

      }
 
@@ -298,7 +298,7 @@ 
 

          ra->port = port;
 

      }
 

  

-     /* SSL, TLS, or other transport stuff */
 

+     /* LDAPS, StartTLS, or other transport stuff */
 

      ra->transport_flags = 0;
 

      (void)agmt_set_transportinfo_no_lock(ra, e);
 

      (void)agmt_set_WaitForAsyncResults(ra, e);
 
@@ -1755,10 +1755,10 @@ 
 

      tmpstr = slapi_entry_attr_get_charptr(e, type_nsds5TransportInfo);
 

      if (!tmpstr || !strcasecmp(tmpstr, "LDAP")) {
 

          ra->transport_flags = 0;
 

-     } else if (strcasecmp(tmpstr, "SSL") == 0) {
 

-         ra->transport_flags = TRANSPORT_FLAG_SSL;
 

-     } else if (strcasecmp(tmpstr, "TLS") == 0) {
 

-         ra->transport_flags = TRANSPORT_FLAG_TLS;
 

+     } else if (strcasecmp(tmpstr, "SSL") == 0 || strcasecmp(tmpstr, "LDAPS") == 0) {
 

+         ra->transport_flags = TRANSPORT_FLAG_LDAPS;
 

+     } else if (strcasecmp(tmpstr, "TLS") == 0 || strcasecmp(tmpstr, "StartTLS") == 0) {
 

+         ra->transport_flags = TRANSPORT_FLAG_STARTTLS;
 

      }
 

      /* else do nothing - invalid value is a no-op */

file modified
+2 -2 
@@ -1177,9 +1177,9 @@ 
 

      /* ugaston: if SSL has been selected in the replication agreement, SSL client
 

       * initialisation should be done before ever trying to open any connection at all.
 

       */
 

-     if (conn->transport_flags == TRANSPORT_FLAG_TLS) {
 

+     if (conn->transport_flags == TRANSPORT_FLAG_STARTTLS) {
 

          secure = SLAPI_LDAP_INIT_FLAG_startTLS;
 

-     } else if (conn->transport_flags == TRANSPORT_FLAG_SSL) {
 

+     } else if (conn->transport_flags == TRANSPORT_FLAG_LDAPS) {
 

          secure = SLAPI_LDAP_INIT_FLAG_SSL;
 

      }

file modified
+2 -2 
@@ -1212,9 +1212,9 @@ 
 

      /* ugaston: if SSL has been selected in the replication agreement, SSL client
 

       * initialisation should be done before ever trying to open any connection at all.
 

       */
 

-     if (conn->transport_flags == TRANSPORT_FLAG_TLS) {
 

+     if (conn->transport_flags == TRANSPORT_FLAG_STARTTLS) {
 

          secure = SLAPI_LDAP_INIT_FLAG_startTLS;
 

-     } else if (conn->transport_flags == TRANSPORT_FLAG_SSL) {
 

+     } else if (conn->transport_flags == TRANSPORT_FLAG_LDAPS) {
 

          secure = SLAPI_LDAP_INIT_FLAG_SSL;
 

      }

Bug Description: nsDS5ReplicaTransportInfo SSL vs TLS is not really clear,
given that most libraries now support TLS as the default "SSL".
We should make this clear in nsDS5ReplicaTransportInfo by allowing:
ldaps -> SSL
StartTLS -> TLS
options. So that it's really clear what you are asking for when you configure it.

Fix Description: Add additional options for nsDS5ReplicaTransportInfo - LDAPS and StartTLS.
Legacy options will stay for some time and will deprecated in later version.
Also, change the DNA plugin values in the same way.
And rename replica flags through the code base as followed:
TRANSPORT_FLAG_SSL -> TRANSPORT_FLAG_LDAPS
TRANSPORT_FLAG_TLS -> TRANSPORT_FLAG_STARTTLS

https://pagure.io/389-ds-base/issue/49109

Reviewed by: ?

Discussion in the #49109 is still going. I've used original option ssl->ldaps and tls->starttls for now.
It is a draft of the patch, we can change it later if we'll agree so.

I'll add a test also a bit later.

Tiny nitpick but I think this should be LDAP+StartTLS instead of StartTLS. But that's just my opinion :)

There are tests in replication with TLS we could use as an inspiration for this?

I've already tested the patch for replica agreement (I used suites/replication/tls_client_auth_repl_test.py for this).
I will create parametrization there for both options and update the pull-request.
I want to be 100% that I don't break anything here. :D

And regarding LDAP+StartTLS. I am okay with it, though I think StartTLS is also pretty understandable enough (we can't have LDAPS+StartTLS, it is against the concept).

Let's see what others think and I can change it accordingly. :)
@mreynolds @lkrispen @tbordaz @mhonek

I think it is a good idea. LDAP_StartTLS is also an option.
Should we accept StartTLS+LDAP and LDAP+StartTLS (or StartTLS_LDAP and LDAP_StartTLS)

rebased onto 3c3ea338888ce46ef98c0cab96911ee960d281c5
6 years ago

As William and Matus proposed, I've changed nsDS5ReplicaTransportInfo: TLS to LDAP+StartTLS.
Also, I've changed existed TLS test suite so it covers the change.

We are still in the decision process. So if we will agree with Theirry's option, I'll change the logic accordingly.

rebased onto 21a8f416a962d77306516b1b4e4343234421e50a
6 years ago

Tiny nitpick is DNA_PROT_LDAP_STARTTLS should be the name given the previous scheme used of the macros.

Beside my tiny nitpick, I think this looks good. Ack from me. (Note I haven't built this or run the test).

@spichugi ping, this is pretty easy to follow up and finish :)

rebased onto 3b132ef0742c9ff70e47344c54ef734805e34954
6 years ago

I have a problem with LDAP+StartTLS. First, this implies the order does not matter: It could also be StartTLS+LDAP. This requires more logic to process the setting. But the main problem I have is that StartTLS implies LDAP. For example, you would never use LDAPS+StartTLS. If you can set "LDAP+StartTLS", then as a end user, who is inexperienced, I would think that LDAPS+StartTLS is also a valid option. It's confusing because it's redundant. I think for clarity & simplicity it should just be "StartTLS" (case insensitive).

rebased onto f6ef311cf35aa21933977d3f53cbfca2bac4bd25
6 years ago

rebased onto 6b05fa750d203478d7566078927d8aa2ff2e71b5
6 years ago

LGTM as long as firstyear's comment about DNA_PROT_LDAP_STARTTLS was addressed (since the PR was rebased I don't know what he was talking about or if you addressed it).

LGTM as long as firstyear's comment about DNA_PROT_LDAP_STARTTLS was addressed (since the PR was rebased I don't know what he was talking about or if you addressed it).

I changed it back too because of the LDAP+StartTLS -> StartTLS change.

Before his comment it was:

+#define DNA_PROT_LDAPS "LDAPS"
+#define DNA_PROT_STARTTLS "LDAP+StartTLS"

William asked for:

+#define DNA_PROT_LDAPS "LDAPS"
+#define DNA_PROT_LDAP_STARTTLS "LDAP+StartTLS"

But we changed LDAP+StartTLS so I changed it back again (it is now like this):

+#define DNA_PROT_LDAPS "LDAPS"
+#define DNA_PROT_STARTTLS "StartTLS"

I can change it back to this, no problem:

+#define DNA_PROT_LDAPS "LDAPS"
+#define DNA_PROT_LDAP_STARTTLS "StartTLS"

But it seems a bit redundant to me.

I see, yes I like it the way you have it now. I also see why William did it the other way (to be consistent with the "LDAP+StartTLS" naming), but I agree it is redundant and I like what you have now.

Fine by me :) thanks for taking it into consideration. Ack.

rebased onto ea033b6
6 years ago

Pull-Request has been merged by spichugi
6 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2672

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Flags
Build
success (100%)
Completed
6 years ago
Changes Summary 6
+59 -4
file changed
dirsrvtests/tests/suites/replication/tls_client_auth_repl_test.py
+12 -6
file changed
ldap/servers/plugins/dna/dna.c
+2 -2
file changed
ldap/servers/plugins/replication/repl5.h
+8 -8
file changed
ldap/servers/plugins/replication/repl5_agmt.c
+2 -2
file changed
ldap/servers/plugins/replication/repl5_connection.c
+2 -2
file changed
ldap/servers/plugins/replication/windows_connection.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.