qpid-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rafael Schloming <...@alum.mit.edu>
Subject Re: Proposed SASL changes (API and functional)
Date Thu, 26 Feb 2015 14:45:32 GMT
On Tue, Feb 24, 2015 at 3:48 PM, Andrew Stitcher <astitcher@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.
>

This seems like a sensible direction in general. I'm definitely interested
in seeing some example usages, but it looks like that's on your list
already.

A couple random comments on the API changes.

I noticed in your description of the API you have added
pn_transport_set_remote_hostname. It's a bit odd to have the setter but no
getter. Secondly there is a terminology conflict here. In general the API
uses the convention that remote_blah refers to information supplied by the
remote peer. You are not using it that way, you are using it to communicate
expected information about the remote peer, and that is actually local
information, not remote information. It really doesn't ever make sense to
have a public set_remote_blah anywhere given the API conventions since the
only place you are ever going to set those fields is with information
supplied to you over the wire by the remote peer.

Also, I'm not convinced you actually need this at all since AMQP defines
the connection hostname and there is already a setter for this. The value
that goes into the hostname frame of a connection is defined to be the
expected/desired hostname that the client will authenticate against (think
vhosts). It is also required to be the same as the one specified in the
sasl-init frame if present. That means pn_connection_set_hostname and
pn_transport_set_remote_hostname as you have defined it are effectively
aliases which is a confusing state of affairs on its own even without the
general conventions around what the remote prefix means.

Another getter/setter asymmetry is the the get_user which doesn't have a
corresponding set_user directly, but does have an indirect one through
set_user_password.

I know the getter/setter stuff is a bit niggling, but beyond being nice to
follow the general conventions, it's actually quite awkward when it comes
to the bindings. In general get/set_foo is mapped into a property, e.g.
obj.foo/obj.foo = blah, and this breaks down if you start having set_foo
without get_foo (write only attributes are kind of ugly/unexpected).

I'd suggest you have get/set_user with set_user being independent of
setting the password. That will map more naturally into an ordinary "user"
property in the bindings, and the setter can return an error code for a
server transport if you want it to be read-only. I also think this is nicer
in general since I expect there are cases where you may want to supply the
password in a different place than you configure the user, and with them
both bundled into the same call, that gets awkward.

--Rafael

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message