Use public key authentication as per v3 by bhuga · Pull Request #15 · github/chatops-controller

bhuga · 2017-05-15T19:36:28Z

Implements https://github.com/github/hubot-classic/pull/2444

# Conflicts: # lib/chatops/controller/test_case_helpers.rb

bhuga · 2017-05-15T22:11:59Z

Far enough along for real eyes.

jbarnette

Various nitpicks inline. I dig it, let me know when I should port kube-me to V3.

jbarnette · 2017-05-16T14:49:16Z

lib/chatops/controller.rb

      end
    end

+    def invalid_time


You could pass timestamp in here to avoid duplicating the header access.

jbarnette · 2017-05-16T14:53:33Z

lib/chatops/controller.rb

+      nonce = request.headers['Chatops-Nonce']
+      timestamp = request.headers['Chatops-Timestamp']
+      begin
+        time = Time.parse(timestamp)


How bout Time.iso8601(timestamp)?

jbarnette · 2017-05-16T14:54:49Z

lib/chatops/controller.rb

+      begin
+        signature_items = signature_header.split(" ", 2)[1].split(",").map { |item| item.split("=", 2) }.to_h
+        signature = signature_items["signature"]
+      rescue NoMethodError


What's this guarding?

I added a comment. It's guarding sundry possible nils in the transformation above.

jbarnette · 2017-05-16T14:56:11Z

lib/chatops/controller.rb

+        decoded_signature = Base64.decode64(signature)
+        digest = OpenSSL::Digest::SHA256.new
+        raise ConfigurationError.new("You need to add a client's public key in .pem format via CHATOPS_AUTH_PUBLIC_KEY") unless ENV["CHATOPS_AUTH_PUBLIC_KEY"].present?
+        if ENV["CHATOPS_AUTH_PUBLIC_KEY"].present?


This will always be true.

jbarnette · 2017-05-16T14:56:47Z

lib/chatops/controller.rb

+        digest = OpenSSL::Digest::SHA256.new
+        raise ConfigurationError.new("You need to add a client's public key in .pem format via CHATOPS_AUTH_PUBLIC_KEY") unless ENV["CHATOPS_AUTH_PUBLIC_KEY"].present?
+        if ENV["CHATOPS_AUTH_PUBLIC_KEY"].present?
+          public_key = OpenSSL::PKey::RSA.new(ENV["CHATOPS_AUTH_PUBLIC_KEY"])


How bout a local for ENV["CHATOPS_AUTH_PUBLIC_KEY"]? You're using it in a few places.

jbarnette · 2017-05-16T14:57:27Z

lib/chatops/controller.rb

+          authenticated = public_key.verify(digest, decoded_signature, signature_string)
+        end
+        if !authenticated && ENV["CHATOPS_AUTH_ALT_PUBLIC_KEY"].present?
+          public_key = OpenSSL::PKey::RSA.new(ENV["CHATOPS_AUTH_ALT_PUBLIC_KEY"])


Same 💭 about a local for these ENV accesses, no big deal though.

jbarnette · 2017-05-16T15:02:58Z

spec/lib/chatops/controller_spec.rb

+    expect(response.status).to eq 403
+  end
+
+  it "doesn't allow requests more than 1 minute old" do


Is it worth testing the future too?

mistydemeo · 2017-05-16T17:57:10Z

lib/chatops/controller.rb

+        response.headers['Chatops-SignatureString'] = signature_string
+        decoded_signature = Base64.decode64(signature)
+        digest = OpenSSL::Digest::SHA256.new
+        raise ConfigurationError.new("You need to add a client's public key in .pem format via CHATOPS_AUTH_PUBLIC_KEY") unless ENV["CHATOPS_AUTH_PUBLIC_KEY"].present?


Maybe wrap this into a block like this (and also assign it to a var like John suggests)?

begin public_key = ENV.fetch("CHATOPS_AUTH_PUBLIC_KEY") rescue KeyError raise ConfigurationError.new("You need to add a client's public key in .pem format via CHATOPS_AUTH_PUBLIC_KEY") end

mistydemeo · 2017-05-16T18:00:05Z

spec/lib/chatops/controller_spec.rb


 describe ActionController::Base, type: :controller do
-  controller do
+ controller do


Did you mean to tweak the whitespace here?

bhuga · 2017-05-17T00:44:57Z

Yalls excellent comments resulted in a huge refactoring to something more rails-reasonable. Ready for 👀 again.

jbarnette · 2017-05-17T13:29:59Z

lib/chatops/controller.rb

+      before_action :ensure_valid_chatops_timestamp, if: :should_authenticate_chatops?
+      before_action :ensure_valid_chatops_signature, if: :should_authenticate_chatops?
+      before_action :ensure_valid_chatops_nonce, if: :should_authenticate_chatops?
+      before_action :ensure_chatops_authenticated, if: :should_authenticate_chatops?


⛳️

with_options if: :should_authenticate_chatops? do before_action :ensure_valid_chatops_url before_action :ensure_valid_chatops_timestamp before_action :ensure_valid_chatops_signature before_action :ensure_valid_chatops_nonce before_action :ensure_chatops_authenticated end

jbarnette · 2017-05-17T13:31:15Z

lib/chatops/controller.rb

+      signature_string = [@chatops_url, @chatops_nonce, @chatops_timestamp, body].join("\n")
+      # We return this just url client debugging.
+      response.headers['Chatops-SignatureString'] = signature_string
+      public_key = ENV["CHATOPS_AUTH_PUBLIC_KEY"]


"I know, I'll use single quotes on odd-numbered lines and double quotes on even lines!"

"I know, I'll use single quotes on odd-numbered lines and double quotes on even lines!"

It was more like "I know a great way to get @jbarnette riled up!"

jbarnette · 2017-05-17T13:32:39Z

lib/chatops/controller.rb

+      response.headers['Chatops-SignatureString'] = signature_string
+      public_key = ENV["CHATOPS_AUTH_PUBLIC_KEY"]
+      alt_public_key = ENV["CHATOPS_AUTH_ALT_PUBLIC_KEY"]
+      raise ConfigurationError.new("You need to add a client's public key in .pem format via CHATOPS_AUTH_PUBLIC_KEY") unless public_key.present?


I think these are fine inline, but they seem like the kind of thing we'd normally break out to a Chatops.auth_public_key accessor etc.

jbarnette · 2017-05-17T13:33:12Z

lib/chatops/controller.rb

+    def ensure_valid_chatops_url
+      base_url = ENV["CHATOPS_AUTH_BASE_URL"]
+      raise ConfigurationError.new("You need to set the server's base URL to authenticate chatops RPC via CHATOPS_AUTH_BASE_URL") unless base_url.present?
+      @chatops_url = base_url + request.path


Any need to care about trailing /?

Any need to care about trailing /?

Sorta. I added another linter here.

That sort of thing is why the Chatops-Signature-String to so what we signed; hopefully that will make such errors easier to find.

jbarnette · 2017-05-17T13:33:34Z

lib/chatops/controller.rb

+      signature_header = request.headers['Chatops-Signature']
+
+      begin
+        # 'Chatops-Signatre: Signature keyid=foo,signature=abc123' => { "keyid"" => "foo", "signature" => "abc123" }


-Signatre, "keyid""

jbarnette · 2017-05-17T13:34:06Z

lib/chatops/controller.rb

+        @chatops_signature = signature_items["signature"]
+      rescue NoMethodError
+        # The signature header munging, if something's amiss, can produce a `nil` that raises a
+        # no method error. We'll just carry on; the nil signature will raise below


Wouldn't want error messages to go out of date

bhuga · 2017-05-17T15:32:53Z

All feedback adopted. Feeling kinda ashamed yall had so much to find. Thanks again.

jbarnette · 2017-05-17T15:52:30Z

lib/chatops.rb

@@ -0,0 +1,21 @@
+module ChatOps


This should be Chatops. Or the file should be chat_ops.rb. Actually let's talk about class and gem names for a sec since they're inconsistent + this is going to be public soon. If it's gonna stick to RubyGems conventions (underscores separate words, dashes separate namespaces), one of these first two approaches would be good:

gem sample path constant

chatops-controller lib/chatops/controller.rb Chatops::Controller

chat_ops-controller lib/chat_ops/controller.rb ChatOps::Controller

These are not great options:

gem sample path constant

chatops_controller lib/chatops_controller.rb ChatopsController

chat_ops_controller lib/chat_ops_controller.rb ChatopsController

Myself, I prefer the first example, because underscores in project names are kinda ugly and mixing em with dashes doubly so. But you could've guessed that. 😁

bhuga · 2017-05-17T16:03:57Z

Actually let's talk about class and gem names for a sec since they're inconsistent + this is going to be public soon.

Good call. I will go ahead and wrap that into this pull since it's version 3.0.

Without the arg, this seems to work only in some versions of rails

bhuga · 2017-05-17T18:21:29Z

Okay, the gem's renamed. One more time, this time with feeling!

bhuga · 2017-05-17T19:48:15Z

Awesome awesome thanks again. Looking good. Next PR is the README cleanup this needs before going open-source.

keithduncan

This is so good and the tests look great too

keithduncan · 2017-05-18T02:07:16Z

lib/chatops.rb

+  end
+
+  def self.alt_public_key
+    ENV["CHATOPS_AUTH_ALT_PUBLIC_KEY"]


We actually don’t need the main v alt split under v3 with public key auth, because the key used to sign is self identified with the keyid param 🤔

We can have a relation of keyid => publickey hosting many live keys. Rolling keys with this scheme would be much simpler than the multiple deploys to populate and move keys from alt to main.

I think it would be worth baking that concept into the gem from the beginning. We can have a default backend that looks the key up in the ENV, but otherwise the notion of supporting multiple and even per-user keys is pressed from the beginning.

keithduncan · 2017-05-18T02:23:00Z

lib/chatops/controller.rb

+      # We return this just to aid client debugging.
+      response.headers["Chatops-Signature-String"] = signature_string
+      raise ConfigurationError.new("You need to add a client's public key in .pem format via #{Chatops.public_key_env_var_name}") unless Chatops.public_key.present?
+      if signature_valid?(Chatops.public_key, @chatops_signature, signature_string) ||


As above I think the key to verify with should be hinted on the incoming keyid field.

keithduncan · 2017-05-18T02:23:23Z

lib/chatops/controller.rb

-      unless authenticated
-        render :status => :forbidden, :plain => "Not authorized"
+      if Chatops.auth_base_url[-1] == "/"
+        raise ConfigurationError.new("Don't include a trailing slash in #{Chatops.auth_base_url_env_var_name}; the rails path will be appended and it must match exactly.")


💯 error handling here.

What do you think about verifying this when included so that you get the error on boot rather than on first request?

If we can detect that the last character is slash, and we don't want slashes, would it be a better user experience to just remove the trailing slash? That would reduce the back and forth a little bit.

keithduncan · 2017-05-18T02:24:54Z

lib/chatops/controller.rb

+        # "Chatops-Signature: Signature keyid=foo,signature=abc123" => { "keyid"" => "foo", "signature" => "abc123" }
+        signature_items = signature_header.split(" ", 2)[1].split(",").map { |item| item.split("=", 2) }.to_h
+        @chatops_signature = signature_items["signature"]
+      rescue NoMethodError


Could we mandate Ruby 2.3 and use the safe nil navigation operator?

Might be worth surverying the existing CRPC commands to see if anything is < 2.3.

It would also be possible to use try on these since ActiveSupport should be around.

keithduncan · 2017-05-18T02:26:09Z

lib/chatops/controller.rb

+    def ensure_valid_chatops_timestamp
+      @chatops_timestamp = request.headers["Chatops-Timestamp"]
+      time = Time.iso8601(@chatops_timestamp)
+      if !(time > 1.minute.ago && time < 1.minute.from_now)


Super minor but if ! can be swapped for unless

keithduncan · 2017-05-18T02:53:19Z

lib/chatops/controller.rb

+    end
+
+    def request_is_chatop?
+      (chatop_names + [:list]).include?(params[:action].to_sym)


It’s worth requiring at least Ruby 2.2.1 for the symbols GC if we String#to_sym user data

technicalpickles

Sorry I'm late to the review party.

Aside from line-level comments, it looks like the README.md wasn't updated to talk about setting up private/public keys at all.

technicalpickles · 2017-05-30T14:24:31Z

lib/chatops/controller.rb

-      unless authenticated
-        render :status => :forbidden, :plain => "Not authorized"
+      if Chatops.auth_base_url[-1] == "/"
+        raise ConfigurationError.new("Don't include a trailing slash in #{Chatops.auth_base_url_env_var_name}; the rails path will be appended and it must match exactly.")


If we can detect that the last character is slash, and we don't want slashes, would it be a better user experience to just remove the trailing slash? That would reduce the back and forth a little bit.

technicalpickles · 2017-05-30T14:26:51Z

lib/chatops/controller.rb

+        # "Chatops-Signature: Signature keyid=foo,signature=abc123" => { "keyid"" => "foo", "signature" => "abc123" }
+        signature_items = signature_header.split(" ", 2)[1].split(",").map { |item| item.split("=", 2) }.to_h
+        @chatops_signature = signature_items["signature"]
+      rescue NoMethodError


Might be worth surverying the existing CRPC commands to see if anything is < 2.3.

It would also be possible to use try on these since ActiveSupport should be around.

technicalpickles · 2017-05-30T14:27:57Z

lib/chatops/controller.rb

+
+      begin
+        # "Chatops-Signature: Signature keyid=foo,signature=abc123" => { "keyid"" => "foo", "signature" => "abc123" }
+        signature_items = signature_header.split(" ", 2)[1].split(",").map { |item| item.split("=", 2) }.to_h


TIL split with a limit

technicalpickles · 2017-05-30T14:30:04Z

lib/chatops/controller.rb

+
+      begin
+        # "Chatops-Signature: Signature keyid=foo,signature=abc123" => { "keyid"" => "foo", "signature" => "abc123" }
+        signature_items = signature_header.split(" ", 2)[1].split(",").map { |item| item.split("=", 2) }.to_h


This is an impressive one-liner, but I am wondering if it'd be easier to grok broken out on multiple lines. That might be a lot in the controller, so it could be logical to have it in some utility method.

technicalpickles · 2017-05-30T14:30:31Z

lib/chatops/controller.rb

+
+    def ensure_valid_chatops_timestamp
+      @chatops_timestamp = request.headers["Chatops-Timestamp"]
+      time = Time.iso8601(@chatops_timestamp)


You can also do @chatops_timestamp.to_time

Ben Lavender added 8 commits May 15, 2017 14:27

Use public key authentication as per v3

a73b042

Merge branch 'master' into v3-auth

e5acce3

# Conflicts: # lib/chatops/controller/test_case_helpers.rb

Fix a rails 5 warning

cee1f19

actions were moved to execute_chatop in the rails 5 branch

83e14ab

Bump to protocol version 3

c7e3cf2

Keep major gem version in sync with protocol version

7045118

Replace the order of nonce and timestamp

a2f083e

Use full signature header

24d89e2

bhuga requested review from jbarnette and mistydemeo May 15, 2017 22:12

Remove X- from chatops headers

4c9f560

jbarnette approved these changes May 16, 2017

View reviewed changes

mistydemeo suggested changes May 16, 2017

View reviewed changes

Ben Lavender added 3 commits May 16, 2017 19:40

Huge refactoring

a4a686f

Add a test for 1 minute in the future

dec6b89

Document what this horrible translation does

eb097d5

jbarnette approved these changes May 17, 2017

View reviewed changes

Ben Lavender added 7 commits May 17, 2017 10:15

Clean up some before filters via with_options

8829162

Fix a docs typo

76e0ccb

Switch to some ChatOps.method's instead of direct ENV access

a7937c1

Use methods to fetch env var names

8c5dcd6

Wouldn't want error messages to go out of date

Add years to @jbarnette's life

45bac4c

Raise an error if a trailing slash is on the auth base url

23d73c3

Fix a whitespace error

ac27ac1

jbarnette reviewed May 17, 2017

View reviewed changes

Rename from ChatOps to Chatops

5d07d75

Rename from chatops_controller to chatops-controller

bc55564

bhuga force-pushed the v3-auth branch from 5daa999 to bc55564 Compare May 17, 2017 16:13

Ben Lavender added 2 commits May 17, 2017 11:26

Fix dummy applications loading of chatops controller

3559cee

Use argument form of with_options

165df0f

Without the arg, this seems to work only in some versions of rails

For the auto-requirers

531bd0e

bhuga merged commit 5586435 into master May 17, 2017

bhuga deleted the v3-auth branch May 17, 2017 19:48

keithduncan reviewed May 18, 2017

View reviewed changes

technicalpickles reviewed May 30, 2017

View reviewed changes

gem	sample path	constant
`chatops-controller`	`lib/chatops/controller.rb`	`Chatops::Controller`
`chat_ops-controller`	`lib/chat_ops/controller.rb`	`ChatOps::Controller`

gem	sample path	constant
`chatops_controller`	`lib/chatops_controller.rb`	`ChatopsController`
`chat_ops_controller`	`lib/chat_ops_controller.rb`	`ChatopsController`

Conversation

bhuga commented May 15, 2017

Uh oh!

bhuga commented May 15, 2017

Uh oh!

jbarnette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhuga commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhuga commented May 17, 2017

Uh oh!

jbarnette May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhuga commented May 17, 2017

Uh oh!

bhuga commented May 17, 2017

Uh oh!

bhuga commented May 17, 2017

Uh oh!

keithduncan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicalpickles left a comment

Choose a reason for hiding this comment

Uh oh!

jbarnette May 17, 2017 •

edited

Loading