Summary: | SIP server closes the TCP connection when it receives an invalid message | ||
---|---|---|---|
Product: | Koha | Reporter: | Marc Balmer <marc> |
Component: | SIP2 | Assignee: | Marc Balmer <marc> |
Status: | CLOSED FIXED | QA Contact: | Ian Walls <koha.sekjal> |
Severity: | normal | ||
Priority: | P5 - low | CC: | dpavlin, paul.poulain, veron |
Version: | 3.8 | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response.
Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response. RAW login can be as robust as telnet login Combined diff for telnet and raw connections Reformatted Patch Signed off patch [SIGNED OFF] Bug 7787: Make the SIP server much more robust. |
Description
Marc Balmer
2012-03-22 08:08:52 UTC
Created attachment 8475 [details] [review] Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response. Tested by me, needs signoff. To test this patch, you need to first start the SIP server, use the following command: # perl -IC4/SIP/ -MILS C4/SIP/SIPServer.pm C4/SIP/SIPconfig.xml In SIPconfig, you need to add a login and institution to be able to login: <accounts> <login id="arcapos" password="arcapos" delimiter="|" error-detect="enabled" institution="kohadev" /> </account> <institutions> <institution id="kohadev" implementation="ILS" parms=""> <policy checkin="true" renewal="true" checkout="true" status_update="false" offline="false" timeout="100" retries="5" /> </institution> </institutions> Then you need to create a patron with the same name (arcapos in the example above) and give that patron circulation rights. Now you can use telnet to login to the SIP server: # telnet localhost 8023 It will prompt for username/password. Note how the server closes the connection when you just hit enter 3 time or send any garbage. With the patch applied, the server will not close the connection. Created attachment 8501 [details] [review] Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response. Created attachment 8502 [details] [review] RAW login can be as robust as telnet login Created attachment 8511 [details] [review] Combined diff for telnet and raw connections I applied patch 8511. Then I followed Marc Balmers test senario with following differences: - Took /debian/templates/SIPconfig.xml as configuration file for SIPServer.pm (without changes) - Created a patron koha/koha with circulation permisssions (as found in SIPconfig.xml) - run script SIPServer.pm in console #1 -> console #1 displays something like: --Snip------------------- SIPServer config: $VAR1 = bless( { (...) 'syslog_facility=local6' ]; --Snip------------------- - then started telnet in console #2 - telnet localhost 8023 - was prompted for login ->console #1 says: Use of uninitialized value $var in hash element at /usr/share/kohaclone/C4/Context.pm line 1024, <STDIN> chunk 2. - Enter some garbage value in (telnet) console #2 -> telnet answer in console #2: 96 -> repeat several times, same answer - Hit return (without input) ->Telnet connection closes (Connection closed by foreign host.) If I repeat again an hit return just after login, connection closes immediately. Fazit: - Entering garbage does not close connection - Hit "Enter" immediately closes connection - Error (Warning?) in Context.pm as mentioned above. Created attachment 10012 [details] [review] Reformatted Patch The original patch gave some whitespace errors on being applied. The reformatted patch fixes these. I also reformatted the commit message so that it was all readable on a standard screen (It was a single line) ... Not sure of the telnet logic as the connection is not broken after three failed logins sending garbage to a telnet port will permanently occupy that session(?) could it generate a kind of DOS against a sip port in this way. Will try testing this. Created attachment 10362 [details] [review] Signed off patch I removed the update to VERSION from the patch as it conflicts with a recently pushed patch which set VERSION to match the Koha Version I've run this against various scenarios with a test client and it does behave in a more standard compliant way. Shutdown is cleaner in most cases and it avoids the 'die mysteriously' behaviour that can cause clients to hang Removes 3 strikes logic in favour of continuous monitoring of the input. If bad input is passed, the RESEND message is returned, rather than the system hanging. This seems like a solid way to go, in my opinion, but I don't have sufficient SIP clients to test against. Personally, I'm inclined to sign off on this, but I'm not 100% it should pass QA just on my say so. Would another member of the QA team please review this patch as well, preferably running it through some kind of live SIP client? This is one of those occasions where having an extra set of QA-eyes will be very helpful. Created attachment 10392 [details] [review] [SIGNED OFF] Bug 7787: Make the SIP server much more robust. Be liberal in what we accept, but strict in what we send: Never exit the server process, but send a SC_RESEND message (96) to the client if we received anything we don't understand. This is consistent with SIP server implementations of other ILSs. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Ian Walls <koha.sekjal@gmail.com> Comment on attachment 10362 [details] [review] Signed off patch Minor conflict with perl critic fixes... fixed now QA comment: This patch removes more code than it adds, ++ I can't test it on a true SIP server, but looking at the code, I agree with Ian, so I set passed QA QA comment: This patch removes more code than it adds, ++ I can't test it on a true SIP server, but looking at the code, I agree with Ian, so I set passed QA There have been no further reports of problems so I am marking this bug resolved. |