Re: c2s per session user data & authreg auth API extension
>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
Re: c2s per session user data & authreg auth API extension
>>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
Dnia 2014-08-14, czw o godzinie 23:45 +, Shawn Debnath pisze:
> 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 :-)
--
Tomasz Sterna :(){ :|:&};:
Instant Messaging Consultant Open Source Developer
http://abadcafe.pl/ http://xiaoka.com/portfolio
signature.asc
Description: This is a digitally signed message part
Re: c2s per session user data & authreg auth API extension
>Dnia 2014-08-14, czw o godzinie 16:20 +, Shawn Debnath pisze:
>> I would change all the APIs and to pass in a pointer to the sess_t as
>> I also need it in check_passsword.
>
>I would advise to include sess_t* in authreg_private then.
>
>It's OK for authreg to dig around session data, but the API should be
>flexible enough to give option to pass anything as authreg_private, not
>only sess_t*.
authreg_private is opaque as far as c2s is concerned. 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. c2s shouldn¹t dictate
what¹s in authreg_private as its a void * member and implementation
dependent.
Once they have sess_t, they can initialize authreg_private as they wish or
leave it NULL. On session closure, c2s checks if the pointer is NULL, if
so,
calls free().
Here¹s what it looks like:
Struct sess_t {
[...
/* Per user session authreg private data */
void*authreg_private;
};
/** returns 1 if the user exists, 0 if not */
int (*user_exists)(authreg_t ar, sess_t sess, const char
*username,const char *realm);
/** return this users cleartext password in the array (digest auth,
password auth) */
int (*get_password)(authreg_t ar, sess_t sess, const char
*username, const char *realm, char password[257]);
/** check the given password against the stored password, 0 if equal,
!0 if not equal (password auth) */
int (*check_password)(authreg_t ar, sess_t sess, const char
*username, const char *realm, char password[257]);
/** store this password (register) */
int (*set_password)(authreg_t ar, sess_t sess, const char
*username, const char *realm, char password[257]);
/** make or break the user (register / register remove) */
int (*create_user)(authreg_t ar, sess_t sess, const char
*username, const char *realm);
int (*delete_user)(authreg_t ar, sess_t sess, const char
*username, const char *realm);
void(*free)(authreg_t ar);
/* Additions at the end - to preserve offsets for existing modules */
/** returns 1 if the user is permitted to authorize as the
requested_user, 0 if not. requested_user is a JID */
int (*user_authz_allowed)(authreg_t ar, sess_t sess, const
char *username, const char *realm, const char *requested_user);
/** Apple extensions for challenge/response authentication methods */
int (*create_challenge)(authreg_t ar, sess_t sess, const char
*username, const char *realm, const char *challenge, int maxlen);
int (*check_response)(authreg_t ar, sess_t sess, const char
*username, const char *realm, const char *challenge, const char *response);
};
Re: c2s per session user data & authreg auth API extension
Dnia 2014-08-14, czw o godzinie 16:20 +, Shawn Debnath pisze:
> I would change all the APIs and to pass in a pointer to the sess_t as
> I also need it in check_passsword.
I would advise to include sess_t* in authreg_private then.
It's OK for authreg to dig around session data, but the API should be
flexible enough to give option to pass anything as authreg_private, not
only sess_t*.
--
Tomasz Sterna :(){ :|:&};:
Instant Messaging Consultant Open Source Developer
http://abadcafe.pl/ http://xiaoka.com/portfolio
signature.asc
Description: This is a digitally signed message part
Re: c2s per session user data & authreg auth API extension
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
Dnia 2014-08-14, czw o godzinie 04:27 +, Shawn Debnath pisze: > - Build a hash table of relevant data and store it in the authreg_t > private data member. Agreed, that needed internal bookkeeping makes it not feasible. > - 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? > 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. 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. ;-) > /* 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); I don't like this approach for two reasons. - custom_auth does not really mean anything. as it is now it is clean - either we have password verification, or challenge/response. - custom_auth is used in "ar_mechs & AR_MECH_TRAD_CRAMMD5", so it is not really custom, but CRAM-MD5, right? 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. :-) -- smk
c2s per session user data & authreg auth API extension
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 */
if(se
