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 2015/07/08 18:08:07 UTC
Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname to
override connection's hostname.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/
-----------------------------------------------------------
Review request for qpid and Andrew Stitcher.
Bugs: proton-939
https://issues.apache.org/jira/browse/proton-939
Repository: qpid-proton-git
Description
-------
Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
Diffs
-----
proton-c/include/proton/ssl.h b250e6a
proton-c/src/transport/transport.c e5e8276
tests/python/proton_tests/ssl.py f3c7f1f
Diff: https://reviews.apache.org/r/36315/diff/
Testing
-------
new unit tests added
Thanks,
Kenneth Giusti
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90968
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Stitcher
On July 8, 2015, 7:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 7:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90977
-----------------------------------------------------------
proton-c/src/transport/transport.c (line 657)
<https://reviews.apache.org/r/36315/#comment144135>
Just checked the files - no tabs there.
I think it's just reviewboard's way of saying 'these lines have simply been indented'
- Kenneth Giusti
On July 8, 2015, 7:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 7:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90967
-----------------------------------------------------------
proton-c/src/transport/transport.c (line 657)
<https://reviews.apache.org/r/36315/#comment144127>
Nit:
Tabs here...
- Andrew Stitcher
On July 8, 2015, 7:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 7:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/
-----------------------------------------------------------
(Updated July 8, 2015, 7:33 p.m.)
Review request for qpid and Andrew Stitcher.
Changes
-------
Avoid setting the connection hostname in any of the security layers if it is not ext.
Bugs: proton-939
https://issues.apache.org/jira/browse/proton-939
Repository: qpid-proton-git
Description
-------
Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
Diffs (updated)
-----
proton-c/include/proton/ssl.h b250e6a
proton-c/src/transport/transport.c e5e8276
tests/python/proton_tests/ssl.py f3c7f1f
Diff: https://reviews.apache.org/r/36315/diff/
Testing
-------
new unit tests added
Thanks,
Kenneth Giusti
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90947
-----------------------------------------------------------
proton-c/src/transport/transport.c (line 661)
<https://reviews.apache.org/r/36315/#comment144108>
hmm, you added a string size check, surely if it's needed here (I'm not sure it is) then it is also needed above in the SASL section. And in that case it might be better to bracket boith in the test.
However I think that setting a null string to another null string isn't an issue (or at least it wasn't in my testing)
- Andrew Stitcher
On July 8, 2015, 4:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 4:08 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Kenneth Giusti <kg...@apache.org>.
> On July 8, 2015, 5:17 p.m., Andrew Stitcher wrote:
> > I think this is probably the correct semantic for the SSL hostname, if you have multiple ways of setting it. However my original intention was to deprecate the separate SSL way to set the peer hostname (I should probably add a deprecated note to the doctext).
> >
> > Is there actually a case where you would ever want the SASL peer hostname ever to be different from the SSL hostname? I can't think of any reason myself.
>
> Andrew Stitcher wrote:
> An alternate implementation (perhaps better) would e to ensure that the hostnames set by different APIs are the same or return an error. This might be more in line with the intent to deprecate the SSL API.
I was thinking of an app that needs to match against a particular Subject Alternate Name, instead of the CN/DNS name itself. Multiple server certs for different DNS names, all sharing a single SAN alias.
> On July 8, 2015, 5:17 p.m., Andrew Stitcher wrote:
> > proton-c/src/transport/transport.c, line 665
> > <https://reviews.apache.org/r/36315/diff/1/?file=1002299#file1002299line665>
> >
> > Nit:
> > I'm guessing there are tab characters here
woop - good eye, I'll fix that.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90912
-----------------------------------------------------------
On July 8, 2015, 4:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 4:08 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
> On July 8, 2015, 5:17 p.m., Andrew Stitcher wrote:
> > I think this is probably the correct semantic for the SSL hostname, if you have multiple ways of setting it. However my original intention was to deprecate the separate SSL way to set the peer hostname (I should probably add a deprecated note to the doctext).
> >
> > Is there actually a case where you would ever want the SASL peer hostname ever to be different from the SSL hostname? I can't think of any reason myself.
>
> Andrew Stitcher wrote:
> An alternate implementation (perhaps better) would e to ensure that the hostnames set by different APIs are the same or return an error. This might be more in line with the intent to deprecate the SSL API.
>
> Kenneth Giusti wrote:
> I was thinking of an app that needs to match against a particular Subject Alternate Name, instead of the CN/DNS name itself. Multiple server certs for different DNS names, all sharing a single SAN alias.
Ok, if there is a good reason then shouldn't be deprecated - However perhaps there shold be a note to say the primary way to do this is the pn_connection_hostname() API?
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90912
-----------------------------------------------------------
On July 8, 2015, 4:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 4:08 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
> On July 8, 2015, 5:17 p.m., Andrew Stitcher wrote:
> > I think this is probably the correct semantic for the SSL hostname, if you have multiple ways of setting it. However my original intention was to deprecate the separate SSL way to set the peer hostname (I should probably add a deprecated note to the doctext).
> >
> > Is there actually a case where you would ever want the SASL peer hostname ever to be different from the SSL hostname? I can't think of any reason myself.
An alternate implementation (perhaps better) would e to ensure that the hostnames set by different APIs are the same or return an error. This might be more in line with the intent to deprecate the SSL API.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90912
-----------------------------------------------------------
On July 8, 2015, 4:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 4:08 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 36315: PROTON-939: allow pn_ssl_set_peer_hostname
to override connection's hostname.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36315/#review90912
-----------------------------------------------------------
Ship it!
I think this is probably the correct semantic for the SSL hostname, if you have multiple ways of setting it. However my original intention was to deprecate the separate SSL way to set the peer hostname (I should probably add a deprecated note to the doctext).
Is there actually a case where you would ever want the SASL peer hostname ever to be different from the SSL hostname? I can't think of any reason myself.
proton-c/src/transport/transport.c (line 665)
<https://reviews.apache.org/r/36315/#comment144082>
Nit:
I'm guessing there are tab characters here
- Andrew Stitcher
On July 8, 2015, 4:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36315/
> -----------------------------------------------------------
>
> (Updated July 8, 2015, 4:08 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Bugs: proton-939
> https://issues.apache.org/jira/browse/proton-939
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Prevents the connection's hostname setting overriding one set via pn_ssl_set_peer_hostname
>
>
> Diffs
> -----
>
> proton-c/include/proton/ssl.h b250e6a
> proton-c/src/transport/transport.c e5e8276
> tests/python/proton_tests/ssl.py f3c7f1f
>
> Diff: https://reviews.apache.org/r/36315/diff/
>
>
> Testing
> -------
>
> new unit tests added
>
>
> Thanks,
>
> Kenneth Giusti
>
>