mod_auth_gssapi: implement basic auth forwarding #151

profihost · 2017-09-19T14:17:27Z

This one implements basic auth forwarding as a fallback if tgt does not work.

(cherry picked from commit d4b190c749fcedf09354322fc8cb5922dfdf5bf6)

simo5 · 2017-09-26T15:12:40Z

Sorry for some reason I failed to see this one when the PR was created, I'll review asap

simo5

I think allowing to fall back to other basic auth providers may be an ok idea, however please change the way this is configured as mentioned in inline comments.
Also you need to add documentation about the new option.
Finally it doesn't look like you ever try the internal auth if forward is set, is this intentional ? If so why ?

simo5 · 2017-09-26T16:11:41Z

src/mod_auth_gssapi.c

@@ -835,6 +835,51 @@ static gss_OID_set mag_get_negotiate_mechs(apr_pool_t *p, gss_OID_set desired)
    }
 }

+
+static const char *add_authn_provider(cmd_parms *cmd, void *config,


please prefix with mag_

also move this function below, among the functions that deal with configuration

This function was copy and pasted from the apache modules/aaa/mod_auth_basic.c file. The idea was to keep it 1:1 as the original function.

Currently each mod_auth_ module has implemented this function:
git grep add_authn_provider
modules/aaa/mod_auth_basic.c:static const char *add_authn_provider(cmd_parms *cmd, void *config,
modules/aaa/mod_auth_basic.c: AP_INIT_ITERATE("AuthBasicProvider", add_authn_provider, NULL, OR_AUTHCFG,
modules/aaa/mod_auth_digest.c:static const char *add_authn_provider(cmd_parms *cmd, void *config,
modules/aaa/mod_auth_digest.c: AP_INIT_ITERATE("AuthDigestProvider", add_authn_provider, NULL, OR_AUTHCFG,
modules/aaa/mod_auth_form.c:static const char *add_authn_provider(cmd_parms * cmd, void *config,
modules/aaa/mod_auth_form.c: AP_INIT_ITERATE("AuthFormProvider", add_authn_provider, NULL, OR_AUTHCFG,

simo5 · 2017-09-26T16:12:59Z

src/mod_auth_gssapi.c

+    if (!conf->providers) {
+        conf->providers = newp;
+    }   
+    else {


} else {

please

copied from the original code - so you use a different coding style than upstream apache?

Replying to just this comment, but please read it as a reply to several of yours: yes we have our own coding style, and yes it doesn't match upstream apache. It is much closer to the krb5 and freeipa style for reasons you can probably imagine.

In general, consistency within the project (i.e., within m_a_g) is paramount, since it makes maintaining the codebase so much easier.

See @frozencemetery's comment, if there is some value in keeping the original it would have to be stored in it's own file, with prominent headers that state exactly where it comes from and what is the license on that piece of code (asuming it is compatible, otherwise we cannot keep a copy) so that people can keep it up to date according to the original source.

It would still require a wrapper function named mag_something in that case as you would have to make the funciton public and we definitelyt do not allow non-namespaced function as public.

simo5 · 2017-09-26T16:15:29Z

src/mod_auth_gssapi.c

+        conf->providers = newp;
+    }   
+    else {
+        authn_provider_list *last = conf->providers;


use for loop here please:
/* append to the end */
for (authn_provider_list *last = conf->providers; last->next; last = last->next);
last->next = newp;

same here: "copied from the original code"

simo5 · 2017-09-26T16:20:10Z

src/mod_auth_gssapi.c

-                return OK;
+        if (apr_table_get(main_req->notes, "AUTH_BASIC_FORWARD") != NULL) {
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0,
+                                  req, "auth_type: %s user: %s main req used auth basic.", type, main_req->user);


This reflow nests too much add a variable to skip subreq handling like:

if (apr_table_get(main ....) {
ap_log_rerror(....)
skip_subreq = true;
}
if ((!skip_subreq) && (type != NULL) && (strcasecmp(type, "GSSAPI") == 0)) {

OK will do so

simo5 · 2017-09-26T16:22:44Z

src/mod_auth_gssapi.c

@@ -1043,6 +1095,59 @@ static int mag_auth(request_rec *req)
        ba_user.length = strlen(ba_user.value);
        ba_pwd.length = strlen(ba_pwd.value);

+        if (cfg->use_basic_auth == BA_FORWARD) {
+            authn_provider_list *current_provider;


call this 'provider', initialize it to NULL

simo5 · 2017-09-26T16:24:28Z

src/mod_auth_gssapi.c

+                }
+
+                /* If we're not really configured for providers, stop now. */
+                if (!cfg->providers) {


too late to check this here, you already segfaulted above.

simo5 · 2017-09-26T16:26:03Z

src/mod_auth_gssapi.c

+
+            current_provider = cfg->providers;
+            do {
+                const authn_provider *provider;


Do not use this variable, you will use provider->provider once you rename as indicated above.

simo5 · 2017-09-26T18:47:05Z

src/mod_auth_gssapi.c

 {
    struct mag_config *cfg = (struct mag_config *)mconfig;

-    cfg->use_basic_auth = on ? true : false;
+    if (strcasecmp(value, "on") == 0) {


This is not backwards compatible, you would need to handle all ways to "turn on" a boolean value, or existing configurations may break.
Rather than changing the meaning of GssapiBasicAuth, I would use the presence of basic auth providers to decide if forwarding should be done.

OK i wasn't sure if this is a good idea and may break existing installations where gssapi suddenly starts forwarding auth.

simo5 · 2017-09-26T18:47:30Z

src/mod_auth_gssapi.c

@@ -1821,6 +1933,8 @@ static void *mag_create_server_config(apr_pool_t *p, server_rec *s)
 }

 static const command_rec mag_commands[] = {
+    AP_INIT_ITERATE("AuthBasicProvider", add_authn_provider, NULL, OR_AUTHCFG,


change to GssapiBasicAuthProviders

Oh I see this is an actual variable that is implemented for mod_auth_basic ... is this the canonical way to handle it, parsing it in each module that wants to deelgate basic auth to another provider ?

Not sure but as mounted out above all original modules implement add_authn_provider and use a specific one. Also ldap itself needs this variable.

simo5 · 2017-09-26T18:48:16Z

src/mod_auth_gssapi.h

+        BA_OFF = 0,
+        BA_FORWARD = 1,
+        BA_ON = 2
+    } use_basic_auth;


drop this enum and just use the providers list below to decide whether forwarding will be done or not.

frozencemetery · 2017-11-06T22:26:15Z

@profihost, still around?

profihost · 2017-11-07T11:18:37Z

Yes, sorry just don't have any spare time these weeks.

simo5 · 2019-03-11T18:32:56Z

Seem this PR got abandoned, I am closing for now, please feel free to reopen (or maybe start with a new one) once you can get back to it.

mod_auth_gssapi: implement basic auth forwarding

e1c68e6

(cherry picked from commit d4b190c749fcedf09354322fc8cb5922dfdf5bf6)

simo5 requested changes Sep 26, 2017

View reviewed changes

simo5 mentioned this pull request Oct 3, 2017

Login form and OTP integration #104

Open

frozencemetery added the needs revision label Nov 2, 2017

simo5 closed this Mar 11, 2019

mod_auth_gssapi: implement basic auth forwarding #151

mod_auth_gssapi: implement basic auth forwarding #151

Uh oh!

Conversation

profihost commented Sep 19, 2017

Uh oh!

simo5 commented Sep 26, 2017

Uh oh!

simo5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frozencemetery Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simo5 Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frozencemetery commented Nov 6, 2017

Uh oh!

profihost commented Nov 7, 2017

Uh oh!

simo5 commented Mar 11, 2019

Uh oh!

Uh oh!

frozencemetery Oct 23, 2017 •

edited

Loading

simo5 Sep 26, 2017 •

edited

Loading