dkim_signature_apply_on needs to be explicitly set in robot.conf; the documented default doesn't apply

[dpoon]

With no DKIM directives in sympa.conf, and the following directives set in robot.conf:

dkim_feature on
dkim_private_key_path …
dkim_signer_domain …
dkim_selector …

… you would expect that Sympa would perform DKIM signing on messages that contain a valid DKIM signature from the sender's MTA, but it doesn't. Rather, you must explicitly specify dkim_signature_apply_on dkim_authenticated_messages, … in robot.conf to make that happen.

Version

6.2.66, but I believe the problematic code is the same in 6.2.72 and main

Installation method

Universe repo for Ubuntu 22.04: https://mirror.it.ubc.ca/ubuntu/pool/universe/s/sympa/sympa_6.2.66~dfsg-2_amd64.deb

Expected behavior

According to Sympa's documentation for DKIM / ARC, dkim_signature_apply_on defaults to md5_authenticated_messages, smime_authenticated_messages, dkim_authenticated_messages, editor_validated_messages, so I expect that it should not be necessary to explicitly set dkim_signature_apply_on … in robot.conf.

Actual behavior

DKIM signing does not happen. To make it happen, one must either specify dkim_feature on in the global sympa.conf or explicitly set dkim_signature_apply_on … in robot.conf.

Additional information

See Conf::_infer_server_specific_parameter_values (which is called from _load in a server-wide context):

sub _infer_server_specific_parameter_values {
    my $param = shift;

    $param->{'config_hash'}{'robot_name'} = '';

    unless (
        Sympa::Tools::Data::smart_eq(
            $param->{'config_hash'}{'dkim_feature'}, 'on'
        )
    ) {
        # dkim_signature_apply_ on nothing if dkim_feature is off
        # Sets empty array.
        $param->{'config_hash'}{'dkim_signature_apply_on'} = [''];
    } else {
        $param->{'config_hash'}{'dkim_signature_apply_on'} =~ s/\s//g;
        my @dkim =
            split(/,/, $param->{'config_hash'}{'dkim_signature_apply_on'});
        $param->{'config_hash'}{'dkim_signature_apply_on'} = \@dkim;
    }

If sympa.conf does not have dkim_feature on, then this code sets dkim_signature_apply_on to an empty list, clobbering the default. When loading robot.conf, the empty value, rather than the documented default, is inherited.

[ikedas]

Actual behavior

DKIM signing does not happen. To make it happen, one must either specify dkim_feature on in the global sympa.conf or explicitly set dkim_signature_apply_on … in robot.conf.

I feel this is expected behavior: dkim_feture on enables DKIM feature.

[dpoon]

I disagree: the behavior is counterintuitive, and I wasted hours trying to figure out why DKIM signing wasn't happening because of it. Since dkim_feature can be set on either sympa.conf or a robot.conf, I feel that it is reasonable to expect that if you set dkim_feature on in robot.conf, the DKIM feature should be enabled for that robot. But since the robot will have dkim_signature_apply_on set to an empty list by default, DKIM will effectively be disabled, and nowhere in the documentation (or common sense) would suggest that that should be expected.

[racke]

I think also that enabling DKIM by robot makes sense. For some domains you might to want DKIM, but not for others. Even we don't change this, it would be good to have correct documentation.

[ikedas]

...I feel that it is reasonable to expect that if you set dkim_feature on in robot.conf, the DKIM feature should be enabled for that robot. But since the robot will have dkim_signature_apply_on set to an empty list by default, DKIM will effectively be disabled...

Ah I see, I never noticed that bug: The default value of dkim_signature_apply_on should not be empty. The code in Conf.pm is often broken and this module should retire in the near future.

Besides, in my opinion, the DKIM (or ARC) feature should be enabled if the relevant parameters (signer_domain, selector and private_key) are available. It is not often that we specify these parameters but do not want to use the feature. It is better to be able to specify dkim_feature off (or arc_feature off) if we want to explicitly disable the feature.

[ikedas]

Please check the PR above.

[dpoon]

PR1740 does fix the default behavior in the situation described in the bug report.

However, if dkim_signature_apply_on is specified in sympa.conf, Conf::get_robot_conf(…, 'dkim_signature_apply_on') will be an ARRAYREF, but if dkim_signature_apply_on is specified in a robot.conf, it will be a comma-separated string. For type consistency, shouldn't there be a similar call to split somewhere in _infer_robot_parameter_values?

[ikedas]

Ah yes, Conf.pm is really broken. Please check additional fixes.

[dpoon]

The second commit splits the string into an ARRAYREF for each robot, but do we need to be concerned that it would be a comma-separated string in the global (non-robot) context?

[ikedas]

The second commit splits the string into an ARRAYREF for each robot, but do we need to be concerned that it would be a comma-separated string in the global (non-robot) context?

I think we need to be concerned. In fact _infer_robot_parameter_values() is called both in global (sympa.conf) and domain (robot.conf) contexts.

[ikedas]

If no objection, I'll merge #1740 .

[dpoon]

Minor follow-up issue: Conf::_check_cpan_modules_required_by_config only works with the global dkim_feature flag, and ignores the per-robot settings.

    if ($param->{'config_hash'}{'dkim_feature'} eq 'on') {
        eval "require Mail::DKIM";
        if ($EVAL_ERROR) {
            $log->syslog('notice',
                'Failed to load Mail::DKIM perl module ; setting "dkim_feature" to "off"'
            );
            $param->{'config_hash'}{'dkim_feature'} = 'off';
            $number_of_missing_modules++;
        }
    }

[ikedas]

I had noticed it as well. This check is unnecessary as the equivalent is performed elsewhere, i.e. _check_cpan_modules_required_by_config() is unnecessary.

Not only in this example, if Conf.pm will be completely refactored, it will disappear. Such work should be done as another issue.