Handle->DID resolution via /.well-known/atproto-did endpoint - #3
Handle->DID resolution via /.well-known/atproto-did endpoint#3anna-germ wants to merge 7 commits into
Conversation
|
Ohh this is a super good idea! Thanks so much for suggesting this. Yeah, the DNS check is kinda awful. Unsure why exactly... I also don't know if this is the best long-term thing, however it does not change public API in any ways, and it seems reasonable to me. So let's give it a shot! |
|
Ahh, ok so the issue here, unfortunately, is that this now has a dependency on Foundation but it is not imported. This actually is a real problem on Linux. But that's something I will address eventually if it becomes important to do so. |
|
#4 should address the URLSession dependency that is failing the Linux tests (it also fixes the visionOS test issue) We could then adopt that API here to make it work on Linux. |
|
Converting to draft as I (and potentially Mark) work on a few to-do’s:
|
This reverts commit 9c360c7.
Improve handle resolution by racing DNS vs. /.well-known
36c50b8 to
5d74541
Compare
|
Hellooooo! In the wake of Mark's PR, and also some work Mark did on running the DNS and well-known checks in parallel, I've replaced this PR with a new version. One note on top of a code-specific flag I'll send:
|
| } | ||
|
|
||
| public protocol ResponseProviding { | ||
| public protocol ResponseProviding: Sendable { |
There was a problem hiding this comment.
Mark pointed out that you have a conditional conformance of ATResolve being Sendable if ResponseProviding is Sendable -- I'm making new problems for you by now requiring ResponseProviding to always be sendable. This is our best attempt at making the task groups work, but would defer to your expertise!
There was a problem hiding this comment.
Very generally speaking, Sendable protocols making the lives of the users of the protocol (that's us here) easier, while making the conformer's lives (that's the libraries clients) harder. In this case, however, I think it is pretty reasonable. So I think it's fine, we just need to remove the conditional conformance.
|
The problem with this newer PR (with the task group) is that it now takes as long as the longest task. I'd add a @mattmassicotte do you have any good suggestions? Some discussion happening here: https://forums.swift.org/t/accept-the-first-task-to-complete/54386 |
|
Cancellation could end up being tricky to support here, because it ultimately relies on the subsystems doing that internally. That thread kind of gets into the details. |
| let didRecord = txtRecords.first { record in | ||
| record.txt.hasPrefix("did=") | ||
| } | ||
| return didRecord?.txt.components(separatedBy: "=").last |
There was a problem hiding this comment.
This could technically have issues, as some did methods can include query strings, so you probably want to just split on = and then drop the first component, then join the rest.
ThisIsMissEm
left a comment
There was a problem hiding this comment.
I suspect the cancellation issue is due to the checkDNS being an attached task, where as in this implementation, they're explicitly detaching the DNS resolution task: https://github.com/ATProtoKit/ATIdentityTools/blob/main/Sources/ATIdentityTools/Handle/HandleResolver.swift#L80 (Note: Apache 2.0 not BSD license)
|
|
||
| static func checkWellKnown(handle: String, provider: Provider) async -> String? { | ||
| do { | ||
| let dataResult = try await provider.data( |
There was a problem hiding this comment.
I'm not sure if the Provider here would allow it, but you could set a maximum response body size to like 1kb, and stop reading the response after that, since anything larger and you know it's not a valid response (it's likely HTML)
There was a problem hiding this comment.
I think it should be possible to make an appropriate provider function that does this, if the underlying request system allows it?
| // Only check Cloudflare and Google DNS servers | ||
| var dnsOptions = CAresDNSResolver.Options.default | ||
| dnsOptions.servers = ["1.1.1.1", "1.0.0.1", "8.8.8.8", "8.8.4.4"] |
There was a problem hiding this comment.
This resolves the DNS timeout issue, but we can also adjust dnsOptions.timeoutMillis and dnsOptions.attempts
There was a problem hiding this comment.
We could make this a variable on the resolver, and allow overriding it, but using these defaults — since they're major DNS server providers. We may want to provide IPv6 options here too.
Another option is to not use AsyncDNSResolver and instead just use DNS over HTTPS: https://github.com/bluesky-social/atproto/blob/9dac8b0c600520ecb0066ac104787b27668dea47/packages/internal/handle-resolver/src/atproto-doh-handle-resolver.ts#L37
which would be somewhat more secure than using standard DNS (which is cleartext). That would also then allow this to be fully cancellable.
There was a problem hiding this comment.
That sounds like a really cool option. Perhaps that could be another thing in the chain of checks this system does?
There was a problem hiding this comment.
Well, with a bit of reorganisation we could hsve different handle resolvers, DNS, HTTP and DNS over HTTPS, and then folks could choose the right methods for them?
i suspect DoH would be superior here even though response times might be slightly higher than DNS
There was a problem hiding this comment.
Sounds great to me! What I was trying to go for was a "all discrete options public API" so you can get the behaviors you need, if you have advanced requirements. And then also something pre-configured so there's an easy (but potentially suboptimal) thing for people that aren't interested in learning how it all works.
|
The use of |
|
Do you still have any interest in pursuing this one? |
|
We aren't pursuing this, you can feel free to close. |
Hi Matt! This may not be the exact way you'd want to implement this, but I wanted to propose an ATResolve update that accounts for the second way people can register handles, via
.well-knownpath instead of DNS TXT record, as per:I've found that the DNS check takes a while (~1 min) if the appropriate TXT file isn't there (it falls back appropriately to the getProfile function, it just takes a while to reach there), and in my experience the well-known check can be slow as well if that endpoint doesn't exist, so I'm suggesting racing the DNS check against the
./well-known/atproto-didand returning whenever we find a value.