ma1uta / ma1sd

Federated Matrix Identity Server (formerly fork of kamax/mxisd)
GNU Affero General Public License v3.0
167 stars 56 forks source link

Enabling exec for autentication causes null pointer log messages #19

Open the-moog opened 4 years ago

the-moog commented 4 years ago

I wrote a little script to hook exec to Django for authentication. I put the following config line in ma1uta.yaml

exec:
  enabled: true  
  auth:  
    enabled: true
    command: '/etc/ma1sd/wagtail_auth.sh'
    args: ['{localpart}']
    input:
      type: 'plain'
      template: '{password}'

This caused a flurry of null pointer exceptions

I found that adding

  directory:
    enabled: false
  identity:
    enabled: false
  profile:
    enabled: false

Fixed it.

Conclusion: Enabling a backend enables the sql queries for API aspects of that backend for which there are no usefull defaults or valid config. If there is no useful default or valid config, then perhaps default value for enabled parameter should be false. I think this same issue exists for other backends.

For completeness (as a useful wiki entry for others) here is the auth script. in /etc/ma1sd/wagtail_auth.sh

#!/bin/bash
. /opt/site/pyenv/bin/activate
cd /opt/site

RES=$(./manage.py authenticate $1 <&0)

if [[ "$RES" == "OK" ]]; then
  exit 0
else
  exit 1
fi

And it's python couterpart in /opt/site/app/base/management/commands

from django.core.management.base import BaseCommand, CommandError
from django.contrib.auth.models import User
from django.contrib.auth.hashers import check_password
import sys
from select import select

class Command(BaseCommand):
    help = "Command line verification of user password"

    def add_arguments(self, parser):
        parser.add_argument('username', type=str)
        parser.add_argument('password', type=str, nargs='?', default='')

    def handle(self, *args, **options):
        username = options['username']
        password = options['password']

        if not len(password):
            # Try STDIN
            rlist, _, _ = select([sys.stdin], [], [], 2)
            if rlist:
                password = sys.stdin.readline()

        if not len(password):
            res = False
            # raise CommandError('No password supplied')

        try:
            user = User.objects.get(username__iexact=username)
        except User.DoesNotExist:
            res = False
            # raise CommandError('User %s does not exist' % username)
        else:
            res = check_password(password.strip(), user.password)

        self.stdout.write("OK" if res else "BAD")
the-moog commented 4 years ago

I just did a build of the latest code, with the --dump option. VERY interesting and informative lots of undocuments options. One suggestion, make the --dump option not exit so that the current config enters the log.

the-moog commented 4 years ago

I notice that in the dump file (for my config, mostly from mxisd, anyway) there are a worring number of NULL values, should I be worried about that?

ma1uta commented 4 years ago

No, you shouldn't worry about that. In the case when some feature is disabled then feature's configuration can be null.

the-moog commented 4 years ago

What about the issue pf enabling one feature implicitly enabling ones that are not configured?

ma1uta commented 4 years ago

I haven't found that functions. There are two cases: first, explicitly turn on feature and configure it (or use some defaults parameters), and second, when feature disabled by default or configuration.

For example for authentication it is possible use sql adapter (plain sql or synapse sql), ldap adapter or exec adapter. If you enable authentication then only configured adapters will be used. So if you configure only one exec adapter then only it will be used, sql and ldap adapters will be disabled.