Network connectivity: Add key management for EAP secured networks #2254

sfoster1 · 2018-09-12T21:05:07Z

Some EAP secured networks (in fact most kinds) either require or allow the use of key files for various tasks, such as client identity authentication through private keys or server identity authentication through x.509 certificates. In some cases, these keys can be large; so, rather than just adding them into the /wifi/configure request, here are some new endpoints to manage wifi keys.

They will be stored on disk in the new subdirectory /data/user_storage/opentrons_data/network_keys, available in opentrons.util.environment.settings as WIFI_KEYS_DIR. To allow upload of identically named keys, the key files will be stored in directories named by a hash of the key file itself. Identically-hashed keys are not allowed.

The shape of the endpoints is:

200 OK
    { keys: [
         {
          uri: '/wifi/keys/some-hex-digest',
          id: 'some-hex-digest',
          name: 'keyfile.pem'
         },
         ...
       ]
    }
}

POST /wifi/keys multipart/form-data with key=<file>
201 Created
{
     uri: '/wifi/keys/some-hex-digest',
     id: 'some-hex-digest',
     name: 'keyfile.pem'
}

If the key already exists:

409 Conflict
{
 message: 'Key file already present'
}

DELETE /wifi/keys/{key-id}
200 OK
{message: 'Key file {key-id} deleted'}

In addition, add proper types to nmcli and wifi endpoints.

Closes #2253

The nmcli module is now fully typed

codecov · 2018-09-12T21:20:42Z

Codecov Report

Merging #2254 into edge will increase coverage by 0.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #2254      +/-   ##
==========================================
+ Coverage   29.67%   30.54%   +0.86%     
==========================================
  Files         497      500       +3     
  Lines        7962     8253     +291     
==========================================
+ Hits         2363     2521     +158     
- Misses       5599     5732     +133

Impacted Files	Coverage Δ
app/src/http-api-client/health.js	`95% <0%> (-5%)`	⬇️
discovery-client/src/poller.js	`92.85% <0%> (-3.58%)`	⬇️
app/src/http-api-client/settings.js	`100% <0%> (ø)`	⬆️
app/src/components/UploadPanel/Upload.js	`0% <0%> (ø)`	⬆️
...l-designer/src/containers/ConnectedNewFileModal.js	`0% <0%> (ø)`	⬆️
...tocol-designer/src/containers/ConnectedTitleBar.js	`0% <0%> (ø)`	⬆️
protocol-designer/src/containers/ConnectedNav.js	`0% <0%> (ø)`	⬆️
app/src/components/DeckMap/ConnectedSlotItem.js	`0% <0%> (ø)`	⬆️
...src/components/ReviewDeckModal/LabwareComponent.js	`0% <0%> (ø)`	⬆️
...ocol-designer/src/containers/ConnectedMainPanel.js	`0% <0%> (ø)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2982e1...1f448b1. Read the comment docs.

mcous

Looks really good! I have a couple change requests related to error responses for configure and key upload

mcous · 2018-09-13T12:39:55Z

api/opentrons/server/endpoints/wifi.py

-                                                hidden=hidden)
-            status = 201 if ok else 401
+        try:
+            checked_sec = sec_translation[security]


I think this will overwrite the ssid missing error, which I think is the more pressing error. Also, to nitpick try/except blocks make me (irrationally?) nervous (thank you Scala). Maybe we could do something like this instead?

# ... security = body.get('security_type') # ... if ssid is None: # ... elif security not in sec_translation: # ... #...

I'd prefer to avoid doing further work here if only because the next PR is going to be about eap configuration and will include a lot of overhauls of this endpoint - I can fold it in there.

mcous · 2018-09-13T14:56:11Z

api/opentrons/server/endpoints/wifi.py

+    """ Add a key file (for later use in EAP config) to the system.
+
+    ```
+    POST /wifi/keys with {'key': <file>} -> 201 Created


Docs nitpick: this kind of makes it seem like the request body should be JSON when it should actually be multipart form data

Sure, how would you like me to specify it?

mcous · 2018-09-13T14:57:22Z

api/opentrons/server/endpoints/wifi.py

+        return web.json_response({'message': "Must upload key file"},
+                                 status=400)
+    data = await request.post()
+    if 'key' not in data:


Would data.get('key') + if keyfile is None: be preferable here?

mcous · 2018-09-13T15:00:01Z

api/opentrons/server/endpoints/wifi.py

+    key_hash = hasher.hexdigest()
+    if key_hash in os.listdir(keys_dir):
+        return web.json_response(
+            {'message': 'Key file already present'}, status=409)


I wonder if it might be more helpful to respond with a 200 OK? The user wanted to upload a key, and that key is on the system. Regardless of if we change the status here, could we put the uri, id, and name in the response body?

mcous · 2018-09-13T15:01:52Z

api/opentrons/server/endpoints/wifi.py

+    POST /wifi/keys with {'key': <file>} -> 201 Created
+    {
+     uri: '/wifi/keys/some-hex-digest',
+     hash: 'some-hex-digest',


hash changed to id in actual response

mcous · 2018-09-13T15:03:08Z

api/opentrons/server/endpoints/wifi.py

+    """ Remove a key.
+
+    ```
+    DELETE /wifi/keys/hex-digest


DELETE /wifi/keys/:id might make it a little easier to understand that the last part of the URL is a parameter

/wifi/keys allows GET to list and POST to add a new keyfile (of arbitrary type and size). These are hashed and stored in /data/user_storage/opentrons_data/network_keys in individual folders by sha256 hash of the contents. This hash is used as a URI for later deletion of the keys, or specification in /wifi/configure eapol options that require key files. Closes #2253

mcous

🍵

mcous · 2018-09-14T12:58:08Z

api/opentrons/server/endpoints/wifi.py

+    If a file with the same hash (regardless of filename) has already been
+    uploaded, returns
+    ```
+    200 OK


mcous · 2018-09-14T13:04:51Z

api/opentrons/server/main.py

@@ -166,6 +166,9 @@ def init(loop=None):
        '/wifi/configure', wifi.configure)
    server.app.router.add_get(
        '/wifi/status', wifi.status)
+    server.app.router.add_post('/wifi/keys', wifi.add_key)


Note to self about app state: the paths and responses of the /wifi/keys endpoints are completely correct (in my mind), but to date the app hasn't really had to deal with ID'd resources. This functionality (along with module firmware update by serial number) will mean we need to make a very considered refactor of the app's state w.r.t. API resources

btmorr

🍐

refactor(api): Add types to nmcli

e42b704

The nmcli module is now fully typed

sfoster1 requested review from mcous, btmorr, sanni-t and Laura-Danielle September 12, 2018 21:05

sfoster1 force-pushed the api_add-wifi-key-management branch from 1f448b1 to 55467b4 Compare September 13, 2018 14:20

mcous suggested changes Sep 13, 2018

View reviewed changes

sfoster1 force-pushed the api_add-wifi-key-management branch 3 times, most recently from 02427b9 to f51b91d Compare September 13, 2018 18:59

sfoster1 force-pushed the api_add-wifi-key-management branch from f51b91d to 7e479de Compare September 13, 2018 19:01

mcous approved these changes Sep 14, 2018

View reviewed changes

mcous added api Affects the `api` project ready for review feature Ticket is a feature request / PR introduces a feature labels Sep 14, 2018

btmorr approved these changes Sep 14, 2018

View reviewed changes

sfoster1 merged commit 250101c into edge Sep 14, 2018

sfoster1 deleted the api_add-wifi-key-management branch September 14, 2018 14:36

mcous added a commit that referenced this pull request Sep 14, 2018

fixup: Add missing changelog entry for #2254

e178f9e

mcous mentioned this pull request Oct 23, 2018

fix(api): Correct GET /wifi/keys response to match documentation #2532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network connectivity: Add key management for EAP secured networks #2254

Network connectivity: Add key management for EAP secured networks #2254

sfoster1 commented Sep 12, 2018 •

edited by btmorr

Loading

codecov bot commented Sep 12, 2018 •

edited

Loading

mcous left a comment

mcous Sep 13, 2018

sfoster1 Sep 13, 2018

mcous Sep 13, 2018

sfoster1 Sep 13, 2018

mcous Sep 13, 2018

mcous Sep 13, 2018

mcous Sep 13, 2018

mcous Sep 13, 2018

mcous left a comment

mcous Sep 14, 2018

mcous Sep 14, 2018

btmorr left a comment

Network connectivity: Add key management for EAP secured networks #2254

Network connectivity: Add key management for EAP secured networks #2254

Conversation

sfoster1 commented Sep 12, 2018 • edited by btmorr Loading

codecov bot commented Sep 12, 2018 • edited Loading

Codecov Report

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btmorr left a comment

Choose a reason for hiding this comment

sfoster1 commented Sep 12, 2018 •

edited by btmorr

Loading

codecov bot commented Sep 12, 2018 •

edited

Loading