Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sub-domain not used in header #67

Closed
gogglespisano opened this issue Oct 10, 2014 · 10 comments
Closed

sub-domain not used in header #67

gogglespisano opened this issue Oct 10, 2014 · 10 comments
Milestone

Comments

@gogglespisano
Copy link
Contributor

I've configured example.com and sub,example.com. Both have all the required dns keys (working on a previous version).

When I send mail from user@sub.example.com the header has d=example.com, not d=sub.example.com. The earlier version sent d=sub.example.com. I've tried change between simple and relaxed with no change.

is this a bug, or a new configuration setting I've missed?

Thanks,
-Stuart

@gogglespisano
Copy link
Contributor Author

The problem seems to be the following code in Exchange.DkimSigner\DkimSigningRoutingAgent.cs

Beyond the sub-domain matching, it appears it would also match abc.net.example.com to abc.net which would be very wrong.

As far as I can tell, the domain list isn't sorted, so as a feature it won't necessarily pick the domain closest to the root domain of a sub-domain. It will pick the last domain in the list with a partial match.

What is the reason for using Contains()? Can == not be used?

    private void SignMailItem(MailItem mailItem)
    {

...
if (mailItem.FromAddress.DomainPart
.ToUpperInvariant()
.Contains(e.Domain.ToUpperInvariant()))
domain = e;

Also, this code doesn't change with each loop interation, so it can be moved above the forech statement.

                if (mailItem.FromAddress == null || mailItem.FromAddress.DomainPart == null)
                {
                    Logger.LogWarning("Invalid from address: '" + mailItem.FromAddress + "'. Not signing email.");
                    continue;
                }

@Pro
Copy link
Owner

Pro commented Oct 21, 2014

Thanks for your issue report.
That's definetly a bug and not a misconfiguration.
As you already wrote the lines

if (mailItem.FromAddress.DomainPart
.ToUpperInvariant()
.Contains(e.Domain.ToUpperInvariant()))
domain = e;

should be changed to

if (mailItem.FromAddress.DomainPart
.ToUpperInvariant()
.Equals(e.Domain.ToUpperInvariant())) {
    domain = e;
    break;
}

and the FromAddress check shoud be out of the loop.

Currently I don't have access to my workstation. I'll fix this when I'm back in one to two weeks.

@gogglespisano
Copy link
Contributor Author

Thanks for taking a look and thanks in advance for the fix.

@AlexLaroche
Copy link
Collaborator

@Pro: We should provide a new release before close a issue.

@gogglespisano
Copy link
Contributor Author

Do you have an ETA for a new release?

@Pro Pro added this to the 2.0.4 milestone Nov 13, 2014
@Pro
Copy link
Owner

Pro commented Nov 13, 2014

It's good coding practice to close an issue as soon it is fixed within the code rather then closing it after the modified code is released.
Otherwise it's difficult to keep track which issues are already fixed (and not yet released) and which still need to be fixed.

The Issue Milestone feature can be used to keep track if the issue is already integrated into a release or not.

@gogglespisano I'll do some test and then release a version including this bugfix today or in the next few days

@Pro
Copy link
Owner

Pro commented Nov 27, 2014

@gogglespisano today I released version 2.1.0 which fixes this issue.

@gogglespisano
Copy link
Contributor Author

I had problems installing 2.1.2. I was upgrading from 2.0.3.

It failed during the zip extraction because of file in use. This happening using the GUI update or downloading and installing from the downloaded files. I had to disable the signer, then upgrade, then enable the signer.

@Pro
Copy link
Owner

Pro commented Nov 30, 2014

Hm, this looks like the MSExchangeTransport wasn't stopped properly.
First the files are unzipped into temp, then the service is stopped and then the files are copied.
Are you sure that zip extraction step failed, or may it have been the copy step? I tested on my machine and it worked fine. I think version 2.1.2 should fix this.

(For the next time please open a new issue instead of replying to an old one, this makes it easier to keep track of different issues, thanks :) )

@gogglespisano
Copy link
Contributor Author

I entered a new issue for it.

The good news is my email is now verifying with the correct domain name ☺

Thanks,
-Stuart

From: Stefan Profanter [mailto:notifications@github.com]
Sent: Sunday, November 30, 2014 1:06 PM
To: Pro/dkim-exchange
Cc: Stuart Wyatt
Subject: Re: [dkim-exchange] sub-domain not used in header (#67)

Hm, this looks like the MSExchangeTransport wasn't stopped properly.
First the files are unzipped into temp, then the service is stopped and then the files are copied.
Are you sure that zip extraction step failed, or may it have been the copy step? I tested on my machine and it worked fine. I think version 2.1.2 should fix this.

(For the next time please open a new issue instead of replying to an old one, this makes it easier to keep track of different issues, thanks :) )


Reply to this email directly or view it on GitHubhttps://github.com//issues/67#issuecomment-65000291.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants