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

Add support for encrypting data sent to Geckoboard. #18

Merged
merged 3 commits into from Oct 7, 2013

Conversation

jeremyajohnson
Copy link
Contributor

Geckoboard is adding support for a feature called client encryption that allows encrypting data sent to Geckoboard and is decrypted client-side after entering the password used for encryption.

This pull request proposes enabling encryption using a decorator argument as follows:

@number_widget(encrypted=True)
def test_number_widget(request):
    ...

@jcassee
Copy link
Owner

jcassee commented Oct 1, 2013

Hi Jeremy,

Thanks for developing this feature!

PyCrypto is required for this feature, and I think it should be an optional dependency. Do you agree? That would mean adding the dependency to setup.py using extras_require. (See the distribute docs)

Could you also add some documentation in __init__.py? Thanks!

Joost

@jeremyajohnson
Copy link
Contributor Author

Hi Joost,

Thanks for the feedback. Yes, I think it makes sense to make it an optional
dependency. I'm new to python setup tools. Can you clarify what steps a new
user would need to take in order to indicate that they want to use the
optional encryption feature? The example in the docs links it to a console
script, which doesn't apply here. Also, how do I make the actual import of
PyCrypto depend on the feature selection (so the "from Crypto import" lines
don't fail)?

Sure, I'll add some documentation.

Jeremy

On Tue, Oct 1, 2013 at 3:26 PM, Joost Cassee notifications@github.comwrote:

Hi Jeremy,

Thanks for developing this feature!

PyCrypto is required for this feature, and I think it should be an
optional dependency. Do you agree? That would mean adding the dependency to
setup.py using extras_require. (See the distribute docshttp://pythonhosted.org/distribute/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies
)

Could you also add some documentation in init.py? Thanks!

Joost


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-25497289
.

@jcassee
Copy link
Owner

jcassee commented Oct 2, 2013

Although I have never used optional dependencies myself, I expect this would be the right way to extend setup.py:

setup(
    ...
    extras_require = {
        'encryption':  ["PyCrypt"],
    }
    ...
)

If another projects depends on django-geckoboard with encryption, it could do so by adding install_requires = ["django-geckoboard[encryption]"] to its setup.py script.

The import itself would go something like this:

try:
    from Crypto.Cipher import AES
    from Crypto import Random
    encryption_enabled = True
except ImportError:
    encryption_enabled = False

@jeremyajohnson
Copy link
Contributor Author

I'm still not clear how this would work in my use case. I don't have a separate setup.py for my Django project in which I would specify that I wanted to use the encryption option. Currently, I would just do pip install django-geckoboard and then pip freeze | requirements.txt. (When we spin up a server, we just run pip install -r requirements.txt). I would expect that django-geckoboard would install any dependencies it needed, whether I used them or not. If I leave PyCrypto as optional as we are discussing, then how would a new developer know that they needed to install it? Something like this perhaps?

class WidgetDecorator(object):
    ...
    def __new__(cls, *args, **kwargs):
        obj = object.__new__(cls)
        obj._encrypted = None
        if 'encrypted' in kwargs:
            if not encryption_enabled:
                raise GeckoboardException('pycrypto package required for use of encryption')
            obj._encrypted = kwargs.pop('encrypted')        

@jcassee
Copy link
Owner

jcassee commented Oct 3, 2013

Although it is not well documented, pip also supports the optional dependency syntax. See also the links in this StackOverflow question

The problem with making the dependency hard is that PyCrypto is a C extension, which makes it more difficult to install than a plain Python one.

The way you suggest checking if PyCrypt is installed sounds good.

- Fix comment (character encoding issue)
@jeremyajohnson
Copy link
Contributor Author

Hi Joost,

I finished making pycrypto a dependency on the encryption feature and also finished adding comments to __init__.py. Let me know if there's anything else needed to finish the pull request. Thanks for your help.

Jeremy

jcassee added a commit that referenced this pull request Oct 7, 2013
Add support for encrypting data sent to Geckoboard.
@jcassee jcassee merged commit 5002174 into jcassee:master Oct 7, 2013
@jcassee
Copy link
Owner

jcassee commented Oct 7, 2013

Thanks a lot, Jeremy!

@jcassee
Copy link
Owner

jcassee commented Oct 7, 2013

One question, is encryption only possible for JSON output, or XML too. I cannot find any docs in the Geckoboard knowledgebase...

@jeremyajohnson
Copy link
Contributor Author

JSON only. I've attached the document Rob sent me. I don't think it's in
their public documentation, yet. I'm beta-testing, and I think they are
enabling encryption on an account-by-account basis right now.

JJ

On Mon, Oct 7, 2013 at 1:26 PM, Joost Cassee notifications@github.comwrote:

One question, is encryption only possible for JSON output, or XML too. I
cannot find any docs in the Geckoboard knowledgebase...


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-25842020
.

@jcassee
Copy link
Owner

jcassee commented Oct 7, 2013

Thanks for the info. The attachment did not come through, but that's alright. I think the decorator should reject sending XML in plain text if encryption is requested, as the data is probably sensitive. Not doing this would even open up the possibility of a man-in-the-middle attack where the attacker changes the request to add the format parameter to make the widget output XML instead of JSON.

I also merged Rob's pull request. Although the tests still pass, could you check I did not make any mistake?

@jcassee
Copy link
Owner

jcassee commented Oct 7, 2013

Last commit raises ValueError on encrypted XML output.

@jeremyajohnson
Copy link
Contributor Author

Excellent point. Your changes look good to me.

On Mon, Oct 7, 2013 at 1:47 PM, Joost Cassee notifications@github.comwrote:

Thanks for the info. The attachment did not come through, but that's
alright. I think the decorator should reject sending XML in plain text if
encryption is requested, as the data is probably sensitive. Not doing this
would even open up the possibility of a man-in-the-middle attack where the
attacker changes the request to add the format parameter to make the widget
output XML instead of JSON.

I also merged Rob's pull request. Although the tests still pass, could you
check I did not make any mistake?


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-25843651
.

@jcassee
Copy link
Owner

jcassee commented Oct 7, 2013

Great, thanks. If Rob also gives green light I will upload a new version to PyPI.

@jeremyajohnson
Copy link
Contributor Author

Let me know when you've updated PyPI. I'm anxious to update our production
code.
JJ

On Mon, Oct 7, 2013 at 2:09 PM, Joost Cassee notifications@github.comwrote:

Great, thanks. If Rob also gives green light I will upload a new version
to PyPI.


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-25845509
.

@jcassee
Copy link
Owner

jcassee commented Oct 10, 2013

I have not heard from Rob, so I'm assuming everything is alright. I'll make a release.

@jeremyajohnson
Copy link
Contributor Author

Awesome, thanks!

On Thu, Oct 10, 2013 at 3:48 AM, Joost Cassee notifications@github.comwrote:

I have not heard from Rob, so I'm assuming everything is alright. I'll
make a release.


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-26043828
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants