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

Add authorization for writes #2

Closed
tarasglek opened this issue Aug 16, 2023 · 14 comments
Closed

Add authorization for writes #2

tarasglek opened this issue Aug 16, 2023 · 14 comments

Comments

@tarasglek
Copy link

Hi I really love the convenience of wsbroad.

Would be really nice to deploy wsbroad as cheap and cheerful low-effort pub-sub.

Only thing that's missing would be to add --auth-rw, ideally similarly to miniserve:

 -a, --auth <AUTH>
          Set authentication. Currently supported formats: username:password, username:sha256:hash,
          username:sha512:hash (e.g. joe:123,
          joe:sha256:a665a45920422f9d417e4867efdc4fb8a04a1f3fff1fa07e998e86f7f7a27ae3)
          
          [env: MINISERVE_AUTH=]

In presence of a flag like this clients would get a 404 if they open a url that hasn't been opened by a writer without the right Authorization: header.

Would be nice to have an equivalent --auth-ro and --auth too.

Hope this isn't out of scope.

@vi
Copy link
Owner

vi commented Aug 16, 2023

Authorization should probably be handled upstream (e.g. on Nginx / Caddy).

But new URL vs existing URL distinction is in wsbroad's purview. Maybe it can support multiple prefixes like /new/<key> for full access, /ro/<key> for read-only access, /rw/<key> for read-write (but not create) access. Client sets would be shared for <key>s.

Upstream web server (which is needed for wss:// anyway) can handle authentication and authorization by selectively limiting/forwarding access to the prefixes.

Do you find this setup useful?

@tarasglek
Copy link
Author

yes, this would also work

@tarasglek
Copy link
Author

tarasglek commented Aug 16, 2023

I would prefer rw to be able to create, though it's trivial to handle calling new if rw/ is 404

@vi
Copy link
Owner

vi commented Aug 16, 2023

RFC: access prefixes.

With --access-prefixes command line parameter, each URL is required to have a prefix. Prefixes are composed of one of more letters (and a trailing slash); each letter denote some authorization. The part after prefix is used as a key for client broadcast domains storage. /rw/qqq and /r/qqq denote the same domain, but with different available operations. Unknown letters cause request failure. /wr/qqq is the same as /rw/qqq.

Letters:

  • r - Allow received from the WebSocket;
  • w - Allow sending to the WebSocket;
  • c - Allow creation of new broadcast domains;
  • 1 to 5 - When --stochastic-queue mode is active, sets priority. Priority does not affect message latency, but may affect which messages get dropped by the queue if reader is slower than writer. 5 is least probability of drops. Ignored when --stochastic-queue is not specified on CLI, but may fail request if specified without w letter. Priority does not affect reading side of read-write clients.

WebSocket messages sent by client connected to /r/ URLs are silently ignored. Clients without c capability are allowed to connect to existing domains even if all other clients with w or c capabilities are left that domain (but some r-s remain). Create-only connections are allowed - no WebSocket messages would be sent or received, but URL would be marked as available for non-c clients to connect as use properly.

With --reflexive command-line option, only rw-capable clients would see the reflected messages.


@tarasglek Does this sound reasonable? Are there any missing details / tricky interactions?

@tarasglek
Copy link
Author

tarasglek commented Aug 17, 2023

Spec is a bit hard to follow without context in this ticket. Happy to propose easier docs for it once it works.

  1. In my mind producers will connect to /wc/topic. Clients will connect to /r/topic or /rw/topic
  • What happens if clients connect first? I expect 403 or 404.
  • /c/topic doesn't make sense in this world as it would be a short-term. Thus no clients will be able to connect to keep it alive. Unless there is some helpful message with ttl for these. Perhaps there needs to be a --expiry-ttl overall?
  • once producer and consumers disconnect, topic is removed. that's fine too
  1. might make sense to retrict order of letters/numbers eg only allow alpanumeric ala crw1, not 1cwr, to avoid accidental permission problems
  2. I would prefer to have clients connect to /r/topic get disconnected on attempting to write to socket. Otherwise you risk hard-to-debug behavior
  3. /w/topic also implies 'r' permissions. That's fine.

@vi
Copy link
Owner

vi commented Aug 17, 2023

/c/topic doesn't make sense in this world as it would be a short-term.

Why short-term? It would be just idling opened WebSocket connection without any sent or received messages.

Perhaps there needs to be a --expiry-ttl overall?

Currently wsbroad does not contain any timers, so introducing TTLs is not planned as a part of this feature.

@vi
Copy link
Owner

vi commented Aug 17, 2023

/w/topic also implies 'r' permissions. That's fine.

That's not in the spec - write-only clients are supposed to work.

Imagine a public endpoint where clients send personally identifiable information. You'll probably want only privileged internal service to read those messages, not clients read each others' messages.

@vi
Copy link
Owner

vi commented Aug 17, 2023

I would prefer to have clients connect to /r/topic get disconnected on attempting to write to socket. Otherwise you risk hard-to-debug behavior.

Reasonable. Maybe there would be a CLI option to adjust that.

@vi
Copy link
Owner

vi commented Aug 17, 2023

might make sense to retrict order of letters/numbers eg only allow alpanumeric ala crw1, not 1cwr, to avoid accidental permission problems

That would slightly complicate the parsing. If go the restriction way, then might as well only allow canonical representations (i.e. no duplicates, only /rw/, not /wr or /rwww/).

@tarasglek
Copy link
Author

ok, overall your spec works well even without my suggestions. Your call on those details.

@vi
Copy link
Owner

vi commented Aug 19, 2023

Pushed --access-prefixes to master.

Does it work properly and match your use case?

@tarasglek
Copy link
Author

tarasglek commented Aug 21, 2023

wsbroad 0.0.0.0:9999 --access-prefixes

  • websocat ws://0.0.0.0:9999/foo fails with WebSocketError: Received unexpected status code (400 Bad Request). Is there a way to see detailed http headers with websocat?
  • websocat ws://0.0.0.0:9999/r/foo , eg accessing /r before c-reating it. wsbroad outputs Rejected: no "c" capability for this client, but client doesn't fail like it does above.
  • websocat ws://0.0.0.0:9999/rw/foo works as expected
  • if client connects with /cw/ or /w/ it doesn't see an incoming data
  • writes do not get rejected from /r/ clients. I would like them to get an immediate drop. --access-prefixes-allow-unsolicited-write suggests that absense of it will cause a disconnect, but I have not been able to observe that. I think you have a logic inversion, having --access-prefixes-allow-unsolicited-write actually works as I expect
  • having a mix of rw r and w clients on same topic behaves as one would expect modulo disconnects

@vi
Copy link
Owner

vi commented Aug 21, 2023

... but client doesn't fail like it does above
writes do not get rejected from /r/ clients

Have you tried sending multiple messages? Websocat (without -E option) may be sluggish to detect disconnections.

@tarasglek
Copy link
Author

i can't repro earlier behavior. I think it was lack of -E that confused me. All good to go.

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

No branches or pull requests

2 participants