#49231 nsslapd-allowed-sasl-mechanisms doesn't reset to default values without a restart
Closed: wontfix 6 years ago Opened 6 years ago by spichugi.

Description of problem:
When we have some value in nsslapd-allowed-sasl-mechanisms attribute of cn=config and if we'll delete the attribute (which will set it to the default), it will be set to supportedSASLMechanisms: EXTERNAL only. Though after a restart we will see a full set of attributes.

Version-Release number of selected component (if applicable):
389-ds-base-1.3.6.1-9.el7.x86_64

How reproducible:
Always

Steps to Reproduce:
1. Check the supportedSASLMechanisms on a clean instance with a default nsslapd-allowed-sasl-mechanisms:
[root@qeos-236 tests]# ldapsearch -h localhost -p 389 -D "cn=directory manager" -w Secret123 -b "" -s base supportedSASLMechanisms
dn:
supportedSASLMechanisms: EXTERNAL
supportedSASLMechanisms: GSS-SPNEGO
supportedSASLMechanisms: GSSAPI
supportedSASLMechanisms: DIGEST-MD5
supportedSASLMechanisms: CRAM-MD5
supportedSASLMechanisms: LOGIN
supportedSASLMechanisms: PLAIN
supportedSASLMechanisms: ANONYMOUS

  1. Set some particular SASL mechanism:
    [root@qeos-236 tests]# ldapmodify -h localhost -p 389 -D "cn=directory manager" -w Secret123
    dn: cn=config
    changetype: modify
    replace: nsslapd-allowed-sasl-mechanisms
    nsslapd-allowed-sasl-mechanisms: DIGEST-MD5

modifying entry "cn=config"

  1. Check the supportedSASLMechanisms:
    [root@qeos-236 tests]# ldapsearch -h localhost -p 389 -D "cn=directory manager" -w Secret123 -b "" -s base supportedSASLMechanisms
    dn:
    supportedSASLMechanisms: EXTERNAL
    supportedSASLMechanisms: DIGEST-MD5

  2. Reset to default by deleting the attribute:
    [root@qeos-236 tests]# ldapmodify -h localhost -p 389 -D "cn=directory manager" -w Secret123
    dn: cn=config
    changetype: modify
    delete: nsslapd-allowed-sasl-mechanisms

modifying entry "cn=config"

  1. Check the supportedSASLMechanisms once againg:
    [root@qeos-236 tests]# ldapsearch -h localhost -p 389 -D "cn=directory manager" -w Secret123 -b "" -s base supportedSASLMechanisms
    dn:
    supportedSASLMechanisms: EXTERNAL

  2. Restart the instance:
    [root@qeos-236 tests]# restart-dirsrv
    Restarting instance "qeos-236"

  3. Check the supportedSASLMechanisms:
    [root@qeos-236 tests]# ldapsearch -h localhost -p 389 -D "cn=directory manager" -w Secret123 -b "" -s base supportedSASLMechanisms
    dn:
    supportedSASLMechanisms: EXTERNAL
    supportedSASLMechanisms: GSS-SPNEGO
    supportedSASLMechanisms: GSSAPI
    supportedSASLMechanisms: DIGEST-MD5
    supportedSASLMechanisms: CRAM-MD5
    supportedSASLMechanisms: LOGIN
    supportedSASLMechanisms: PLAIN
    supportedSASLMechanisms: ANONYMOUS

Actual results:
On a step 5, we see only supportedSASLMechanisms: EXTERNAL.

Expected results:
On a step 5, we should see a full set of supportedSASLMechanisms.

Additional info:
Issue is covered by TET.
sasl_allowed_mechanisms_8 in sasl test suite.


Metadata Update from @spichugi:
- Custom field type adjusted to defect
- Issue assigned to firstyear

6 years ago

Metadata Update from @spichugi:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1444938

6 years ago

Metadata Update from @spichugi:
- Custom field component adjusted to Directory Server
- Custom field origin adjusted to QE
- Custom field version adjusted to 1.3.6

6 years ago

It looks like there are two parts to the issue

First, is that config_set_allowed_sasl_mechs does not correctly handle the reset case.

The second is that the rootdse, is not reading the correct set of allowed mechs.

Metadata Update from @firstyear:
- Custom field reviewstatus adjusted to review

6 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to ack (was: review)

6 years ago

commit 6395357
To ssh://git@pagure.io/389-ds-base.git
565314c..6395357 master -> master

Metadata Update from @firstyear:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

6 years ago

Hi William,
I have found some strange behaviour (the test in the commit were failing).
Build tested: 389-ds-base-1.3.6.1-13.el7

  1. After we install the server we have a default set of supportedSASLMechanisms. And the test assumes that we have PLAIN. But we do not:

    assert('PLAIN' in orig_mechs) 
    E       AssertionError: assert 'PLAIN' in ['EXTERNAL', 'GSS-SPNEGO', 'GSSAPI', 'DIGEST-MD5', 'CRAM-MD5', 'ANONYMOUS']
    
  2. When we set nsslapd-allowed-sasl-mechanisms to PLAIN (manualy or through the test):

    [root@qeos-164 ds]# ldapmodify -h localhost -p 389 -D "cn=directory manager" -w Secret123
    dn: cn=config
    changetype: modify
    replace: nsslapd-allowed-sasl-mechanisms
    nsslapd-allowed-sasl-mechanisms: PLAIN
    

We don't have it in supportedSASLMechanisms, and we don't have EXTERNAL either.
Though from the docs: "Hence, the EXTERNAL mechanism cannot be restricted or controlled. It will always appear in the supported mechanisms list, regardless what is set in the nsslapd-allowed-sasl-mechanisms attribute."

So my question is:
Was there any change in the supportedSASLMechanisms feature that we miss? If so, we need to adjust the test case.
If not and the documentation is right, we have a regression.


Build tested:
389-ds-base-1.3.7.0-20170509git6fc82a3.fc25.x86_64

On the master branch it is better (it has supportedSASLMechanisms: PLAIN as a default and we can set it), but still, it possibly has a flaw.
As I said before, documentation says that supportedSASLMechanisms: EXTERNAL should be present with any nsslapd-allowed-sasl-mechanisms value.

And we dont have it with PLAIN:

ldapmodify -h localhost -p 389 -D "cn=directory manager" -w Secret123
dn: cn=config
changetype: modify
replace: nsslapd-allowed-sasl-mechanisms
nsslapd-allowed-sasl-mechanisms: PLAIN

modifying entry "cn=config"

ldapsearch -h localhost -p 389 -D "cn=directory manager" -w Secret123 -b "" -s base supportedSASLMechanisms

dn:
supportedSASLMechanisms: PLAIN

So, supportedSASLMechanisms: EXTERNAL is not present.

Metadata Update from @spichugi:
- Issue status updated to: Open (was: Closed)

6 years ago
nsslapd-allowed-sasl-mechanisms: PLAIN

^ If you, the admin says "only allow PLAIN", then this is exactly what it's doing. We are only allowing PLAIN. EXTERNAL may be supported on all connections, but if you take it out of the allowed list, this is the result.

We can't have both "implicit" mechanisms, and a whitelist at the same time - they contradict. So we need to decide if the correct behaviour is to "force" certain default mechanisms, or if we are to respect our configuration whitelist.

An aspect of this issue, is that the allowed sasl mech list never worked properly with the check on the rootDSE - now it does. So this has likely always been the case, but now we are actually able to perceive it. Previously it would lie and say a connection supports X, Y, Z, but in reality supported non.

My vote is that this is the correct behaviour of the feature. :)

^ If you, the admin says "only allow PLAIN", then this is exactly what it's doing. We are only allowing PLAIN. EXTERNAL may be supported on all connections, but if you take it out of the allowed list, this is the result.
We can't have both "implicit" mechanisms, and a whitelist at the same time - they contradict. So we need to decide if the correct behaviour is to "force" certain default mechanisms, or if we are to respect our configuration whitelist.
An aspect of this issue, is that the allowed sasl mech list never worked properly with the check on the rootDSE - now it does. So this has likely always been the case, but now we are actually able to perceive it. Previously it would lie and say a connection supports X, Y, Z, but in reality supported non.
My vote is that this is the correct behaviour of the feature. :)

This part is collective developer decision. I am only saying that it is not expected behaviour for now. The documentation has very explicit information:
"Hence, the EXTERNAL mechanism cannot be restricted or controlled. It will always appear in the supported mechanisms list, regardless what is set in the nsslapd-allowed-sasl-mechanisms attribute."
https://access.redhat.com/documentation/en-US/Red_Hat_Directory_Server/10/html-single/Configuration_Command_and_File_Reference/index.html#nsslapd-allowed-sasl-mechanisms

So we need to modify it if we will decide to change the behaviour.

Besides that, William, my main point is the RHEL 7.4 regression we have. The test from the ticket is failing on 389-ds-base-1.3.6.1-13.el7. I've described it in the beginning of my previous message.
Shortly, there are two issues:
- PLAIN is not in the default list.
- PLAIN isn't added to supportedSASLMechanisms via nsslapd-allowed-sasl-mechanisms: PLAIN -> we have blank supportedSASLMechanisms.

We need to keep EXTERNAL in the supported list. The cat is out of the bag, this is how its been for many releases now - we need to keep it for backwards compatibility. I think in 1.4 it is something we could change, but for now it should not.

On a side note - in my testing the source code used in 1.3.6.1-13.el7 does show PLAIN as a supported mechanism. I did not test the rpm though (I need a lab machine first).

Okay, it looks like this would work.

        supported = slapi_get_supported_saslmechanisms_copy();
        if ( (pmech = supported) != NULL ) while (1) {
            if (*pmech == NULL) {
                /*
                 * As we call the safe function, we receive a strdup'd saslmechanisms
                 * charray. Therefore, we need to remove it instead of NULLing it
                 */
                charray_free(supported);
                pmech = supported = NULL;
                break;
            }
            if (!strcasecmp (saslmech, *pmech)) break;
            ++pmech;
        }

Because the call to slapi_get_supported_saslmechanisms DOES NOT check the allowed list, this means that the allowed list .... basically does nothing.

ldap/servers/slapd/proto-slap.h:556:char **config_get_allowed_sasl_mechs_array(void);
ldap/servers/slapd/saslbind.c:797:    config_ret = config_get_allowed_sasl_mechs_array();
ldap/servers/slapd/libglobs.c:7089:config_get_allowed_sasl_mechs_array(void)

In fact, it's only checked in ids_sasl_listmech, and that in turn:

ldap/servers/slapd/rootdse.c:213:   if (( strs = ids_sasl_listmech (pb)) != NULL ) {
ldap/servers/slapd/proto-slap.h:1469:char **ids_sasl_listmech(Slapi_PBlock *pb);
ldap/servers/slapd/saslbind.c:754:char **ids_sasl_listmech(Slapi_PBlock *pb)
ldap/servers/slapd/saslbind.c:765:    slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_listmech", "=>\n");
ldap/servers/slapd/saslbind.c:784:                slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_listmech", "sasl library mechs: %s\n", str);
ldap/servers/slapd/saslbind.c:810:    slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_listmech", "<=\n");

ids_sasl_listmech only is called from rootdse.

So actually, our sasl allowed config option .... does literally nothing. It does not disallow anything.

The bigger issue now is do we bother keeping allowed sasl mechs? Or throw it out because it doesn't do anything and just complicates the code. I'm in favour to throw it out, because admins can leave it on the cn=config, where it has always (and will continue) to do nothing. I'm also all for reducing our configuration complexity too.

So here is the shortlist of options

  • Remove nsslapd-sasl-allowed-mechanisms as an option
  • Fix the option to enforce the whitelist of options, and it is the complete list
  • Fix the option to enforce the whitelist of options, and EXTERNAL is always allowed even if excluded.

Keep in mind: all current deployments this option does not exclude anything, so some sites may be relying on the broken behaviour.

My vote is to remove the option as it does nothing - this maintains the current behaviour of the server, and reduces our configuration complexity.

My understanding of nsslapd-sasl-allowed-mechanisms is that it provides a libsasl callback (SASL_CB_GETOPT) of the mechanisms supported by DS, independently of the mechanisms supported by libsasl.
should nsslapd-sasl-allowed-mechanisms be enforced at the libsasl level or at DS level, I do not know.

Regarding the set/reset of this value, it may exists bugs like the one showed in the initial description of the problem.

Regarding coherency between "supportedSASLMechanisms" lookup (supported_saslmechanisms array) and nsslapd-sasl-allowed-mechanisms, I believe it should return something like supported_saslmechanisms-nsslapd-sasl-allowed-mechanisms

My understanding of nsslapd-sasl-allowed-mechanisms is that it provides a libsasl callback (SASL_CB_GETOPT) of the mechanisms supported by DS, independently of the mechanisms supported by libsasl.
should nsslapd-sasl-allowed-mechanisms be enforced at the libsasl level or at DS level, I do not know.

Regarding the set/reset of this value, it may exists bugs like the one showed in the initial description of the problem.

Regarding coherency between "supportedSASLMechanisms" lookup (supported_saslmechanisms array) and nsslapd-sasl-allowed-mechanisms, I believe it should return something like supported_saslmechanisms-nsslapd-sasl-allowed-mechanisms

static int ids_sasl_getopt(
    void *context __attribute__((unused)),
    const char *plugin_name,
    const char *option,
    const char **result, 
    unsigned *len
)
{
...
    if (strcasecmp(option, "enable") == 0) {
...
    } else if (strcasecmp(option, "mech_list") == 0){
        *result = config_get_allowed_sasl_mechs();
    }

It looks like getopt just uses the config get allowed, so that may explain how this operates. I think that answers part of the issue then - perhaps my understanding was flawed.

Saying this, it still means if the allowed list was only "X, Y", then EXTERNAL is still excluded in this scheme. This would again be changing behaviour ....

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.3.6.0

6 years ago

We need to add EXTERNAL back to the supported list, this is blocking IPA for RHEL 7.4

@mreynolds based on the code in ids_sasl_getopt, even though we might have advertised EXTERNAL, ids_sasl_getopt would not present it as an option. I think that what we are seeing here is that now we can see the actual whitelist in place. Before we couldn't. Another aspect is that EXTERNAL seems to bypass lots of the saslbind code itself too.

I think adding this back in is fine, but I'm going to add lots of comments, and I think it would be good to really clean our SASL mech handling.

@mreynolds and @spichugi This patch resolves the issue, and has a test case to show it works. Sorry for the long time on this, I really just wanted to be sure we were doing the right thing here and that we really understood the existing behaviour. I appreciate your patience.

@mreynolds and @spichugi This patch resolves the issue, and has a test case to show it works. Sorry for the long time on this, I really just wanted to be sure we were doing the right thing here and that we really understood the existing behaviour. I appreciate your patience.

Yeah, so EXTERNAL, as I understand it, really has nothing to do with SASL, its used for SSL client authentication. The only issue is that if we previously advertised it, so we should not remove it. I was trying to figure out how your fix removed EXTERNAL as it's not obvious looking at the code changes, but I got side tracked on other build issues ;-) Anyway I'll be reviewing this patch shortly.

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

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.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

3 years ago

Login to comment on this ticket.

Metadata