You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2012/12/10 23:06:35 UTC
Review Request: PROTON-161: check peer certificate for expected peer hostname
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/
-----------------------------------------------------------
Review request for qpid and Rafael Schloming.
Description
-------
Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
This addresses bug proton-161.
https://issues.apache.org/jira/browse/proton-161
Diffs
-----
/proton/trunk/proton-c/bindings/python/proton.py 1419512
/proton/trunk/proton-c/include/proton/ssl.h 1419512
/proton/trunk/proton-c/src/ssl/openssl.c 1419512
/proton/trunk/tests/proton_tests/ssl.py 1419512
/proton/trunk/tests/proton_tests/ssl_db/README.txt 1419512
/proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1419512
/proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
/proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1419512
/proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1419512
/proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1419512
/proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1419512
Diff: https://reviews.apache.org/r/8449/diff/
Testing
-------
Updated ssl unit tests.
Thanks,
Kenneth Giusti
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Kenneth Giusti <kg...@apache.org>.
> On Dec. 11, 2012, 2 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/include/proton/ssl.h, line 261
> > <https://reviews.apache.org/r/8449/diff/1/?file=236662#file236662line261>
> >
> > Maybe I'm missing something here, but isn't this backwards? Isn't the wildcard pattern actually contained in the wildcard certificate? Or is this something distinct from wildcard certificates, in which case, how does this interact with them?
I think I misunderstood this comment against the original bug:
https://issues.apache.org/jira/browse/PROTON-161?focusedCommentId=13504688&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13504688
I thought this was a request for client-side wildcards to avoid the need for a callback to the client application to perform any special-case matching. There's a request for a "pluggable" hostname check mechanism, which wouldn't be possible for any of the swig'ed languages.
In any case, wildcards _in the certificate_ must be supported and I missed that in this patch. I'll work on that.
> On Dec. 11, 2012, 2 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/include/proton/ssl.h, line 262
> > <https://reviews.apache.org/r/8449/diff/1/?file=236662#file236662line262>
> >
> > The API is a bit odd here. It seems like there is overlap, e.g. sometimes the match is against the hostname, and sometimes the match is against the hostname_match thingy.
> >
> > I'm also confused about the scenario in which we would ever need to supply a pattern against which to do a non-exact match. It seems like wildcard certificates would eliminate that need. I can see how maybe if you have a virtual server farm and you don't know which certificate you're going to get you might want to supply a pattern as the client, but I haven't ever heard of that as a standard pattern. (I'm not saying it isn't, I could just be ignorant here.) If it's not a standard pattern then maybe we should wait until a use case comes up to offer it?
I think the API may be trying to do too much here. There are three separate aspects it's trying to cover:
1) Sending SNI to the server (see: http://en.wikipedia.org/wiki/Server_Name_Indication)
2) validating the name contained in the certificate sent by the peer, and
3) support for "custom" client side name checking.
#1 is pretty straightforward - we need to send the server the name we're expecting to authenticate so the server can select the correct certificate. This is provided by the set_peer_hostname call.
Configuration for #2 should be simplified by using the name given by #1 by default. And I think this would cover most use cases.
The API becomes more complex if we want to:
o) support #3, which includes overriding the hostname set by #1
o) allow SNI, but disable host name check (#2)
Both of these features led to the addition of the "set_peer_hostname_pattern" api.
As far as wildcard certificates are concerned: I think this is a separate issue from the client configuration. Yes, we need to support them for #2 regardless of how/if we allow #3 [and the current patch does *not* - I need to fix that].
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/#review14290
-----------------------------------------------------------
On Dec. 10, 2012, 10:06 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8449/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2012, 10:06 p.m.)
>
>
> Review request for qpid and Rafael Schloming.
>
>
> Description
> -------
>
> Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
>
>
> This addresses bug proton-161.
> https://issues.apache.org/jira/browse/proton-161
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/python/proton.py 1419512
> /proton/trunk/proton-c/include/proton/ssl.h 1419512
> /proton/trunk/proton-c/src/ssl/openssl.c 1419512
> /proton/trunk/tests/proton_tests/ssl.py 1419512
> /proton/trunk/tests/proton_tests/ssl_db/README.txt 1419512
> /proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
> /proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1419512
>
> Diff: https://reviews.apache.org/r/8449/diff/
>
>
> Testing
> -------
>
> Updated ssl unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/#review14290
-----------------------------------------------------------
/proton/trunk/proton-c/bindings/python/proton.py
<https://reviews.apache.org/r/8449/#comment30439>
Is there a reason this can't be done as a property like the rest of the API?
/proton/trunk/proton-c/include/proton/ssl.h
<https://reviews.apache.org/r/8449/#comment30440>
Seems odd to call it a flag. Also, it should end in _t
/proton/trunk/proton-c/include/proton/ssl.h
<https://reviews.apache.org/r/8449/#comment30442>
Maybe I'm missing something here, but isn't this backwards? Isn't the wildcard pattern actually contained in the wildcard certificate? Or is this something distinct from wildcard certificates, in which case, how does this interact with them?
/proton/trunk/proton-c/include/proton/ssl.h
<https://reviews.apache.org/r/8449/#comment30443>
The API is a bit odd here. It seems like there is overlap, e.g. sometimes the match is against the hostname, and sometimes the match is against the hostname_match thingy.
I'm also confused about the scenario in which we would ever need to supply a pattern against which to do a non-exact match. It seems like wildcard certificates would eliminate that need. I can see how maybe if you have a virtual server farm and you don't know which certificate you're going to get you might want to supply a pattern as the client, but I haven't ever heard of that as a standard pattern. (I'm not saying it isn't, I could just be ignorant here.) If it's not a standard pattern then maybe we should wait until a use case comes up to offer it?
/proton/trunk/proton-c/src/ssl/openssl.c
<https://reviews.apache.org/r/8449/#comment30441>
I suggest we define our own function interface to do this kind of matching. We could implement it trivially on unix with fnmatch, and I beleive windows has similar API, but we could also just write our own impl if it's not available on windows.
- Rafael Schloming
On Dec. 10, 2012, 10:06 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8449/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2012, 10:06 p.m.)
>
>
> Review request for qpid and Rafael Schloming.
>
>
> Description
> -------
>
> Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
>
>
> This addresses bug proton-161.
> https://issues.apache.org/jira/browse/proton-161
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/python/proton.py 1419512
> /proton/trunk/proton-c/include/proton/ssl.h 1419512
> /proton/trunk/proton-c/src/ssl/openssl.c 1419512
> /proton/trunk/tests/proton_tests/ssl.py 1419512
> /proton/trunk/tests/proton_tests/ssl_db/README.txt 1419512
> /proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
> /proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1419512
> /proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1419512
>
> Diff: https://reviews.apache.org/r/8449/diff/
>
>
> Testing
> -------
>
> Updated ssl unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/
-----------------------------------------------------------
(Updated Dec. 14, 2012, 7:15 p.m.)
Review request for qpid and Rafael Schloming.
Changes
-------
Final - just added the SSL unit tests.
Description
-------
Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
This addresses bug proton-161.
https://issues.apache.org/jira/browse/proton-161
Diffs (updated)
-----
/proton/trunk/proton-c/bindings/python/proton.py 1421779
/proton/trunk/proton-c/bindings/python/python.i 1421779
/proton/trunk/proton-c/include/proton/ssl.h 1421779
/proton/trunk/proton-c/src/ssl/openssl.c 1421779
/proton/trunk/proton-c/src/ssl/ssl_stub.c 1421779
/proton/trunk/proton-j/proton/src/main/scripts/proton.py 1421779
/proton/trunk/tests/proton_tests/ssl.py 1421779
/proton/trunk/tests/proton_tests/ssl_db/README.txt 1421779
/proton/trunk/tests/proton_tests/ssl_db/bad-server-certificate.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/bad-server-private-key.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1421779
/proton/trunk/tests/proton_tests/ssl_db/server-wc-certificate.pem PRE-CREATION
/proton/trunk/tests/proton_tests/ssl_db/server-wc-private-key.pem PRE-CREATION
Diff: https://reviews.apache.org/r/8449/diff/
Testing
-------
Updated ssl unit tests.
Thanks,
Kenneth Giusti
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/#review14503
-----------------------------------------------------------
Ship it!
One final comment on the naming of the enumeration. It strikes me that VERIFY_PEER_NAME sounds like it does less than VERIFY_PEER, although actually it's doing more. I also think the default should be to do the host check, so maybe we should have VERIFY_PEER do the full check and then introduce some other name to do the partial check. Happy to ship it now though and tweak the names later.
- Rafael Schloming
On Dec. 13, 2012, 10:51 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8449/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 10:51 p.m.)
>
>
> Review request for qpid and Rafael Schloming.
>
>
> Description
> -------
>
> Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
>
>
> This addresses bug proton-161.
> https://issues.apache.org/jira/browse/proton-161
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/python/proton.py 1420994
> /proton/trunk/proton-c/bindings/python/python.i 1420994
> /proton/trunk/proton-c/include/proton/ssl.h 1420994
> /proton/trunk/proton-c/src/ssl/openssl.c 1420994
> /proton/trunk/proton-c/src/ssl/ssl_stub.c 1420994
> /proton/trunk/tests/proton_tests/ssl.py 1420994
> /proton/trunk/tests/proton_tests/ssl_db/README.txt 1420994
> /proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1420994
> /proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
> /proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1420994
> /proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1420994
> /proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1420994
> /proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1420994
>
> Diff: https://reviews.apache.org/r/8449/diff/
>
>
> Testing
> -------
>
> Updated ssl unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/
-----------------------------------------------------------
(Updated Dec. 13, 2012, 10:51 p.m.)
Review request for qpid and Rafael Schloming.
Changes
-------
Third time's a charm:
- made peer_hostname a property per Rafi's suggestion
- added a new verify mode instead of the check_cert flag: PN_SSL_VERIFY_PEER_NAME - like PN_SSL_VERIFY_PEER, but with the name check
done?
Description
-------
Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
This addresses bug proton-161.
https://issues.apache.org/jira/browse/proton-161
Diffs (updated)
-----
/proton/trunk/proton-c/bindings/python/proton.py 1420994
/proton/trunk/proton-c/bindings/python/python.i 1420994
/proton/trunk/proton-c/include/proton/ssl.h 1420994
/proton/trunk/proton-c/src/ssl/openssl.c 1420994
/proton/trunk/proton-c/src/ssl/ssl_stub.c 1420994
/proton/trunk/tests/proton_tests/ssl.py 1420994
/proton/trunk/tests/proton_tests/ssl_db/README.txt 1420994
/proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1420994
/proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
/proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1420994
/proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1420994
/proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1420994
/proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1420994
Diff: https://reviews.apache.org/r/8449/diff/
Testing
-------
Updated ssl unit tests.
Thanks,
Kenneth Giusti
Re: Review Request: PROTON-161: check peer certificate for expected peer
hostname
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8449/
-----------------------------------------------------------
(Updated Dec. 12, 2012, 10:23 p.m.)
Review request for qpid and Rafael Schloming.
Changes
-------
Take #2.
Added support for checking SubjectAltName DNS entries, as described in RFC2818.
Added support for matching certificates that contain wildcarded DNS names.
Removed the whole "set_peer_hostname_match" pattern stuff.
Added a bool flag to set_peer_hostname, if True, check hostname against certficate, else don't (just do SNI)
.... Profit???
Description
-------
Adds an API call to set the hostname which is used for Server Name Indication as well as Common Name matching. Right now only support exact text match - no wildcarding.
This addresses bug proton-161.
https://issues.apache.org/jira/browse/proton-161
Diffs (updated)
-----
/proton/trunk/proton-c/bindings/python/proton.py 1420411
/proton/trunk/proton-c/include/proton/ssl.h 1420411
/proton/trunk/proton-c/src/ssl/openssl.c 1420411
/proton/trunk/tests/proton_tests/ssl.py 1420411
/proton/trunk/tests/proton_tests/ssl_db/README.txt 1420411
/proton/trunk/tests/proton_tests/ssl_db/ca-certificate.pem 1420411
/proton/trunk/tests/proton_tests/ssl_db/ca-private-key.pem PRE-CREATION
/proton/trunk/tests/proton_tests/ssl_db/client-certificate.pem 1420411
/proton/trunk/tests/proton_tests/ssl_db/client-private-key.pem 1420411
/proton/trunk/tests/proton_tests/ssl_db/server-certificate.pem 1420411
/proton/trunk/tests/proton_tests/ssl_db/server-private-key.pem 1420411
Diff: https://reviews.apache.org/r/8449/diff/
Testing
-------
Updated ssl unit tests.
Thanks,
Kenneth Giusti