<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [x] Related to issue https://kamailio.org/mailman3/hyperkitty/list/sr-users@lists.kamailio.org/th... #### Description <!-- Describe your changes in detail --> This PR fixes the bug when failing to parse a URN scheme with a '@' character in the host name.
- Format of file - Add some comments on magic numbers You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4336
-- Commit Summary --
* core: Fix urn parsing failing if @ is present * core: add some comment on the hardcoded numbers
-- File Changes --
M src/core/parser/parse_uri.c (24)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4336.patch https://github.com/kamailio/kamailio/pull/4336.diff
sergey-safarov left a comment (kamailio/kamailio#4336)
Do I understand properly that here should be "if equal urn schema go to error" ``` case '@': \ if(scheme != URN_SCH) { \ goto error_bad_char; \ } ```
xkaraman left a comment (kamailio/kamailio#4336)
Do I understand properly that here should be "if equal urn schema go to error"
if(scheme != URN_SCH) { \ goto error_bad_char; \ }
No.
If it's NOT a URN scheme, then '@' is not a valid charachter in host part.
If it is a URN scheme, '@' is just another character in the host name.
miconda left a comment (kamailio/kamailio#4336)
If it's NOT a URN scheme, then '@' is an invalid charachter in host part and should error out.
If it is a URN scheme, '@' is just another character in the host name and it's valid.
I haven't looked at the code, but from your comments above I guess that the URN value is stored in the host field of the URI structure? Would it be a case where the part after the `@` should be actually used as a host/domain (ie., the urn with the role of the username, and the value after `@` as domain/ip address)?
henningw left a comment (kamailio/kamailio#4336)
If it's NOT a URN scheme, then '@' is an invalid charachter in host part and should error out. If it is a URN scheme, '@' is just another character in the host name and it's valid.
I haven't looked at the code, but from your comments above I guess that the URN value is stored in the host field of the URI structure? Would it be a case where the part after the `@` should be actually used as a host/domain (ie., the urn with the role of the username, and the value after `@` as domain/ip address)?
I don't work daily with URNs, but I think according to their definition there is no concept of a host name in it. Its basically a character for the purposes of the URN. There are other standards which are using URNs (e.g. Canonical Text Services) which uses it or adressing of sub-elements inside a specific content. The parser is taking some shortcuts, it would be probably better if it would be separate from the URI/URL part. Its also probably does not fully support the mentioned RFC.
sergey-safarov left a comment (kamailio/kamailio#4336)
As I know "urn" cannot contain `@` char. Here is [RFC link](https://www.rfc-editor.org/rfc/rfc5031.html#page-5) ``` service-URN = "URN:service:" service service = top-level *("." sub-service) top-level = let-dig [ *25let-dig-hyp let-dig ] sub-service = let-dig [ *let-dig-hyp let-dig ] let-dig-hyp = let-dig / "-" let-dig = ALPHA / DIGIT ALPHA = %x41-5A / %x61-7A ; A-Z / a-z DIGIT = %x30-39 ; 0-9 ``` As I understanding allowed letters, digits and "-" char only.
xkaraman left a comment (kamailio/kamailio#4336)
Hmm interesting Following https://datatracker.ietf.org/doc/html/rfc8141#section-2 which is a generic URN RFC i imagine, the `@` character is allowed as part of `pchar` defined in [RFC 3986 ](https://datatracker.ietf.org/doc/html/rfc3986).
Now about service URNs seems to not follow this RFC, or i might misread something, or something i don't fully understand.
henningw left a comment (kamailio/kamailio#4336)
As I know "urn" cannot contain `@` char. Here is [RFC link](https://www.rfc-editor.org/rfc/rfc5031.html#page-5)
service-URN = "URN:service:" service service = top-level *("." sub-service) top-level = let-dig [ *25let-dig-hyp let-dig ] sub-service = let-dig [ *let-dig-hyp let-dig ] let-dig-hyp = let-dig / "-" let-dig = ALPHA / DIGIT ALPHA = %x41-5A / %x61-7A ; A-Z / a-z DIGIT = %x30-39 ; 0-9As I understanding allowed letters, digits and "-" char only.
Yes, but this is the standard for the URN service namespace, not the generic URN standard.
sergey-safarov left a comment (kamailio/kamailio#4336)
@xkaraman @henningw thanks for the clarification.
miconda left a comment (kamailio/kamailio#4336)
OK, if `@` is not indicating the start of a fqdn/ip, but it is part of the urn itself, then I am fine to merge.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
sergey-safarov
-
Xenofon Karamanos