feat(dkim): make key size configurable and allow keys to be rotated

app/controllers/domains_controller.rb

@@ -98,9 +98,48 @@ class DomainsController < ApplicationController
     redirect_to [:verify, organization, @server, @domain], alert: "You can't set up DNS for this domain until it has been verified."
   end
 
+  def regenerate_dkim
+    if @domain.pending_dkim_key?
+      redirect_to_with_json [:setup, organization, @server, @domain],
+                            alert: "A DKIM key change is already in progress for #{@domain.name}. Publish the new " \
+                                   "record shown below, or cancel the change, before generating another key."
+      return
+    end
+
+    was_verified = @domain.dkim_status == "OK"
+    @domain.regenerate_dkim_key!
+    if was_verified
+      redirect_to_with_json [:setup, organization, @server, @domain],
+                            notice: "A new DKIM key has been generated for #{@domain.name}. Add the new DNS record below — " \
+                                    "your existing key will remain active until the new record has been verified. Keep the " \
+                                    "old DNS record published afterward while messages signed with it may still be queued, " \
+                                    "held, or available for redelivery."
+    else
+      redirect_to_with_json [:setup, organization, @server, @domain], notice: "A new DKIM key has been generated for #{@domain.name}. Update your DKIM DNS record with the new value below."
+    end
+  end
+
+  def cancel_dkim_regeneration
+    @domain.cancel_pending_dkim_key!
+    redirect_to_with_json [:setup, organization, @server, @domain], notice: "The pending DKIM key for #{@domain.name} has been discarded. Your existing key remains active."
+  end
+
   def check
+    had_pending_dkim_key = @domain.pending_dkim_key?
+    previous_dkim_record_name = @domain.dkim_record_name
     if @domain.check_dns(:manual)
-      redirect_to_with_json [organization, @server, :domains], notice: "Your DNS records for #{@domain.name} look good!"
+      if had_pending_dkim_key && !@domain.pending_dkim_key?
+        redirect_to_with_json [:setup, organization, @server, @domain],
+                              notice: "Your DNS records for #{@domain.name} look good! Your new DKIM key is now active. " \
+                                      "Keep the old record at #{previous_dkim_record_name} published while messages " \
+                                      "signed with the old key may still be queued, held, or available for redelivery."
+      elsif @domain.pending_dkim_key?
+        redirect_to_with_json [:setup, organization, @server, @domain],
+                              alert: "We couldn't verify the record for your new DKIM key yet. Your existing key remains " \
+                                     "active. Check below for the expected record details."
+      else
+        redirect_to_with_json [organization, @server, :domains], notice: "Your DNS records for #{@domain.name} look good!"
+      end
     else
       redirect_to_with_json [:setup, organization, @server, @domain], alert: "There seems to be something wrong with your DNS records. Check below for information."
     end

app/models/concerns/has_dns_checks.rb

@@ -72,6 +72,17 @@ module HasDNSChecks
   #
 
   def check_dkim_record
+    # If a new DKIM key is waiting to be activated, promote it to be the active
+    # key as soon as its DNS record is in place. The matching DNS response has
+    # already verified the key, so avoid a second lookup which could return a
+    # different answer while DNS changes are propagating.
+    if pending_dkim_key? && pending_dkim_record_live?
+      activate_pending_dkim_key
+      self.dkim_status = "OK"
+      self.dkim_error = nil
+      return true
+    end
+
     domain = "#{dkim_record_name}.#{name}"
     records = resolver.txt(domain)
     if records.empty?
@@ -98,6 +109,16 @@ module HasDNSChecks
     save!
   end
 
+  # Returns true if the DNS record for the pending DKIM key has been published
+  # correctly.
+  def pending_dkim_record_live?
+    records = resolver.txt("#{pending_dkim_record_name}.#{name}")
+    return false unless records.size == 1
+
+    sanitised_record = records.first.strip.ends_with?(";") ? records.first.strip : "#{records.first.strip};"
+    sanitised_record == pending_dkim_record
+  end
+
   #
   # MX
   #

app/models/domain.rb

@@ -4,31 +4,33 @@
 #
 # Table name: domains
 #
-#  id                     :integer          not null, primary key
-#  server_id              :integer
-#  uuid                   :string(255)
-#  name                   :string(255)
-#  verification_token     :string(255)
-#  verification_method    :string(255)
-#  verified_at            :datetime
-#  dkim_private_key       :text(65535)
-#  created_at             :datetime
-#  updated_at             :datetime
-#  dns_checked_at         :datetime
-#  spf_status             :string(255)
-#  spf_error              :string(255)
-#  dkim_status            :string(255)
-#  dkim_error             :string(255)
-#  mx_status              :string(255)
-#  mx_error               :string(255)
-#  return_path_status     :string(255)
-#  return_path_error      :string(255)
-#  outgoing               :boolean          default(TRUE)
-#  incoming               :boolean          default(TRUE)
-#  owner_type             :string(255)
-#  owner_id               :integer
-#  dkim_identifier_string :string(255)
-#  use_for_any            :boolean
+#  id                             :integer          not null, primary key
+#  dkim_error                     :string(255)
+#  dkim_identifier_string         :string(255)
+#  dkim_private_key               :text(65535)
+#  dkim_status                    :string(255)
+#  dns_checked_at                 :datetime
+#  incoming                       :boolean          default(TRUE)
+#  mx_error                       :string(255)
+#  mx_status                      :string(255)
+#  name                           :string(255)
+#  outgoing                       :boolean          default(TRUE)
+#  owner_type                     :string(255)
+#  pending_dkim_identifier_string :string(255)
+#  pending_dkim_private_key       :text(65535)
+#  return_path_error              :string(255)
+#  return_path_status             :string(255)
+#  spf_error                      :string(255)
+#  spf_status                     :string(255)
+#  use_for_any                    :boolean
+#  uuid                           :string(255)
+#  verification_method            :string(255)
+#  verification_token             :string(255)
+#  verified_at                    :datetime
+#  created_at                     :datetime
+#  updated_at                     :datetime
+#  owner_id                       :integer
+#  server_id                      :integer
 #
 # Indexes
 #
@@ -82,7 +84,7 @@ class Domain < ApplicationRecord
   end
 
   def generate_dkim_key
-    self.dkim_private_key = OpenSSL::PKey::RSA.new(1024).to_s
+    self.dkim_private_key = OpenSSL::PKey::RSA.new(Postal::Config.dns.dkim_key_size).to_s
   end
 
   def dkim_key
@@ -91,6 +93,57 @@ class Domain < ApplicationRecord
     @dkim_key ||= OpenSSL::PKey::RSA.new(dkim_private_key)
   end
 
+  def pending_dkim_key
+    return nil unless pending_dkim_private_key
+
+    @pending_dkim_key ||= OpenSSL::PKey::RSA.new(pending_dkim_private_key)
+  end
+
+  def pending_dkim_key?
+    pending_dkim_private_key.present?
+  end
+
+  # Generate a new DKIM key for this domain. If the current key is verified and
+  # in use for signing, the new key is stored as a pending key under a new
+  # identifier so that signing continues with the current key until the new
+  # key's DNS record has been published and verified. Otherwise, the key is
+  # replaced immediately and any in-flight key change is abandoned, since the
+  # new key supersedes it.
+  def regenerate_dkim_key!
+    if dkim_status == "OK"
+      self.pending_dkim_private_key = OpenSSL::PKey::RSA.new(Postal::Config.dns.dkim_key_size).to_s
+      self.pending_dkim_identifier_string = generate_unique_dkim_identifier_string
+    else
+      generate_dkim_key
+      self.dkim_status = nil
+      self.dkim_error = nil
+      self.pending_dkim_private_key = nil
+      self.pending_dkim_identifier_string = nil
+      @dkim_key = nil
+    end
+    @pending_dkim_key = nil
+    save!
+  end
+
+  # Promote the pending DKIM key to be the active key. Does not save the record.
+  def activate_pending_dkim_key
+    return unless pending_dkim_key?
+
+    self.dkim_private_key = pending_dkim_private_key
+    self.dkim_identifier_string = pending_dkim_identifier_string
+    self.pending_dkim_private_key = nil
+    self.pending_dkim_identifier_string = nil
+    @dkim_key = nil
+    @pending_dkim_key = nil
+  end
+
+  def cancel_pending_dkim_key!
+    self.pending_dkim_private_key = nil
+    self.pending_dkim_identifier_string = nil
+    @pending_dkim_key = nil
+    save!
+  end
+
   def to_param
     uuid
   end
@@ -108,23 +161,27 @@ class Domain < ApplicationRecord
   end
 
   def dkim_record
-    return if dkim_key.nil?
-
-    public_key = dkim_key.public_key.to_s.gsub(/-+[A-Z ]+-+\n/, "").gsub(/\n/, "")
-    "v=DKIM1; t=s; h=sha256; p=#{public_key};"
+    build_dkim_record(dkim_key)
   end
 
   def dkim_identifier
-    return nil unless dkim_identifier_string
-
-    Postal::Config.dns.dkim_identifier + "-#{dkim_identifier_string}"
+    build_dkim_identifier(dkim_identifier_string)
   end
 
   def dkim_record_name
-    identifier = dkim_identifier
-    return if identifier.nil?
+    build_dkim_record_name(dkim_identifier)
+  end
 
-    "#{identifier}._domainkey"
+  def pending_dkim_record
+    build_dkim_record(pending_dkim_key)
+  end
+
+  def pending_dkim_identifier
+    build_dkim_identifier(pending_dkim_identifier_string)
+  end
+
+  def pending_dkim_record_name
+    build_dkim_record_name(pending_dkim_identifier)
   end
 
   def return_path_domain
@@ -160,6 +217,36 @@ class Domain < ApplicationRecord
 
   private
 
+  def build_dkim_record(key)
+    return if key.nil?
+
+    public_key = key.public_key.to_s.gsub(/-+[A-Z ]+-+\n/, "").gsub(/\n/, "")
+    "v=DKIM1; t=s; h=sha256; p=#{public_key};"
+  end
+
+  def build_dkim_identifier(identifier_string)
+    return nil unless identifier_string
+
+    Postal::Config.dns.dkim_identifier + "-#{identifier_string}"
+  end
+
+  def build_dkim_record_name(identifier)
+    return if identifier.nil?
+
+    "#{identifier}._domainkey"
+  end
+
+  # Generates a random identifier string in the same format as the
+  # `random_string :dkim_identifier_string` declaration, unique across both the
+  # active and pending identifier columns.
+  def generate_unique_dkim_identifier_string
+    loop do
+      string = Nifty::Utils::RandomString.generate(length: 6, upper_letters_only: true)
+      scope = self.class.where(dkim_identifier_string: string).or(self.class.where(pending_dkim_identifier_string: string))
+      return string unless scope.exists?
+    end
+  end
+
   def update_verification_token_on_method_change
     return unless verification_method_changed?
 

app/views/domains/index.html.haml

@@ -34,6 +34,8 @@
               = link_to domain.name, [:setup, organization, @server, domain]
               - if domain.use_for_any?
                 %span.label.label--blue Any
+              - if domain.pending_dkim_key?
+                = link_to "New DKIM key pending", [:setup, organization, @server, domain], :class => "label label--orange", :title => "A new DKIM key is waiting for its DNS record to be published."
             %ul.domainList__checks
               - if domain.spf_status == 'OK'
                 %li.domainList__check.domainList__check--ok SPF

app/views/domains/setup.html.haml

@@ -47,7 +47,11 @@
   %pre.codeBlock.u-margin= @domain.spf_record
 
   %h3.pageContent__subTitle DKIM Record
-  - if @domain.dkim_status == 'OK'
+  - if @domain.pending_dkim_key?
+    %p.pageContent__text.u-orange.u-bold
+      %span.label.label--orange Key change in progress
+      You've generated a new DKIM key which isn't active yet. Both records are shown below.
+  - elsif @domain.dkim_status == 'OK'
     %p.pageContent__text.u-green.u-bold
       %span.label.label--green Good
       Your DKIM record looks good!
@@ -56,10 +60,34 @@
       %span.label.label--orange Warning
       = @domain.dkim_error
 
-  %p.pageContent__text
-    You need to add a new TXT record with the name <b>#{@domain.dkim_record_name}</b>
-    with the following content.
-  %pre.codeBlock.u-margin= @domain.dkim_record
+  - if @domain.pending_dkim_key?
+    %p.pageContent__text.u-bold
+      %span.label.label--green Current
+      Keep this record exactly as it is
+    %p.pageContent__text
+      This is the record you already have published at <b>#{@domain.dkim_record_name}</b>.
+      It's signing your mail right now, so leave it in place.
+    %pre.codeBlock.u-margin= @domain.dkim_record
+
+    %p.pageContent__text.u-bold
+      %span.label.label--orange New
+      Add this record as well
+    %p.pageContent__text
+      Add a <b>second</b> TXT record with the name <b>#{@domain.pending_dkim_record_name}</b>
+      and the content below, alongside the record above rather than replacing it. Once we detect
+      it (checked hourly, or when you use the button above) we'll switch to signing with the new
+      key automatically. After the switch, keep the old record published while messages signed
+      with it may still be queued, held, or available for redelivery.
+    %pre.codeBlock.u-margin= @domain.pending_dkim_record
+    %p.pageContent__text
+      = link_to "Cancel new DKIM key", [:cancel_dkim_regeneration, organization, @server, @domain], :remote => true, :method => :post, :data => { :confirm => "Are you sure? The new key will be discarded and your existing key will remain active." }, :class => 'button button--neutral'
+  - else
+    %p.pageContent__text
+      You need to add a new TXT record with the name <b>#{@domain.dkim_record_name}</b>
+      with the following content.
+    %pre.codeBlock.u-margin= @domain.dkim_record
+    %p.pageContent__text
+      = link_to "Regenerate DKIM key", [:regenerate_dkim, organization, @server, @domain], :remote => true, :method => :post, :data => { :confirm => "Are you sure you wish to generate a new DKIM key? You'll need to add a new DNS record before the new key can be used." }, :class => 'button button--neutral'
 
   %h3.pageContent__subTitle Return Path
   - if @domain.return_path_status == 'OK'