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

Move handle resolution to .well-known #1048

Merged
merged 19 commits into from
May 26, 2023
Merged

Move handle resolution to .well-known #1048

merged 19 commits into from
May 26, 2023

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented May 16, 2023

This moves handle resolution to .well-known

Specifically, a handle is resolved by calling GET http(s)://${handle}/.well-known/atproto-did

The expected response is text/plain and simply contains the associated DID

Closes #969

@bnewbold
Copy link
Collaborator

It feels to me like this is generic to DIDs in general, not just atproto, and we could represent that in the path name. Eg, /.well-known/did/${handle} or /.well-known/handle-did/${handle}.

Is it a requirement that the handle in the hostname match the handle in the path?

@bnewbold
Copy link
Collaborator

I actually really like the sub-directory thing, makes it quite easy to do virtual hosting with a bunch of domains and DIDs using something like nginx

@dholms
Copy link
Collaborator Author

dholms commented May 16, 2023

While it is generic to dids, I think it's useful for different ecosystems to be able to differentiate the dids/handles associated with each.

For instance, with the DNS records we do
TXT. _atproto.${handle} "did=did:...."

Is it a requirement that the handle in the hostname match the handle in the path?

I don't have a super strong opinion on this. But I do think it's reasonable to be able to ask an authority what a subdomain resolves to. ie: asking bsky.social what dholms.bsky.social resolves to.

Though canonically, you're encouraged to ask the hostname that matches the handle

@dholms
Copy link
Collaborator Author

dholms commented May 16, 2023

I was trying to match our DNS semantics with the pathing here. maybe .well-known/atproto-did/${handle}?

@@ -48,12 +48,12 @@ describe('handle resolution', () => {
})

it('handles a bad DNS resolution', async () => {
await expect(resolveDns('bad.test')).rejects.toThrow(NoHandleRecordError)
const did = resolveDns('bad.test')
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be an await statement? it's failing tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup 👍

Copy link
Contributor

@Evalprime Evalprime left a comment

Choose a reason for hiding this comment

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

line 51 in the resolve test needs an await statement

@Evalprime
Copy link
Contributor

take a look at https://github.com/protocol-registries/well-known-uris, it's not required obviously but does add some security to the path

@dholms
Copy link
Collaborator Author

dholms commented May 16, 2023

Yeah I think we'll need to wait until we have an actual spec before we can register there

@Evalprime
Copy link
Contributor

is the spec documented and will the lexicon work with the site?

Comment on lines 13 to 18
const dnsRes = await dnsPromise
if (dnsRes) {
httpAbort.abort()
return dnsRes
}
return httpPromise
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :)

@dholms dholms marked this pull request as ready for review May 17, 2023 00:01
Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Yep I'm good with this

@@ -7,6 +7,7 @@
"description": "Provides the DID of a repo.",
"parameters": {
"type": "params",
"required": ["handle"],
"properties": {
"handle": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probs drop the "if not specified" bit of the description (at least it was still in the comment)

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Yep I'm good with this

Comment on lines -13 to +14
COPY ./packages/did-resolver ./packages/did-resolver
COPY ./packages/identifier ./packages/identifier
COPY ./packages/identity ./packages/identity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for remembering this 👍


const url = `${pds.url}/.well-known/atproto-did`
try {
const res = await fetch(url, { headers: { host: handle } })
Copy link
Collaborator

Choose a reason for hiding this comment

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

💎

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Good stuff!

Co-authored-by: devin ivy <devinivy@gmail.com>
@AaronGoldman
Copy link

For some reason I was expecting

https://${handle}/.well-known/_atproto -> did=${did}

To match the DNS form as closely as possible 🤷‍♂️

Comment on lines 84 to 85
const data = await res.json()
return data.did
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going away soon so I don't think it's a biggie, but we might want to ensure that the result here is a string (if not a valid did).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's a good point 👍

@dholms dholms merged commit 743eaf1 into main May 26, 2023
@dholms dholms deleted the well-known-handles branch May 26, 2023 00:04
mloar pushed a commit to mloar/atproto that referenced this pull request Sep 26, 2023
* move handle resolution to .well-known

* required handle on resolveHandle

* rm test

* tidy

* tidy

* fix up appview

* missing await

* atproto-handle -> atproto-did

* shift did & handle resolution to new identity package

* fix up network mocks

* fix up another test

* one more

* drop lex comment

* rm handle param

* Update packages/identity/src/handle/index.ts

Co-authored-by: devin ivy <devinivy@gmail.com>

* still temporarily support xrpc handle resolution

* typo

* ensure return value is a string

---------

Co-authored-by: devin ivy <devinivy@gmail.com>
@zfaizal2
Copy link

zfaizal2 commented Oct 3, 2023

Curious, how are you configuring your domains to support wildcard subdomains (ex. foobar.bsky.social/.well-known/atproto-did)

mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
* move handle resolution to .well-known

* required handle on resolveHandle

* rm test

* tidy

* tidy

* fix up appview

* missing await

* atproto-handle -> atproto-did

* shift did & handle resolution to new identity package

* fix up network mocks

* fix up another test

* one more

* drop lex comment

* rm handle param

* Update packages/identity/src/handle/index.ts

Co-authored-by: devin ivy <devinivy@gmail.com>

* still temporarily support xrpc handle resolution

* typo

* ensure return value is a string

---------

Co-authored-by: devin ivy <devinivy@gmail.com>
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.

Do not allow publicsuffix domains as handles
7 participants