From d96eddbadeda600595d6247486f9ceb8e99cbc15 Mon Sep 17 00:00:00 2001 From: Adam Cooke Date: Thu, 1 Oct 2026 12:32:46 +0100 Subject: [PATCH] feat(dkim): make key size configurable and allow keys to be rotated --- app/controllers/domains_controller.rb | 41 ++- app/models/concerns/has_dns_checks.rb | 21 ++ app/models/domain.rb | 159 +++++++-- app/views/domains/index.html.haml | 2 + app/views/domains/setup.html.haml | 38 +- config/routes.rb | 4 + ...0000_add_pending_dkim_fields_to_domains.rb | 10 + db/schema.rb | 4 +- doc/config/environment-variables.md | 1 + doc/config/yaml.yml | 2 + lib/postal/config_schema.rb | 14 + spec/factories/domain_factory.rb | 52 +-- spec/models/domain_spec.rb | 325 ++++++++++++++++-- spec/requests/domains_controller_spec.rb | 144 ++++++++ spec/views/domains/index.html.haml_spec.rb | 48 +++ spec/views/domains/setup.html.haml_spec.rb | 73 ++++ 16 files changed, 845 insertions(+), 93 deletions(-) create mode 100644 db/migrate/20260903100000_add_pending_dkim_fields_to_domains.rb create mode 100644 spec/requests/domains_controller_spec.rb create mode 100644 spec/views/domains/index.html.haml_spec.rb create mode 100644 spec/views/domains/setup.html.haml_spec.rb diff --git a/app/controllers/domains_controller.rb b/app/controllers/domains_controller.rb index 146df1d..229d27e 100644 --- a/app/controllers/domains_controller.rb +++ b/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 diff --git a/app/models/concerns/has_dns_checks.rb b/app/models/concerns/has_dns_checks.rb index 789aeff..97e0406 100644 --- a/app/models/concerns/has_dns_checks.rb +++ b/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 # diff --git a/app/models/domain.rb b/app/models/domain.rb index fcd4eb3..cb63929 100644 --- a/app/models/domain.rb +++ b/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? diff --git a/app/views/domains/index.html.haml b/app/views/domains/index.html.haml index 2238e01..55c66ec 100644 --- a/app/views/domains/index.html.haml +++ b/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 diff --git a/app/views/domains/setup.html.haml b/app/views/domains/setup.html.haml index 34d9e5a..4c53198 100644 --- a/app/views/domains/setup.html.haml +++ b/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 #{@domain.dkim_record_name} - 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 #{@domain.dkim_record_name}. + 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 second TXT record with the name #{@domain.pending_dkim_record_name} + 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 #{@domain.dkim_record_name} + 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' diff --git a/config/routes.rb b/config/routes.rb index 11ef179..9e94812 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,12 +12,16 @@ Rails.application.routes.draw do match :verify, on: :member, via: [:get, :post] get :setup, on: :member post :check, on: :member + post :regenerate_dkim, on: :member + post :cancel_dkim_regeneration, on: :member end resources :servers, except: [:index] do resources :domains, only: [:index, :new, :create, :destroy] do match :verify, on: :member, via: [:get, :post] get :setup, on: :member post :check, on: :member + post :regenerate_dkim, on: :member + post :cancel_dkim_regeneration, on: :member end resources :track_domains do post :toggle_ssl, on: :member diff --git a/db/migrate/20260903100000_add_pending_dkim_fields_to_domains.rb b/db/migrate/20260903100000_add_pending_dkim_fields_to_domains.rb new file mode 100644 index 0000000..b46ad93 --- /dev/null +++ b/db/migrate/20260903100000_add_pending_dkim_fields_to_domains.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddPendingDKIMFieldsToDomains < ActiveRecord::Migration[7.0] + + def change + add_column :domains, :pending_dkim_private_key, :text + add_column :domains, :pending_dkim_identifier_string, :string + end + +end diff --git a/db/schema.rb b/db/schema.rb index 9ab08fd..acf8c4d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_03_11_205229) do +ActiveRecord::Schema[7.1].define(version: 2026_09_03_100000) do create_table "additional_route_endpoints", id: :integer, charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.integer "route_id" t.string "endpoint_type" @@ -99,6 +99,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_03_11_205229) do t.integer "owner_id" t.string "dkim_identifier_string" t.boolean "use_for_any" + t.text "pending_dkim_private_key" + t.string "pending_dkim_identifier_string" t.index ["server_id"], name: "index_domains_on_server_id" t.index ["uuid"], name: "index_domains_on_uuid", length: 8 end diff --git a/doc/config/environment-variables.md b/doc/config/environment-variables.md index e287459..06c092a 100644 --- a/doc/config/environment-variables.md +++ b/doc/config/environment-variables.md @@ -68,6 +68,7 @@ This document contains all the environment variables which are available for thi | `DNS_TRACK_DOMAIN` | String | The CNAME which tracking domains should be pointed to | track.postal.example.com | | `DNS_HELO_HOSTNAME` | String | The hostname to use in HELO/EHLO when connecting to external SMTP servers | | | `DNS_DKIM_IDENTIFIER` | String | The identifier to use for DKIM keys in DNS records | postal | +| `DNS_DKIM_KEY_SIZE` | Integer | The size (in bits) of RSA key to generate for DKIM signing (one of 1024, 2048, 3072 or 4096). Note that records for 2048-bit and larger keys exceed 255 characters and must be published as a split (multi-string) TXT record. | 2048 | | `DNS_DOMAIN_VERIFY_PREFIX` | String | The prefix to add before TXT record verification string | postal-verification | | `DNS_CUSTOM_RETURN_PATH_PREFIX` | String | The domain to use on external domains which points to the Postal return path domain | psrp | | `DNS_TIMEOUT` | Integer | The timeout to wait for DNS resolution | 5 | diff --git a/doc/config/yaml.yml b/doc/config/yaml.yml index a91c303..ded6630 100644 --- a/doc/config/yaml.yml +++ b/doc/config/yaml.yml @@ -147,6 +147,8 @@ dns: helo_hostname: # The identifier to use for DKIM keys in DNS records dkim_identifier: postal + # The size (in bits) of RSA key to generate for DKIM signing (one of 1024, 2048, 3072 or 4096). Note that records for 2048-bit and larger keys exceed 255 characters and must be published as a split (multi-string) TXT record. + dkim_key_size: 2048 # The prefix to add before TXT record verification string domain_verify_prefix: postal-verification # The domain to use on external domains which points to the Postal return path domain diff --git a/lib/postal/config_schema.rb b/lib/postal/config_schema.rb index 241d627..ec08fbc 100644 --- a/lib/postal/config_schema.rb +++ b/lib/postal/config_schema.rb @@ -365,6 +365,20 @@ module Postal default "postal" end + integer :dkim_key_size do + description "The size (in bits) of RSA key to generate for DKIM signing (one of 1024, 2048, 3072 or 4096). " \ + "Note that records for 2048-bit and larger keys exceed 255 characters and must be " \ + "published as a split (multi-string) TXT record." + default 2048 + transform do |value| + unless value.nil? || [1024, 2048, 3072, 4096].include?(value) + raise Konfig::Error, "dns.dkim_key_size must be one of 1024, 2048, 3072 or 4096 (got #{value})" + end + + value + end + end + string :domain_verify_prefix do description "The prefix to add before TXT record verification string" default "postal-verification" diff --git a/spec/factories/domain_factory.rb b/spec/factories/domain_factory.rb index 4aeebe8..480461f 100644 --- a/spec/factories/domain_factory.rb +++ b/spec/factories/domain_factory.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 # diff --git a/spec/models/domain_spec.rb b/spec/models/domain_spec.rb index 70dae6f..597d5dd 100644 --- a/spec/models/domain_spec.rb +++ b/spec/models/domain_spec.rb @@ -4,31 +4,33 @@ # # Table name: domains # -# 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) -# 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 +# 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 # @@ -162,6 +164,22 @@ describe Domain do it "generates a new dkim key" do expect { domain.generate_dkim_key }.to change { domain.dkim_private_key }.from(nil).to(match(/\A-+BEGIN RSA PRIVATE KEY-+/)) end + + it "generates a key of the configured size" do + domain.generate_dkim_key + expect(OpenSSL::PKey::RSA.new(domain.dkim_private_key).n.num_bits).to eq Postal::Config.dns.dkim_key_size + end + + context "when a different key size is configured" do + before do + allow(Postal::Config.dns).to receive(:dkim_key_size).and_return(1024) + end + + it "generates a key of that size" do + domain.generate_dkim_key + expect(OpenSSL::PKey::RSA.new(domain.dkim_private_key).n.num_bits).to eq 1024 + end + end end describe "#dkim_key" do @@ -183,6 +201,138 @@ describe Domain do end end + describe "#pending_dkim_key" do + context "when the domain has a pending DKIM key" do + let(:domain) { create(:domain, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "returns the pending dkim key as a OpenSSL::PKey::RSA" do + expect(domain.pending_dkim_key).to be_a OpenSSL::PKey::RSA + expect(domain.pending_dkim_key.to_s).to eq domain.pending_dkim_private_key + end + end + + context "when the domain has no pending DKIM key" do + let(:domain) { create(:domain) } + + it "returns nil" do + expect(domain.pending_dkim_key).to be_nil + end + end + end + + describe "#regenerate_dkim_key!" do + context "when the current DKIM record is verified" do + let(:domain) { create(:domain, dkim_status: "OK") } + + it "stores the new key as a pending key with a new identifier" do + domain.regenerate_dkim_key! + expect(domain.pending_dkim_private_key).to match(/\A-+BEGIN RSA PRIVATE KEY-+/) + expect(domain.pending_dkim_identifier_string).to be_present + expect(domain.pending_dkim_identifier_string).to_not eq domain.dkim_identifier_string + end + + it "does not change the active key or status" do + expect { domain.regenerate_dkim_key! }.to_not(change { domain.reload.dkim_private_key }) + expect(domain.dkim_status).to eq "OK" + end + end + + context "when the current DKIM record is not verified" do + let(:domain) { create(:domain, dkim_status: "Missing", dkim_error: "No TXT records") } + + it "replaces the active key immediately without creating a pending key" do + original_key = domain.dkim_key.to_s + domain.regenerate_dkim_key! + expect(domain.reload.dkim_private_key).to_not eq original_key + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.dkim_key.to_s).to eq domain.dkim_private_key + end + + it "resets the DKIM status" do + domain.regenerate_dkim_key! + expect(domain.dkim_status).to be_nil + expect(domain.dkim_error).to be_nil + end + + context "when a key change was already in progress" do + # This happens when the active record breaks (so the status is no longer + # "OK") while a pending key is waiting to be activated. The immediate + # replacement supersedes the in-flight change, so the stale pending key + # must not be left behind to be activated later. + let(:domain) { create(:domain, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + domain.update!(dkim_status: "Missing", dkim_error: "No TXT records") + end + + it "abandons the pending key" do + expect(domain.pending_dkim_private_key).to_not be_nil + domain.regenerate_dkim_key! + expect(domain.reload.pending_dkim_private_key).to be_nil + expect(domain.pending_dkim_identifier_string).to be_nil + expect(domain.pending_dkim_key).to be_nil + end + + it "replaces the active key rather than promoting the pending one" do + stale_pending_key = domain.pending_dkim_private_key + original_key = domain.dkim_private_key + domain.regenerate_dkim_key! + expect(domain.reload.dkim_private_key).to_not eq original_key + expect(domain.dkim_private_key).to_not eq stale_pending_key + end + end + end + end + + describe "#activate_pending_dkim_key" do + context "when there is no pending key" do + let(:domain) { create(:domain) } + + it "does nothing" do + expect { domain.activate_pending_dkim_key }.to_not(change { domain.dkim_private_key }) + end + end + + context "when there is a pending key" do + let(:domain) { create(:domain, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "promotes the pending key and identifier and clears the pending values" do + pending_key = domain.pending_dkim_private_key + pending_identifier = domain.pending_dkim_identifier_string + domain.activate_pending_dkim_key + expect(domain.dkim_private_key).to eq pending_key + expect(domain.dkim_identifier_string).to eq pending_identifier + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.pending_dkim_identifier_string).to be_nil + expect(domain.dkim_key.to_s).to eq pending_key + expect(domain.pending_dkim_key).to be_nil + end + end + end + + describe "#cancel_pending_dkim_key!" do + let(:domain) { create(:domain, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "discards the pending key without changing the active key" do + expect { domain.cancel_pending_dkim_key! }.to_not(change { domain.reload.dkim_private_key }) + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.pending_dkim_identifier_string).to be_nil + end + end + describe "#to_param" do context "when the domain has not been saved" do it "returns nil" do @@ -274,6 +424,131 @@ describe Domain do end end + describe "#pending_dkim_record" do + context "when the domain has no pending DKIM key" do + it "returns nil" do + expect(domain.pending_dkim_record).to be_nil + end + end + + context "when the domain has a pending DKIM key" do + let(:domain) { create(:domain, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "returns the DKIM record for the pending key" do + expect(domain.pending_dkim_record).to match(/\Av=DKIM1; t=s; h=sha256; p=.*;\z/) + expect(domain.pending_dkim_record).to_not eq domain.dkim_record + end + + it "returns a record name using the pending identifier" do + expect(domain.pending_dkim_record_name).to eq "#{Postal::Config.dns.dkim_identifier}-#{domain.pending_dkim_identifier_string}._domainkey" + end + end + end + + describe "#check_dkim_record" do + let(:domain) { create(:domain, dkim_status: "OK") } + let(:resolver) { instance_double(DNSResolver) } + + before do + allow(domain).to receive(:resolver).and_return(resolver) + end + + context "when there is a pending DKIM key" do + before do + domain.regenerate_dkim_key! + end + + context "when the pending record has been published" do + it "activates and verifies the pending key from a single DNS response" do + responses = [[domain.pending_dkim_record], []] + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}") { responses.shift } + + pending_key = domain.pending_dkim_private_key + domain.check_dkim_record + expect(domain.dkim_private_key).to eq pending_key + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.dkim_status).to eq "OK" + expect(domain.dkim_error).to be_nil + expect(responses).to eq([[]]) + end + end + + context "when multiple pending records have been published" do + before do + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}").and_return([domain.pending_dkim_record, domain.pending_dkim_record]) + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([domain.dkim_record]) + end + + it "keeps the current key active and retains the pending key" do + original_key = domain.dkim_private_key + domain.check_dkim_record + expect(domain.dkim_private_key).to eq original_key + expect(domain.pending_dkim_private_key).to_not be_nil + expect(domain.dkim_status).to eq "OK" + end + end + + context "when the pending record has not been published" do + before do + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}").and_return([]) + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([domain.dkim_record]) + end + + it "keeps the current key active and retains the pending key" do + original_key = domain.dkim_private_key + domain.check_dkim_record + expect(domain.dkim_private_key).to eq original_key + expect(domain.pending_dkim_private_key).to_not be_nil + expect(domain.dkim_status).to eq "OK" + end + end + + context "when the pending record does not match" do + before do + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}").and_return(["v=DKIM1; t=s; h=sha256; p=something;"]) + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([domain.dkim_record]) + end + + it "keeps the current key active and retains the pending key" do + original_key = domain.dkim_private_key + domain.check_dkim_record + expect(domain.dkim_private_key).to eq original_key + expect(domain.pending_dkim_private_key).to_not be_nil + expect(domain.dkim_status).to eq "OK" + end + end + end + + context "when there is no pending DKIM key" do + context "when the record is published" do + before do + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([domain.dkim_record]) + end + + it "marks the record as OK" do + domain.check_dkim_record + expect(domain.dkim_status).to eq "OK" + expect(domain.dkim_error).to be_nil + end + end + + context "when the record is missing" do + before do + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([]) + end + + it "marks the record as missing" do + domain.check_dkim_record + expect(domain.dkim_status).to eq "Missing" + end + end + end + end + describe "#return_path_domain" do it "returns the return path domain" do expect(domain.return_path_domain).to eq "#{Postal::Config.dns.custom_return_path_prefix}.#{domain.name}" diff --git a/spec/requests/domains_controller_spec.rb b/spec/requests/domains_controller_spec.rb new file mode 100644 index 0000000..4024bd2 --- /dev/null +++ b/spec/requests/domains_controller_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "DomainsController", type: :request do + let(:user) { create(:user, admin: true) } + let(:organization) { create(:organization, owner: user) } + let(:server) { create(:server, organization: organization) } + + before do + post "/login", params: { email_address: user.email_address, password: "passw0rd" } + end + + describe "POST /org/:org/servers/:server/domains/:domain/regenerate_dkim" do + context "when the domain's DKIM record is verified" do + let(:domain) { create(:domain, owner: server, dkim_status: "OK") } + + it "generates a pending DKIM key without changing the active key" do + original_key = domain.dkim_private_key + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/regenerate_dkim" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + expect(flash[:notice]).to include("Keep the old DNS record published afterward") + expect(flash[:notice]).to include("queued, held, or available for redelivery") + expect(flash[:notice]).to_not include("you can remove the old record") + domain.reload + expect(domain.dkim_private_key).to eq original_key + expect(domain.pending_dkim_private_key).to_not be_nil + end + end + + context "when the domain's DKIM record is not verified" do + let(:domain) { create(:domain, owner: server, dkim_status: "Missing") } + + it "replaces the active key immediately" do + original_key = domain.dkim_private_key + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/regenerate_dkim" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + domain.reload + expect(domain.dkim_private_key).to_not eq original_key + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.dkim_status).to be_nil + end + end + + context "when a key change is already in progress" do + let(:domain) { create(:domain, owner: server, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "refuses and leaves the pending key untouched" do + existing_pending_key = domain.pending_dkim_private_key + existing_pending_identifier = domain.pending_dkim_identifier_string + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/regenerate_dkim" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + expect(flash[:alert]).to include("already in progress") + domain.reload + expect(domain.pending_dkim_private_key).to eq existing_pending_key + expect(domain.pending_dkim_identifier_string).to eq existing_pending_identifier + end + + it "does not change the active key" do + expect do + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/regenerate_dkim" + end.to_not(change { domain.reload.dkim_private_key }) + end + end + end + + describe "POST /org/:org/domains/:domain/regenerate_dkim" do + let(:domain) { create(:domain, owner: organization, dkim_status: "OK") } + + it "generates a pending DKIM key for an organization-owned domain" do + post "/org/#{organization.permalink}/domains/#{domain.uuid}/regenerate_dkim" + expect(response).to redirect_to(setup_organization_domain_path(organization, domain)) + expect(domain.reload.pending_dkim_private_key).to_not be_nil + end + end + + describe "POST /org/:org/servers/:server/domains/:domain/check with a pending DKIM key" do + let(:domain) { create(:domain, owner: server, dkim_status: "OK") } + let(:resolver) { instance_double(DNSResolver) } + + before do + domain.regenerate_dkim_key! + allow(DNSResolver).to receive(:for_domain).and_return(resolver) + allow(resolver).to receive(:txt).with(domain.name).and_return([domain.spf_record]) + allow(resolver).to receive(:mx).with(domain.name).and_return([]) + allow(resolver).to receive(:cname).and_return([]) + end + + context "when the pending DKIM record has been published" do + before do + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}").and_return([domain.pending_dkim_record]) + end + + it "activates the pending key and tells the user to retain the old record" do + old_record_name = domain.dkim_record_name + new_key = domain.pending_dkim_private_key + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/check" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + expect(flash[:notice]).to include("new DKIM key is now active") + expect(flash[:notice]).to include(old_record_name) + expect(flash[:notice]).to include("queued, held, or available for redelivery") + expect(flash[:notice]).to_not include("can remove") + domain.reload + expect(domain.dkim_private_key).to eq new_key + expect(domain.pending_dkim_private_key).to be_nil + expect(domain.dkim_status).to eq "OK" + end + end + + context "when the pending DKIM record has not been published" do + before do + allow(resolver).to receive(:txt).with("#{domain.pending_dkim_record_name}.#{domain.name}").and_return([]) + allow(resolver).to receive(:txt).with("#{domain.dkim_record_name}.#{domain.name}").and_return([domain.dkim_record]) + end + + it "keeps the current key active and tells the user the new record wasn't verified" do + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/check" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + expect(flash[:alert]).to include("couldn't verify the record for your new DKIM key") + domain.reload + expect(domain.pending_dkim_private_key).to_not be_nil + expect(domain.dkim_status).to eq "OK" + end + end + end + + describe "POST /org/:org/servers/:server/domains/:domain/cancel_dkim_regeneration" do + let(:domain) { create(:domain, owner: server, dkim_status: "OK") } + + before do + domain.regenerate_dkim_key! + end + + it "discards the pending DKIM key" do + post "/org/#{organization.permalink}/servers/#{server.permalink}/domains/#{domain.uuid}/cancel_dkim_regeneration" + expect(response).to redirect_to(setup_organization_server_domain_path(organization, server, domain)) + expect(domain.reload.pending_dkim_private_key).to be_nil + end + end +end diff --git a/spec/views/domains/index.html.haml_spec.rb b/spec/views/domains/index.html.haml_spec.rb new file mode 100644 index 0000000..65b9eef --- /dev/null +++ b/spec/views/domains/index.html.haml_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "domains/index", type: :view do + let(:user) { create(:user, admin: true) } + let(:organization) { create(:organization, owner: user) } + let(:domain) { create(:domain, owner: organization, dkim_status: "OK") } + + before do + assign(:organization, organization) + assign(:domains, [domain]) + assign(:server, nil) + without_partial_double_verification do + allow(view).to receive(:organization).and_return(organization) + allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:page_title).and_return(["Postal"]) + end + end + + context "when no key change is in progress" do + it "does not flag a pending DKIM key" do + render + expect(rendered).to_not include("New DKIM key pending") + end + end + + context "when a key change is in progress" do + before do + domain.regenerate_dkim_key! + end + + it "flags the domain as having a pending DKIM key" do + render + expect(rendered).to include("New DKIM key pending") + end + + it "links the flag to the DNS setup page" do + render + expect(rendered).to include(setup_organization_domain_path(organization, domain)) + end + + it "still shows DKIM as OK because the active key is unaffected" do + render + expect(rendered).to include("domainList__check--ok") + end + end +end diff --git a/spec/views/domains/setup.html.haml_spec.rb b/spec/views/domains/setup.html.haml_spec.rb new file mode 100644 index 0000000..6df9185 --- /dev/null +++ b/spec/views/domains/setup.html.haml_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "domains/setup", type: :view do + let(:user) { create(:user, admin: true) } + let(:organization) { create(:organization, owner: user) } + let(:domain) { create(:domain, owner: organization, dkim_status: "OK") } + + before do + assign(:organization, organization) + assign(:domain, domain) + assign(:server, nil) + without_partial_double_verification do + allow(view).to receive(:organization).and_return(organization) + allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:page_title).and_return(["Postal"]) + end + end + + context "when no key change is in progress" do + it "shows the active record and offers regeneration" do + render + expect(rendered).to include(domain.dkim_record_name) + expect(rendered).to include(domain.dkim_record) + expect(rendered).to include("Regenerate DKIM key") + expect(rendered).to_not include("Key change in progress") + expect(rendered).to_not include("Cancel new DKIM key") + end + end + + context "when a key change is in progress" do + before do + domain.regenerate_dkim_key! + end + + it "shows both the current and the new record" do + render + expect(rendered).to include(domain.dkim_record_name) + expect(rendered).to include(domain.dkim_record) + expect(rendered).to include(domain.pending_dkim_record_name) + expect(rendered).to include(domain.pending_dkim_record) + end + + it "labels which record is active and which must be added" do + render + expect(rendered).to include("Key change in progress") + expect(rendered).to include("Keep this record exactly as it is") + expect(rendered).to include("Add this record as well") + end + + it "tells the user to add the new record rather than replace the old one" do + render + expect(rendered).to include("alongside the record above rather than replacing it") + end + + it "does not describe the active record as one which needs adding" do + render + expect(rendered).to_not include("You need to add a new TXT record") + end + + it "offers cancellation instead of another regeneration" do + render + expect(rendered).to include("Cancel new DKIM key") + expect(rendered).to_not include("Regenerate DKIM key") + end + + it "does not claim the DKIM record is fully good" do + render + expect(rendered).to_not include("Your DKIM record looks good!") + end + end +end