Add support for subdomain checking in aliases and socket aliases #3458
Conversation
There was a problem hiding this comment.
The overall approach of the patch is good, great idea here! With a bit of changes, we could merge it, please see my comments.
What needs to be taken into consideration is the extension of the domain module (when aliases are provisioned via DB), to also support sub-domains - maybe something like an extra flag column. This will do a 100% coverage of the SIP-domains support in OpenSIPS
| @@ -341,6 +341,7 @@ CR \n | |||
|
|
|||
| ANY "any" | |||
| ANYCAST ("anycast"|"ANYCAST") | |||
| SUBDOMAIN ("subdomain"|"SUBDOMAIN") | |||
There was a problem hiding this comment.
I would go for something more suggestive, like accept_subdomains
| alias_def: any_alias { $$=$1; } | ||
| | any_alias { IFOR(); | ||
| memset(&p_tmp, 0, sizeof(p_tmp)); | ||
| } id_lst_params { IFOR(); |
There was a problem hiding this comment.
I think we can simplify the grammar and get rid of the id_lst_params ; here, in the alias_def definition, we can expand to any_alias or to any_alias + SUBDOMAIN
| static void fill_alias_socket(struct listen_param *param, struct socket_id *s) { | ||
| s->flags |= param->flags; | ||
| } | ||
|
|
There was a problem hiding this comment.
we can also avoid this function if we do the grammar simplification I mentioned before
| @@ -40,7 +39,7 @@ struct alias_function* alias_fcts = NULL; | |||
| * if proto==0, the alias will match all the protocols | |||
| * returns 1 if a new alias was added, 0 if a matching alias was already on | |||
| * the list and -1 on error */ | |||
| int add_alias(char* name, int len, unsigned short port, unsigned short proto) | |||
| int add_alias(char* name, int len, unsigned short port, unsigned short proto, enum si_flags flags) | |||
There was a problem hiding this comment.
I would totally avoid this kind of mixture before data types. The si_flags enum is particular to sockets (socket info, si) and it should not be used as flags in the context of another data structure, like the alias here. I guess adding some simple define-based flags will be better.
On the other hand, I see how easy is to pass the flags from socket to alias, later, in the socket fixing code, but that may addressed with a conversion macro.
Nevertheless, this is more about code consistency, not about the actual functionality.
| len_to_compare = a->alias.len; | ||
|
|
||
| if (strncasecmp(alias_to_compare, name_to_compare, len_to_compare)==0) | ||
| return 1; |
There was a problem hiding this comment.
This check is not correct, it is insufficient - you need to also check that the first char (in the name), after the left trim, is actually a dot. Otherwise, the subdomain definition of mario.com will match the name of supermario.com, while we want only the super.mario.com, right ?
PS: try to reduce the number of extra local variables, if not really needed.
|
Hi Bogdan, thanks for the feedback I'll work on integrating the changes you requested in the coming week |
Summary
Adds support for subdomains in aliases and socket aliases
Details
New feature related to this issue #1466
When defining sockets or aliases we can mark them with subdomain so that when check_self is called it will match when there the host to check is subdomain of the alias, definition of the socket and alias as so
If either are set up this way then when check_self is called for example in the TM topology_hiding then if the request domain is something like
resource.my.subdomain.comordifferent.resource.my.subdomain.comit will match the alias but also a deliberate design decision to allowmy.subdomain.comto match too.Solution
A flag is added, when the
subdomain|SUBDOMAINflag is on the socket or alias this allows it to match addresses that match that alias subdomain while preserving strict matching when the flag is not setCompatibility
The flag is a new flag so existing config where the subdomain flag is not on the alias or socket will behave like it already does without this change, it's backwards compatible
Closing issues
closes #1466