mesosphere / marathon-lb

Marathon-lb is a service discovery & load balancing tool for DC/OS
Apache License 2.0
449 stars 300 forks source link

cryptography version/usages breaking change #421

Closed cornelf closed 5 years ago

cornelf commented 7 years ago

Since marathon-lb v1.3.3 the requirement on cryptography changed from https://github.com/mesosphere/marathon-lb/blob/v1.3.3/requirements.txt#L7 to https://github.com/mesosphere/marathon-lb/blob/master/requirements.txt#L1

Although https://github.com/mesosphere/marathon-lb/blob/v1.3.3/requirements.txt#L7 matches https://github.com/mesosphere/marathon-lb/blob/v1.3.5/requirements.txt#L7 - 1.3.3 works; 1.3.5 doesn't so the problem must be something introduced in the framework between 1.3.3 and 1.3.5.

The breaking side effect of that is:

2017-02-22 14:35:13,609 marathon_lb: Unexpected error!
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/jwt/algorithms.py", line 167, in prepare_key
    key = load_pem_private_key(key, password=None, backend=default_backend())
  File "/usr/local/lib/python3.5/dist-packages/cryptography/hazmat/primitives/serialization.py", line 20, in load_pem_private_key
    return backend.load_pem_private_key(data, password)
  File "/usr/local/lib/python3.5/dist-packages/cryptography/hazmat/backends/multibackend.py", line 305, in load_pem_private_key
    return b.load_pem_private_key(data, password)
  File "/usr/local/lib/python3.5/dist-packages/cryptography/hazmat/backends/openssl/backend.py", line 1084, in load_pem_private_key
    password,
  File "/usr/local/lib/python3.5/dist-packages/cryptography/hazmat/backends/openssl/backend.py", line 1253, in _load_key
    self._handle_key_loading_error()
  File "/usr/local/lib/python3.5/dist-packages/cryptography/hazmat/backends/openssl/backend.py", line 1325, in _handle_key_loading_error
    raise ValueError("Could not unserialize key data.")
ValueError: Could not unserialize key data.

when following the instructions from: https://docs.mesosphere.com/1.8/administration/id-and-access-mgt/service-auth/mlb-auth/

This basically only affects EE deployments where auth needs to be strict.

brndnmtthws commented 7 years ago

Yikes! I think that was a mistake on my part.

Unfortunately I don't have access to the repo anymore, but hopefully someone can fix it.

cornelf commented 7 years ago

it is only affecting the valued users, so it should go up on the priority list.

brndnmtthws commented 7 years ago

Unfortunately nobody in management at Mesosphere cares about this project, so I'm certain that will happen. That's part of the reason why I quit my job there.

I'd suggest forking the project for the time being.

cornelf commented 7 years ago

is it not rather here: https://github.com/mesosphere/marathon-lb/commit/96007ce9e28158cff52cdcb8b4afa2fa0942e6f2 - and private key is not expected to be base64 encoded anymore?

brndnmtthws commented 7 years ago

You could ask @alberts. I don't know why that was done.

gpaul commented 7 years ago

Keys are usually already PEM-encoded (base64 wrapped in the usual header and footer '---' lines.) Re-encoding them as base64 again doesn't make much sense. I wasn't around at the time but I believe that we stopped re-encoding it which is why that line was removed. Of course, we should have done so in a backwards-compatible way, perhaps by testing whether private_key succesfully decodes as base64 and using the result if it does.

nobody in management [...] cares about this project

I don't know anything about that, but fixing trivial bugs is certainly in scope.

gpaul commented 7 years ago

@cornelf, are you able to base64-decode the value you are using and use that instead? I'm still happy to fix this but it will be good to know that you're no longer blocked and that fixing this will actually solve your problem.

brndnmtthws commented 7 years ago

I don't know anything about that, but fixing trivial bugs is certainly in scope.

Make sure you get that in writing before you sign any contracts.

cornelf commented 7 years ago

@gpaul I obviously sorted it after digging to the commit https://github.com/mesosphere/marathon-lb/commit/96007ce9e28158cff52cdcb8b4afa2fa0942e6f2 I say it is better to leave it as is and reaffirm here https://docs.mesosphere.com/1.8/administration/id-and-access-mgt/service-auth/mlb-auth/ that the key doesn't need to be base64 encoded from version 1.3.4 onwards

Thanks for looking into this. Thanks @brndnmtthws as well for your help. Cheer up!

cornelf commented 7 years ago

Indeed it was that ^^ - so using non base64 encoded values for key and secret ftw. [this comment didn't go through at the time it was written]

emanic commented 7 years ago

Hi @cornelf this is a great idea to add a note to the documentation. Thanks @gpaul for bringing this to my attention. I will update the documentation around this issue.