This is the first patch of multiple fixing an initial set of these warnings.

Cancel this, this patch has a flaw.

commit 161cc23
To ssh://git@pagure.io/389-ds-base.git
3bf56ee..161cc23 master -> master

@mreynolds This patch should resolve the CI warn we have had lately. Sorry it took me so long.

Ack!

commit f2b43de
To ssh://git@pagure.io/389-ds-base.git
c3da727..f2b43de master -> master

You added a duplicate code block under: case SLAPD_SSLCLIENTAUTH_REQUIRED

Yes, this was intentional. The reason I made the mistake in the first place was because of the extremely similar variable name we are setting in SSL option, and because of the fall through behaviour.

While duplicating this may seem silly, it makes the code more readable and clear - you can now see there are a pair of options that directly affect the behaviour of this setting, rather than the murky implementation that was there before I think.

This is all because William-of-the-future will be silly and will surely make the mistake again :)

They are identical accept for the logging line, which is actually wrong as well. In the duplicate code the log function says a FALSE option is reported, but in fact TRUE was used:

57 if ((err = SSL_OptionSet(pr_sock, SSL_REQUEST_CERTIFICATE, PR_TRUE)) < 0) {
58 PRErrorCode prerr = PR_GetError();
59 slapi_log_err(SLAPI_LOG_ERR, "Security Initialization",
60 "SSL_OptionSet(SSL_REQUEST_CERTIFICATE,PR_TRUE) %d " SLAPI_COMPONENT_NAME_NSPR " error %d (%s)\n",
61 err, prerr, slapd_pr_strerror(prerr));
62 }
63 + if ((err = SSL_OptionSet(pr_sock, SSL_REQUIRE_CERTIFICATE, PR_TRUE)) < 0) {
64 + PRErrorCode prerr = PR_GetError();
65 + slapi_log_err(SLAPI_LOG_ERR, "Security Initialization",
66 + "SSL_OptionSet(SSL_REQUIRE_CERTIFICATE,PR_FALSE) %d " SLAPI_COMPONENT_NAME_NSPR " error %d (%s)\n",

We say it's FALSE, but actually we used TRUE

67 + err, prerr, slapd_pr_strerror(prerr));
68 + }

I still don't understand why you need duplicate code, and I think it is still a mistake. Look at everything under the switch option: case SLAPD_SSLCLIENTAUTH_REQUIRED

There are two back to back code blocks that are identical - except for the logging message which is wrong anyway.

@mreynolds I missed the log message you are correct.

Before the behaviour relied on:

• The assumed default of NSS (required == false when you set request == true)
• The fall through behaviour of the case switch:

case request:
     request(true) ...
case required:
     required(true) ...

Now, if you set required, we don't set request but we have to.

This behaviour is totally unclear. When you switch on the flag why is request and required taken when required is true? Does required imply request in nss (nss docs say it does not, but the behaviour here implies otherwise)? This is why I introduced the mistake in the first place was that I could not see what was going on.

I know that I'm duplicating code, but it makes the behaviour completely explicit and clear - no assumed nss options, no case-switch magic. It's 100% outlined what path was taken and options set, and why.

So I'll fix the log message, but please lets keep the duplication for clarities sake.

I think the original code was an example of simplification and reduction of code by using knowledge of nss behaviour and case fall thru, this all breaks once the nss behaviour changes or if you mess up the switch by adding a break.
For clarity I don't mind to have some code duplication, but if you want to have no duplication we could replace the switch by ifs

if (request||required)
    request(true)
    if (required)
        required(true)
    else
        required(false)

Sorry I think everyone is missing what I am trying to say. After I apply the patch this is the code in ssl.c

       case SLAPD_SSLCLIENTAUTH_REQUIRED:
            /* Give the client a clear opportunity to send her certificate: */
            /*
             * REQUEST is true
             * REQUIRED is true
             */
            if ((err = SSL_OptionSet(pr_sock, SSL_REQUEST_CERTIFICATE, PR_TRUE)) < 0) {
                PRErrorCode prerr = PR_GetError();
                slapi_log_err(SLAPI_LOG_ERR, "Security Initialization",
                              "SSL_OptionSet(SSL_REQUEST_CERTIFICATE,PR_TRUE) %d " SLAPI_COMPONENT_NAME_NSPR " error %d (%s)\n",
                              err, prerr, slapd_pr_strerror(prerr));
            }
            if ((err = SSL_OptionSet(pr_sock, SSL_REQUIRE_CERTIFICATE, PR_TRUE)) < 0) {
                PRErrorCode prerr = PR_GetError();
                slapi_log_err(SLAPI_LOG_ERR, "Security Initialization",
                              "SSL_OptionSet(SSL_REQUIRE_CERTIFICATE,PR_FALSE) %d " SLAPI_COMPONENT_NAME_NSPR " error %d (%s)\n",
                              err, prerr, slapd_pr_strerror(prerr));
            }
            break;
        default:
            break;
        }
    }

We now set the same SSL option twice, why?

Ahhhh, nevermind!! They are different, ugh, taking foot out of mouth...

ack

@mreynolds This is exactly what tripped me up too! I thought they were the same option! Maybe I should add a huge comment there because we both made this mistake :)

@mreynolds This is exactly what tripped me up too! I thought they were the same option! Maybe I should add a huge comment there because we both made this mistake :)

That makes me feel slightly less stupid :-p

commit 506abaa9a89256ca9c86506cc30d20a70f5c5b29
To ssh://git@pagure.io/389-ds-base.git
16aeef6..01272f7 master -> master

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/2334

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.