Bug #534
closedV případě serverové výjimky RPC volání nevrátí validní SOAP
Added by Pavel Kácha over 11 years ago. Updated over 11 years ago.
0%
Description
V současné době se na serveru volá errMsg, která vygeneruje výjimku s plaintextem chyby, za ním ale ještě SOAP dispečer pošle obecný fault, který se navíc o délku chybového plaintextu nevejde do spočítané délky odpovědi, z pohledu klienta pak odpověď vypadá například:
8<--------------------
Errors in config file '/opt/warden-server/etc/warden-server.conf': Missing right curly or square bracket at /opt/warden-server/etc/warden-server.conf line 48, at end of line
syntax error at /opt/warden-server/etc/warden-server.conf line 48, at EOF
<soap:Envelope xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:soapenc="http://schemas.xmlsoap.org/soap/encoding/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" soap:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/"><soap:Body><soap:Fault><faultcode>soap:Client</fau
8<--------------------
Krom uložení lokálního stavu do logu je potřeba chyby upravit tak, aby SOAPová odpověď mohla klientovi vrátit korektní 'fault', nejlépe právě s textem chyby, který se posílá errMsg.
Related issues
Updated by Tomáš Plesník over 11 years ago
Tak jsme cele vcerejsi odpoledne stravili s Kubou dumanim, jak vyresit jiz od zacatku se tahnouci problem s navratovymi hodnotami a chybovymi hlaskami od serveru ke klientovi (ticket 534). Po dlouhe diskuzi, zkouseni a premysleni, nam pripadne jako nejlepsi to cele predelat nasledujicim zpusobem, ktery je velice jednoduchy a primocary:
- funkce budou vracet pomoci return dva prvky
- 1. navratovou hodnotu 0 (false - nekde se stala chyba) nebo 1 (true - vse dopadlo uspesne)
- 2. popis chyby pokud navratova hodnota je 0 a SOAP objekt nebo pole SOAP objektu nebo undef pokud navratova hodnota funkce je 1
Na serveru by to pak vypadalo nasledovne:
return (0, "Error message");
nebo
return (1, undef); return (1, $soap_object); return (1, @soap_object_array);
V klientovy by se pak toto zpracovavalo pomoci:
- ziskani navratove hodnoty 0 nebo 1:
my $ret_val = $response->result;
- ziskani chybove zpravy, undef nebo dat:
my $params = $response->paramsout;
Dle naseho nazoru nam zavedeni tohoto pristupu zajisti pouzitelnost i pri vyhozeni SOAPu, vyresi ticket 519, zjednodusi praci s chybami a daty, usnadni testovani a vse celkove sjednoti. Pri aktualni implementaci totiz sice vime, ze se volani nepovedlo, ale pouze tise doufame, ze se povedlo, protoze pro to nemame kontrolu.
U Kubovych Unit testu zase pomuze s jejich zjednodusenim, ktere je take dulezite, protoze pak staci testovat navratovou hodnotu fce 1/0 pomoci "ok()". Neni potreba resit toto slozite pres "is()" a pripravovat si dummy data a vypisy z certifikatu pro porovnani vystupu testovane fce.
Pavle, muzes nam k tomu dat nejaky komentar, nez se pustim do prepsani?
Diky a s pozdravem,
Tom a Kuba
Updated by Pavel Kácha over 11 years ago
- Assignee set to Tomáš Plesník
- Priority changed from Low to Normal
(0, result)¶
Nejprve k (0, result). Myslím, že ok(volaniMetody(param)) tím nezískáme, dokonce tím ztratíme i booleanské chování výsledku, tj. možnost testovat chyby pomocí:
errMsg("blabla") unless res=volaniMetody(param)
nebo
volaniMetody(param) or errMsg("blabla")
Protože pole (0, errmsg) neevaluuje jako false, bude potřeba použít
meziproměnnou:
@res = volaniMetody(param);
errMsg("blabla") unless @res[0];
nebo (pro testování)
@res = volaniMetody(param); ok(@res[0]);
Ne že by to byla nějaká krize, ale zesložiťuje to kód na řešení chyb, který by měl působit spíš tak, že je tam jen mimochodem, kdyby něco (proto se právě často používá ono volani() or return 0, je to jednoduché a minimalistické).
Možná by stálo za to se zamyslet, jaké vlastně funkce, návratové hodnoty a v jakém kontextu voláme.
V dalším textu neřeším WardenStatus, WardenReq a WardenConf což jsou prakticky klientské knihovny.
Vracení klientovi¶
Důležité - s řešením chyb souvisí jak je budeme komunikovat klientovi. Tady záleží, co poleze ven, současné "die" rozbíjí odpověď, ale v současné době před SOAP objektem vypadne ještě chybová hláška z die nebo z errMsg, což na klientovi způsobí chybu parsování a paradoxně tedy chybu odchytí, i když nelogicky a nerobustně (viz #534).
Klientský kód ale už v současné době kupodivu počítá s $response->fault a ošetří ji (viz např. WardenClientReceive.pm:94), takže pokud server vrátí korektní SOAPí "fault" s "faultstring", máme vyřešenou i zpětnou kompatibilitu a api se nemění.
Protože SOAP::Fault se vrací pomocí 'die', nabízí se i mezikrok - sjednotit nejprve do současné verze nekonzistence 'die' a 'errMsg', případně odchytit ještě neodchytávané chyby (execute, dekódování certifikátu), to všechno pomocí úpravy errMsg, která by zalogovala stack (tak jako Suki v #522) a vyhodila výjimku SOAP::Fault. To je celkem minimalistická změna v kódu. A dalšímu čištění a zbavení se výjimek (pokud je opravdu nechceme) se věnovat v příští revizi, kde bychom došli k tomu, že se SOAP::Fault bude vracet jen případně místo returnu na konci rutiny.
Co máme za chyby a co potřebujeme chytat?¶
U odrážek + označuje můj návrh. Poznámka 'neřeší' znamená, že současný kód na možné selhání kašle.
- Máme dva typy chyb:
- externí - klient s nimi může něco dělat, jsou pro něho užitečné - měly by tedy putovat korektně SOAPem.
- interní - něco se serveru nepovedlo zjistit, vytvořit, ověřit, chyba kódu), ty by měly klientovi korektně po SOAPu hlásit něco jako "Server error in getAltNames" nebo "Server error writing to database" a zalogovat detaily pro správce/vývojáře do logu.
- + Funkce, ve kterých můžou nastat jenom interní chyby, mohou jenom vracet 0/undef a skutečný důvod psát do logu.
- + Funkce, které generují jen jednu externí chybu, můžou vracet 0/undef.
- + Z funkcí, které generují víc externích/interních, musíme dostat rozlišení chyby/chybovou hlášku.
Pomocné funkce:¶
- write2log(prio, msg)
- může selhat otevření, zápis nebo zavření logového soketu
- + pokud se nepodaří otevřít, zapsat, nebo zavřít, nedá se nic dělat, jediná možnost je vypsat alespoň na stderr spolu s varováním, že nejde psát do logu a třeba vrátit 0
- getAltNames() -> @alt_names
- nemusí existovat proměnné SSL_CLIENT_S_DN_CN nebo SSL_CLIENT_CERT -> neřeší
- může selhat decode_base64 nebo Crypt::X509->new(cert => $der) -> neřeší
- + interní chyba, zapsat do serverového logu a vrátit undef
- authorizeClient(alt_names, ip, service_type, client_type, function_name) -> %ret{dns, cidr, receive_own}
- může selhat prepare -> die
- nemusí najít klienta -> die
- nepasuje cidr -> die
- + může vrátit hlášku v %ret{err_msg}
- + pak je prepare interní chyba, takže write2log a err_msg="Authorization server error" a ostatní patří do logu
- + ALE u autorizace je zvykem z bezpečnostních důvodů nehlásit důvod selhání, dle mého by měla pouze vrátit undef a důvod plivnout s detaily do logu
SOAP funkce:¶
- saveNewEvent($class, \%data), getNewEvents($class, \%data)
- ... a všecny ostatní SOAPové funkce, protože mají podobnou strukturu: autentizace, autorizace, prepare, execute. Kromě getLastId, která neautentizuje (hm, ale možná by měla).
- může selhat getAltNames -> v současné době ne, tj. neřeší
- nemusí existovat SSL_CLIENT_S_DN_CN nebo REMOTE_ADDR -> neřeší
- může selhat authorizeClient -> neřeší, spoléhá na die
- může selhat prepare -> die
- může selhat čtení nebo zápis z/do db (execute) -> neřeší
- + getAltNames - interní chyba
- + chybějící env vars - interní chyba
- + selhání authorizeClient - "Access denied + auth detaily, které klient poslal
- + selhání prepare - interní chyba
- + selhání execute - interní chyba
Zamyslete se, udělejte si názor, pobavíme se o tom tedy na VC.
Updated by Pavel Kácha over 11 years ago
Takže: * write2log, getAltNames, authorizeClient - chyby logovat, return 1/0 * SOAP funkce - die nahradit pomocnou funkcí, která zaloguje a vyhodí SOAP::Fault s hláškou
- bylo by dobré zvážit chytání chyb, které jsem v předchozím textu označil 'neřeší'
- zvážit u interních chyb jinou hlášku (ukecanější) do logu a jinou ("server error") klientovi
Updated by Tomáš Plesník over 11 years ago
Jen doformatuji a doplnim:
Zmeny ve funkcich:
- write2log, getAltNames, authorizeClient - chyby logovat do syslogu, return 1/0
- SOAP funkce - die nahradit pomocnou funkcí (errSoapMsg), která zaloguje a vyhodí SOAP::Fault s hláškou
- vypustit errMsg a trim (trim stejne pouziva pouze errMsg)
- bylo by dobré zvážit chytání chyb, které jsem v předchozím textu označil 'neřeší'
- zvážit u interních chyb jinou hlášku (ukecanější) do logu a jinou ("server error") klientovi
Updated by Tomáš Plesník over 11 years ago
Tomáš Plesník wrote:
Jen doformatuji a doplnim:
Zmeny ve funkcich:
- write2log, getAltNames, authorizeClient - chyby logovat do syslogu, return 1/0
- SOAP funkce - die nahradit pomocnou funkcí (errSoapMsg), která zaloguje a vyhodí SOAP::Fault s hláškou
Tady bych prave rozdeloval hlaseni do logu (ukacenejsi pomoci write2log) a klientovi (mene ukecane pomoci errSoapMsg)
- vypustit errMsg a trim (trim stejne pouziva pouze errMsg)
- bylo by dobré zvážit chytání chyb, které jsem v předchozím textu označil 'neřeší'
- zvážit u interních chyb jinou hlášku (ukecanější) do logu a jinou ("server error") klientovi
Viz komentar u odrazky SOAP funkce.
Updated by Pavel Kácha over 11 years ago
No, já to myslel tak, že ukecanější půjde pomocí write2log (třeba včetně Carp::longmess), méně ukecané pomocí SOAP::Fault, a to celé zabalí errSoapMsg, která obojí zavolá se správnými parametry... třeba:
# stejná hláška na obě strany
# dobře, možná do logu s longmess a klientovi bez
errSoapMsg("Unauthorized access to '$function_name' from: '$ip' etc. etc.");
# hláška do logu, klientovi půjde jenom "Internal server error"
errSoapMsg("Cannot prepare authorization statement in $function_name: $DBI::errstr", 1);
Updated by Tomáš Plesník over 11 years ago
Pavel Kácha wrote:
No, já to myslel tak, že ukecanější půjde pomocí write2log (třeba včetně Carp::longmess), méně ukecané pomocí SOAP::Fault, a to celé zabalí errSoapMsg, která obojí zavolá se správnými parametry... třeba:
[...]
Dobre tim padem bych to videl na jednoduchou variantu. Vytvorit novou funkci errMsg, ktera v sobe bude mit obsazenu puvodni write2log pro logovani do Syslogu a navic jeste SOAP::Fault pro zasilani zprav klientovi. Tato funkce by pak byla volana se dva parametry: 1. chybova hlaska do syslogu, 2. chybova hlaska pro klienta:
errMsg("Cannot prepare authorization statement in $function_name: $DBI::errstr", "Internal server error.");
Updated by Pavel Kácha over 11 years ago
A původní errMsg vystřelit a na správných místech nahradit die nebo exitem (týká se asi jen čtení konfiguráku). Ok, varianta s dvěmi stringy může být užitečnější v tom, že i hláška pro uživatele může být informativnější pro správce, až ji od uživatele nafasuje, ve stylu "Internal 'prepare' server error".
Updated by Tomáš Plesník over 11 years ago
Pavel Kácha wrote:
A původní errMsg vystřelit a na správných místech nahradit die nebo exitem (týká se asi jen čtení konfiguráku).
Toto uz je hotove. Tykalo se pouze cteni konfiguracniho souboru.
Ok, varianta s dvěmi stringy může být užitečnější v tom, že i hláška pro uživatele může být informativnější pro správce, až ji od uživatele nafasuje, ve stylu "Internal 'prepare' server error".
Ano, presne tak. Naprosto souhlasim.
Updated by Tomáš Plesník over 11 years ago
Modul SOAP::Fault umoznuje nastavit objektu celkem 4 parametry:
- faultcode(optional value)
$fault->faultcode('MethodUnknown'); Returns the current fault code or sets it if a value is given.
- faultstring(optional value)
$fault->faultstring("There is no $method here"); Returns or sets the fault string.
- faultactor(optional value)
$fault->faultcode($header->actor); Returns or sets the fault-actor element. Note that the actor isn't always required in a SOAP fault.
- faultdetail(optional value)
$fault->faultcode(bless { proxy => $ip }, 'Err'); Returns or sets the fault's detail element. Like the actor, this isn't always a required element. Note that fault detail content in a message is represented as tag blocks. Thus, the values passed to this accessor when setting the value are either SOAP::Data objects, or more general blessed hash references.
Ja osobne jsem toho nazoru, ze by nam melo stacit plnit pouze faultstring a pokud bychom chteli do Wardenu zatahnout i cisla chyb, tak potom i faultcode, zde si ale nejsem jisty. Pavle, ses stejneho nazoru?
Updated by Pavel Kácha over 11 years ago
Taky si myslím. Stačí faultstring.
Napadlo mě - až si s tím budeš hrát, zkus jestli se SOAP::Fault nedá vrátit i returnem, nejen diem - mohli bychom se pak výjimek kompletně zbavit, a hlavně bychom mohli (podobně jako v klientovi) chytat ostatní výjimky, neočekávané, nebo ty z volaného kódu, a slušně se k nim zachovat.
Updated by Tomáš Plesník over 11 years ago
Na serveru vznikla nova funkce sendMsg, ktera do sebe spojuje puvodni write2log pro zalogani zpravy do Syslogu a die pro ukonceni zpracovani funkce a poslani hlasky klientovi v pripade chyby. Vse je nakomitovane do GITu, viz 697a22dd.
Ve funkci sendMsg jsem zkousel namisto doporucovaneho die pouzit i return a s nejvetsi pravdepodobnosti se to chova stejne jako puvodni die, ktery ukoncil volanou SOAP funkci a poslal chybovou hlasku klientovi.
Pokud nema nikod vyhrady, tak ticket muze byt uzavren.
Updated by Pavel Kácha over 11 years ago
- Status changed from Resolved to Closed