JobsController: Accept base64 encoded payloads #18

pjambet · 2017-01-25T21:09:13Z

Pings can now be base64 encoded. Simply base 64 encode the public id and
pass the ?use_base64=true query string parameter.

This is useful when using RSA encryption and sending a publicly
encrypted payload.

Dealing with base64 encoded payload simplifies integration with other
platforms (such as the JVM) since we're not relying on some internal
ruby byte representation like the default implementation does.

werkshy

Sure. We can reduce this added complexity later (see issue #19).

pjambet · 2017-01-25T21:37:41Z

#19 is interesting, but as long as we have clients relying on the current behavior we can't easily remove the whole encryption layer.

I suggest merging this and removing the whole unnecessary encryption handling later on.

pjambet · 2017-01-25T21:37:48Z

You're faster than me!

pjambet · 2017-01-25T21:43:22Z

I just created a v1.0 tag https://github.com/harrystech/cronut/tree/v1.0 that we can use as a reference later on when we remove encryption support.

tvogels01 · 2017-01-25T22:30:19Z

README.md

+end
+
+conn.post "/ping/", {:public_id => OpenSSL::PKey::RSA.new(public_key).public_encrypt(str)}
+```


I would leave a pointer to ping-me-maybe here which does this for you. https://github.com/harrystech/ping-me-maybe

tvogels01 · 2017-01-25T22:31:37Z

README.md

+and this can be quite tricky to reproduce when trying to ping cronut from
+another environment, such as the JVM.
+In this can you can base 64 encode the encrypted bytes and pass the
+`use_base64=true` querystring parameter.


This will look funny one day, meaning when we've turned off encryption, why would we still pass this parameter in, always? Would it be simpler to have a new route? /ping/v2? or /ping64?

That's an interesting point but I don't really see this as a major problem.

If we decide to turn off the whole encryption support, then clients will have to be updated and they will stop passing this flag as well as sending unencrypted payloads.

If we keep encryption, then it seems like an "ok" approach that doesn't require to maintain multiple endpoints.

I'm just not sure what the multiple endpoints approach really gets us since the behavior is fairly similar.

Do other people have strong opinions here? @werkshy @blahblahblah-

Note that there is also ignorance involved as to how routes work. But basically,

GET /ping ping(use_base64=False) GET /v2/ping ping(use_base64=True)

is what I had in mind ...

Do you think that having two routes is a clear win over a single one and the switch being made by checking the presence & value of the query string param?

They both seem fairly similar to me and I don't see one approach as strictly better than the other one.

I care less than this conversation may suggest ... but we do have cronut out in the open and while we can "force" our clients to use base64 or abandon this paranoid approach all together, you'd be dropping a published interface with the proposed approach.

Option A
(1) Add new endpoint that expects payload to be base64-encoded.

Option B
(1) Add optional parameter to enable base64
(2) Move all of our old clients to use base64 and sending that parameter
(3) Drop the optional parameter in all our user code since it's now silly and the default and only possible value.
(4) Deal with the wrath of anybody using cronut (along with ping-me-maybe, maybe) where their clients no longer.

I'd go with (A) -- too lazy to think beyond step 1 :-)

But (B) doesn't need to be as "complicated" since we don't need to force all our clients to switch to base 64, especially since there seems to be motivation to actually stop sending encrypted data.

It's also important to note, that since we're not providing a hosted solution, we don't really need to worry that much about other clients since releasing a new version doesn't mean that we'll break their clients. I actually created a v1.0 tag for this purpose, so that if we decide to drop encryption support we can drop a line in a readme with a link to the release that does support encryption.

That being said, I made the changes and added a new route under v2/ping

tvogels01 · 2017-01-25T22:33:42Z

app/controllers/jobs_controller.rb

@@ -105,6 +105,9 @@ def ping
    begin
      str = params[:public_id]
      if Encryptor.enabled?
+        if use_base64?
+          str = Base64.decode64(str)


What was difficult about debugging this? If was the silence of cronut when the encryption was wrong, maybe we can add a log statement in case of failure. True also for Base64 ... if it can't be decoded, should that be quiet?

That's a good point, I actually added locally a puts statement in the rescue block to give some information in the logs about the failure. I'll add it again, it's pretty useful!

pjambet · 2017-01-26T17:20:03Z

I added a puts statement for e.message in main rescue block, it'll be helpful to see what happened in the logs.

Also added a note to the README and changed the rescue from Exception to StandardError

Pings can now be base64 encoded. Simply base 64 encode the public id and pass the `?use_base64=true` query string parameter. This is useful when using RSA encryption and sending a publicly encrypted payload. Dealing with base64 encoded payload simplifies integration with other platforms (such as the JVM) since we're not relying on some internal ruby byte representation like the default implementation does.

rescuing StandardError is preferred over Exception since there are some Exception we might not want to rescue from.

pjambet · 2017-01-27T17:59:42Z

Both routes use the same endpoint, and you could pass ?use_base64=false to v2/ping, which I think is fine and doesn't need to be documented since the use_base64 flag is just used internally.

tvogels01 · 2017-01-30T16:07:45Z

BTW ... don't let my silly comments distract you from merging.

pjambet assigned werkshy, pettyjamesm and blahblahblah- Jan 25, 2017

werkshy approved these changes Jan 25, 2017

View reviewed changes

pjambet mentioned this pull request Jan 25, 2017

Authentication Is Too Complicated #19

Open

tvogels01 reviewed Jan 25, 2017

View reviewed changes

pjambet force-pushed the add-base64-support branch from 8575f42 to 49180bd Compare January 26, 2017 17:17

pjambet force-pushed the add-base64-support branch from 81fe646 to 8a666e4 Compare January 27, 2017 17:55

pjambet added 2 commits January 27, 2017 09:57

JobsController: rescue StandardError

e9eb268

rescuing StandardError is preferred over Exception since there are some Exception we might not want to rescue from.

pjambet force-pushed the add-base64-support branch from 8a666e4 to e9eb268 Compare January 27, 2017 17:57

pjambet merged commit 56deac2 into master Jan 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JobsController: Accept base64 encoded payloads #18

JobsController: Accept base64 encoded payloads #18

pjambet commented Jan 25, 2017

werkshy left a comment

pjambet commented Jan 25, 2017

pjambet commented Jan 25, 2017

pjambet commented Jan 25, 2017

tvogels01 Jan 25, 2017

pjambet Jan 26, 2017

tvogels01 Jan 25, 2017

pjambet Jan 26, 2017

tvogels01 Jan 26, 2017

pjambet Jan 26, 2017

tvogels01 Jan 27, 2017

pjambet Jan 27, 2017

tvogels01 Jan 25, 2017

pjambet Jan 26, 2017

pjambet commented Jan 26, 2017

pjambet commented Jan 27, 2017

tvogels01 commented Jan 30, 2017

JobsController: Accept base64 encoded payloads #18

JobsController: Accept base64 encoded payloads #18

Conversation

pjambet commented Jan 25, 2017

werkshy left a comment

Choose a reason for hiding this comment

pjambet commented Jan 25, 2017

pjambet commented Jan 25, 2017

pjambet commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjambet commented Jan 26, 2017

pjambet commented Jan 27, 2017

tvogels01 commented Jan 30, 2017