c2s per session user data authreg auth API extension

[Shawn Debnath]

Hi there guys,

I am building a XMPP service around jabberd2 and I am very impressed
with the current architecture. However, as I am getting further into
our implementation, a couple of problems have become apparent.

Problems:
- We need to maintain per user session data in c2s. For example:
  authentication token and unique user ID that is system and session
  specific. authreg_t has a private member to hold authreg related data
  but this is module wide and not tied to an individual user.
- For CRAM-MD5, to generate the proper challenge, we need more than the
  username. Here, realm would be good to have and for verifying the
  response, the associated resource would be needed to generate the
  unique login ID in our system.

For us, the two are intertwined and the current APIs and data structures
do not provide a solution to the above problems, to the best of my
knowledge. If I am mistaken, please do let me know :)

Solutions:

There needs to be a way for a 3rd party to plug in and provide the
necessary information to respond to the initial auth request and also
have all the relevant information to properly authenticate the user
and be able to retain user  resource specific tokens throughout the
lifetime of the user's session.

- Build a hash table of relevant data and store it in the authreg_t
  private data member.

  Pros: Fits nicely into the existing architecture
  Cons: Its up to the authreg provider to maintain proper state and
moves session related data away from the session and into a component
which outlives the span of an individual session. Doesn¹t help much
With problem (1).

- Retrofit existing interfaces with the necessary data.
  a. Introduce void *sess_private in sess_t.
  b. APIs to add pointer to sess_private along with realm and
 resource arguments when available.

struct sess_st {
  ...
  void*   sess_private; /* Data per session per user */
}


  int (*create_challenge)(authreg_t ar, sess_private *data,
const char *username, const char *realm, const char *challenge,
int maxlen);
  int (*check_response)(authreg_t ar, sess_private *data,
const char *username, const char *realm, const char *resource,
 const char *challenge, const char *response);

  Pros: Maintain same methods but new parameters, faster approach.
  Cons: (BIG)breaks everyone out there. In some cases, other 3rd parties
may want similar mechanism for plain text login as well and this
approach wouldn't work for them.


- Introduce the private data in sess_t as above and also new functions in
  authreg_t that allows for generic authentication handling by third
  parties. 

  (see sess_private above)

  /* Extension for custom authentication providers */
  int (*custom_auth_get)(authreg_t ar, authdata_t data);
  int (*custom_auth_set)(authreg_t ar, authdata_t data);
  

  where

  /* Custom authentication data object */
  struct authdata_st {
void **sess_private;
const char *username;
const char *resource;
const char *realm;
const char *password;
const char *digest;
char *challenge;
int challenge_maxlen;
const char *response;
  };

  The authdata_st can be expanded as needed without changing the APIs.
  The APIs mimic authreg_auth_[get/set] functions and for get, in the
  case of CRAM-MD5, returns a challenge based on the contextual data. In
  the case of set, it has all the relevant pieces of information to
  serve the request as necessary and return true or false for success
  or failure.

  Pros: A very, very generic way to leave it to the authreg implementor
to authenticate the user, as long as the right results are returned.
  Cons: New APIs, potentially very generic and open.


I chose the third route, because others could implement their own
authentication schemes now that all the relevant and required pieces of
information is available. I put together a quick implementation and the
patch is added below. I coded it up in a way that if the custom
authentication approach fails, we fall back on the existing methods so
the impact would be inconsequential if an auth provider doesn't
implement the change. This applies to both get  set functionality. See
the patch for more details.

I am trying to come up with a solution that would be accepted
by the current maintainers and that I can submit back
to the community. This patch by no means is the final version and I open
to all feedback, be it naming, implementation, or the approach in
general, as long as it solves the two problems stated above.

Looking forward to the feedback, thanks.

Shawn



---


diff --git a/c2s/authreg.c b/c2s/authreg.c
index 627d22e..ace320c 100644
--- a/c2s/authreg.c
+++ b/c2s/authreg.c
@@ -135,6 +135,8 @@ static void _authreg_auth_get(c2s_t c2s, sess_t sess,
nad_t nad) {
 int ns, elem, attr, err;
 char username[1024], id[128];
 int ar_mechs;
+authdata_t ad;
+int processed = 0;
 
 /* can't auth if they're active */
 

Re: c2s per session user data authreg auth API extension

[Shawn Debnath]

Great! Yes I was a bit iffy on the ³custom² approach myself given it
really didn¹t align with any authentication method. But if API
breakage is okay, this is definitely the cleanest way.

- Retrofit existing interfaces with the necessary data.
   a. Introduce void *sess_private in sess_t.

It's not really sess_private, but authreg_private, right?

Yep agreed. Though what would be better is if we could somehow classify
it as per user per session data. Thinking of user_st.module_data in sm.h.
But authreg_private works if no other suggestions are found.


   int (*create_challenge)(authreg_t ar, sess_private *data,
 const char *username, const char *realm, const char *challenge,
 int maxlen);
   int (*check_response)(authreg_t ar, sess_private *data,
 const char *username, const char *realm, const char *resource,
  const char *challenge, const char *response);
 
   Pros: Maintain same methods but new parameters, faster approach.
   Cons: (BIG)breaks everyone out there. In some cases, other 3rd parties
 may want similar mechanism for plain text login as well and this
 approach wouldn't work for them.

Agreed.
I think we should extend all authreg calls with a pointer to session
attached, authreg private data.
In the simplest case it could be even set to point to sess_st, for the
mechanizm to dig in session by itself.
This is how it is done all around jabberd2.

Yep agreed, providing sess_st avoids another API change down the road and
gives access to all the necessary data needed.

Also good point, that create_challenge misses realm parameter.

If we go for it, we will just release 2.4.x line which hints API
breakage. ;-)

This is perfect. Though, just a heads up, I may have some more suggestions
on the storage_* side as well once I dig into it a bit more :)


Let's just implement CRAM-MD5 properly, with all needed features, even
if it means API changes.
We're open source - we are not afraid to change things. :-)

I would change all the APIs and to pass in a pointer to the sess_t as I
also need it in check_passsword. Similar issues, I need to store the
authentication token for a successful verification. Also, this way the APIs
are uniform.

This is good stuff. These changes help any organization providing
centralized token based authentication where the resource specific and
user session needs to be stashed away properly.

I should be able to get a patch/pull reques back out by EOD today.

Thanks,
Shawn

Re: c2s per session user data authreg auth API extension

[Shawn Debnath]

I have modified the
 APIs to pass sess_t and then the implementation can choose to pack it
 in their private authreg_private data if they so choose.

WFM :-)

Changes almost ready to go, will submit a pull request within the hour.

Re: c2s per session user data authreg auth API extension

[Shawn Debnath]

Changes almost ready to go, will submit a pull request within the hour.

Pull request submitted: https://github.com/jabberd2/jabberd2/pull/72
Issue create to track (perhaps excessive):
https://github.com/jabberd2/jabberd2/issues/71

Looking forward to comments and feedback!

Thanks,
Shawn

xhash and it's key

[Shawn Debnath]

Hey there,

Turns out that when inserting items in xhash, the code stores a pointer to
the key passed in by the user in the xhash node and uses that later to
compare in _get. The problem is that it breaks scenarios where the user may
use a temp buffer to build the key, then insert or put it in the xhash
and then free the buffer memory. Assumption here is that xhash code
would allocate necessary buffer to store internal data and not rely on
user supplied memory to maintain it=A9=F6s internal data structures.

In the example above, the put succeeds, user frees the memory, so its
Pointing to garbage and a later a call to _get can't find the entry
because the keys mismatch, i.e., a user supplied valid string key and
NULL memory that the xhash node points to.

Any ideas if there was a particular reason this was designed this way? I
imagine, in most of the cases the key is inside the object being stored
so it works out. However, as you can see, the xhash implementation
can¹t be fully exploited/used.

It would be trivial, I would imagine, to make a copy of the key and
store that instead of just the pointer to user supplied memory. On
removal in zap_internal, the memory would be freed.

Thoughts? Concerns? Let me know :)

Shawn

Re: xhash and it's key

[Shawn Debnath]

Pull request #79 submitted.

https://github.com/jabberd2/jabberd2/pull/79


On 8/28/14, 10:51, Shawn Debnath sh...@debnath.net wrote:

Hey there,

Turns out that when inserting items in xhash, the code stores a pointer to
the key passed in by the user in the xhash node and uses that later to
compare in _get. The problem is that it breaks scenarios where the user
may
use a temp buffer to build the key, then insert or put it in the xhash
and then free the buffer memory. Assumption here is that xhash code
would allocate necessary buffer to store internal data and not rely on
user supplied memory to maintain it=A9=F6s internal data structures.

In the example above, the put succeeds, user frees the memory, so its
Pointing to garbage and a later a call to _get can't find the entry
because the keys mismatch, i.e., a user supplied valid string key and
NULL memory that the xhash node points to.

Any ideas if there was a particular reason this was designed this way? I
imagine, in most of the cases the key is inside the object being stored
so it works out. However, as you can see, the xhash implementation
can¹t be fully exploited/used.

It would be trivial, I would imagine, to make a copy of the key and
store that instead of just the pointer to user supplied memory. On
removal in zap_internal, the memory would be freed.

Thoughts? Concerns? Let me know :)

Shawn

Re: xhash and it's key

[Shawn Debnath]

Could you rethink your design to include the identifier as a part of the
object it names?

Yep, looks like this is a cleaner way out.

Re: Loqui IM doesn’t work with the XEP-198 by jabberd2

[Shawn Debnath]

On 5/26/15, 9:08 AM, Matěj Cepl mc...@cepl.eu wrote:

(a follow-up to the issue 
https://github.com/loqui/im/issues/732#issuecomment-105519240)

I have a jabberd2 XMPP server on domain ceplovi.cz and I would 
like to connect to it with Loqui. It works perfectly well with 
pidgin, bitlbee, empathy, displays well on IM Observatory, but 
Loqui just ALWAYS ends with “Authentication failed” (it is not 
a temporary failure). The strange thing is that jabberd2 logs on 
the server don’t show any activity when I try to login.

The Authentication Failed” message is likely a standard canned
response from Loqui when they can’t establish a connection with
your jabberd2 server.

Loqui IM people do think that it is because of discrepancy 
between the implementation of XEP-198 by jabberd2 and (they say) 
more recent version of it expected by Loqui IM.

Are you able to rebuild jabberd2 from source? Enabling the debug 
option and obtaining the detailed output can lead to better
information about the exact cause of the failure. Once rebuilt,
you can specify the file to send the debug output to in the
component config files. I would enable debug on router, c2s and
sm just in case. Streams codebase hasn’t changed in a while AFAIK. 

For more info: 
https://github.com/jabberd2/jabberd2/wiki/InstallGuide-Troubleshooting

Re: missing presence packet

[Shawn Debnath]

Typically, the hyphen is a special character and the string containing it has 
to be escaped correctly. Also evident from the way “object-sequence” or the 
column names are escaped in the debug log. Have you tried running the query on 
the database itself to check the results?

Googling for mysql specific escape character suggests usage of ‘`’ character.





On 12/8/15, 12:01 PM, "Stepan Salenikovich" 
 wrote:

>Hi,
>So I recompiled with --debug-enabled and ran with -D
>
>checking the sm debug log, it seems the issue is that when user A logs back 
>on, it's roster appears empty (ie: the mysql query returns with 0 items)... 
>however, when I perform that same query on the db myself, I get several items. 
>Heres the relevant log, the important query being "prepared sql: SELECT * FROM 
>`roster-items` WHERE `collection-owner` = 'a...@foo-bar.domain.com' ORDER BY 
>`object-sequence`":
>
>Tue Dec  8 12:20:36 2015 sess.c:111 session requested for 
>a...@foo-bar.domain.com/resource-A
>Tue Dec  8 12:20:36 2015 mm.c:663 dispatching user-load chain
>Tue Dec  8 12:20:36 2015 mm.c:676 calling module active
>Tue Dec  8 12:20:36 2015 storage.c:224 storage_get: type=active 
>owner=a...@foo-bar.domain.com filter=(null)
>Tue Dec  8 12:20:36 2015 storage_mysql.c:324 generated filter: 
>`collection-owner` = 'a...@foo-bar.domain.com'
>Tue Dec  8 12:20:36 2015 storage_mysql.c:330 prepared sql: SELECT * FROM 
>`active` WHERE `collection-owner` = 'a...@foo-bar.domain.com' ORDER BY 
>`object-sequence`
>Tue Dec  8 12:20:36 2015 storage_mysql.c:351 1 tuples returned
>Tue Dec  8 12:20:36 2015 object.c:85 creating new object
>Tue Dec  8 12:20:36 2015 storage_mysql.c:395 unknown field type 8, ignoring it
>Tue Dec  8 12:20:36 2015 object.c:136 adding field time (val e1997bdc type 1) 
>to object
>Tue Dec  8 12:20:36 2015 object.c:283 got field time (val 5667115d type 1) to 
>object
>Tue Dec  8 12:20:36 2015 mm.c:676 calling module roster
>Tue Dec  8 12:20:36 2015 mod_roster.c:744 loading roster for 
>a...@foo-bar.domain.com
>Tue Dec  8 12:20:36 2015 storage.c:224 storage_get: type=roster-items 
>owner=a...@foo-bar.domain.com filter=(null)
>Tue Dec  8 12:20:36 2015 storage_mysql.c:324 generated filter: 
>`collection-owner` = 'a...@foo-bar.domain.com'
>Tue Dec  8 12:20:36 2015 storage_mysql.c:330 prepared sql: SELECT * FROM 
>`roster-items` WHERE `collection-owner` = 'a...@foo-bar.domain.com' ORDER BY 
>`object-sequence`
>Tue Dec  8 12:20:36 2015 storage.c:224 storage_get: type=roster-groups 
>owner=a...@foo-bar.domain.com filter=(null)
>Tue Dec  8 12:20:36 2015 storage_mysql.c:324 generated filter: 
>`collection-owner` = 'a...@foo-bar.domain.com'
>Tue Dec  8 12:20:36 2015 storage_mysql.c:330 prepared sql: SELECT * FROM 
>`roster-groups` WHERE `collection-owner` = 'a...@foo-bar.domain.com' ORDER BY 
>`object-sequence`
>
>
>The only difference I've been able to find so far between this server and the 
>one on which the query works as it should, is that the jid of the user on the 
>working server is "a...@foo.domain.com" instead of 
>"a...@foo-bar.domain.com"... I don't know if there could be a bug in 
>mysql_query() causing it to not like value strings containing '-'... seems 
>unlikely...
>
>-stepan
>
>- Original Message -
>From: "Tomasz Sterna" 
>To: "jabberd2" 
>Sent: Sunday, December 6, 2015 5:19:24 AM
>Subject: Re: missing presence packet
>
>W dniu 04.12.2015, pią o godzinie 15∶05 -0500, użytkownik Stepan
>Salenikovich napisał:
>> So I'm looking for suggestions as to how this could be debuged... or
>> any tips as to where to look.
>
>Turn on debug logs -D on both c2s and sm and analyse what happens when
>A logs back on.
>
>
>-- 
> /o__ 
>(_<^' I'm a soldier, not a diplomat. I can only tell the truth.
>
>

Re: Future of jabberd

[Shawn Debnath]

I agree with Tomasz that there are changes that are definitely needed 
given how much technology has changed since jabberd2 was first released.

Re 1. Merging separate daemons to one.

I am not sure if merging them into one process is the best idea. It sure is
convenient, but isolation is a nice thing to have. Specially, when you
have unauthorized users hammering on C2S. To be honest, I believe, that
instead of going micro – go macro. Think distributed scalable server platform
rather than ease of running on one machine. In today’s services oriented world,
this approach will find a better home I believe. Which brings me to router. 
Today, it’s a bottleneck. There are several ways, as mentioned in Tomasz’s 
email, to fix this issue and something that would be great to tackle this as
part of this re-architecture. MoongooseIM is a good place to look at for
scalable XMPP platform architecture.

4. Configuration interface

Subscription based model works well and its okay to force developers
to write config change handlers. It’s 2016, time to not argue about this
point given the advantages.

5. JavaScript support.

Personally, C/C++ is fine, however, if folks insist, would be good to keep
the existing setup and allow for JS SM components as a compile
time option.

6. String handling

Something that would be nice to see is allowing SM modules or others
to pre-process the PKT to extract as much meaningful data once (in the
beginning) and then passing that object around. Currently, at each
stage of the SM module call chain, we have to re-process the PKT or NAD
to extract the same data over and over again. Unnecessary work that 
can be avoided easily. Easy to do in C++ but a bit awkward in C.


On 5/30/16, 1:31 AM, "Tomasz Sterna"  wrote:

>There are some things we already talked about on Gitter channel [1],
>but I would like to raise them on the ML for peer review.
>
>As you can see from late activity, jabberd2 project is far from dead.
>With the inclusion of new features like WebSocket support, C99 code
>compatibility, IPv6 improvements, modern TLS handling, SASL Anonymous,
>password hashing, CRAM-MD5 and more... it is not a stale codebase
>anymore.
>
>But it is far from modern too...
>There are some changes I would like to introduce in the near future and
>I would like to hear your thoughts about:
>
>1. Merging separate daemons to one.
>Current design of jabberd2 with separate router, sm, c2s, s2s processes
>is designed to allow nice separation of concerns and distribution of
>processing. Separate processes are proved to be better approach than
>threads too.
>But most installations of jabberd are not distributed, with one
>instance of each component. Especially when c2s and sm got vhost
>support and are able to handle more than one domain.
>Also, modern OS architectures are tuned for event processing rather
>than multithreading, so event based architecture is better suited for
>them. Even jabberd2 process internally is event based on MIO.
>So, it makes sense to allow for running all component instances in one
>process, especially on amateur, low load servers.
>Merging processes will allow for having one main loop only, so
>maintaining bugfixes in it will be easier (main.c of all processes is a
>copy-paste, with all the bugs, so bugs are also multiplied).
>
>2. Phasing out MIO.
>This is closely related to above. MIO used by jabberd2 does not have
>clerar main loop support, which is implemented separately in each
>component main.c and is hardly pluggable.
>Also, the way MIO is implemented (in .h file, with platform specific
>bits in .c) makes it a maintanance nightmare.
>I would really like to replace it with a modern, upstream maintained
>event library. The nicest one I know is libuv, which also gives us nice
>platform independence layer.
>I already have a working c2s port to libuv as a PoC.
>
>3. Phasing out router.
>router component is the one binding all the others.
>In current design it is the single point of failure. Other components
>already support multiple instances, but router proved to be difficult
>to multiply.
>The most radical, yet compelling solution to this problem is getting
>rid of the router at all. There are many cooked solutions for local
>packet distribution, which Local Message Bus [2] looks like most
>promising solution. I would see either Mbus [3] or NN_BUS [4] taking
>role of router component.
>The added advantage of using a Message Bus is the ability to connect to
>the bus with alternative implementations to perform own actions.
>i.e. having the ability to use CLI tools to eavesdrop and send messages
>to the bus proved to be priceless when I implemented a PoC of the Bus
>in experimental jabberd branch.
>Bus also solves the problem of distribution - it is up to the
>deployment administrator whether one sets up local, one-machine only
>bus or a network distributed one.
>
>4. Configuration interface.
>A the moment jabberd is configured with static XML files loaded at