DomainKeys/DKIM

[AlexLaroche]

Should be interessant to add a DomainKeys signer at your existing DKIM signer and use any combinaison between Dkim, DomainKeys or DKIM/DomainKeys as it's possible in the EA DomainKeys/DKIM solution from Email Architect.

[Pro]

I think DomainKeys isn't worth the time to implement it since DKIM is the successor of DomainKeys and accepted of much more companies.

More important would be to implement #22 and #23, but if you want to invest your time into DomainKeys feel free to add it.

[AlexLaroche]

I don't agree with you that implement DomainKeys isn't worth the time. I already know that DKIM is the successor of DomainKeys. Unfortunately, DomainKeys is still use in some anti-spam filter of companies depending of worldwide location of the email destinations. For that reason, it's for me a good reason to spend time to add this feature (if you send some newsletters or don't be block by the anti-spam filter, it's useful).

What is the best ways to change the app.config file to add the implementation? The only encryption algorithm support is RsaSha1. Two canonicalization are possible : Simple and Nofws. The domain key aren't affect by this change, we can use the same private key and the same selector. RecipientRule and SenderRule can be easy implement too.

[Pro]

Ah, ok, didn't know that. Then it would be nice to have DomainKeys too.

Regarding the app.config, we always have to keep backwards compatibility, at least for a few months until everyone has an updated version (we can check the config version and adapt to it programatically and warn the user to update the config, see for example here: https://github.com/Pro/dkim-exchange/blob/c5239cdc530fe474e919d2dde5ed7bcbce783405/Src/Exchange.DkimSigner/DkimSigningRoutingAgentFactory.cs#L127 (line 127-133)).

I think the best and cleanest solution would be to set the properties of DKIM and DomainKey in the custom section, and allow on each domain disabling and enabling DKIM and/or DomainKey:

<domainSection>
    <Domains>
        <Domain Domain="example.com" Selector="sel2012" PrivateKeyFile="keys/example.com.private" DKIM="true" DomainKey="false" SenderRule="user@.*" RecipientRule="yahoo\.[^\.]+"/>
    </Domains>
</domainSection>
<customSection>
    <general LogLevel="3" HeadersToSign="From; Subject; To; Date; Message-ID;"/>
    <dkim Algorithm="RsaSha1" HeaderCanonicalization="Simple" BodyCanonicalization="Simple"/>
    <domainKey Algorithm="RsaSha1" HeaderCanonicalization="Simple" BodyCanonicalization="Simple"/>
</customSection>

What's your opinion on this format?

[AlexLaroche]

The proposed solution for the app.config is very great. It's a great idea to activate DKIM and DomainKeys by domain. It's also add the possibility to disable a domain configuration.

But I'm not sure that it's a great idea to put a lot constraints in the code for backwards compatibility as you proposed.

1. Many network administrators doesn't read the event log until a problem occur. They will not see the notice.
2. Any decent network administrator will read the README file before to install any new version of a software on a server.
3. The backward compatibility introduce risk of mistake or unpredictable comportment for the software by the complexity added if it's not well implement or in any subsequent updates.

Could better solution could be a validation of the configuration file (app.config) when the DKIM Signer agent is load. (Occur when the EdgeTransport service start) When a GUI for installation and for configuration will be available, the impact will be maybe less because the upgrade could be automatic.

What's your opinion about that?

[Pro]

That's also a good idea to keep the code clean and checking the config when the agent is created. I think we can simply throw an Exception in the constructor, which is then shown in the install script and should provide additional info about the wrong config parameter.

[AlexLaroche]

Do you where we can throw a exception in this function if the configuration file isn't correct? (unsupported option is pass)

This function is call for every domain and general configuration. We can remove the backwards compatibility if we can throw here a exception.

         public static TConfig GetCustomConfig<TConfig>(string sectionName) where TConfig : ConfigurationSection
        {
            AppDomain.CurrentDomain.AssemblyResolve += new ResolveEventHandler(ConfigResolveEventHandler);
            configurationDefiningAssembly = Assembly.LoadFrom(Assembly.GetExecutingAssembly().Location);
            var exeFileMap = new ExeConfigurationFileMap();
            exeFileMap.ExeConfigFilename = Assembly.GetExecutingAssembly().Location + ".config";
            var customConfig = ConfigurationManager.OpenMappedExeConfiguration(exeFileMap, ConfigurationUserLevel.None);
            var returnConfig = customConfig.GetSection(sectionName) as TConfig;
            AppDomain.CurrentDomain.AssemblyResolve -= ConfigResolveEventHandler;
            return returnConfig;
        }

[Pro]

I think it would be best to throw the exception in the Initialize function instead of the GetCustomConfig: https://github.com/Pro/dkim-exchange/blob/5e40356f0ee97a355c0a239e3cbac69b68228b16/Src/Exchange.DkimSigner/DkimSigningRoutingAgentFactory.cs#L74

There are already some examples on ConfigurationErrorsException calls. Instead of the Resources.* calls it would be better to write the error message directly, i.e. throw new ConfigurationErrorsException("Domain example.com error: attribute xy is replaced by zz.") This makes the code more readable.