Noisily fail when JWT can't be encoded

[cablett]

Hi @tpitale !

When delivery method is postback, we found there's a hidden failure if a mailbox's JWT secret_file config isn't set up properly, and I think it might be better if there's a bit more noise about that in the logs.

There are two failure modes here for the secret_file key value pair:

1. If the key and value are present in the config, but the file in the value is not readable (for example, secret_file: ~/.nonexistent_file, not enough permissions to read, etc)
2. If the key/value pair is missing entirely or set to nil.

We don't want to crash out entirely, because some other valid mailboxes should still be able to process things (and I know it would just start up anyway because you've told me in the past that it's how mailroom is designed to work :smile_cat:).

I thought something like:

diff --git a/lib/mail_room/jwt.rb b/lib/mail_room/jwt.rb
index da2b4a5..7111e28 100644
--- a/lib/mail_room/jwt.rb
+++ b/lib/mail_room/jwt.rb
@@ -21,7 +21,7 @@ module MailRoom
     end

     def valid?
-      [@header, @secret_path, @issuer, @algorithm].none?(&:nil?)
+      missing_fields.empty?
     end

     def token
@@ -35,5 +35,13 @@ module MailRoom
       }
       ::JWT.encode payload, secret, @algorithm
     end
+
+    def required_fields
+      %i[header secret_path issuer algorithm]
+    end
+
+    def missing_fields
+      required_fields.select{|field| instance_variable_get("@#{field}").nil?}
+    end
   end
 end

That way we can do something like

diff --git a/lib/mail_room/delivery/postback.rb b/lib/mail_room/delivery/postback.rb
index a149f3b..ada4c46 100644
--- a/lib/mail_room/delivery/postback.rb
+++ b/lib/mail_room/delivery/postback.rb
@@ -99,10 +99,19 @@ module MailRoom
       end

       def config_request_jwt_auth(request)
-        return unless @delivery_options.jwt_auth?
+        return if @delivery_options.token_auth || @delivery_options.basic_auth?
+        unless @delivery_options.jwt_auth?
+          # this configuration is not valid for this mailbox
+          @delivery_options.logger.debug({ delivery_method: 'Postback', action: "Misconfigured JWT - missing #{jwt_missing_fields.join(', ')}" })
+          return
+        end

         request.headers[@delivery_options.jwt.header] = @delivery_options.jwt.token
       end
+
+      def jwt_missing_fields
+        self[:jwt].missing_fields
+      end
     end
   end
 end

But I'm not sure if it's intended to add "all the auth" to a single request to ensure at least something works. I suppose what I'm saying is I'm not sure how we'd distinguish between "JWT auth not being used for postback so it's not filled in" and "JWT auth is broken because some fields are missing".

Do you think there's a solution that might work here?

Could be something as simple as logging "none of the configs will work" if none are set up properly.

if !@delivery_options.token_auth || !@delivery_options.basic_auth? || !@delivery_options.jwt_auth?
  # postback will never work unless they are terribly insecure with empty strings for token/username/password
  @delivery_options.logger.debug({ delivery_method: 'Postback', action: "Misconfigured mailbox #{name_of_mailbox}" })

What do you think?

[cablett]

cc @stanhu @nguyenquangminh0711 if you have any thoughts here too :+1:

[tpitale]

I may have been elixir-land too long, but I feel like MailRoom's approach is (or should be) "let it fail". I'm happy to let everything crash with an error if the secret isn't there. maybe? I could be completely forgetting, it's been so long.

[cablett]

That's OK. I think we may want to log something like "failure" if a 500 or similar comes back from the postback. Maybe that's enough. :thinking:

[tpitale]

@cablett I've been away a very long time, sorry for that. If logging is insufficient, feel free to reopen.