crossbario / crossbar

Crossbar.io - WAMP application router
https://crossbar.io/
Other
2.05k stars 275 forks source link

implement dynamic node key #1906

Closed om26er closed 2 years ago

om26er commented 2 years ago

This is work in progress as it needs to be extended so that crossbar workers also read the "correct" private key.

oberstet commented 2 years ago

yeah, this is how to expand the CLI to worker options passing. unfortunately, it won't work .. pls see https://github.com/crossbario/crossbar/issues/1902#issuecomment-971165831 =(

om26er commented 2 years ago

This PR is still not ready. However, after https://github.com/crossbario/autobahn-python/pull/1501 lands, this PR at least allows rlinks to use the key instance defined in Crossbar json config.

om26er commented 2 years ago

The CI is blocked because it seems crossbar is not able to find the new class that was introduced in latest autobahn-python. Specifically SigningKeyBase.

The relevant traceback looks like

ImportError: cannot import name 'SigningKeyBase' from 'autobahn.wamp.cryptosign' 
oberstet commented 2 years ago

is not able to find the new class that was introduced in latest autobahn-python

yeah, the test needs to run on an installation of AB from the GH master branch for that to work .. not sure, the CI setup

https://github.com/crossbario/crossbar/blob/93f3931eb3fb34bae59c6a8d3d4bcbbef7131d67/.github/workflows/main.yml#L137

installs

https://github.com/crossbario/crossbar/blob/93f3931eb3fb34bae59c6a8d3d4bcbbef7131d67/requirements-latest.txt#L2

SigningKeyBase is on AB master now, right?

oberstet commented 2 years ago

does it work if you pip install in a fresh venv git+https://github.com/crossbario/autobahn-python.git@master#egg=autobahn[twisted,encryption,compress,serialization,scram,xbr]?

another trap: above works, but not in the CI - maybe because in the CI the AB package is already installed when it hits above requirements-latest? such stuff can be fun ... not sure if it applies here

om26er commented 2 years ago

Yes, here https://github.com/crossbario/autobahn-python/blob/062e5e6492beb92292934455ae6a95bce4a0da19/autobahn/wamp/cryptosign.py#L432

I took a deeper look it seems the file /opt/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/site-packages/autobahn/wamp/cryptosign.py on matterhorn doesn't really have that class (I tried cat in CI https://github.com/crossbario/crossbar/runs/4267160342 to check that). So it's likely being imported from the wrong location or there is a difference between which interpreter is being used

oberstet commented 2 years ago

you could add the failing import in the CI before the actual testsuite starts here https://github.com/crossbario/autobahn-python/blob/062e5e6492beb92292934455ae6a95bce4a0da19/.github/workflows/main.yml#L108

om26er commented 2 years ago

does it work if you pip install in a fresh venv git+https://github.com/crossbario/autobahn-python.git@master#egg=autobahn[twisted,encryption,compress,serialization,scram,xbr]?

just tried in a clean env, it works yes. I believe the issue is with the environment (matterhorn).

you could add the failing import in the CI before the actual testsuite starts here https://github.com/crossbario/autobahn-python/blob/062e5e6492beb92292934455ae6a95bce4a0da19/.github/workflows/main.yml#L108

tried that and it's imported fine https://github.com/crossbario/autobahn-python/runs/4267541701?check_suite_focus=true#step:7:16

om26er commented 2 years ago

To verify a few things and probably find a fix, I would probably need SSH access to matterhorn, if you are comfortable with that

om26er commented 2 years ago

To verify a few things and probably find a fix, I would probably need SSH access to matterhorn, if you are comfortable with that

Not needed. I was able to fix this. It seems the issue was caused due to the hard-coded version of pip and autobahn not updating when installed from github because it also expects a change in version inside setup.py (https://stackoverflow.com/a/68432209).

oberstet commented 2 years ago

@om26er pip again. we will see how the now current pip version behaves - I added the pip version pinning because pip changed how multiple confluent deps are resolved, which broke xbr related stuff. forgot the details. we will see. python deps are like the GIL .. endless, never really fixed.

oberstet commented 2 years ago

I had a look through the changes again at this point, and this already looks quite good! this will work, and fits into nicely=)

The one thing we need to discuss/change IMO is check_node_key. The node config item for the configuration of the way and the details of accessing node key/signing. Here is a proposal:

File-based:

{
   "controller": {
      "keyring": {
         "type": "file",
         "path": "/home/homer/key.bin"
      }
   }
}

HW-based:

{
   "controller": {
      "keyring": {
         "type": "hsm",
         "driver": "NXP-SE05xC",
         "port": "/dev/ttyACM0"
      }
   }
}

"type": "hsm"

A Hardware Security Module (HSM) is a secure crypto processor with the main purpose of managing cryptographic keys and offer accelerated cryptographic operations using such keys. The modules typically offer protection features like strong authentication and physical tamper resistance. Main features of an HSM include on board key generation and storage, accelerated symmetric and asymmetric encryption and backup of sensitive material in encrypted form.

"driver": "NXP-SE05xC"

This is supposed to support exactly 3 chips: NXP SE050C1, SE050C2, SE051C2

https://www.nxp.com/products/security-and-authentication/authentication/edgelock-se050-plug-trust-secure-element-family-enhanced-iot-security-with-maximum-flexibility:SE050#compaTableTitle https://www.nxp.com/products/security-and-authentication/authentication/edgelock-se051-proven-easy-to-use-iot-security-solution-with-support-for-updatability-and-custom-applets:SE051#compaTableTitle


another detail: IMO instead of "nodekey" a better name could be "keystore" or "keyring" - because a node's key might only be one of the (signing) keys available via the type configured in controller.

om26er commented 2 years ago

Updated the PR to support file-based and hardware keys. This does reduce the flexibility and future extensibility (but is probably more secure ?) i.e. to support a new type of hardware key, we would have to make changes to both crossbar and autobahn.

Also for the NXP chip, it seems there are no python bindings, so that's something we'd probably end up writing and eventually integrated into CB/AB. On our hardware the secure element is actually accessible on the I2C interface (specifically /dev/i2c-6)

oberstet commented 2 years ago

This does reduce the flexibility and future extensibility (but is probably more secure ?)

no, would disagree with this, it doesn't reduce future extensibility, but properly defines a narrow interface to extend the system

eg allowing to specify a class name is bad design (in this case .. for a container worker wamp class, that's different)

a configuration item is an interface. the purpose of an interface is primarily to structure a big system into loosely coupled parts. narrow/strict interfaces make it loosely coupled

a configuration item of course also transports some info .. the minimal info, constrained to the maximum

Also for the NXP chip, it seems there are no python bindings

really? forgot everything already;) in any case, should be easy .. just use a I2C library (take care of async or wrappers) and bit bang the chip's registers.

eg a I2C lib like this https://github.com/kplindegaard/smbus2 or similar .. depends also on hosting env etc

om26er commented 2 years ago

Do you think we should land this now. A follow up PR that I will propose tomorrow will take care of making sure all workers use the same mechanism to access the key.

This would allow us to also land a few other PRs that are blocked due to failing CI

oberstet commented 2 years ago

sure! AB is merged, this one runs green .. merged. thanks a lot =)