Re: Proposed SASL changes (API and functional)
- Original Message - From: Gordon Sim g...@redhat.com To: us...@qpid.apache.org Cc: proton@qpid.apache.org Sent: Monday, March 2, 2015 2:30:28 PM Subject: Re: Proposed SASL changes (API and functional) On 02/24/2015 08:48 PM, Andrew Stitcher wrote: In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. To me it seems a bit late in the cycle for 0.9 for changes of this magnitude and I'd prefer to see it included during 0.10 with any accompanying changes to ssl. +1 to that. While I like where these API changes are headed, I don't think we should hold 0.9 up. I'd prefer cutting 0.9 without these changes. Put the API changes on trunk once 0.9 is out and give us some more time to 'kick the tires'. -K
Re: Proposed SASL changes (API and functional)
On 27 February 2015 at 11:56, Robbie Gemmell robbie.gemm...@gmail.com wrote:
On 26 February 2015 at 17:52, Andrew Stitcher astitc...@redhat.com wrote:
On Thu, 2015-02-26 at 12:28 +, Robbie Gemmell wrote:
...
I'm going to post my comments here and on the wiki, as I dont think
many (except maybe you) will actually see them on the wiki ;)
Thank you for the excellent feedback. I'm going to answer on the wiki -
as it'll save me from cutting and pasting.
[I did try to add the lists as watchers, but that doesn't seem to be
possible in any obvious way]
wiki link:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812468#comment-51812468
Andrew
Replied on the Wiki, but also including below for anyone not clicking
these links ;)
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812571#comment-51812571
3. Do you really mean to have only ANONYMOUS for 0.9 for Proton-J?
That would render it fairly useless if the existing transport/sasl API
is removed since that would prevent people using any existing
implementation they already have (ANONYMOUS, PLAIN, CRAM-MD5 thus far
for the JMS client, with EXTERNAL sitting on the TODO list).
4. I do still think there is a change (by having to handle the SASL
previously, you would always know the authentication result before the
connection ever happened, and you had to call done on the server to
say it was, whereas now you get a transport event in either case), but
I take your point that you could adjust to waiting for the successful
connection open state/event to be reached and both approaches would
then appear to work the same from that point.
5. I'd say the positive list of mechs to use is more straight forward
in many scenarios, and the exclusion list could work better in some
others. Offering both options would let people do whatever they want.
- The exclusion list might be something you used initially to e.g
prevent use of ANONYMOUS, but you have that scenario covered by
pn_transport_require_auth already.
- After that, it is presumably to be used for things like disabling
mechs that are either not considered secure enough (e.g PLAIN, because
you are requiring everying uses SCRAM-SHA-foo today because its
Friday), or are installed but not configured/usable (e.g GSSAPI as you
mentioned). Between those, you would need to know the name of every
mechanism that could ever be present, and then exclude them all in
case they should be installed (possibly later). Or, you might better
get what you want by being able to specify only the mechs you want to
be enabled and have already installed as a result.
- On what to do to do if a positively-requested mechanism isnt
available, as I said we could posibly give the choice of of throwing
to indicate the requirement couldn't be met or potentially making that
optional, because if you positively-specify more than one you probabyl
dont care which is used too much.
6. I saw the deprecation, but I wasnt aware the default had already
been changed for allow skip, was that ever mentioned anywhere, e.g a
JIRA raised to cover a significant change in behaviour? I think its
the wrong decision personally, and code using the engine should have
to opt-in to permitting non-SASL connections (and ANONYMOUS). If the
rest of the current SASL API is going away and the default remains
changed, then it seems allow skip should probably just be removed too,
since its very likely the only people using it will be configuring the
new behaviour and there is a new API for anyone wanting to configure
the historical default behaviour when they realise (this will
presumably be release-noted?) it becomes necessary to secure it with
the new release.
Replied on the wiki once more.
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812858#comment-51812858
..but again for anyone not reading there here is the reply (forgive
the wiki text):
{quote}
5. The exclusion list is not meant to be used to exclude ANONYMOUS in
any way, as ANONYMOUS won't even be offered if authentication is
required.
{quote}
I realise, I was just using that as an example of one of reasons I
might use an exclude list, and highlighted that it wouldnt be required
in that case because of the alternative method.
That said, ANONYMOUS presumably will be offered if the 'force
anonymous' method is also in use?
{quote}
I think having both exclusion and inclusion in the API is the worst
option, because then the coder has to understand how the two lists
interact (and in the past exactly this sort of issue has been a
security risk). As it stands, with the cyrus sasl code you do have
both options, just at different levels, you can include mechs by using
the cyrus sasl configuration files, and exclude mechs using the API.
I think the idea of forcing a specific single mech has some merit - a
generalisation of the
Re: Proposed SASL changes (API and functional)
On 02/24/2015 08:48 PM, Andrew Stitcher wrote: In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. To me it seems a bit late in the cycle for 0.9 for changes of this magnitude and I'd prefer to see it included during 0.10 with any accompanying changes to ssl.
Re: Proposed SASL changes (API and functional)
On 2 March 2015 at 19:30, Gordon Sim g...@redhat.com wrote: On 02/24/2015 08:48 PM, Andrew Stitcher wrote: In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. To me it seems a bit late in the cycle for 0.9 for changes of this magnitude and I'd prefer to see it included during 0.10 with any accompanying changes to ssl. I've been starting to think that way too. Shipping 0.9 now without the major SASL changes, then turning 0.10 around much more quickly with those and anything else finished in a similar timeframe seems like a nicer prospect to me. Its already 2 months behind schedule afterall and there are dependent releases (0.32) already long underway. Robbie
Re: Proposed SASL changes (API and functional)
On 26 February 2015 at 17:52, Andrew Stitcher astitc...@redhat.com wrote: On Thu, 2015-02-26 at 12:28 +, Robbie Gemmell wrote: ... I'm going to post my comments here and on the wiki, as I dont think many (except maybe you) will actually see them on the wiki ;) Thank you for the excellent feedback. I'm going to answer on the wiki - as it'll save me from cutting and pasting. [I did try to add the lists as watchers, but that doesn't seem to be possible in any obvious way] wiki link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812468#comment-51812468 Andrew Replied on the Wiki, but also including below for anyone not clicking these links ;) https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812571#comment-51812571 3. Do you really mean to have only ANONYMOUS for 0.9 for Proton-J? That would render it fairly useless if the existing transport/sasl API is removed since that would prevent people using any existing implementation they already have (ANONYMOUS, PLAIN, CRAM-MD5 thus far for the JMS client, with EXTERNAL sitting on the TODO list). 4. I do still think there is a change (by having to handle the SASL previously, you would always know the authentication result before the connection ever happened, and you had to call done on the server to say it was, whereas now you get a transport event in either case), but I take your point that you could adjust to waiting for the successful connection open state/event to be reached and both approaches would then appear to work the same from that point. 5. I'd say the positive list of mechs to use is more straight forward in many scenarios, and the exclusion list could work better in some others. Offering both options would let people do whatever they want. - The exclusion list might be something you used initially to e.g prevent use of ANONYMOUS, but you have that scenario covered by pn_transport_require_auth already. - After that, it is presumably to be used for things like disabling mechs that are either not considered secure enough (e.g PLAIN, because you are requiring everying uses SCRAM-SHA-foo today because its Friday), or are installed but not configured/usable (e.g GSSAPI as you mentioned). Between those, you would need to know the name of every mechanism that could ever be present, and then exclude them all in case they should be installed (possibly later). Or, you might better get what you want by being able to specify only the mechs you want to be enabled and have already installed as a result. - On what to do to do if a positively-requested mechanism isnt available, as I said we could posibly give the choice of of throwing to indicate the requirement couldn't be met or potentially making that optional, because if you positively-specify more than one you probabyl dont care which is used too much. 6. I saw the deprecation, but I wasnt aware the default had already been changed for allow skip, was that ever mentioned anywhere, e.g a JIRA raised to cover a significant change in behaviour? I think its the wrong decision personally, and code using the engine should have to opt-in to permitting non-SASL connections (and ANONYMOUS). If the rest of the current SASL API is going away and the default remains changed, then it seems allow skip should probably just be removed too, since its very likely the only people using it will be configuring the new behaviour and there is a new API for anyone wanting to configure the historical default behaviour when they realise (this will presumably be release-noted?) it becomes necessary to secure it with the new release.
Re: Proposed SASL changes (API and functional)
This is from Andrew's wiki comment. Sorry to paste it back to the list, but I'm having some difficulty commenting there: 1. Setters with no getters Philosophically I don't agree that you need to make all properties read/write. I see no particular reason to make these properties readable since they will never change once they are set, or in the case of the password should actually not be accessible once set (because the implementation *should* be scrubbing the bytes from memory after use). In my view if the application needs the value again it already has it. In the case of the read-only property authenticated user I definitely think that needs to be read only. Having said that, I don't feel that strongly about getters for the client username and hostname. There's actually an important point here worth noting. With reactive use, I don't think it's true, pragmatically speaking, that the application has the value again when it needs it. When your primary means of operating on a connection is through handling events, the only state you have easy access to is what is provided by those events. Taking your suggestion, if I wanted to do something simple like log a debug statement from an event handler and include the hostname and/or username of the connection in that info, my only recourse would be to malloc some sort of ancillary struct and attach it to the connection and fill it in with the very same hostname that the connection is holding internally, or alternatively access some sort of global state that holds a map from the connection back to that same information. If your point is that this is possible then of course that's true, but it doesn't seem at all reasonable. 1. inconsistency with existing use of remote in API I take your point - I'm happy to remove remote from the API name - would connected be all right? pn_transport_set_hostname() just doesn't seem specific enough to me - it might just as well be telling the transport what *our* hostname is. 2. Redundancy of pn_connection_set_hostname() and pn_transport_set_something_hostname() Yes these are definitely redundant, and I would need to deprecate the connection version and set it from the transport when binding them together - good catch. The transport version must be primary, as (in principle at least, if not in the current implementation) you don't need the connection object until you have authenticated the transport and authentication and encryption may to know need the hostname you are connecting to. I think it would have to be an error to rebind (on reconnect) a connection with a different hostname than the transport hostname. This isn't consistent with how the connection and transport are actually used. With the reactive API, the user creates the connection endpoint first and configures it with all the relevant details that it cares about. The transport isn't created at all until the client actually opens the connection (which could be somewhere completely different from where it configures the connection). It's also important to note that the user doesn't actually create the transport at all. The default handlers do this when the PN_CONNECTION_LOCAL_OPEN event occurs. The users don't even need to be aware that a transport object exists at all if they don't care to customize it. This is a nice property and would be difficult to maintain if you start pushing connection level configuration like hostname into the transport. I think if you flip things around it actually makes more sense. As a server you are going to have a transport prior to having a connection, and in this case you want to access the hostname-that-your-remote-peer desires for vhost purposes, but it makes no sense to actually set it. As a client, a transport is pretty much useless until it is bound to a connection, as you can't safely do much more than send the protocol header, so the natural sequence is to create the connection first and set the hostname you want to connect to, and not worry about the Transport. 1. Having a separate set_user/set_password That would make sense. However from this conversation I'm wondering actually if we should more carefully distinguish the client and server ends. And so have a client only API to set user/password and a server only API to extract the authenticated username. So in conclusion how about: - Changing pn_transport_set_remote_hostname() → pn_transport_set_connect_hostname() (connect/connected/connection?) - Adding pn_transport_get_connect_hostname() (connect/connected/connection?) - Deprecating pn_connection_set/get_hostname() in favour of pn_transport_set/get_connect_hostname() Actually changing the pn_transport_bind() code would be required too. - Removing pn_transport_set_user_password() and pn_transport_get_user() - Replacing them with
Re: Proposed SASL changes (API and functional)
On Wed, Feb 25, 2015 at 7:45 PM, Andrew Stitcher astitc...@redhat.com wrote: If it is ok with them I will copy the comments over there: Alan, Jakub? Sorry, I missed the wiki part. Feel free to copy my comment there if you want.
Re: Proposed SASL changes (API and functional)
On Thu, 2015-02-26 at 12:28 +, Robbie Gemmell wrote: ... I'm going to post my comments here and on the wiki, as I dont think many (except maybe you) will actually see them on the wiki ;) Thank you for the excellent feedback. I'm going to answer on the wiki - as it'll save me from cutting and pasting. [I did try to add the lists as watchers, but that doesn't seem to be possible in any obvious way] wiki link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=51812103focusedCommentId=51812468#comment-51812468 Andrew
Re: Proposed SASL changes (API and functional)
On Wed, 2015-02-25 at 13:45 -0500, Andrew Stitcher wrote: On Tue, 2015-02-24 at 15:48 -0500, Andrew Stitcher wrote: ... If you are at all interested please go and look at the proposal and comment on it there. Thank you very much to Alan and Jakub for commenting on my proposal. The reason I asked people to comment over on the wiki is that it is very hard to find a discussion like this related to a specific proposal after some time has elapsed if it is in email, whereas actually attached to the proposal on the wiki keeps all the relevant comments together. If it is ok with them I will copy the comments over there: Alan, Jakub? Copy away
Re: Proposed SASL changes (API and functional)
On 25 February 2015 at 18:40, Andrew Stitcher astitc...@redhat.com wrote: On Wed, 2015-02-25 at 10:27 +0100, Jakub Scholz wrote: ... But I find this part a bit dangerous: Classically in protocols where SASL was not optional the way to avoid double authentication was to use the EXTERNAL SASL mechanism. With AMQP, SASL is optional, so if SSL is used for client authentication the SASL layer could be entirely omitted and so using EXTERNAL is not necessary. This is really just a statement about how AMQP 1.0 works - if you like - it is an aside praising the good protocol design sense of the standard's authors (you know who you are!). I understand the idea and I would even agree that this is the proper way how to do it in the long term. But I'm not sure whether all brokers support this concept. For example, I'm not sure whether you can configure the Qpid C++ broker in a way to accept AMQP 1.0 connections with SSL Client Authentication without SASL EXTERNAL while at the same time accepting AMQP 0-10 connections only with SASL EXTERNAL. Therefore I would be afraid that allowing SSL Client Authentication only without SASL could cause some serious incompatibilities - I think both should be possible / supported. And both are supported. The qpidd 0-10 support is not going to change. The qpidd 1.0 support is on a different code path so there is little bleed over in functionality. The proton server code can auto detect which protocol layers the client is using, and subject to it being an allowed protocol configuration, authenticate it. Other AMQP 1.0 implementations may not support leaving out the SASL layer and so you can certainly always tell the client to use it (even if it adds no useful functionality as in the ANONYMOUS and EXTERNAL cases). So as far as the current plans for proton go if you require SSL client authentication it will happen whether or not a SASL layer is there. As EXTERNAL and better SSL integration with the transport code is not yet implemented there may be something significant I've missed in this analysis, in which case it's all subject to change! I hope that helps. I'm going to post my comments here and on the wiki, as I dont think many (except maybe you) will actually see them on the wiki ;) Assuming I've read the email thread correctly, you do plan on implementing EXTERNAL so that clients can be authenticated using SSL client auth + EXTERNAL. I think that is a good idea, as even though it can be ommitted not all brokers or clients will want to support doing so. There may also be cases where implementations handle the SSL by themselves and only want to do SASL via proton (albeit by reading/writing bytes currently). Possibly less so on the C side, but almost everyone seems to do that with Proton-J. Mentioning Proton-J, are there any plans there? The completion of the SASL stage only being indicated by an event, combined with removal of the old read/write sasl bytes methods, seems to end any prospect of not using the events if you want SASL (at least, not without moving to intercepting the raw SASL frames before the transport). Does this mark the end of the old non-events API? The method for excluding certain server mechanisms seems a bit odd to me. A method to configure the mechanisms to be used feels like it might be a better fit (either ignoring mechanism that arent installed, or throwing to indicate they arent present, or making that a choice), what do you think? How does pn_transport_require_auth interact with old 'allow skip' functionality that let transports with SASL configured permit non-SASL connections? The two have different defaults, as skipping wasnt allowed previously (hence the old method being added), whereas apparently it now would be by default. That itself feels a little wrong to me, I think people should have to opt-in to that behaviour as they did previously. You mention removing deprecated APIs in the form of pn_sasl_client/pn_sasl_server. I think its worth mentioning these were only to become deprecated in 0.9. Robbie
Re: Proposed SASL changes (API and functional)
On Tue, 2015-02-24 at 15:48 -0500, Andrew Stitcher wrote: As many of you know I've been working on implementing a SASL AMQP protocol layer that does more than PLAIN and ANONYMOUS for proton-c. I'm currently in at a point where the work is reasonably functional (with some gaps) I've put together a fairly comprehensive account of this work on the Apache wiki: https://cwiki.apache.org/confluence/x/B5cWAw If you are at all interested please go and look at the proposal and comment on it there. You can see my actual code changes in my github proton repo: https://github.com/astitcher/qpid-proton/commits/sasl-work [This is my working branch, so not all the changes make a lot of sense, just pay attention to the tip of the branch] In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. This looks very good to me. One ignorant question: Qpid has a min/max Security Strength Factor for encryption rather than a binary enable/disable. Is that relevant here? Cheers, Alan.
Re: Proposed SASL changes (API and functional)
On Wed, 2015-02-25 at 10:27 +0100, Jakub Scholz wrote: ... But I find this part a bit dangerous: Classically in protocols where SASL was not optional the way to avoid double authentication was to use the EXTERNAL SASL mechanism. With AMQP, SASL is optional, so if SSL is used for client authentication the SASL layer could be entirely omitted and so using EXTERNAL is not necessary. This is really just a statement about how AMQP 1.0 works - if you like - it is an aside praising the good protocol design sense of the standard's authors (you know who you are!). I understand the idea and I would even agree that this is the proper way how to do it in the long term. But I'm not sure whether all brokers support this concept. For example, I'm not sure whether you can configure the Qpid C++ broker in a way to accept AMQP 1.0 connections with SSL Client Authentication without SASL EXTERNAL while at the same time accepting AMQP 0-10 connections only with SASL EXTERNAL. Therefore I would be afraid that allowing SSL Client Authentication only without SASL could cause some serious incompatibilities - I think both should be possible / supported. And both are supported. The qpidd 0-10 support is not going to change. The qpidd 1.0 support is on a different code path so there is little bleed over in functionality. The proton server code can auto detect which protocol layers the client is using, and subject to it being an allowed protocol configuration, authenticate it. Other AMQP 1.0 implementations may not support leaving out the SASL layer and so you can certainly always tell the client to use it (even if it adds no useful functionality as in the ANONYMOUS and EXTERNAL cases). So as far as the current plans for proton go if you require SSL client authentication it will happen whether or not a SASL layer is there. As EXTERNAL and better SSL integration with the transport code is not yet implemented there may be something significant I've missed in this analysis, in which case it's all subject to change! I hope that helps. Andrew
Re: Proposed SASL changes (API and functional)
On Tue, 2015-02-24 at 15:48 -0500, Andrew Stitcher wrote: ... If you are at all interested please go and look at the proposal and comment on it there. Thank you very much to Alan and Jakub for commenting on my proposal. The reason I asked people to comment over on the wiki is that it is very hard to find a discussion like this related to a specific proposal after some time has elapsed if it is in email, whereas actually attached to the proposal on the wiki keeps all the relevant comments together. If it is ok with them I will copy the comments over there: Alan, Jakub? Thanks Andrew
Re: Proposed SASL changes (API and functional)
On Wed, 2015-02-25 at 10:46 -0500, Alan Conway wrote: ... One ignorant question: Qpid has a min/max Security Strength Factor for encryption rather than a binary enable/disable. Is that relevant here? (Hardly an ignorant question!) You make a very good point, and this design may indeed be a little simplistic - largely because I've not implemented the encryption side yet! 1. I doubt that max ssf is all that useful in practice. 2. Effectively pn_transport_require_encryption() is the same as setting min ssf 1, but is simpler to understand! An alternative might be pn_transport_require_ssf(int) however that isn't as clear and it's not obvious how to choose the ssf value. Perhaps the '1' should be configurable differently. Some input from those who did the similar work in qpidd might be useful. Just some random wittering. Andrew
Re: Proposed SASL changes (API and functional)
Hi Andrew, I'm definitely not a Proton expert, so please excuse me if I missed something. But I find this part a bit dangerous: Classically in protocols where SASL was not optional the way to avoid double authentication was to use the EXTERNAL SASL mechanism. With AMQP, SASL is optional, so if SSL is used for client authentication the SASL layer could be entirely omitted and so using EXTERNAL is not necessary. I understand the idea and I would even agree that this is the proper way how to do it in the long term. But I'm not sure whether all brokers support this concept. For example, I'm not sure whether you can configure the Qpid C++ broker in a way to accept AMQP 1.0 connections with SSL Client Authentication without SASL EXTERNAL while at the same time accepting AMQP 0-10 connections only with SASL EXTERNAL. Therefore I would be afraid that allowing SSL Client Authentication only without SASL could cause some serious incompatibilities - I think both should be possible / supported. Regards Jakub On Tue, Feb 24, 2015 at 9:48 PM, Andrew Stitcher astitc...@redhat.com wrote: As many of you know I've been working on implementing a SASL AMQP protocol layer that does more than PLAIN and ANONYMOUS for proton-c. I'm currently in at a point where the work is reasonably functional (with some gaps) I've put together a fairly comprehensive account of this work on the Apache wiki: https://cwiki.apache.org/confluence/x/B5cWAw If you are at all interested please go and look at the proposal and comment on it there. You can see my actual code changes in my github proton repo: https://github.com/astitcher/qpid-proton/commits/sasl-work [This is my working branch, so not all the changes make a lot of sense, just pay attention to the tip of the branch] In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. Thanks. Andrew
Proposed SASL changes (API and functional)
As many of you know I've been working on implementing a SASL AMQP protocol layer that does more than PLAIN and ANONYMOUS for proton-c. I'm currently in at a point where the work is reasonably functional (with some gaps) I've put together a fairly comprehensive account of this work on the Apache wiki: https://cwiki.apache.org/confluence/x/B5cWAw If you are at all interested please go and look at the proposal and comment on it there. You can see my actual code changes in my github proton repo: https://github.com/astitcher/qpid-proton/commits/sasl-work [This is my working branch, so not all the changes make a lot of sense, just pay attention to the tip of the branch] In a short while when people have had enough time to absorb the proposal and comment I will post a code review of the actual code changes. As there are substantial API changes I'd like to get this in for 0.9 because we were intending to stabilise the API at this point. Thanks. Andrew
