diameter: answer with an error Result-Code when a handler raises - #338
diameter: answer with an error Result-Code when a handler raises#338hsdfat wants to merge 4 commits into
Conversation
Takuto88
left a comment
Takuto88
left a comment
There was a problem hiding this comment.
Thank you very much for this contribution! During review, a few things popped up that I'd like to address first before merging as this also introduces new subtle bugs. After that, this is good to go.
While you are at it, can you also resolve the merge conflicts that were created while I merged PR #337 by rebasing please?
| except Exception as e: | ||
| self.logTool.log(service='HSS', level='error', message=f"[diameter.py] [generateDiameterResponse] [{diameterApplication.get('requestAcronym', '')}] Error generating response: {traceback.format_exc()}", redisClient=self.redisMessaging) | ||
| return '' | ||
| return self.Respond_ResultCode(packet_vars, avps, diameterApplication['failureResultCode'], experimental=diameterApplication['applicationId'] != 0) |
There was a problem hiding this comment.
The Experimental-Result should be chosen by the code namespace, not by the application id.
experimental=diameterApplication['applicationId'] != 0 means every non-zero-application entry whose failureResultCode is a base RFC 6733 code answers with Experimental-Result{Vendor-Id 10415, Experimental-Result-Code <code>} and no Result-Code.
This effects for example PUR/PUA and NOR/NOA (S6a) and Re CCR/CCA, all using 5012 - DIAMETER_UNABLE_TO_COMPLY is a base-namespace code, so an MME/PGW decoding it against vendor 10415 sees an undefined code. This wants a per-entry flag in diameterResponseList, not a derivation from applicationId.
Can you please add a test for that that reproduces this issue and a fix?
| def get_required_avp_data(self, avps, avp_code, vendor_id=None): | ||
| #Returns the data of the first AVP with avp_code, or raises DiameterMissingAvp so the request is answered with DIAMETER_MISSING_AVP | ||
| data = self.get_avp_data(avps, avp_code) | ||
| if not data: |
There was a problem hiding this comment.
This guard would not catch empty data. Please also add if not data or not data[0] plus a test case for that.
| failed_avp = self.generate_vendor_avp(e.avp_code, e.avp_flags, e.vendor_id, e.avp_data) | ||
| else: | ||
| failed_avp = self.generate_avp(e.avp_code, e.avp_flags, e.avp_data) | ||
| return self.Respond_ResultCode(packet_vars, avps, e.result_code, failed_avp=failed_avp) |
There was a problem hiding this comment.
Both new error returns are inside the per-application try whose except Exception: continue (line 1283) swallows everything. If Respond_ResultCode raises, the loop continues, nothing matches, and control reaches prom_diam_response_count_application_id_successful before return ''.
Therefore, a dropped request counted as a successful response, which the old return '' didn't do. Given the PR's premise, build the fallback outside that try.
|
|
||
| response = self.generate_diameter_packet("01", "60", int(packet_vars['command_code']), int(packet_vars['ApplicationId']), packet_vars['hop-by-hop-identifier'], packet_vars['end-to-end-identifier'], avp) #Generate Diameter packet | ||
| for sub_avp in avps_to_check['sub_avps']: | ||
| if sub_avp['vendor_id']: |
There was a problem hiding this comment.
A vendor ID of 0 would take the else branch here I think. The decoder sets this to an empty string when the vendor bit is clear (see line 697 - 702). So this should be
if sub_avp['vendor_id'] != '': #Decoder sets '' when the V bit is clear, an int (possibly 0) when setA comment would be warranted here as this is not what one expects.
generateDiameterResponse() catches exceptions raised by the Answer_* handlers and returns '', which hssService drops, so a request with a missing or malformed AVP never gets an answer and the peer waits for the transaction to time out. Every diameterResponseList entry already carries a failureResultCode, but it was never read, and Respond_ResultCode() was never called. Wire the two together: the dispatcher's except branch now returns the generic error answer built by Respond_ResultCode() with the command's failureResultCode (as Experimental-Result for 3GPP applications, Result-Code for the base protocol). Respond_ResultCode() had no callers, so its output is fixed as well: - send either Result-Code or Experimental-Result, not both - copy only the Proxiable bit from the request instead of setting the E-bit, which RFC 6733 reserves for protocol errors - rebuild Vendor-Specific-Application-Id from sub_avps (the decoder empties misc_data for grouped AVPs, so it was echoed empty) and echo Auth-Session-State - accept an optional pre-encoded Failed-AVP Refs nickvsnetworking#308
…r malformed MAR AVPs Add DiameterAvpError with the DiameterMissingAvp (5005) and DiameterInvalidAvpValue (5004) subclasses, plus a get_required_avp_data() accessor that raises DiameterMissingAvp instead of IndexError when an AVP is absent. generateDiameterResponse() turns these into an error answer with the matching Result-Code and a Failed-AVP naming the offending AVP (RFC 6733 section 7.5), and keeps the command's failureResultCode as the fallback for any other exception. The Cx MAR handler now raises them for a missing Public-Identity, User-Name or Session-Id and for a User-Name without a domain part, which were the cases reported in nickvsnetworking#308. A well-formed MAR is answered exactly as before. tests/test_diameter_errors.py builds the requests with the library's own encoders and checks each answer, including a valid MAR as a regression guard. It needs only the sqlite test database. Refs nickvsnetworking#308
… handler entry Swap the unguarded Session-Id and User-Name reads at the top of the ULR, AIR, PUR, NOR, UAR, SAR, LIR, RTA, Sh UDR, Sh PUR and LRR handlers from get_avp_data(...)[0] to get_required_avp_data(), so a request without them is answered with DIAMETER_MISSING_AVP and a Failed-AVP rather than the command's generic fallback code. Reads inside try/except blocks that already build their own answer are left alone. Refs nickvsnetworking#308
- Choose Result-Code vs Experimental-Result from a per-entry failureResultCodeExperimental flag in diameterResponseList instead of deriving it from the application id: the 5012 fallback of PUR/NOR/CCR is an RFC 6733 code and must go in Result-Code. - get_required_avp_data() also rejects an AVP that is present with an empty payload; Respond_ResultCode() no longer echoes an empty Session-Id or Auth-Session-State. - Build the fallback answer outside the per-application try, so a failure while building it is counted as a failed response and the request is dropped as before, instead of being counted as successful. Error answers are counted under the failure metric. - Echo Vendor-Specific-Application-Id sub AVPs by checking vendor_id against '' (the decoder's value when the V bit is clear), so a vendor id of 0 keeps its V bit. - Merge the CHANGELOG entry into the existing Unreleased section.
|
Thanks for the careful review, all four are addressed in the new commit, and the branch is rebased on master over #337:
|
d532e36 to
2d9707b
Compare
|
Thanks, I will take a look sometime next week as it is quite late where I am now and I don't have time before then. Given the super-human turnaround time, I suspect that this is not your work alone but you are using an LLM to facilitate those changes, am I right? No human would have been able to work that fast. To me, that's fine as my personal view is that this is a tool like anything else, yet you as a human are still responsible for the application of the tool. Note: This is my personal view as the project does not have an official stance on AI usage yet. Until that changes, I'd like to add a git trailer for transparency like: For example: If the commits of your earlier PR were also LLM-based, it's fine that they have no trailer now. I would't rewrite master for that. That's on me for not asking ;) |
| ### Fixed | ||
|
|
||
| - Fix `RuntimeError: dictionary changed size during iteration` in the diameter service when peers connect or disconnect while `activePeers` is being iterated ([#310](https://github.com/nickvsnetworking/pyhss/issues/310)). | ||
| - Answer Diameter requests that carry a missing or malformed AVP with an error Result-Code |
There was a problem hiding this comment.
Can you add a link to the issue here?
2 participants
Fixes #308
Before. When an
Answer_*handler raises on a request that is well-formed on the wire but has a missing or malformed AVP,generateDiameterResponsecatches the exception, logs it and returns'';hssServiceskips empty responses, so no answer is sent and the peer transaction times out. Every entry ofdiameterResponseListalready carries afailureResultCode(5012 for base commands, 4100/5001 for the 3GPP applications) that nothing reads, andRespond_ResultCode()(the "generic error handler with Result Code as input") has no callers and is not spec-valid as written: it emits bothResult-CodeandExperimental-Result, sets the E-bit for 4xxx/5xxx codes, and its Vendor-Specific-Application-Id echo walksmisc_data, which the decoder leaves empty for grouped AVPs.After, in three commits:
Respond_ResultCode()becomes a valid generic error answer (Session-Id echoed when present, Origin-Host/Realm, echoed Vendor-Specific-Application-Id and Auth-Session-State, eitherResult-Code268 orExperimental-Result297 {Vendor-Id 10415, Experimental-Result-Code 298}, optionalFailed-AVP279, P-bit copied from the request, no E-bit), and the dispatcher'sexceptbranch returns it with the command'sfailureResultCodeinstead of''. Any handler exception now produces an answer.DiameterMissingAvp/DiameterInvalidAvpValueexceptions and aget_required_avp_data()accessor, raised from the MAR handler for the cases in the issue (no Session-Id, no Public-Identity, User-Name without@, non-UTF-8 User-Name), so those answerDIAMETER_MISSING_AVP5005 /DIAMETER_INVALID_AVP_VALUE5004 inResult-Codewith aFailed-AVP(RFC 6733 §7.1.5, §7.5).get_avp_data(avps, 263)[0]/ User-Name reads swapped one-for-one to the accessor, so a missing Session-Id on any of those commands answers 5005 rather than raisingIndexError. Reads already inside a handler's owntry/exceptare left as they are.Valid requests are unchanged;
test_valid_mar_is_still_answered_normallyguards that.Driving the real dispatcher with the library's own encoders, on master every malformed MAR variant returned
''; now:Tests:
tests/test_diameter_errors.py(6 cases, sqlite only, no Redis) plus the existing suite;ruff check/ruff format --checkclean; the new file is added to the ruff include list and carries the SPDX header.CHANGELOG.mdgets an[Unreleased]entry perdocs/release.md.