Re: Proposed SASL changes (API and functional)

[Robbie Gemmell <ro...@gmail.com>]

On 27 February 2015 at 11:56, Robbie Gemmell <ro...@gmail.com> wrote:
> On 26 February 2015 at 17:52, Andrew Stitcher <as...@redhat.com> wrote:
>> On Thu, 2015-02-26 at 12:28 +0000, 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=51812103&focusedCommentId=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=51812103&focusedCommentId=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=51812103&focusedCommentId=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 proposed "force ANONYMOUS" option - it still
leaves open the question of interaction with the exclusion list
though.
{quote}

I dont think we need both either, just the positive/inlusion list, but
I also dont see it needing to be that confusing if we did, with the
simplest case just being to make specifying them mutually exclusive.

I think if you generalise the forcing then it simply becomes a
'positive/inclusion' list that can take 1 or more entries, which
removes any need for wondering about interaction with an exclusion
lists or specific methods for forcing ANONYMOUS etc (which itself
already has interesting interaction issues with 'requires auth' and
the exclusion list, since someone might put ANONYMOUS in there even if
that isnt whats intended).


{quote}
6. I don't (didn't?) think the change of default is that significant
as all the server code in the proton tree previously allowed (or even
required) anonymous , in that code even though SASL was required the
ANONYMOUS mech was specified. All that the new default has done is to
remove a level of unnecessary processing. Of course the situation in
the Java tree might be different.
{quote}

We may be discussing things from different viewpoints. I dont care
about the server code in the proton tree, since we can change that to
do whatever we like based on what the engine does. That code is not
the only use of the engine.


{quote}
In any case, I find it hard to believe that someone writing a
production secured server wouldn't think about security enough to know
whether they wanted authenticated and/or encrypted connections and
specify them as such.
{quote}

That you believe it to be so certain to be specified is more reason in
my mind to default to the previous behaviour.


{quote}
I will emphasize that the old "skip SASL" option was not in itself
related to authentication or not as the ANONYMOUS SASL mech was still
allowed. The new code unifies the default across the layers and allows
you to skip an unnecessary layer - This change should definitely be
release noted.
{quote}

As above, it might have been allowed in *our* code, but that is not
the only code to consider.


{quote}
Your point about just removing pn_sasl_allow_skip() is noted (and I
agree), given my previous reasoning for pn_sasl_client() and
pn_sasl_server() it should go (and it will force anyone using it to
understand the change in default too)
{quote}

Unfortunately the only people likely to be using it will be the people
who wouldnt really need to understand the change in default.