ssl: When no SNI is present, use the _server_ IP as 'servername' #1032

Merged
merged 1 commit into from Dec 31, 2013

Conversation

Projects
None yet
3 participants
@smunaut
Contributor

smunaut commented Nov 20, 2013

Previously this used the client IP ... which is really annoying and
I don't see what use case this would fit.

Matching on the server IP is already possible using the v_target_ip
match module. However matching on the IP of servername allows better
control because it will be the IP only if the client doesn't do SNI
while v_target_ip will always match even if the client sent an explicit
name. So this way you can use IP matching only has a fallback for non-SNI.

Signed-off-by: Sylvain Munaut tnt@246tNt.com

ssl: When no SNI is present, use the _server_ IP as 'servername'
Previously this used the client IP ... which is really annoying and
I don't see what use case this would fit.

Matching on the server IP is already possible using the v_target_ip
match module. However matching on the IP of servername allows better
control because it will be the IP only if the client doesn't do SNI
while v_target_ip will always match even if the client sent an explicit
name. So this way you can use IP matching only has a fallback for non-SNI.

Signed-off-by: Sylvain Munaut <tnt@246tNt.com>
@skinkie

This comment has been minimized.

Show comment Hide comment
@skinkie

skinkie Nov 21, 2013

Member

I need to better understand the implications of this code in practise. It looks good, but I don't understand the consequences.

Member

skinkie commented Nov 21, 2013

I need to better understand the implications of this code in practise. It looks good, but I don't understand the consequences.

@smunaut

This comment has been minimized.

Show comment Hide comment
@smunaut

smunaut Nov 21, 2013

Contributor

The current code does :

cherokee_socket_ntop (&conn->socket, tmp.buf, tmp.size);

But never sets tmp.len properly, so the end result is that this 'server name' will never match anything when trying to match it in vrule_rehost.c (because the pcre match call is limited to the .len of the cherokee_buffer).

So all in all, nobody ever could have successfully used the previous code.

Contributor

smunaut commented Nov 21, 2013

The current code does :

cherokee_socket_ntop (&conn->socket, tmp.buf, tmp.size);

But never sets tmp.len properly, so the end result is that this 'server name' will never match anything when trying to match it in vrule_rehost.c (because the pcre match call is limited to the .len of the cherokee_buffer).

So all in all, nobody ever could have successfully used the previous code.

@alobbs

This comment has been minimized.

Show comment Hide comment
@alobbs

alobbs Nov 21, 2013

Member

First of all, good catch.
Now, wouldn't something like "tmp.len = strlen(tmp.buf);" fix it up in a easier way?

Member

alobbs commented Nov 21, 2013

First of all, good catch.
Now, wouldn't something like "tmp.len = strlen(tmp.buf);" fix it up in a easier way?

@smunaut

This comment has been minimized.

Show comment Hide comment
@smunaut

smunaut Nov 21, 2013

Contributor

Well ... this fills it with the client IP address ... I want the server IP address in there.

Contributor

smunaut commented Nov 21, 2013

Well ... this fills it with the client IP address ... I want the server IP address in there.

@alobbs

This comment has been minimized.

Show comment Hide comment
@alobbs

alobbs Nov 21, 2013

Member

Oops! I oversaw that

Member

alobbs commented Nov 21, 2013

Oops! I oversaw that

@ghost ghost assigned skinkie Dec 30, 2013

skinkie added a commit that referenced this pull request Dec 31, 2013

Merge pull request #1032 from smunaut/fix_no_sni_servername
ssl: When no SNI is present, use the _server_ IP as 'servername'

Thanks @smunaut for your awesome contribution!

@skinkie skinkie merged commit 4e0a493 into cherokee:master Dec 31, 2013

@smunaut smunaut deleted the smunaut:fix_no_sni_servername branch Jan 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment