I'm working on a patch and test case for this.

Hey mate, thanks for your patch. It looks really good. Of course, I have some comments though.

I think the parameter nsSSLRenegotiate isn't a good name. We should probably call this either nsTLSAllowClientRenegotiation to make it clearer as to the intent of the variable.

You do some manual LDAP setup for TLS on the server. Perhaps look at this test case as an idea to make this briefer? https://pagure.io/lib389/blob/master/f/lib389/tests/tls_external_test.py#_21

Note the use of the rsa/config modules - they accept a generic get/set (attr, val) api, so this should make things nicer for you.

Try to avoid the NSPR PRtypes. I wish to remove them from the server. I'd rather see this as int_fast16_t or similar.

With your check you have:

200 +        if (!PL_strcasecmp(val, "off")) {
201 +            renegotiation = SSL_RENEGOTIATE_NEVER;
202 +        } else if (PL_strcasecmp(val, "on")) {
203 +            slapd_SSL_warn("The value of nsSSLRenegotiate (\"%s\") is invalid."
204 +                           " Ignoring it and setting it to default.", val);
205 +        }

The issue with this is that what if the value is "bob"?. If it was me, I would make this logic:

if (val == 'on') {
} else {
    // assume off
}

Finally, can we give the 01core389.ldif schema DESC a better value? Describe briefly what the atr controls, IE "this controls allowing RFC 5746 client renegotiation" would suffice :) we plan to expose these desc's via the CLI later as help basically.

I hope that these comments help you, and again, thanks for your work on this feature!

New version of the patch:

• Renamed the attribute nsTLSAllowClientRenegotiation as you suggested, and put in a useful description in the schema definition.
• Changed the accepted values of the attribute to "yes" and "no" to fit better with the "allow" naming.
• Changed the PRBool to an int_fast16_t, with the appropriate casts.
• Rewrote the test case using the LDAP config APIs you mentioned, it's much nicer using those.

I've not changed the check logic because I think it's correct as-is, but I've put a comment block in to make it clearer what I'm trying to do. An invalid value results in the default setting being used (allow reneg), and the warning message, and this is tested for in the test case.

Thanks for the feedback!

commit 7c9c39e
To ssh://git@pagure.io/389-ds-base.git
7efc2d2..7c9c39e master -> master

Thanks for the patch, it's much appreciated!

Issue in CI test broken jenkins nightly CI testing.

a8628e2..1301db4 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/2362

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.