I agree that the definition and implementation are inconsistent. But the current singlke string attr is also hard to read and enforcing it would require a schema change.

couldn't we make multivalued work instead ? as fat as I see in slapd_ssl_init we copy the attr value to a cipher_string. Couldn't we just concatenate the values and proceed like with a single value ?

I agree with @lkrispen. Lets make it work for multivalued, and just retain the ability to parse the "all on one line" version.

And as an extension, we should install it as multivalued by default - perhaps we can add a migration ni fedse.c (which is used early in boot) to convert single -> multivalued.

multivalue makes more sense from an administration perspective because then we can ldap modify these values a bit more sensibly.

IMHO, I prefer keeping it simple (in code, and partially user facing features), but am not going to push hard either. :)

If I missed something in your suggestions, guys, please bear with me.

I agree being multi-valued makes it more readable. Apart from at least one more for in the code, we have potential issues with current behaviour of the special keywords default and +all:

• default is parsed only if being declared solely. What if there are more values for the attribute? (OTOH, I guess it might be as benevolently used as the +all keyword is).
• +all is parsed only at the beginning of the line. Given the values of the attribute would be unordered, we don't know where is the start.

Given we go with multi-valued, +/- does not make sense any more (e.g. order would matter, because "-cipherX, then +cipherX" yields different result than "+cipherX, then -cipherX" does. Or should we count the individual + and - occurrences? Also, default is destructive when being set.

Only union of results of resolving individual values (IIUC this is what @firstyear meant) would make sense, keeping backwards compatibility. Which is doable. Which also adds a magnitude of complexity and a need for a temporary array in the code. We would need to cease from doing side effects in the for loops and rather do it at the very end of the function (which is good!). And additionally, probably we'd need to read the state of NSS cipher settings at the beginning to begin with the state we'd otherwise have when setting the ciphers directly. Finally, it would be even nicer to not enforce the obligatory + and account for it implicitly when there is no -.

In the end, the actual effect is quite nice, but will take some man hours (and I won't do it without proper tests! ;)) to change the whole lot.

Please, let me know what you think.

@mhonek It's much much better to invest the time today to do this correctly, than to "half bake" it and have to maintain some more techdebt for along time.

I'd encourage you not only to implement this change, but to change dse.ldif by default to use the multivalued syntax. Then over the course of "years" when people upgrade the old syntax phases out.

For bonus marks you can always have it so that if you detect the "old" syntax, you parse it into multivalued instead ;)

Thanks!

@lkrispen Just for clarity, as I have explained in my previous comment, pure concatenation is not an option because subtraction is anti-cummutative which is an issue if we cannot ensure order of the values set.

@firstyear I guess we are misunderstanding each other. I definitely don't want to make any half baked solutions.

dse.ldif by default does not contain any mention of nsSSL3Ciphers attribute. What to change there then?

What I suggested is this: keep parsing as we did so far, do that for each attribute value, and finally make an union of these evaluated values effective. We wouldn't have to migrate anything. Additionally, if we make + sign optional, we can let users set values to be solely names of the ciphers as well. Is this the "half baked" and incorrect solution? If so, why? What do you suggest to do differently?

@firstyear I'm not sure I understand what solution you meant; I probably don't get what other changes you meant apart from taking all the values of the multi-valued attribute into account. Is it such that only names of the ciphers would be accepted as attribute values (after migration, of course)? If so I have at least one thing which would be missing: removing particular ciphers (let's say I want default but not cipher A, there wouldn't be a way to do this; and I guess it might be a common requirement from an administrator's management who read that in some article somewhere..). And even if we allowed for - before a cipher name we would fail due to anti-commutativity described explained above. Please, make your suggestion more clear for my head is not clever enough. :)

Thank you.

@firstyear Any ideas regarding what I wrote above? :) Thanks.

An idea popped up we could create a new attribute (like nsTLSCiphers) with a desired behaviour and deprecate the nsSSL3Ciphers converting the configuration accordingly with a migration code.

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

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.