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

[WIP] encrypted_ props #254

Closed
wants to merge 1 commit into from
Closed

[WIP] encrypted_ props #254

wants to merge 1 commit into from

Conversation

nicolaskruchten
Copy link
Contributor

This PR is definitely incomplete but shows a possible direction for built-in support for server-side encrypted props: basically transparently encrypting and decrypting any prop whose name starts with encrypted_. This would go hand in hand with a new dcc.Store component which would accept encrypted_* props and render into the DOM as null, as a safer replacement for using hidden <div> elements to store data on the client. The current downsides of using hidden <div>s are that 1) data is sent to the client in the clear which developers might not want clients to have access to and 2) data can be manipulated by the client to trick the app into doing undesirable things. Encrypting/signing the data solves both of these problems.

If any prop is to be encrypted, the user will need to provide either an encryption_keystring which is used to generate a fernet_key or specify the fernet_key directly if the built-in KDF is somehow insufficient.

Major limitations of this PR today:

  • no encryption of initial layout result
  • no tests
  • no exception handling for data that has been tampered with

I'm submitting it here for feedback on the implementation direction (the idea of providing a generic mechanism for encryption, this specific mechanism, the implications for specifying keys etc) rather than the quality of the implementation :) If the direction isn't good, I won't invest more time in finishing it!

@nicolaskruchten nicolaskruchten requested a review from chriddyp May 4, 2018 02:38
@chriddyp
Copy link
Member

chriddyp commented May 27, 2018

👍 on encrypted props!

no encryption of initial layout result

Instead of putting this in dash.py, I think it would be easier to either:

  1. Put this in Dash's base component class (https://github.com/plotly/dash/blob/master/dash/development/base_component.py)
  2. Or make the base component more extensible by allowing component property transforms and put the logic in the dash-core-components

Some background:

Right now, a components properties are serialized directly as JSON:

def to_plotly_json(self):
# Add normal properties
props = {
p: getattr(self, p)
for p in self._prop_names # pylint: disable=no-member
if hasattr(self, p)
}
# Add the wildcard properties data-* and aria-*
props.update({
k: getattr(self, k)
for k in self.__dict__
if any(k.startswith(w) for w in
self._valid_wildcard_attributes) # pylint:disable=no-member
})
as_json = {
'props': props,
'type': self._type, # pylint: disable=no-member
'namespace': self._namespace # pylint: disable=no-member
}
return as_json

with the plotly.utils.PlotlyJSONEncoder:
https://github.com/plotly/plotly.py/blob/1de7b493c8bb12f5fefd109de413eeaaa634ab8b/plotly/utils.py#L130-L139

here:

dash/dash/dash.py

Lines 163 to 167 in 6a1809f

return flask.Response(
json.dumps(layout,
cls=plotly.utils.PlotlyJSONEncoder),
mimetype='application/json'
)

and here:

dash/dash/dash.py

Lines 511 to 530 in 6a1809f

def wrap_func(func):
@wraps(func)
def add_context(*args, **kwargs):
output_value = func(*args, **kwargs)
response = {
'response': {
'props': {
output.component_property: output_value
}
}
}
return flask.Response(
json.dumps(response,
cls=plotly.utils.PlotlyJSONEncoder),
mimetype='application/json'
)
self.callback_map[callback_id]['callback'] = add_context

Now, if we added encrypt logic to the base component class (method 1), we'd modify the to_plotly_json method and add the encrypt and decrypt functions to that component class.

If we put this encrypt logic in dash-core-components (method 2), then we'd need to transform arguments before request and from response, perhaps by adding an overridable function in the dash.development.base_component.Component class that its super class's to_plotly_json would call. Then, this logic would go in the dcc.Store component, like:
https://github.com/plotly/dash-core-components/blob/7c2b102c680d9fe0cb9df61f1682feccfa3ddc05/dash_core_components/__init__.py#L59-L64

if component.__name__ == 'Store':
    def transform(attribute, value):
          if attribute.startswith('encrypt'):
                return encrypt(value)
    def inverse_transform(attribute, value):
          if attribute.startswith('encrypt'):
                return dencrypt(value)
    component.transform = transform
    component.inverse_transform = inverse_transform

With both methods, we would need to add a custom JSON decoder when deserializing (Instead of json.loads(obj), it'd need to be json.loads(obj, cls=DashJSONDecoder) (https://docs.python.org/2/library/json.html#encoders-and-decoders). In method 1, we could just use the same DashJSONDecoder. In method 2, we would need to chain together the custom/extensible DashJSONDecoders from all of the component libraries.


Pros of making dash's base component class extensible and putting encrypt logic in dash-core-components component class:

  • transform and inverse_transform is generally useful for other components. For example, our table component could take Table(dataframe=df) rather than Table(rows=df.to_dict())
  • Keeps dash small

Pros of putting encrypt logic in dash's base component:

  • encrypt_* could be used by all component authors

In both cases, we'd need some way to transfer the secret keys over to the component. The only coupling between the base component and the app instance is where the app calls json.dumps and json.loads. Perhaps it could pass in a secret key in those calls somehow.

@nicolaskruchten
Copy link
Contributor Author

nicolaskruchten commented May 30, 2018

OK, so this PR is a dead-end basically :) I don't think the "pro" of the base_component option is particularly compelling: I haven't been able to think of a single use-case for encrypted props other than a dcc.Store component so rolling this into a bigger transform/inverse_transform feature is 👍 in my opinion.

I'm going to close this PR and create a proper issue for the need for a dcc.Store component.

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.

2 participants