Hi Noriko,

{{{
(void)getSSLVersionRangeOL(&optval, NULL);
}}}

But you have :

{{{
int
getSSLVersionRangeOL(int min, int max)
}}}

Followed by:

{{{
if ((rc = ldap_set_option(ld, LDAP_OPT_X_TLS_PROTOCOL_MIN, &optval))) {
}}}

Can you check this return?

Perhaps:

{{{
int olresult = getSSLVersionRangeOL(&optval, NULL);
...
if (olresult == 0 && (rc = ldap_set_option(ld, LDAP_OPT_X_TLS_PROTOCOL_MIN, &optval))) {
}}}

Also, your message in the error says:

{{{
"failed: unable to set minimum TLS protocol level to SSL3\n");
}}}

But doesn't this change allow us to set other protocol minimums? Perhaps this error should be generic, rather than saying SSL3?

Otherwise, I really like this patch. I think it's important we are able to control this.

Could we perhaps also have the option to use https://fedoraproject.org/wiki/Changes/CryptoPolicy ?? Does this apply here or not? It would be good to have this controlled by the system policy by default, and then can be overriden if we want ... Maybe this is a future extension to this ticket?

I have not built or tested this yet.

Thanks for your time!

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

If you don't like it, I could change getSSLVersionRangeOL to void and remove the SSL init check...

I'm not so sure about the system policy yet. We may need to have some controle independent from the system itself. For instance, we may need to enable SSLv3 when the replication consumer only supports the SSL version, but the system itself does not want to enable it... Probably, getting the default value from the system policy and still being able to have a control to override it if necessary, everyone could be happier... And yes, it'd be better to open a new ticket for it.

git patch file (master) -- revised based upon the reviews by William (Thanks!)
0001-Ticket-48784-Make-the-SSL-version-set-to-the-client-.patch

Replying to [comment:3 nhosoi]:

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

Actually I thought TLS 1.0 was NOT safe from POODLE - only 1.1 and up was safe.

Replying to [comment:3 nhosoi]:

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

If you don't like it, I could change getSSLVersionRangeOL to void and remove the SSL init check...

Well, if we return the int, we should be doing something with it. So I think either we check it to confirm a version was found, or we remove it and make the function void, if it is guaranteed to never fail.

I'm not so sure about the system policy yet. We may need to have some controle independent from the system itself. For instance, we may need to enable SSLv3 when the replication consumer only supports the SSL version, but the system itself does not want to enable it... Probably, getting the default value from the system policy and still being able to have a control to override it if necessary, everyone could be happier... And yes, it'd be better to open a new ticket for it.

Probably a new ticket for it I think. It will probably be a bit of investigation to make it work.

Actually I thought TLS 1.0 was NOT safe from POODLE - only 1.1 and up was safe.

I think Mark is right here. By default we should be setting the highest TLS version we can, because often the version changes are due to actual protocol weaknesses in TLS itself. IE we should default to TLS1.2 if we can.

The answer from the security team.

On 04/04/2016 10:26 PM, Huzaifa Sidhpurwala wrote:

Currently, we are not aware of any attacks which are feasible against a
proper implementation of TLS 1.0 (openssl, nss, gnutls we ship). However
that being said, the safest option is always to use the highest version
available ie TLS 1.2 and fall back to lower versions only, if you cant
use 1.2.

The above is general advice in all cases. If you have a special case in
mind, let me know and we can discuss.

My answer is based on the bits of information i got from the mail i was
copied on :)

This is the access log snippet of the replication. As you see, even though the min value is TLS1.0 (or even setting to SSL3), the higherst available version is picked. So, we may not have to worry too much about it.
{{{
[..] conn=3 TLS1.2 128-bit AES-GCM; client CN=test.localdomain0,OU=389 Directory Server; issuer CN=CAcert
[..] conn=3 TLS1.2 client bound as uid=repl_mgr1,cn=config
}}}

git patch file (master) -- CI test
0002-Ticket-48784-CI-test-added-test-cases-for-ticket-487.patch

Code looks good, runs on my machine, and passes. Ack.

Reviewed by William (Thanks!!)

Pushed to master:
4c66307..fa620fc master -> master
commit 0be073f
commit f295342

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1844

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.