Opened 5 years ago

Closed 4 years ago

#1488 closed enhancement (fixed)

SAM spec improvements

Reported by: Leon Mergen Owned by: zzz
Priority: minor Milestone: 0.9.24
Component: apps/SAM Version: 0.9.18
Keywords: Cc:
Parent Tickets: Sensitive: no

Description

After recently implementing a SAM API, I have found room for improvement in the documented spec. Specifically, more consistency and clarity is required on some points.

Attached you will find patches that improve this documentation. What I have done is be more consistent in the naming of the variables (e.g. $foo means unquoted variable, $(foo) means quoted variable, {$foo,$bar} means either $foo of $bar), and formalizing the protocol.

The java SAM implementation still adheres to this protocol, although it's merely by accident and not because of the spec is actually properly implemented (notably when an I2P_ERROR is generated, the quoted message is not properly escaped).

Furthermore, in order to pave the way for the future, I recommend that we provide the support to escape the following characters when inside a quoted variable, based on the same characteristics of a JSON value: ", \, \t, \r, \n, \b, \f

Since the protocol is ASCII-only, we do not have to take into account the escaping of unicode characters. The character escaping is not strictly required at this point, but it would be a good idea if new APIs implementing the SAM spec will support this.

Attached are attachments that improve upon the documentation to support this.

Subtickets

#1325: SAM - Parsing of Propertiesclosedmkvore

Attachments (3)

0001-Rewrites-SAMv3-spec.patch (19.7 KB) - added by Leon Mergen 5 years ago.
0002-Fixes-some-protocol-specification-errors.patch (1.9 KB) - added by Leon Mergen 5 years ago.
0003-Improves-protocol-specification.patch (4.7 KB) - added by Leon Mergen 5 years ago.

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by Leon Mergen

Changed 5 years ago by Leon Mergen

Changed 5 years ago by Leon Mergen

comment:1 Changed 5 years ago by zzz

I'm all for improving the spec and the implementation, but let's be careful not to break anything. I saw some discussion on IRC where people sounded awfully confident about what features are or aren't not used. There's a large number of old and unmaintained SAM apps out there and it's hard to be sure about that. Any talk about the Java implementation being broken because it doesn't match the spec needs to be carefully considered.

The same guy that coded SAM wrote the spec, and probably in that order, so I'd say the implementation is controlling. We want to avoid breaking some obscure app, or iMule which is probably the biggest.

We have some other SAM tickets and we've marked them all low-priority for now. If somebody wants to tackle them, great. At least half the effort will be testing.

Thanks for your contribution.

comment:2 Changed 5 years ago by Leon Mergen

I've been very careful to take that into consideration. All existing SAM implementations are compatible with this spec — it just excludes some corner cases which, in practice, never occur: for example, messages sent as quoted strings are not escaped within the SAM implementation. This is not a problem, because there probably never is a quote inside the actual message, but it might be a good idea to narrow this down in the spec right now.

The discussion on IRC was by the ip2d maintainer about whether STREAM FORWARD was ever used, and that his implementation of SAM does not support this (yet). It would be very extreme to get rid of that feature from the SAM spec.

This patch is about tidying up some things: being more consistent in the variable namings, adding some warnings where appropriate, and narrowing down the spec where appropriate. The existing SAM implementation in I2P completely follows this spec (albeit more by accident).

comment:3 Changed 5 years ago by str4d

Thanks for your efforts!

+1 on zzz's caution. Fixing the spec to match the current implementation is great. But if there is a corner case that the current java impl allows that the spec doesn't, and the java impl was tightened up to match the spec, then it could break apps that happened to use that corner case. From reading the patches, most of it is just tidying up the spec to be self-consistent (using the same notation for single parameters, explicitly defining when an option can have several values etc). The only major corner case is that SAM doesn't encode any of its message strings, and I personally struggle to see how any developer could leverage that in a legitimate way when IIRC the only message strings are returned errors, but even in the 9 months since 3.1 was released someone *could* have misused it.

After discussion on IRC I am in favor of leaving any protocol tightening to v3.2. That is, the SAM server would not enforce any character encoding (or any other shouldn't-have-been corner cases that are removed) for versions < 3.2. Thus, as far as these patches go, the ones that merely clean up the spec could be applied soon, along with warnings about all corner cases that exist but should not be relied on. Then in a separate ticket, the spec can be patched to say that in 3.2, these corner cases are now invalid.

In other words, these patches are probably very close to all being applied, in that they describe what the protocol should be doing, but need to also describe what is currently permitted that will not be in future versions.

comment:4 Changed 5 years ago by Leon Mergen

Ok, if mkvore agrees, I will:

  1. create a separate patch which only adds documentation / consistency to the documentation and leaves the documentation about the protocol unchanged; in other words, this will add the few warnings about race conditions i encountered and use consistent variable naming. I will also make an explicit note at the protocol section that acknowledges this protocol definition is pretty loose, and will be tightened in a future release.
  1. create a separate ticket for a SAMv3.2, that tightens the protocol definition and adds character escaping.
  1. if there is support in doing so, I will clean up the SAMv3.2 protocol code to handle character escaping properly. I will use my Haskell SAM API to stress-test the protocol and ensure backwards compatibility, but, since we do not want to risk breaking backwards compatibility in any case, keep this out of SAMv3.1

Also, I would like to know what the opinion is about APIs that implement the protocol incorrectly. For example, the SAM spec explicitly says that parameters can be sent in any order (for example, STREAM STATUS A=B C=D and STREAM STATUS C=D A=B should both be interpreted as the same message), but in practice I have found libraries that do not implement this correctly. I would say we should err on the side of enforcing backwards compatibility unless it's totally undesirable (for example, between major version upgrades); but this is a topic that needs to be discussed.

comment:5 Changed 5 years ago by zzz

mkvore has not been heard from in many years, so his agreement can't be a precondition for anything.

comment:6 Changed 5 years ago by Leon Mergen

Ah I'm sorry, I assumed he would make the call since he is the owner of this ticket.

In any case, let's just rephrase it with that if people generally think this is a good path to follow, I volunteer for making it work.

comment:7 Changed 5 years ago by orignal

DATAGRAM SEND message should be mentioned in "SAM repliable datagrams : sending a datagram" rather then in "V1/V2 Compatible Datagram Handling".

"SAM repliable datagrams : forwarding datagrams" doesn't explain how a received datagram should be forwarded: through session ceonnection same way as DATAGRAM RECEIVE or through UDP.

comment:8 Changed 5 years ago by Leon Mergen

Resolution: wontfix
Status: newclosed

Closing ticket due to obvious lack of interest to improve upon the current situation.

comment:9 Changed 5 years ago by zzz

Resolution: wontfix
Status: closedreopened

Reopening.

I'm actually working on #1572 now and cleaning up the specs as I go. There's also #1323 to work on which will result in SAM v3.2. Perhaps orignal's issues in comment 7 above should have been in a new ticket, but they aren't and so we should leave the ticket open for that also, so it doesn't get forgotten. There isn't any dedicated SAM maintainer since mkvore vanished years ago, but don't confuse lack of time for "lack of interest". Lots of projects are using SAM and good docs can only help.

comment:10 Changed 5 years ago by zzz

Several doc updates pushed in 9d86b2244685ee80948d970ffb1c0a6b4d8b22ca.

Took some of the changes from attachement 0001. The big new part in 0001 (and modified in 0002 and 0003) is documenting a proposal for escaping in key-value pairs. Since we don't do that now, I of course didn't include it.

comment:11 Changed 5 years ago by zzz

Above checkin addresses the first part of comment 7 but not the second.

After a discussion on IRC, we can't find anybody that knows how datagram forwarding is supposed to work. For further research.

comment:12 Changed 5 years ago by zzz

Confirmed by psi and by inspection of SAMv3DatagramSession that forwarded datagrams are sent to the specified host:port via UDP.

v3 spec clarified in 81f1b032f5c2e27cd3ac512dc8459f1fa6041eea.

This resolves the second part of comment 7.

comment:13 Changed 4 years ago by zzz

Add a subticket #1325.

comment:14 Changed 4 years ago by zzz

Milestone: undecided0.9.24
Owner: changed from mkvore to zzz
Status: reopenedaccepted

parser rewritten in 99478af9bea7951220238bc86b25d5af44d271ec to be 0.9.23-5
backslash escapes in 99478af9bea7951220238bc86b25d5af44d271ec to be 0.9.23-5
More to do including docs and UTF-8

comment:15 Changed 4 years ago by zzz

comment:16 Changed 4 years ago by zzz

Resolution: fixed
Status: acceptedclosed

Parser improvements documented in bb0c0da197c30a7587ed0154cb37e3b7899de7d5
I believe this completes this ticket.

Note: See TracTickets for help on using tickets.