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

NIP-41: Authentication #92

Closed
wants to merge 4 commits into from
Closed

NIP-41: Authentication #92

wants to merge 4 commits into from

Conversation

Semisol
Copy link
Collaborator

@Semisol Semisol commented Dec 15, 2022

No description provided.

@Semisol Semisol requested review from cameri and fiatjaf December 15, 2022 22:11
@Semisol Semisol self-assigned this Dec 15, 2022
41.md Outdated Show resolved Hide resolved
## Signed event verification
Relays when processing `AUTH` responses SHOULD verify the following before accepting the response:
- that the `kind` is `22223`
- that the event was recently signed (~10 minutes, by `created_at`)
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this requires that the client and the server use time sync of some kind

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that is normal. if your clocks are out of sync more than a few minutes, you probably have issues anyway with other stuff, and most devices already sync time using ntp

Copy link

Choose a reason for hiding this comment

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

The server can send its timestamp inside the <data object>, so the client knows what created_at values it signs will be considered valid.

@fernandolguevara
Copy link
Contributor

auth can be handled at HTTP before upgrades the connection to ws

@Semisol
Copy link
Collaborator Author

Semisol commented Dec 24, 2022

auth can be handled at HTTP before upgrades the connection to ws

it can, but this allows the relay to request auth when required. also, web API does not provide any good way of sending headers for auth

@Semisol Semisol requested a review from Giszmo December 24, 2022 11:12
@Semisol
Copy link
Collaborator Author

Semisol commented Dec 24, 2022

note: this probably has too many commits, might be best to squash merge it.

@gkbrk
Copy link

gkbrk commented Dec 24, 2022

Should this be a separate event kind? If there is any relay recommendations, chat or tweeting going on at the Ephemeral Events Reserved kind 22241, that can potentially be used for authenticating to relays. I think either this event number should be excluded from the Ephemeral Events Reserved range, or this NIP should use a different kind.

Example chat with kind=22241 events.

A: Hey man, which relay do you use these days?
B: wss://relay.example.com/
A: Thanks.

Now we have a valid AUTH signature from B that can be used for 10 minutes.


A relay SHOULD send the [`OK`](https://github.com/nostr-protocol/nips/blob/master/20.md) command after they receive a
non-rejecting authentication response, and use one of the following `message` prefixes if the event sent cannot be verified:
- `too-old:`: The event signed has a too old `created_at` date.
Copy link

@gkbrk gkbrk Dec 24, 2022

Choose a reason for hiding this comment

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

Should a relay reject events that are signed too much into the future? Maybe -/+ 10 minutes instead of just allowing up to 10 minutes in the past.


A client may send one of the following to the relay:
- `["AUTH", <signed event>]` to indicate it has accepted the request. This may also be sent without an authentication request.
- `["AUTH", null]` to indicate it has rejected the request.
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this? It's an unnecessary complication. Clients gain nothing from sending this, and relays gain nothing from expecting it.


Please note that the `OK` message should only be sent as a response to other commands that the client sends instead of the `AUTH` command if the issue is not related to the authentication event being incorrectly signed (example: not on whitelist).

Relays SHOULD send [`EOSE`](https://github.com/nostr-protocol/nips/blob/master/15.md) when an authentication request is triggered by a `REQ` command, and not send stored events after the `EOSE` when authentication is completed.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this.

- `already-used:`: This event was already used to authenticate to this relay.
- `bad-signature:`: The event has a bad signature.

Please note that the `OK` message should only be sent as a response to other commands that the client sends instead of the `AUTH` command if the issue is not related to the authentication event being incorrectly signed (example: not on whitelist).
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the OK stuff. NIP-20 OK messages have an event id in them. Here there is no event id to use. I think this entire thing will be very much ok without the OKs. Much simpler to understand and implement. The client knows when it is signing stuff correctly, it doesn't need the relay to tell them.

If it is signing stuff wrong it doesn't have to handle that error message, it must necessarily fix the code that is producing bad signatures. Once it is fixed, the signatures will be good forever. There is nothing for the user to do here at runtime.

@fiatjaf
Copy link
Member

fiatjaf commented Dec 24, 2022

I may be missing something, but to me the flow to me should be:

  1. If the client knows the relay requires auth already, so it sends an auth request before doing anything and then does whatever it wants to do. The websocket messages ordered, so it knows the relay will receive his AUTH before the other commands.
  2. If the client doesn't know and tries to do something it can't, the relay can send an auth request, then the client can send an AUTH or not. At this point the client doesn't know for sure what triggered the auth request, so it will probably have to restart all its subscriptions or do something else. In either case the OK or EOSE messages do not help very much, so it is better to eliminate them and then this NIP becomes super simple.

@Semisol Semisol marked this pull request as draft December 25, 2022 16:15
NIP-41
======

Authentication of clients to relays
Copy link
Member

@Giszmo Giszmo Dec 25, 2022

Choose a reason for hiding this comment

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

The authentication is not of a client but of a pubkey.
The nip as is contains a commitment to the relay and the pubkey but not to the client.

In theory, a proxy that gets to read the communication could take over the authenticated connection and run queries using the authentication.

Alice (Client) <--WS--> Eve (proxy) <--WS--> Bob (Relay)

Should we make sure of end-to-end encryption before trusting anything that reaches the relay via this websocket?

Suggested change
Authentication of clients to relays
Pubkey authentication to relays
Suggested change
Authentication of clients to relays
Authentication to relays

The URL MUST:
- Have a trailing slash if the path is `/`
- Not have a port number if protocol is `ws` and port is `80` or protocol is `wss` and port is `443`
- Not include the search/query
Copy link
Member

@Giszmo Giszmo Dec 25, 2022

Choose a reason for hiding this comment

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

This part is not clear. What would the "search/query" even be in this context? Does this line really resolve any real issue?

Suggested change
- Not include the search/query

Copy link
Member

@fiatjaf fiatjaf Dec 26, 2022

Choose a reason for hiding this comment

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

Querystring.

Copy link
Member

Choose a reason for hiding this comment

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

"querystring" as in wss://some.relay.com/?query=string ? Is that a thing? How about wss://some.relay.com/#tag=string ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, why not? I think relay URLs with querystring arguments can be used in some interesting ways. The # stuff I don't know. Is that sent to the server? I thought it was a client thing.

Copy link
Member

@Giszmo Giszmo Dec 26, 2022

Choose a reason for hiding this comment

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

Browsers interpret #something client side, only but of course, links can contain them, maybe to signal nip support: wss://some.relay.nostr/#nips=1,2,3,4

To identify towards the relay, it should be omitted for normalization.

On the other hand, ...?geo=afrika could connect you to a different machine behind the same load balancer maybe? Or, ... I don't know. Hence my question: Why do we exclude the query thing? Does this solve a known problem? I think this rule could get in the way of creative use cases and probably is confusing else.

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of this content field is just to prevent phishing attacks (or something, I forgot the name) in which one relay could take a signed event from someone else and use it to authenticate to a different relay.

For that purpose, only the domain name ("hostname") should be enough, as long as the relay knows its own domain name it can just reject events with signatures containing different domain names.

Copy link
Member

Choose a reason for hiding this comment

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

So you want to limit it to protocol://domain[:port]/, with port only being allowed for non-standard ports. Why consider the path if query isn't to be considered?

wss://relay.minds.com/nostr/v1/ws indeed uses a path but what's the qualitative difference to using a query string? Why allow one but not the other?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. I think we should either do everything including query string or just the domain without scheme or port.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clients MUST NOT send # to the server. I believe servers actively reject the fragment section and client libraries send them absolutely never even if you try to get them to.

- Not have a port number if protocol is `ws` and port is `80` or protocol is `wss` and port is `443`
- Not include the search/query

Relays SHOULD treat authenticaiton events with a valid delegation as if it was the delegator authenticating.
Copy link
Member

Choose a reason for hiding this comment

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

typo

Suggested change
Relays SHOULD treat authenticaiton events with a valid delegation as if it was the delegator authenticating.
Relays SHOULD treat authentication events with a valid delegation as if it was the delegator authenticating.

@Semisol
Copy link
Collaborator Author

Semisol commented Jan 2, 2023

NIP needs a lot of rework. Need to specify:

  • If multiple users can be authenticated per session, if so, how:
    • Default user is null and ["EXECAS", "<pubkey>", <cmd here>] is used
    • Default user is last authenticated and ["EXECAS", "<pubkey>", <cmd here>] is used for any that aren't last
  • How this functions with delegation
  • and a few more stuff

@fiatjaf
Copy link
Member

fiatjaf commented Jan 2, 2023

I've proposed NIP-42 at #141 that just does the basic of the basic and is already super powerful. I think this NIP was already too complicated and EXECAS stuff would triple that complication. It is an interesting idea, but I think we should take smaller steps. Auth is needed today.

@Semisol
Copy link
Collaborator Author

Semisol commented Jan 10, 2023

superseded by #141

@Semisol Semisol closed this Jan 10, 2023
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.

6 participants