Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thoughts on Message Digest #23

Closed
nunosilva800 opened this issue Mar 4, 2016 · 7 comments
Closed

Thoughts on Message Digest #23

nunosilva800 opened this issue Mar 4, 2016 · 7 comments

Comments

@nunosilva800
Copy link

I'm trying to use this gem in an service-oriented architecture, where there are multiple writers to SQS queues.

The Rails app is the "center-piece" and also where all job processing is performed.

Due to the checks of a message-digest and the "origin=AEJ" header (https://github.com/tawan/active-elastic-job/blob/master/lib/active_elastic_job/rack/sqs_message_consumer.rb#L59) it is currently very hard to have publishers to SQS, other than the Rails app (with the same secret_key_base).

I believe I'll be able to "fake" these headers so that I can have my other publishers' jobs accepted. (Either that or I'll fork this gem and delete that check :P)

Either way, this got me thinking if this feature isn't "too much", and maybe it should be configurable (at least to turn on/off).

I think the check for localhost requests is all ok, but other than that it feels like this gem is doing too much. It's the user's job to ensure that their queues aren't publicly accessible and AWS provides all the tools to make that happen.

In my case the other publishers aren't rails/ruby apps, but even if they were, I'd have to ensure those other rails apps had the same secret_key_base, which is a bad thing.

@tawan
Copy link
Collaborator

tawan commented Mar 6, 2016

Hi @Onumis ,

Thanks for your input. I can understand your points, however I'd rather leave the security measures in place for now. The focus of this gem is simplicity and ease of use. A user should be able to use the gem and the gem takes care of protecting the web-environment from being spoofed into processing jobs. Even though the middleware can be disabled by an environment variable in the web environment, I'd rather keep the additional layers of protection in place - a user might forget to set the environment variable.

You can of course fork the gem and adapt it to your needs, but there is another option:
You can write your own middleware and replace the SQSMessageConsumer. In your middleware you can remove checks that you don't need. Have a look on Rails' guide on Rack. It explains how to delete and insert middleware.

I leave this issue open, let's see how others think about it.

Thanks again for your input.

@nunosilva800
Copy link
Author

@tawan thanks! I'm gonna give a try with by writing my own middleware and swapping it with SQSMessageConsumer.

@nunosilva800
Copy link
Author

By the way @tawan you can avoid

a user might forget to set the environment variable.

If you instead use an env var to activate the worker: ENABLE_SQS_CONSUMER=TRUE

That way the web environment keeps as is, and the user has to explicitly activate this on the worker environment.

@tawan
Copy link
Collaborator

tawan commented Mar 14, 2016

@Onumis That is correct. The opt-in environment variable would have been a better solution. I don't know if I can introduce this change and be backwards compatible, though. I'll think about it, but thanks for noticing.

@nunosilva800
Copy link
Author

Better start with a deprecation warning and only do the change in a future release.

@zaaroth
Copy link

zaaroth commented Mar 14, 2016

I am not sure that would be better. In my case, for instance, I got a single web environment and a few worker environments. This is probably the average setup because you need a worker environment per queue. I believe simpler applications would use only a single queue, but most are likely to use more than one. At least to me it is easier to disable the consumer on the web environment (once) and be free to create more queues not having to worry with that.

@tawan
Copy link
Collaborator

tawan commented Apr 3, 2016

@zaaroth Makes sense. I'll leave it as it is.

@tawan tawan closed this as completed Jul 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants