[serf] r2495 committed - More auth framework refactoring: move auth schemes vtable definitions...

[se...@googlecode.com]

Revision: 2495
Author: chemodax
Date: Wed Apr 8 17:46:40 2015 UTC
Log: More auth framework refactoring: move auth schemes vtable
definitions
to appropriate file with authn scheme implementation.

* auth/auth.c
(default_auth_response_handler): Remove.
(serf_authn_schemes): Change type to array of pointers to
serf__authn_scheme_t structures.
(handle_auth_headers): Update code to reflect serf_authn_schemes type
change.

* auth/auth.h
(serf__init_basic, serf__init_basic_connection,
serf__handle_basic_auth, serf__setup_request_basic_auth,
serf__init_digest, serf__init_digest_connection,
serf__handle_digest_auth, serf__setup_request_digest_auth,
serf__validate_response_digest_auth, serf__init_spnego,
serf__init_spnego_connection, serf__handle_spnego_auth,
serf__setup_request_spnego_auth, serf__validate_response_spnego_auth):
Remove function declarations -- we use authn scheme vtable to
access them.

(serf__basic_authn_scheme, serf__digest_authn_scheme,
serf__spnego_authn_scheme, serf__ntlm_authn_scheme): Declare authn
schemes.

* auth/auth_basic.c
(serf__handle_basic_auth, serf__init_basic,
serf__init_basic_connection, serf__setup_request_basic_auth): Mark
as static.
(validate_response_func): New.
(serf__basic_authn_scheme): Factored out from serf_authn_schemes in
auth.c

* auth/auth_digest.c
(serf__handle_digest_auth, serf__init_digest,
serf__init_digest_connection, serf__setup_request_digest_auth,
serf__validate_response_digest_auth): Mark as static.
(serf__digest_authn_scheme): Factored out from serf_authn_schemes in
auth.c

* auth/auth_spnego.c
(serf__init_spnego_connection, serf__handle_spnego_auth,
serf__setup_request_spnego_auth): Mark as static.
(serf__spnego_authn_scheme, serf__ntlm_authn_scheme): Factored out
from serf_authn_schemes in auth.c

https://code.google.com/p/serf/source/detail?r=2495

Modified:
/trunk/auth/auth.c
/trunk/auth/auth.h
/trunk/auth/auth_basic.c
/trunk/auth/auth_digest.c
/trunk/auth/auth_spnego.c

=======================================
--- /trunk/auth/auth.c Wed Apr 8 12:45:40 2015 UTC
+++ /trunk/auth/auth.c Wed Apr 8 17:46:40 2015 UTC
@@ -21,18 +21,6 @@
#include <apr_base64.h>
#include <apr_strings.h>
#include <apr_lib.h>
-
-static apr_status_t
-default_auth_response_handler(const serf__authn_scheme_t *scheme,
- peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- serf_bucket_t *response,
- apr_pool_t *pool)
-{
- return APR_SUCCESS;
-}

/* These authentication schemes are in order of decreasing security, the
topmost
scheme will be used first when the server supports it.
@@ -42,55 +30,19 @@

Use lower case for the scheme names to enable case insensitive matching.
*/
-static const serf__authn_scheme_t serf_authn_schemes[] = {
+static const serf__authn_scheme_t *serf_authn_schemes[] = {
#ifdef SERF_HAVE_SPNEGO
- {
- "Negotiate",
- "negotiate",
- SERF_AUTHN_NEGOTIATE,
- serf__init_spnego,
- serf__init_spnego_connection,
- serf__handle_spnego_auth,
- serf__setup_request_spnego_auth,
- serf__validate_response_spnego_auth,
- },
+ &serf__spnego_authn_scheme,
#ifdef WIN32
- {
- "NTLM",
- "ntlm",
- SERF_AUTHN_NTLM,
- serf__init_spnego,
- serf__init_spnego_connection,
- serf__handle_spnego_auth,
- serf__setup_request_spnego_auth,
- serf__validate_response_spnego_auth,
- },
+ &serf__ntlm_authn_scheme,
#endif /* #ifdef WIN32 */
#endif /* SERF_HAVE_SPNEGO */
- {
- "Digest",
- "digest",
- SERF_AUTHN_DIGEST,
- serf__init_digest,
- serf__init_digest_connection,
- serf__handle_digest_auth,
- serf__setup_request_digest_auth,
- serf__validate_response_digest_auth,
- },
- {
- "Basic",
- "basic",
- SERF_AUTHN_BASIC,
- serf__init_basic,
- serf__init_basic_connection,
- serf__handle_basic_auth,
- serf__setup_request_basic_auth,
- default_auth_response_handler,
- },
+ &serf__digest_authn_scheme,
+ &serf__basic_authn_scheme,
/* ADD NEW AUTHENTICATION IMPLEMENTATIONS HERE (as they're written) */

/* sentinel */
- { 0 }
+ NULL
};


@@ -125,7 +77,7 @@
serf_bucket_t *response,
apr_pool_t *pool)
{
- const serf__authn_scheme_t *scheme;
+ int scheme_idx;
serf_connection_t *conn = request->conn;
serf_context_t *ctx = conn->ctx;
apr_status_t status;
@@ -135,10 +87,11 @@
/* Find the matching authentication handler.
Note that we don't reuse the auth scheme stored in the context,
as that may have changed. (ex. fallback from ntlm to basic.) */
- for (scheme = serf_authn_schemes; scheme->name != 0; ++scheme) {
+ for (scheme_idx = 0; serf_authn_schemes[scheme_idx]; ++scheme_idx) {
const char *auth_hdr;
serf__auth_handler_func_t handler;
serf__authn_info_t *authn_info;
+ const serf__authn_scheme_t *scheme =
serf_authn_schemes[scheme_idx];

if (! (ctx->authn_types & scheme->type))
continue;
=======================================
--- /trunk/auth/auth.h Wed Apr 8 13:04:15 2015 UTC
+++ /trunk/auth/auth.h Wed Apr 8 17:46:40 2015 UTC
@@ -129,85 +129,20 @@
apr_pool_t *pool);

/** Basic authentication **/
-apr_status_t serf__init_basic(int code,
- serf_context_t *ctx,
- apr_pool_t *pool);
-apr_status_t serf__init_basic_connection(const serf__authn_scheme_t
*scheme,
- int code,
- serf_connection_t *conn,
- apr_pool_t *pool);
-apr_status_t serf__handle_basic_auth(int code,
- serf_request_t *request,
- serf_bucket_t *response,
- const char *auth_hdr,
- const char *auth_attr,
- apr_pool_t *pool);
-apr_status_t serf__setup_request_basic_auth(peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- const char *method,
- const char *uri,
- serf_bucket_t *hdrs_bkt);
-
+extern const serf__authn_scheme_t serf__basic_authn_scheme;
+;
/** Digest authentication **/
-apr_status_t serf__init_digest(int code,
- serf_context_t *ctx,
- apr_pool_t *pool);
-apr_status_t serf__init_digest_connection(const serf__authn_scheme_t
*scheme,
- int code,
- serf_connection_t *conn,
- apr_pool_t *pool);
-apr_status_t serf__handle_digest_auth(int code,
- serf_request_t *request,
- serf_bucket_t *response,
- const char *auth_hdr,
- const char *auth_attr,
- apr_pool_t *pool);
-apr_status_t serf__setup_request_digest_auth(peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- const char *method,
- const char *uri,
- serf_bucket_t *hdrs_bkt);
-apr_status_t serf__validate_response_digest_auth(const
serf__authn_scheme_t *scheme,
- peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- serf_bucket_t *response,
- apr_pool_t *pool);
+extern const serf__authn_scheme_t serf__digest_authn_scheme;

#ifdef SERF_HAVE_SPNEGO
/** Kerberos authentication **/
-apr_status_t serf__init_spnego(int code,
- serf_context_t *ctx,
- apr_pool_t *pool);
-apr_status_t serf__init_spnego_connection(const serf__authn_scheme_t
*scheme,
- int code,
- serf_connection_t *conn,
- apr_pool_t *pool);
-apr_status_t serf__handle_spnego_auth(int code,
- serf_request_t *request,
- serf_bucket_t *response,
- const char *auth_hdr,
- const char *auth_attr,
- apr_pool_t *pool);
-apr_status_t serf__setup_request_spnego_auth(peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- const char *method,
- const char *uri,
- serf_bucket_t *hdrs_bkt);
-apr_status_t serf__validate_response_spnego_auth(const
serf__authn_scheme_t *scheme,
- peer_t peer,
- int code,
- serf_connection_t *conn,
- serf_request_t *request,
- serf_bucket_t *response,
- apr_pool_t *pool);
+
+extern const serf__authn_scheme_t serf__spnego_authn_scheme;
+
+#ifdef WIN32
+extern const serf__authn_scheme_t serf__ntlm_authn_scheme;
+#endif /* #ifdef WIN32 */
+
#endif /* SERF_HAVE_SPNEGO */

#ifdef __cplusplus
=======================================
--- /trunk/auth/auth_basic.c Wed Jul 31 10:29:41 2013 UTC
+++ /trunk/auth/auth_basic.c Wed Apr 8 17:46:40 2015 UTC
@@ -30,7 +30,7 @@
const char *value;
} basic_authn_info_t;

-apr_status_t
+static apr_status_t
serf__handle_basic_auth(int code,
serf_request_t *request,
serf_bucket_t *response,
@@ -110,7 +110,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__init_basic(int code,
serf_context_t *ctx,
apr_pool_t *pool)
@@ -124,7 +124,7 @@
context instead of per connection.
TODO: we currently don't cache this info per realm, so each time a
request
'switches realms', we have to ask the application for new credentials.
*/
-apr_status_t
+static apr_status_t
serf__init_basic_connection(const serf__authn_scheme_t *scheme,
int code,
serf_connection_t *conn,
@@ -146,7 +146,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__setup_request_basic_auth(peer_t peer,
int code,
serf_connection_t *conn,
@@ -174,3 +174,26 @@

return SERF_ERROR_AUTHN_FAILED;
}
+
+static apr_status_t
+validate_response_func(const serf__authn_scheme_t *scheme,
+ peer_t peer,
+ int code,
+ serf_connection_t *conn,
+ serf_request_t *request,
+ serf_bucket_t *response,
+ apr_pool_t *pool)
+{
+ return APR_SUCCESS;
+}
+
+const serf__authn_scheme_t serf__basic_authn_scheme = {
+ "Basic",
+ "basic",
+ SERF_AUTHN_BASIC,
+ serf__init_basic,
+ serf__init_basic_connection,
+ serf__handle_basic_auth,
+ serf__setup_request_basic_auth,
+ validate_response_func,
+};
=======================================
--- /trunk/auth/auth_digest.c Mon Aug 18 13:25:38 2014 UTC
+++ /trunk/auth/auth_digest.c Wed Apr 8 17:46:40 2015 UTC
@@ -227,7 +227,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__handle_digest_auth(int code,
serf_request_t *request,
serf_bucket_t *response,
@@ -358,7 +358,7 @@
return status;
}

-apr_status_t
+static apr_status_t
serf__init_digest(int code,
serf_context_t *ctx,
apr_pool_t *pool)
@@ -366,7 +366,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__init_digest_connection(const serf__authn_scheme_t *scheme,
int code,
serf_connection_t *conn,
@@ -391,7 +391,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__setup_request_digest_auth(peer_t peer,
int code,
serf_connection_t *conn,
@@ -455,7 +455,7 @@
return APR_SUCCESS;
}

-apr_status_t
+static apr_status_t
serf__validate_response_digest_auth(const serf__authn_scheme_t *scheme,
peer_t peer,
int code,
@@ -554,3 +554,14 @@

return APR_SUCCESS;
}
+
+const serf__authn_scheme_t serf__digest_authn_scheme = {
+ "Digest",
+ "digest",
+ SERF_AUTHN_DIGEST,
+ serf__init_digest,
+ serf__init_digest_connection,
+ serf__handle_digest_auth,
+ serf__setup_request_digest_auth,
+ serf__validate_response_digest_auth,
+};
=======================================
--- /trunk/auth/auth_spnego.c Mon Aug 18 13:25:38 2014 UTC
+++ /trunk/auth/auth_spnego.c Wed Apr 8 17:46:40 2015 UTC
@@ -368,7 +368,7 @@

/* A new connection is created to a server that's known to use
Kerberos. */
-apr_status_t
+static apr_status_t
serf__init_spnego_connection(const serf__authn_scheme_t *scheme,
int code,
serf_connection_t *conn,
@@ -412,7 +412,7 @@
}

/* A 40x response was received, handle the authentication. */
-apr_status_t
+static apr_status_t
serf__handle_spnego_auth(int code,
serf_request_t *request,
serf_bucket_t *response,
@@ -435,7 +435,7 @@
}

/* Setup the authn headers on this request message. */
-apr_status_t
+static apr_status_t
serf__setup_request_spnego_auth(peer_t peer,
int code,
serf_connection_t *conn,
@@ -648,4 +648,28 @@
return APR_SUCCESS;
}

+const serf__authn_scheme_t serf__spnego_authn_scheme = {
+ "Negotiate",
+ "negotiate",
+ SERF_AUTHN_NEGOTIATE,
+ serf__init_spnego,
+ serf__init_spnego_connection,
+ serf__handle_spnego_auth,
+ serf__setup_request_spnego_auth,
+ serf__validate_response_spnego_auth,
+};
+
+#ifdef WIN32
+const serf__authn_scheme_t serf__ntlm_authn_scheme = {
+ "NTLM",
+ "ntlm",
+ SERF_AUTHN_NTLM,
+ serf__init_spnego,
+ serf__init_spnego_connection,
+ serf__handle_spnego_auth,
+ serf__setup_request_spnego_auth,
+ serf__validate_response_spnego_auth,
+};
+#endif /* #ifdef WIN32 */
+
#endif /* SERF_HAVE_SPNEGO */

[Lieven Govaerts]

Hey Ivan,

On Wed, Apr 8, 2015 at 7:47 PM, <se...@googlecode.com> wrote:
> Revision: 2495
> Author: chemodax
> Date: Wed Apr 8 17:46:40 2015 UTC
> Log: More auth framework refactoring: move auth schemes vtable
> definitions
> to appropriate file with authn scheme implementation.
>

just checking, suppose that we branch serf 1.4.x next weekend, will
this refactoring work be finished?
Do you plan any major destabilising changes? Or moving code around to
improve readability/maintainability?

regards,

Lieven

> --
> You received this message because you are subscribed to the Google Groups
> "Serf Development List" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to serf-dev+u...@googlegroups.com.
> To post to this group, send email to serf...@googlegroups.com.
> Visit this group at http://groups.google.com/group/serf-dev.
> For more options, visit https://groups.google.com/d/optout.

[Ivan Zhakov]

On 8 April 2015 at 20:52, Lieven Govaerts <l...@mobsol.be> wrote:
>
> Hey Ivan,
>
> On Wed, Apr 8, 2015 at 7:47 PM, <se...@googlecode.com> wrote:
> > Revision: 2495
> > Author: chemodax
> > Date: Wed Apr 8 17:46:40 2015 UTC
> > Log: More auth framework refactoring: move auth schemes vtable
> > definitions
> > to appropriate file with authn scheme implementation.
> >
>
> just checking, suppose that we branch serf 1.4.x next weekend, will
> this refactoring work be finished?
> Do you plan any major destabilising changes? Or moving code around to
> improve readability/maintainability?
>

Hi Lieven,

I keep in mind potential serf 1.4.x branch. These commits are just
moving code around to improve readability. They don't change behavior
in any way.

I've some ideas with improving auth framework, but I'm going to try
them in branch especially until serf 1.4.x branch.

Does it work for you?

PS: I found some nasty bugs with simulations authentication across
different connection, but didn't file issue yet :(

--
Ivan Zhakov

[Lieven Govaerts]

On Wed, Apr 8, 2015 at 8:09 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 8 April 2015 at 20:52, Lieven Govaerts <l...@mobsol.be> wrote:
>>
>> Hey Ivan,
>>
>> On Wed, Apr 8, 2015 at 7:47 PM,  <se...@googlecode.com> wrote:
>> > Revision: 2495
>> > Author:   chemodax
>> > Date:     Wed Apr  8 17:46:40 2015 UTC
>> > Log:      More auth framework refactoring: move auth schemes vtable
>> > definitions
>> > to appropriate file with authn scheme implementation.
>> >
>>
>> just checking, suppose that we branch serf 1.4.x next weekend, will
>> this refactoring work be finished?
>> Do you plan any major destabilising changes? Or moving code around to
>> improve readability/maintainability?
>>
> Hi Lieven,
>
> I keep in mind potential serf 1.4.x branch. These commits are just
> moving code around to improve readability. They don't change behavior
> in any way.

So no problem taking them in 1.4.0 then.

> I've some ideas with improving auth framework, but I'm going to try
> them in branch especially until serf 1.4.x branch.
>
> Does it work for you?

Sure, or you can them on trunk and I'll remove them from the 1.4.x branch if these changes aren't finished yet.

> PS: I found some nasty bugs with simulations authentication across
> different connection, but didn't file issue yet :(

Ouch. I guess fixes won't require api changes? So they can be released in 1.4.1 ?

Lieven
> --
> Ivan Zhakov

[Ivan Zhakov]

I don't think these fixes require API changes, but may require
significant rewrite of auth framework internals: some per-host state
should be made per-connection. I want to refactor code first, add new
tests and then start making real changes.

--
Ivan Zhakov

[Greg Stein]

On Thu, Apr 9, 2015 at 3:26 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:

>...

I don't think these fixes require API changes, but may require

significant rewrite of auth framework internals: some per-host state
should be made per-connection. I want to refactor code first, add new
tests and then start making real changes.

Authentication is supposed to be per-request. The people who designed NTLM auth mucked that up and went per-connection. But:

If you're talking about the cached credential data, then that should remain per-host. I don't see why/how that should be per-connection. ? ... credential data is based on host, path, and realm. Nothing so transient as connections.

And that definitely doesn't seem like something to squeeze in for the 1.4 release.

Cheers,

-g

[Ivan Zhakov]

On 9 April 2015 at 11:32, Greg Stein <gst...@gmail.com> wrote:
> On Thu, Apr 9, 2015 at 3:26 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>...
>>
>> I don't think these fixes require API changes, but may require
>> significant rewrite of auth framework internals: some per-host state
>> should be made per-connection. I want to refactor code first, add new
>> tests and then start making real changes.
>
>
> Authentication is supposed to be per-request. The people who designed NTLM
> auth mucked that up and went per-connection. But:
>
> If you're talking about the cached credential data, then that should remain
> per-host.

I'm talking about other auth related data that should be stored
per-connection in some cases: like active authenticate scheme we
challenging over connection. NTLM/Negotiate also need some
per-connection state. I agree that this is contradicts with stateless
HTTP principle, but this how NTLM/Negotiate works.

> I don't see why/how that should be per-connection. ? ...
> credential data is based on host, path, and realm. Nothing so transient as
> connections.

Credential data will remain per-host of course.

--
Ivan Zhakov