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 nip-42: authentication #141

Merged
merged 9 commits into from
Jan 16, 2023
Merged

add nip-42: authentication #141

merged 9 commits into from
Jan 16, 2023

Conversation

fiatjaf
Copy link
Member

@fiatjaf fiatjaf commented Jan 2, 2023

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 2, 2023

Who else is implementing chat stuff so I can tag them here? @ArcadeCityMayor

42.md Outdated Show resolved Hide resolved
42.md Outdated Show resolved Hide resolved
42.md Outdated Show resolved Hide resolved
@ismyhc
Copy link

ismyhc commented Jan 3, 2023

Im for this nip 100%. I believe that a simple method to auth (prove ownership of keys) was missing from the protocol. I think this solves that as well as lends the ability for a relay to restrict access to events. Of course the relay still has access, but it's the clients choice to store where they see fit.

This gives me more hope for Seer :)

42.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jan 4, 2023

Concept ACK

Copy link
Collaborator

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

concept ACK, preferably do not merge until a working relay and client is released

42.md Outdated Show resolved Hide resolved
@jblachly
Copy link

jblachly commented Jan 5, 2023

Sorry to be a late entrant to the conversation. What is the rationale behind the combination of (~10 minute time window, URL of relay in content), versus a traditional challenge-response?

Challenge-response requires current possession of the key, whereas an unlimited number of these authentication messages could be pre-generated with temporary possession of a key (admittedly less likely than simply exfiltrating the key itself).

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 5, 2023

Challenge-response is harder to implement and there are no benefits whatsoever, or at least I am not aware of any. What are the benefits? "temporary possession of a key", as you noticed yourself, is not a real thing that exists.

@cameri
Copy link
Member

cameri commented Jan 15, 2023

@Semisol

I think the people just proposing a challenge are missing the point that they can forward the challenge to someone on another relay and impersonate them. Anyway, a few requests:

Challenge + Relay URL solves that problem.

@davestgermain
Copy link

I think the people just proposing a challenge are missing the point that they can forward the challenge to someone on another relay and impersonate them. Anyway, a few requests:

That would be a malicious client.
A malicious client could also read your decrypted DMs and forward them to the NSA.

@cameri
Copy link
Member

cameri commented Jan 15, 2023

I think the people just proposing a challenge are missing the point that they can forward the challenge to someone on another relay and impersonate them. Anyway, a few requests:

That would be a malicious client. A malicious client could also read your decrypted DMs and forward them to the NSA.

@cameri cameri closed this Jan 15, 2023
@cameri cameri reopened this Jan 15, 2023
@cameri
Copy link
Member

cameri commented Jan 15, 2023

Sorry, misclicked!

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 15, 2023

Should clients be simple now and relays complex? Seems like we all forgot something here hehe

I don't follow. Do you think relays should be simple and clients complex or vice-versa?

I'm just curious, because regardless of your answer, I don't think keeping track challenges issued by the relay to each client is less complex than keeping track of used challenges in the last 10 minutes, or am I wrong?

To keep track of used events one can just use a global like

var challenges = []

// cleanup stuff every 3 minutes
setInterval(() => {
  challenges = challenges.slice(challenges.findIndex(([d]) => d < Date.now() - 60 * 10 * 1000))
}, 60 * 3 * 1000)

// then when a client sends an AUTH message
let challenge = authEvent.tags.find(([t, v]) => t === 'challenge')[1]
if (challenges.find(([_, c]) => c === challenge)) throw new Error('bad auth: reused challenge')
challenges.push([Date.now(), challenge])

Now to keep track of challenges issued to each client it would be something like

var challenges = {}

// when client connects
let challenge = somehowGenerateRandomString()
challenges[ws] = challenge

// then when client sends an AUTH message
let challenge = authEvent.tags.find(([t, v]) => t === 'challenge')[1]
let expected = challenges[ws]
if (!expected || challenge !== expected) throw new Error('bad auth: reused challenge')

// then when client disconnects
ws.on('close', () => {
  delete challenges[ws]
})

@davestgermain
Copy link

davestgermain commented Jan 15, 2023

I'm just curious, because regardless of your answer, I don't think keeping track challenges issued by the relay to each client is less complex than keeping track of used challenges in the last 10 minutes, or am I wrong?

What if your relay is multi-process?

What happened to the idea of keeping the challenge scoped to the websocket connection? There's no need to remember "used challenges". The relay just needs to keep track of one challenge per connection. For instance, my python relay is issuing challenges in the form of '<client_ip_address>:<random 4 bytes>'. (I could easily switch to sending a hashed challenge with secret)

@cameri
Copy link
Member

cameri commented Jan 15, 2023

@fiatjaf this wouldn't work on Nostream since new connections are handled by a different process and the memory isn't shared. The challenges would have to be cached using Redis or on a primary process and it could lead to race conditions if locks are not used.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 15, 2023

You mean each connection has a different process? Don't you already use Redis in this case? With Redis this is much easier. Just save the challenge on Redis and make it expire after 10 minutes.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 15, 2023

What if your relay is multi-process?

Then you store the used challenges on Redis and that is super easy.

What happened to the idea of keeping the challenge scoped to the websocket > connection? There's no need to remember "used challenges".

This is for the version of this NIP that doesn't require the relay to send a challenge to the client.

@cameri
Copy link
Member

cameri commented Jan 15, 2023

Nostream would need to use a lock to make sure only one authentication attempt can use the challenge (which is now a shared resource) so it will slow down considerably. If locks are not used then a race condition is theoretically possible where more than 1 client can auth if timed correctly.

@Semisol
Copy link
Collaborator

Semisol commented Jan 15, 2023

Nostream would need to use a lock to make sure only one authentication attempt can use the challenge (which is now a shared resource) so it will slow down considerably. If locks are not used then a race condition is theoretically possible where more than 1 client can auth if timed correctly.

... each socket has its own challenge and auth state. there is no one challenge that is shared across all clients

@Semisol
Copy link
Collaborator

Semisol commented Jan 15, 2023

I think the people just proposing a challenge are missing the point that they can forward the challenge to someone on another relay and impersonate them. Anyway, a few requests:

That would be a malicious client. A malicious client could also read your decrypted DMs and forward them to the NSA.

You're getting it wrong. Relay A could connect to relay B, get a challenge, send that to you, and you sign it, which then allows relay A to impersonate you on relay B

@davestgermain
Copy link

You're getting it wrong. Relay A could connect to relay B, get a challenge, send that to you, and you sign it, which then allows relay A to impersonate you on relay B

True, and that would be a malicious relay...

So, keep the relay domain in the tags, as the current proposal states.

@cameri
Copy link
Member

cameri commented Jan 15, 2023

Nostream would need to use a lock to make sure only one authentication attempt can use the challenge (which is now a shared resource) so it will slow down considerably. If locks are not used then a race condition is theoretically possible where more than 1 client can auth if timed correctly.

... each socket has its own challenge and auth state. there is no one challenge that is shared across all clients

I'm responding to Fiatjaf's suggestion on caching challenges so they are not re-used, it introduces a "double-spend" problem that needs solving. If the challenge is per socket we don't need to check any cache, cause every connection has it's own challenge.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 16, 2023

nbd-wtf/go-nostr@9775016

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 16, 2023

fiatjaf/relayer@fe91197

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 16, 2023

I'm going to merge this if no one objects until tomorrow.

(Except @Semisol's objections that we must have a way for the client to initiate the process, which I don't think is reasonable as I think I've explained before and it can always be added if it proves necessary.)

@Semisol
Copy link
Collaborator

Semisol commented Jan 16, 2023

(Except @Semisol's objections that we must have a way for the client to initiate the process, which I don't think is reasonable as I think I've explained before and it can always be added if it proves necessary.)

It's just a handler for the AUTH command that starts the authentication flow. It's simple so why not have it in the initial spec?
Most relays will probably have a dedicated function to call for doing authentication anyway so the implementation should be at most a few lines.

@cameri
Copy link
Member

cameri commented Jan 16, 2023

(Except @Semisol's objections that we must have a way for the client to initiate the process, which I don't think is reasonable as I think I've explained before and it can always be added if it proves necessary.)

It's just a handler for the AUTH command that starts the authentication flow. It's simple so why not have it in the initial spec? Most relays will probably have a dedicated function to call for doing authentication anyway so the implementation should be at most a few lines.

I'll support it. It's good for testing as well.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 16, 2023

"Why not?" is the source of all protocol bloat.

@fiatjaf fiatjaf merged commit be0a426 into master Jan 16, 2023
@fiatjaf fiatjaf deleted the auth branch January 16, 2023 11:28
@alexgleason
Copy link
Member

This NIP could be expanded to let relays reject creating events from a pubkey you didn't authenticate with. Currently nothing prevents me from taking someone else's signed event from one relay and putting it onto another relay.

@phyro
Copy link

phyro commented Mar 1, 2023

@alexgleason I wouldn't put this restriction on for several reasons:

  1. It prevents me from backing up tweets of other people (potentially influencers like Elon)
  2. Losing the private key makes it impossible to re-upload events
  3. It prevents potential privacy improvements like doing dandelion++ or similar for event publish/broadcast

In general, if you speak out publicly, you should expect and assume others will hear your voice. In this case, the voice just takes the shape of this Event object.

@fiatjaf
Copy link
Member Author

fiatjaf commented Mar 1, 2023

This NIP could be expanded to let relays reject creating events from a pubkey you didn't authenticate with.

They can already do this if they want. It's a relay policy.

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.