dkim icon indicating copy to clipboard operation
dkim copied to clipboard

General improvements

Open denysvitali opened this issue 4 years ago • 7 comments

  • add ed25519-sha256 support
  • refactor a lot of code to improve multi-algorithm handling
  • improve CLI tools
  • add Makefile
  • improve README

Feel free to merge the changes / suggest areas of improvement :+1: Thanks for sharing your code though! Super useful and super good!

denysvitali avatar Jun 01 '20 14:06 denysvitali

Thanks, it might take me a bit to review since it's a fairly big refactoring (for the size of the project at least). If you're in a hurry and there's a logical way to split it into smaller PRs they might individually go faster, but just skimming this a couple things:

  1. Calling it "DKIM Tools" is a little confusing, because it's not (and not intended to be) a port of the standard dkim tools, though I don't mind renaming it to take my name out.
  2. Why is the signing test removed?
  3. Since the code base has generally been stable since I originally wrote this I was a bad programmer and didn't bother to include tests for e1128f0 or 20ca4af .. can you confirm that there's no regressions with those? (Ideally with tests, but if you just manually check them that's good enough for me.)
  4. Your new test case has spaces between the ':' in the fieldnames in the header.. I don't think I've ever seen a DKIM signature in the wild signed that way, it might be a good idea to check if the standard says that's valid
  5. For the README if you're listing known usages I've started using the package in driusan/webmail too
  6. For the README you can give yourself more credit for the rewrite

driusan avatar Jun 01 '20 15:06 driusan

  1. I called it DKIM Tools because I thought it is the perfect toolset to work with DKIM, hence the name :+1: . "driusan's DKIM tools" makes it look like a less production-ready software than it really is (in my opinion), but I don't mind keeping the name as it is
  2. I need to double check, but IIRC I've only moved some files around and didn't remove any test
  3. Need to double check, I'll probably come back with a test case as well
  4. My new test case comes directly from RFC8463, which includes the following:
   The text in each line of the message starts at the first position
   except for the continuation lines on the DKIM-Signature header
   fields, which start with a single space.  A blank line follows the
   "Joe." line.

   DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed;
    d=football.example.com; [email protected];
    q=dns/txt; s=brisbane; t=1528637909; h=from : to :
    subject : date : message-id : from : subject : date;
    bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=;
    b=/gCrinpcQOoIfuHNQIbq4pgh9kyIK3AQUdt9OdqQehSwhEIug4D11Bus
    Fa3bT3FY5OsU7ZbnKELq+eXdp1Q1Dw==
   DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
    d=football.example.com; [email protected];
    q=dns/txt; s=test; t=1528637909; h=from : to : subject :
    date : message-id : from : subject : date;
    bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=;
    b=F45dVWDfMbQDGHJFlXUNB2HKfbCeLRyhDXgFpEL8GwpsRe0IeIixNTe3
    DhCVlUrSjV4BwcVcOF6+FF3Zo9Rpo1tFOeS9mPYQTnGdaSGsgeefOsk2Jz
    dA+L10TeYt9BgDfQNZtKdN1WO//KgIqXP7OdEFE4LjFYNcUxZQ4FADY+8=
   From: Joe SixPack <[email protected]>
   To: Suzie Q <[email protected]>
   Subject: Is dinner ready?
   Date: Fri, 11 Jul 2003 21:00:37 -0700 (PDT)
   Message-ID: <[email protected]>

   Hi.

   We lost the game.  Are you hungry yet?

   Joe.

If you're referring to the header field: value format, I guess it is pretty common and should work too. If you're referring to the spaces in between the DKIM-Signature header value, it should be accepted too (this comes directly from the RFC)

  1. Might as well add that, thanks!
  2. No credits needed, I want this project to go big and to improve. I've only reworked it to make it suit my needs and I'm super glad you made it open source with a permissive license (MIT). Thanks :rocket:

denysvitali avatar Jun 01 '20 15:06 denysvitali

Re: 2. You're right, disregard. It's not showing up as a rename in the GitHub diff because the added test makes it dissimilar enough for git to not consider it a rename and I thought you deleted it while skimming because the files aren't very close in the "Files Changed" tab on GitHub.

Re: 4 If it comes from the RFC, it must be good. It's been a while since I read it and I'm just used to not seeing the spaces in the wild. For example, on the github notification about your comment GitHub signed the email as:

h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID:
	 List-Archive:List-Post:List-Unsubscribe:From; 

driusan avatar Jun 01 '20 15:06 driusan

Hi there – gentle ping to see if there’s any chance of this moving forward?

I arrived here from the chasquid documentation and would love to see Ed25519 support.

emilazy avatar Feb 23 '21 05:02 emilazy

@emilazy , in the meantime you can use my fork 👍

denysvitali avatar Feb 23 '21 07:02 denysvitali

Sorry about the delay. I think the main things holding this up are:

  1. The unnecessary pulling in of a lot of non-stdlib dependencies for the tests, when a simple if statements could be used without pulling in any dependencies at all.
  2. The splitting of the Algorithm interface into smaller interfaces.

There's also a lot of unnecessary style changes which make it time consuming to review, but the above 2 points (especially point 1) are the blockers.

driusan avatar Feb 23 '21 15:02 driusan

Sorry for not updating the PR. I totally forgot about it and I'll improve it as soon as possible!

denysvitali avatar Jul 18 '21 19:07 denysvitali