-
Notifications
You must be signed in to change notification settings - Fork 972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TLS in asterisk currently does not accept wildcarded DN's when connecting #276
base: master
Are you sure you want to change the base?
Conversation
… a TLS/SSL host. This tweak makes that work. Fixed asterisk#269
REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process. If you don't want it cherry-picked, please add a comment with If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add The currently active branches are now 18, 20, 21 and master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chan_sip no longer accepts changes here. Is this applicable to anything else that would merit its inclusion? If so, the commit message should be framed as such.
This PR currently has 2 commits that need to be squashed together.
There are some coding guideline violations that need to be addressed as well.
The existing commit message does not comply with the commit message guidelines.
@@ -85,21 +85,21 @@ static void session_instance_destructor(void *obj) | |||
static int check_tcptls_cert_name(ASN1_STRING *cert_str, const char *hostname, const char *desc) | |||
{ | |||
unsigned char *str; | |||
int ret; | |||
int ret = -1,len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace needed between ,
return -1; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
if (ret < 0 || !str) { | ||
len = ASN1_STRING_to_UTF8(&str, cert_str); | ||
if (len < 0 || !str) { | ||
ast_log(LOG_WARNING, "Mailformed string in certificate\n", desc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not print out the string?
} else if (!strcasecmp(hostname, (char *) str)) { | ||
ret = 0; | ||
} else { | ||
ret = -1; | ||
} else if (len > 2 && str[0] == '*' && str[1] == '.' && len - 2 <= strlen(hostname) && strcasecmp(hostname+strlen(hostname)-len+2, str+2) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space needed around +. Please see the coding guidelines. Same for the - and + operators.
Also use !strcasecmp, not == 0
} else { | ||
ret = -1; | ||
} else if (len > 2 && str[0] == '*' && str[1] == '.' && len - 2 <= strlen(hostname) && strcasecmp(hostname+strlen(hostname)-len+2, str+2) == 0) { | ||
ast_log(LOG_WARNING,"Warning: allowing match on wildcard (%s =~ %s)\n", hostname, str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space needed after , before "
No need to print "Warning: ", remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will allow *.example.com
to match any.number.of.subdomain.labels.example.com
which is incorrect.
b15287c
to
1862a36
Compare
TLS currently does not accept wildcarded DN's when connecting to a TLS/SSL host. This tweak makes that work. Fixes/addresses issue #269