-
Notifications
You must be signed in to change notification settings - Fork 89
Define /unix
#174
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
Define /unix
#174
Conversation
Adds a protocol note for how to encode paths to Unix domain sockets as strings, that may include the delimiting character of `/`. This allows us to append other tuples to the multiaddr while also ensuring we can round-trip the address to a string and back. This doesn't affect the binary representation of the multiaddr since everything is length-delimited. Takes inspiration from #164 and proposes using URI encoding for the segment, the same as the `/http-path` component. One difference is if the path is to represent the filesystem root, it must be included in the value portion of the tuple, otherwise it can be omitted.
Need to resolve how to encode unix paths so peer ids can be appended to them - multiformats/multiaddr#174
Are there any backwards compatibility issues? Is anyone using this multiaddr currently? |
I'm not sure how heavily it's utilized, but kubo can use those addresses. e.g. https://github.com/ipfs/kubo/blob/4009ad3e5a502518ddc7d48a888707f812ddc629/docs/config.md?plain=1#L219 |
I haven't looked too deeply at usage, but I'd suspect so. The question is what to do about it. There are a bunch of issues that are basically about the failures related to protocols (like http or unix) in taking unescaped paths like #139 #55. They were also known from the earliest days multiformats/go-multiaddr#31. Maybe the answer is to just break it, or to say that implementations MAY/SHOULD/MUST preferentially try looking for the entire path as a socket and if that fails will try with just the first path segment as a socket. Not specific to this PR, but between http-path and this perhaps worth biting the bullet on specifying how "path" types should be done in general. Maybe the answer is just to require them all to be escaped, maybe it's something else (e.g. closer to @Stebalien's proposal in multiformats/multiformats#55). |
There will be backwards compatibility issues with the string representation of the path due to the escaping, though not with the binary representation. I think this is used but not very commonly. The js stack can also use unix addresses but currently only as a terminal element, that is no tuples can follow it. Arguably the lack of escaping has harmed it's use - I know I've tried to use it a few times over the years but always come up hard against the inability to append anything else to the address and the various issues asking for clarification etc speak to a need for this.
I wonder if this could get exploited by making longer paths with segments that mirror tuples after the unix section? Possibly with
I kind of think so. The original PR was merged in haste, the question about if we need to append things to it was unresolved. It was flagged as experimental and subject to change so..
It's interesting that - it seems to mandate parsing paths left to right, we recently switched multiaddr-to-uri to parsing right to left to support some forms of multiaddr. It also doesn't seem to say much about escaping so we'd probably still need to solve the same problem there. |
Are there any further thoughts here? |
@achingbrain: Since I just noticed it wasn’t mentioned I'd just like to mention my old MultiAddr proposal from 2019 again: #87 It’s mainly about argument to individual MultiAddr path segments, but it also proposes syntax for encoding paths in MultiAddr segments:
This is IMHO significantly easier to read than using percent-encoded local file system paths. |
Thanks, I didn't see that issue. I like your proposal for using To solve this particular issue though, I would prefer to be consistent with other multiaddr tuple types. HTTP has already solved this problem by percent escaping the characters so I think we should use that here too. |
@achingbrain: I can see where you’re coming from, but I’d just like the point out that the major benefit, besides nicer style, is the fact that it’s backwards-compatible! (We are talking about MultiAddr v1 here after all. 🙂) I know most of the MultiFormat people tend to lean on the string-representation => unimportant side, but at least By contrast, Not saying you shouldn’t go with your proposal, but that is something to thoroughly consider I think! |
Are there any limitations on the character set that can be used in a path? I'm assuming that the characters are all UTF-8 encoded so unicode characters like ↑ (e.g. up arrow, UTF-8: |
Not really, it's kind of the wild west since most "unix" implementations can trace their lineage prior to UTF-8 and other modern conveniences. POSIX has a definition of portable file name characters but realistically it could be UTF-8, ASCII or char[]. I think just not NUL As per this proposal Full details are in RFC 3986, linked to from the proposed changes in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting on the spec itself, it seems reasonable. I won't comment on whether this is safe to deploy or if it will break existing users of the unix multiaddr component.
Given this has been open for over six months I think this has been satisfied. I will merge this next week unless there are strenuous objections. |
This is a PoC that aims to support multiformats/multiaddr#174 while not breaking existing Kubo users. See TODO in daemon.go – likely we want to move this to https://github.com/multiformats/go-multiaddr
If this only governs the serialised form: Please do not merge this while either (a) this implies that an initial / is inserted if a path doesn't have one or (b) this implies that a relative path somehow magically becomes an absolute one. These are both severe footguns, and in the (a) case it also excludes serialising abstract socketaddrs. If this governs both the serialised form and opines on ORM/binary forms: Please do not merge this. The spec is not reasonable because it (a) doesn't include abstract socketaddrs, (b) allows dropping the initial / which is nonsensical since that, instead of indicating a relative path (which is normal and real), somehow indicates an absolute path despite indicating the opposite, (c) allows dropping the initial / which precludes further extensions that would allow encoding abstract addresses. Also, this completely misses the 3 unix socket types (STREAM/DGRAM/SEQPACKET). These map directly onto tcp/udp/{nothing}. What's the convention for these? I'm implementing a libp2p unix-domain back-end where I need to parse Multiaddrs and I would like to not clash with the ecosystem. (If there is no ecosystem I will define one following the principles below.) The two real address formats are:
(1) are subject to normal path lookup (2) are used directly as keys into a kernel hashmap Canonically, these are rendered (1) directly and (2) with an I don't really see a reason to not render these directly (with an optional percent-encoding step for display, sure), so To actually use them you would strictly need to add |
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
God that's a lot of writing to say "i think we should just percent-encode the address instead of making it worse". Implementation of this proposal in https://github.com/libp2p/rust-libp2p/pull/6056/files#diff-ccb7f1d7de0deedbdbbcb4262ff70cd8658a1a0178fb3795e1b1a9dbbd00f919R363 |
Ripped 100% off transports/tcp Uses proposed encoding from multiformats/multiaddr#174 (comment)
@nabijaczleweli thanks for your feedback and the additional use cases.
As noted in the OP, this only affects the string representation of a multiaddr.
The assumption in this PR is that a relative path is only useful in the current context - if you try to interpret it starting from a different directory the path may not resolve or may resolve to an unexpected file which also seems like a significant footgun. I can appreciate a light-touch garbage-in/garbage-out approach however so I am not opposed to dropping the initial
This sounds reasonable.
I think there are two ways forward here:
|
My preference would be for option 2 since relative paths in multiaddrs seem to be too fragile to be useful. What do you think @MarcoPolo? |
Sorry, I don't think this makes sense. "This is different if you resolve it in a different context" is true of all paths/addresses, the point of an address is to tell the specific process you're giving it to how to call the peer you're thinking of. The "root" directory and the current directory are exactly the same in this model, and both are start-of-lookup inodes stored in the resolving process, configurable by the resolving process. Restricting this to just the root directory is strictly worse (and more ambiguous and less readable and less convention-following) since controlling the root directory is more convoluted and more disruptive for the resolving process than controlling the current directory. You could analyse inet sockets the same way: on the same host, depending on the time, network namespace, routing table, &c., two different inet addresses can resolve to the same peer and two different peers can be reachable at the same address. The same more obviously applies to dns entries. I think splitting unix into unix and unix-abstract makes sense (and this additionally cleans up the into of "unix works everywhere, unix-abstract works on linux" from "unix sometimes only works on linux, depending on the data") and GIGO percent-encoding them both makes the most sense. |
Please cf. #179 for my proposed wording. |
It's not really. Something like
..which
It's possible, if the resolving process doesn't have access to the root or some intermediate directory(s). Maybe then you'd need relative paths and accept that it's all GIGO. Anyway, like I said, I think just percent-encoding the address and dropping the
Great, it sounds like you're in favour of option 1. I'm fine with that. I think we can make the amendments to I see you've added a bunch of other stuff to #179, I don't want simply being able to escape unix paths (the real win here) to get bogged down in discussion of other features. |
I'm happy with the current wording, I can rebase #179 when this is merged. |
Summary
Adds a protocol note for how to encode paths to Unix domain sockets as strings, that may include the delimiting character of
/
.This allows us to append other tuples to the multiaddr while also ensuring we can round-trip the address to a string and back.
This doesn't affect the binary representation of the multiaddr since everything is length-delimited.
Takes inspiration from #164 and proposes using URI encoding for the segment, the same as the
/http-path
component.One difference is if the path is to represent the filesystem root, it must be included in the value portion of the tuple, otherwise it can be omitted.
Before Merge