Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Laurens Van Houtven]

*deep necromantic thread magic*

On Thu, Oct 3, 2013 at 11:54 AM, Glyph  wrote:

> If I can change "proto" to mean "actually the protocol not something else"
> then that seems plenty easy to add, and it would definitely be cool if
> people don't have to mess with this nonsense themselves for something as
> ostensibly simple as having access to the protocol :-)
>
> Keep in mind that in the authentication case I mentioned, your post-auth
> object may well subclass AMP and therefore "actually" be a protocol; but it
> still won't have a transport.  What do you propose happen in that case?
>

Isn't the post-auth object a box receiver? I mean, yes, it can be a box
receiver by virtue of subclassing AMP and therefore *secretly* being a
proto. I'd be totally fine (for my use case) to just have the transport
available somewhere :) (The transport that the AMP bytes are being sent
over; so not, say, a TCP transport underlying a TLS connection or
something.)

For fixing this (I'll file tickets if you confirm):

- ResponderLocator's behavior is pretty much just broken. It's using
Command.makeResponse. That says it wants an AMP. It's clearly just getting
a responder locator. However, it seems like the problem here is
makeResponse's crappy interface. It shouldn't want an AMP. So, we should
fix makeResponse's docstring, and all of the other cases where t.p.amp asks
for an AMP and shouldn't.
- The proto argument name is stupid. Can we fix it?
- There should be a new method on IArgumentType, parallel to fromBox (but
not toBox?), that gets a reference to the transport passed to it. If so:
where does it get that?

Does that sound about right?

cheers
lvh
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Glyph]

> On Oct 1, 2013, at 1:13 AM, Laurens Van Houtven <_...@lvh.io> wrote:
> 
>> On Tue, Oct 1, 2013 at 12:59 AM, Glyph  wrote:
>>  
>> Most of the code I can think of that wants to use that really wants the 
>> transport rather than the "protocol",
> 
> Yes, but having the protocol would also immediately give you access to the 
> transport, and, from what I understand in most cases of AMP, also everything 
> else :)

The problem here is that you can do AMP decoding without a protocol anywhere in 
sight.  A CommandLocator by itself, passed appropriate boxes to the callables 
returned by locateResponder, is capable of speaking AMP just fine, as long as 
you don't care about speaking it to an actual byte stream :-).  (And ostensibly 
this is one way to speak AMP over transports other than BinaryBoxProtocol.)

In the case of AMP routes (something it would be very nice to integrate into 
the main protocol), you have multiple command locators per transport, and each 
one might have its own properties that would be interesting to Argument types,

This is why putting requirements on the thing-doing-the-parsing in the Argument 
definition makes sense; the Command could interrogate its prospective Responder 
class, asking each of its Argument objects if it will be able to satisfy their 
requirements in turn, at the time that CommandLocator.__metaclass__.__new__ 
gets invoked, rather than once your protocol is already trying to respond to 
commands.

>> but nothing within AMP itself actually uses those arguments; in fact, 
>> searching the usual suspects (epsilon, vertex) I can't even find any 
>> Arguments that use the 'proto' argument for anything useful.
> 
> I suppose it's too late to get "proto" to actually mean "proto" and not 
> "boxSender"?

I believe it currently means "responderLocator".  _wrapWithSerialization is 
where the magic happens if you want to confirm.

> It would definitely be a backwards-incompatible change, and I do actually 
> have some code that somehow relied on it being the boxSender (actually, I 
> think I saved that code in txampext, mostly, except I renamed that thing 
> "proto").

It's probably too late for this method, but you can always add a new one :-)

>> If I recall, I believe the idea behind it was to allow an AMP responder 
>> within Vertex to return the peer's IP address back to the peer, from within 
>> an authenticated AMP route that (because it was a route) wasn't necessarily 
>> connected directly to the transport (and therefore couldn't just do 
>> self.transport.getPeer()).  Ironically I don't think it'll actually work for 
>> that now :-).
>> 
>> When we pull the authentication logic in from 
>> ,
>>  you might write a responder that's interested in authentication information 
>> that lives in some other relation to the protocol.
> 
> I wrote very similar deep-in-AMP auth logic once, and did look at that code 
> (but ended up not using it because I use TLS, so I don't need hand-rolled 
> challenge response or OTP systems).

The hand-rolled CR/OTP crap is really not supposed to be the interesting part. 
Really, that should just be SASL.  The interesting part is the integration with 
cred.

(Augh, and we didn't know the difference between a one-time password and a 
one-time pad, and the wrong word is right there in the wire format... Augh augh 
augh)

> When you say "other relation to the protocol", does that mean "it can be the 
> protocol because the protocol will have some kind of reference to it"?

Notice that in that authentication code, CredReceiver *sets self.boxReceiver* 
to the result of portal.login.  It hands off processing of the parsed AMP boxes 
to another IBoxReceiver.  So the thing parsing the commands - the 
post-authentication protocol - is what the Arguments will currently have direct 
access to (as that will be the CommandLocator as well) but that object will 
have no transport; only a boxSender.

>> So in order to fix fromBox/toBox, we need to do a fix that firms up that 
>> contract and perhaps exposes more than a Protocol object.  The *recommended* 
>> API should be more or less like what ExposingArgument is doing - specify an 
>> Argument that asks for a particular attribute of the transport or the 
>> protocol or the authentication context or whatever, the implementation 
>> details may involve other lower-level public APIs.
> 
> That still sounds like it can be done by making "proto" actually the proto 
> ;-) So, basically, the question is if "proto" being the locator is a bug that 
> I can fix, or an interface that I can't.

You could add a new interface where it's "fixed", but given the case I just 
described above, what does "fixed" mean?

>>> My contributions to AMP have been more of the defect-findy kind, but I 
>>> could certainly turn them more into the code-contributy kind. I imagine I'm 
>>> not the first person to want tests for command classes 

Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Laurens Van Houtven]

On Tue, Oct 1, 2013 at 12:59 AM, Glyph  wrote:


> Most of the code I can think of that wants to use that really wants the *
> transport* rather than the "protocol",
>

Yes, but having the protocol would also immediately give you access to the
transport, and, from what I understand in most cases of AMP, also
everything else :)


> but nothing within AMP itself actually uses those arguments; in fact,
> searching the usual suspects (epsilon, vertex) I can't even find any
> Arguments that use the 'proto' argument for anything useful.
>

I suppose it's too late to get "proto" to actually mean "proto" and not
"boxSender"? It would definitely be a backwards-incompatible change, and I
do actually have some code that somehow relied on it being the boxSender
(actually, I think I saved that code in txampext, mostly, except I renamed
that thing "proto").


> If I recall, I believe the idea behind it was to allow an AMP responder
> within Vertex to return the peer's IP address back to the peer, from within
> an authenticated AMP route that (because it was a route) wasn't necessarily
> connected directly to the transport (and therefore couldn't just do
> self.transport.getPeer()).  Ironically I don't think it'll actually work
> for that now :-).
>
> When we pull the authentication logic in from <
> http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/epsilon/ampauth.py>,
> you might write a responder that's interested in authentication information
> that lives in some other relation to the protocol.
>

I wrote very similar deep-in-AMP auth logic once, and did look at that code
(but ended up not using it because I use TLS, so I don't need hand-rolled
challenge response or OTP systems).

When you say "other relation to the protocol", does that mean "it can be
the protocol because the protocol will have some kind of reference to it"?


> So in order to fix fromBox/toBox, we need to do a fix that firms up that
> contract and perhaps exposes more than a Protocol object.  The
> *recommended* API should be more or less like what ExposingArgument is
> doing - specify an Argument that asks for a particular attribute of the
> transport or the protocol or the authentication context or whatever, the
> implementation details may involve other lower-level public APIs.
>

That still sounds like it can be done by making "proto" actually the proto
;-) So, basically, the question is if "proto" being the locator is a bug
that I can fix, or an interface that I can't.


> My contributions to AMP have been more of the defect-findy kind, but I
> could certainly turn them more into the code-contributy kind. I imagine I'm
> not the first person to want tests for command classes (
> https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or
> a nested AMP box (
> https://github.com/lvh/txampext/blob/master/txampext/nested.py).
>
>
> That would be cool.  And, you know, that auth thing I said :-).
>

If I can change "proto" to mean "actually the protocol not something else"
then that seems plenty easy to add, and it would definitely be cool if
people don't have to mess with this nonsense themselves for something as
ostensibly simple as having access to the protocol :-)

cheers
lvh
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Glyph]

On Sep 30, 2013, at 12:09 PM, Laurens Van Houtven <_...@lvh.io> wrote:

> Hi Glyph,
> 
> Thanks for your response!
> 
> On Mon, Sep 30, 2013 at 8:41 PM, Glyph  wrote:
> On Sep 30, 2013, at 2:45 AM, Laurens Van Houtven <_...@lvh.io> wrote:
> 
>> What am I doing wrong? Is this a bug?
> 
> I think it's pretty clearly a bug.  Calling the argument "proto" in the first 
> place indicates the nature of the confusion.
> 
> There are parts of the flow here from bytes to method execution and back 
> (like _wrapWithSerialization) which are nice for composition, but the fact 
> that they're private sort of ruins their utility for extensibility.
> 
> Looking at the code you're trying to write in txampext though, the problem 
> appears to be simply that you're writing functionality close enough to AMP's 
> core that you should be making the changes to AMP directly, and fixing the 
> issue by making changes to AMP itself rather than trying to work around it 
> externally.  The way I was going to recommend fixing it before I clicked on 
> your link was by writing something like ExposingArgument and accessing the 
> locator/receiver/sender via that new API rather than via the 'proto' argument 
> at all :)
> 
> I'm a little confused why that would help; you're saying there should be a 
> new API that gives arguments access to the locator, receiver, sender? What 
> would that look like? Something along the lines of fromBox/toBox, or are you 
> thinking of a more direct approach where the locator has a reference to the 
> other components? (Given your suggestion of not going through the proto 
> argument, I imagine something closer to the latter.)

For someone confused about why it would help, you are pretty close to the mark 
:).

I am not trying to propose a specific new implementation mechanism, but rather 
to say that fromBox/toBox are broken, in that the contract of the 'proto' 
argument is incompletely specified.  Most of the code I can think of that wants 
to use that really wants the transport rather than the "protocol", but nothing 
within AMP itself actually uses those arguments; in fact, searching the usual 
suspects (epsilon, vertex) I can't even find any Arguments that use the 'proto' 
argument for anything useful.

If I recall, I believe the idea behind it was to allow an AMP responder within 
Vertex to return the peer's IP address back to the peer, from within an 
authenticated AMP route that (because it was a route) wasn't necessarily 
connected directly to the transport (and therefore couldn't just do 
self.transport.getPeer()).  Ironically I don't think it'll actually work for 
that now :-).

When we pull the authentication logic in from 
,
 you might write a responder that's interested in authentication information 
that lives in some other relation to the protocol.

So in order to fix fromBox/toBox, we need to do a fix that firms up that 
contract and perhaps exposes more than a Protocol object.  The *recommended* 
API should be more or less like what ExposingArgument is doing - specify an 
Argument that asks for a particular attribute of the transport or the protocol 
or the authentication context or whatever, the implementation details may 
involve other lower-level public APIs.

> My contributions to AMP have been more of the defect-findy kind, but I could 
> certainly turn them more into the code-contributy kind. I imagine I'm not the 
> first person to want tests for command classes 
> (https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or a 
> nested AMP box 
> (https://github.com/lvh/txampext/blob/master/txampext/nested.py).

That would be cool.  And, you know, that auth thing I said :-).

> I look forward to being in the same locality as you, I presume it will make 
> me more productive ;)

Living in that particular locale is going to spoil me.  I feel like I may need 
to move somewhere more remote so that I am forced to have nice transparent 
discussions on the record like this one, on mailing lists on IRC :-).

-glyph

___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Laurens Van Houtven]

Hi Glyph,

Thanks for your response!

On Mon, Sep 30, 2013 at 8:41 PM, Glyph  wrote:

> On Sep 30, 2013, at 2:45 AM, Laurens Van Houtven <_...@lvh.io> wrote:
>
> What am I doing wrong? Is this a bug?
>
>
> I think it's pretty clearly a bug.  Calling the argument "proto" in the
> first place indicates the nature of the confusion.
>
> There are parts of the flow here from bytes to method execution and back
> (like _wrapWithSerialization) which are nice for composition, but the fact
> that they're private sort of ruins their utility for extensibility.
>
> Looking at the code you're trying to write in txampext though, the problem
> appears to be simply that you're writing functionality close enough to
> AMP's core that you should be making the changes to AMP directly, and
> fixing the issue by making changes to AMP itself rather than trying to work
> around it externally.  The way I was going to recommend fixing it before I
> clicked on your link was by writing something like ExposingArgument and
> accessing the locator/receiver/sender via that new API rather than via the
> 'proto' argument at all :)
>

I'm a little confused why that would help; you're saying there should be a
new API that gives arguments access to the locator, receiver, sender? What
would that look like? Something along the lines of fromBox/toBox, or are
you thinking of a more direct approach where the locator has a reference to
the other components? (Given your suggestion of not going through the proto
argument, I imagine something closer to the latter.)

My contributions to AMP have been more of the defect-findy kind, but I
could certainly turn them more into the code-contributy kind. I imagine I'm
not the first person to want tests for command classes (
https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or a
nested AMP box (
https://github.com/lvh/txampext/blob/master/txampext/nested.py).

I look forward to being in the same locality as you, I presume it will make
me more productive ;)

-glyph
>

cheers
lvh
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Glyph]

On Sep 30, 2013, at 2:45 AM, Laurens Van Houtven <_...@lvh.io> wrote:

> What am I doing wrong? Is this a bug?

I think it's pretty clearly a bug.  Calling the argument "proto" in the first 
place indicates the nature of the confusion.

There are parts of the flow here from bytes to method execution and back (like 
_wrapWithSerialization) which are nice for composition, but the fact that 
they're private sort of ruins their utility for extensibility.

Looking at the code you're trying to write in txampext though, the problem 
appears to be simply that you're writing functionality close enough to AMP's 
core that you should be making the changes to AMP directly, and fixing the 
issue by making changes to AMP itself rather than trying to work around it 
externally.  The way I was going to recommend fixing it before I clicked on 
your link was by writing something like ExposingArgument and accessing the 
locator/receiver/sender via that new API rather than via the 'proto' argument 
at all :)

-glyph___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?

[Laurens Van Houtven]

Hi everyone,


I think I've hit one of those cases where AMP really seems to want
everything (locator, receiver, sender) to be an instance of t.p.amp.AMP :-(

I've written some code that tries to multiplex stream transports over AMP:

https://github.com/lvh/txampext/blob/multiplexing/txampext/multiplexing.py

The repo contains an example server and client, which demonstrate the issue:

https://github.com/lvh/txampext/blob/multiplexing/docs/examples/multiplexing_client.py
https://github.com/lvh/txampext/blob/multiplexing/docs/examples/multiplexing_server.py

In order to do some of this multiplexing, I need access to the protocol
instance inside the responder on the server side. Fortunately, I already
had some code that exposed box senders (after a lot of advice from Glyph).
I modified it to expose the protocol as well:

https://github.com/lvh/txampext/blob/multiplexing/txampext/exposed.py#L41

However, it turns out fromBox gets called with the *responder locator* as
the "proto" argument, not the actual protocol.

The server has a pudb call that makes it easy (?!) to trace this down. The
CommandLocator class, inside doit (a function defined in
_wrapWithSerialization) passes "self" to command.parseArguments:

https://twistedmatrix.com/trac/browser/trunk/twisted/protocols/amp.py#L1015

This is the part where I think the contract is broken, since parseArguments
claims to want the protocol (well, it says it wants the AMP protocol,
which, subclassing everything, is also all of the things, of course), but
receives the responder locator.

What am I doing wrong? Is this a bug?

confusedly,
lvh
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python