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

Set up basic websocket server #56

Merged
merged 8 commits into from
Mar 10, 2022
Merged

Set up basic websocket server #56

merged 8 commits into from
Mar 10, 2022

Conversation

NotUnlikeTheWaves
Copy link
Collaborator

@NotUnlikeTheWaves NotUnlikeTheWaves commented Jan 15, 2022

Hello, this PR adds a basic endpoint for the websocket server.

Some things to note:

  1. Channels overrides the routing (see the ProtocolTypeRouting in delfitlm/asgi.py)
  2. It seems that for the proxy/deploy we use WSGI/uWSGI. Since this PR introduces websockets over ASGI, we should consider if it's possible to move the project over the ASGI. Note that channels is built on top of ASGI and to my knowledge doesn't work with WSGI.
  3. Websocket endpoints are defined in the websocket/routing.py file (in this setup). For now this seems fine, maybe if we end up having a lot of endpoints this can be restructured

I've tested this with websocat (websocat ws://127.0.0.1:8000/ws/echo/). You should be greeted and every message you sent should be sent back to you in a JSON format.

From this setup we can create specific endpoints to handle commands and such

@NotUnlikeTheWaves
Copy link
Collaborator Author

Something to consider: How to deal with authorization? Currently I don't have a clear view of how authorization currently works in our application.

@StefanoSperetta
Copy link
Member

Very nice! Authentication is still TBD: a the moment we have a password-based system encoding the password in plain text in the JSON transactions on the POST server. We would love to transition to an API key that can only be used to log in for submission (and not for the website) such that a compromised API key would not endanger the data.

For the websocket, we might actually authenticate just once when the connection is established (eventually via a simple JSON tuple) and then the server would drop the connection if the user/key is not valid.

@StefanoSperetta
Copy link
Member

StefanoSperetta commented Jan 16, 2022

One more option: the request to establish the websocket connection (before the protocol upgrade stage) is done as a regular HTTP request. Would it be possible to intercept that header? If so, we might use the Authentication header for the same purpose and accept the protocol upgrade only in case of successful authentication...

Something like this could be an easy way: django/channels#903 (comment)

@NotUnlikeTheWaves
Copy link
Collaborator Author

Very nice point, I wasn't aware websockets worked in that way (establish over HTTP before upgrading). This could be an interesting way to work with it. I'll take a look at it. I've also found that channels should be able to plug into our current authentication middleware https://channels.readthedocs.io/en/stable/topics/authentication.html.

I'll have a look at both and try to find which works best with our current setup

@StefanoSperetta
Copy link
Member

I think this is the best approach! the custom middleware seems to intercept the HTTP headers (so before the protocol upgrade), then we should have a very clean websocket (basically only data) while all the authentication remains handled by the HTTP part. I am not sure it makes a real difference to use the standard HTTP header or an "Authentication" header (which usually is Base64 encoded)... since all the data will be under SSL it might not really make a difference but just be more intuitive.

One point could be where to send the user name: if we send it only in the authentication header (or in another one), this might not be accessible to the process handling the data transmitted so we will have to pass that value somehow (such that, for example, it is added to the database). Otherwise, we might send the API key in the authentication header so authentication is only performed with that (or both user name and API key in the authentication header) and then add also the username in the JSON data transfer so that it is easier to handle in the downstream data processing (basically each JSON block comes with a user name).

And, if no API key is fund, we drop the connection altogether and ignore the problem.

@NotUnlikeTheWaves
Copy link
Collaborator Author

I'll try to come back to this next week. The past weeks and this week too have been busy for me. If it suits you we could merge this as is so it's available to the main branch. Then we can create a follow-up issue for the authentication and potentially prioritize it above other work?

If you prefer to leave this PR open that's fine too, I wanted to give you an update on how much time I can commit for now. Things should calm down halfway next week for me

Copy link
Collaborator

@iiacoban42 iiacoban42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge this one and the API keys PR and then reiterate with authentication for websocket connections. I'll add an issue for configuring nginx with asgi as well.

@iiacoban42 iiacoban42 merged commit d503143 into main Mar 10, 2022
@iiacoban42 iiacoban42 deleted the websocket branch July 22, 2022 13:32
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.

Implement a Channel for communicating with EWI ground station server
3 participants