FreeRADIUS / freeradius-server

FreeRADIUS - A multi-protocol policy server.
http://freeradius.org
GNU General Public License v2.0
2.13k stars 1.08k forks source link

Multiple Python Modules #1845

Open sword2100 opened 8 years ago

sword2100 commented 8 years ago

Issue type

Defect description

Using SLES 12 SP1 with github source compiled version 3.0.13

radiusd: FreeRADIUS Version 3.0.13, for host x86_64-unknown-linux-gnu, built on Nov 14 2016 at 10:58:45
FreeRADIUS Version 3.0.13
Copyright (C) 1999-2016 The FreeRADIUS server project and contributors

Trying to use multiple python modules/files. I have created two python files:

Changed the needed lines at python module

python one{
        python_path="/usr/local/etc/raddb/mods-config/python/:/usr/lib64/python2.7/:/usr/lib64/python2.7/site-packages/:/usr/lib64/python2.7/lib-dynload/"
        module = one

        mod_authorize = ${.module}
        func_authorize = authorize
}

python two{
        python_path="/usr/local/etc/raddb/mods-config/python/:/usr/lib64/python2.7/:/usr/lib64/python2.7/site-packages/:/usr/lib64/python2.7/lib-dynload/"
        module = two

        mod_authorize = ${.module}
        func_authorize = authorize
}

And added to the default server

Running FreeRadius with radius -XXX and get this message

Debug:   # Instantiating module "one" from file /usr/local/etc/raddb/mods-enabled/python
Info: Python version: 2.7.9 (default, Dec 21 2014, 11:02:59) [GCC]
Debug:   # Instantiating module "two" from file /usr/local/etc/raddb/mods-enabled/python
Error: python_function_load - Module 'two' not found
Error: <type 'exceptions.SystemError'> (null argument to internal routine)
Error: python_function_load - Failed to import python function 'two.authorize'
Error: /usr/local/etc/raddb/mods-enabled/python[27]: Instantiation failed for module "two"

It looks like for the second module the python_path is not working. I've already tried to export the PYTHONPATH="[as in module]" but same message.

herwinw commented 8 years ago

The code that sets the python path basicly looks like this:

path = talloc_strdup(NULL, inst->python_path);
PySys_SetPath(path);
talloc_free(path);

There is no reference to the actual interpreter that should be used here, so I guess this is a kind of global setting. I can't really find any information on how to set the path for a specific interpreter (which still would be flawed, because the cext_compat option allows you to reuse the main interpreter)

A possible option would to move the python path out of the instance config to a global setting, but this doesn't really fit well with the config format used in Freeradius.

Is it possible to set the Python path from within a script? In that case I guess the best solution here would be to encourage that, and eventually drop the python_path config option.

arr2036 commented 8 years ago

Yeah I remember it being global. Maybe just complain that it's already set?

arr2036 commented 8 years ago

...and yes Python really doesn't embed well.

herwinw commented 8 years ago

Yeah I remember it being global. Maybe just complain that it's already set?

That can still cause some unexpected behaviour. Imagine this config

python python_stable {
  module = blah
  ...
}
python python_testing {
  python_path = /somewhere/python/testing
  module = blah
 ....
}

This won't issue any complaints because we only set it once, but suddenly your python_stable call uses the testing libraries.

arr2036 commented 8 years ago

Refuse to start if it's not set to the same value across all instances.

m-blanke commented 7 years ago

I'll have a look whether or not the python3 module still has the same issue, as soon as I've fixed some other issues.

herwinw commented 7 years ago

Refuse to start if it's not set to the same value across all instances.

I'm afraid that's the only working option left. Then again, I would probably start cursing at my monitor when I would get that as a config error, it would just feel a bit retarded.

alandekok commented 7 years ago

I've pushed documentation to help. For now, I don't think it's worth fixing v3. This may be easier to fix in v4.

erindru commented 7 years ago

So right now, theres no way of instantiating multiple python modules in v3?

Thats a bugger - I have a throughput issue where I have used the python module to make an AMQP request to another service authenticate the user. However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

I was hoping to be able to instantiate multiple instances of the python module and use the redundant-load-balance option to load balance between them - will this be possible in v4? I tried it in 3.0.13 but I encountered the same issue as @sword2100

herwinw commented 7 years ago

In reply to @erindru

So right now, theres no way of instantiating multiple python modules in v3?

That's not what this case is about. It's perfectly possible possible to instantiate to have multiple Python modules, just as long as every module uses the same python_path. This looks like a limitation in Python itself, a few workarounds have been discussed here, but none of them really work.

I have a throughput issue where I have used the python module to make an AMQP request to another service

This kind of things might be easier with the rlm_rest module.

However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

Wouldn't just increasing the number of threads fix this (or is the python module not thread safe?)

arr2036 commented 7 years ago

@erindru You can instantiate multiple python modules but it won't improve performance because you're stuck with a single interpreter.

If you're really suffering from blocking the async rlm_rest module in v4.0.x allows many thousands of concurrent requests.

arr2036 commented 7 years ago

LDAP would benefit from a global configuration section as well.

@alandekok What do you think about having

global {
    ldap {
        debug_level = 0x000
    }

    python {
        path = "whatever"
    }
}

and an extra global instantiation callback in the modules?

There's just some stuff which doesn't belong in module instance configs and needs to be set globally.

alanbuxey commented 7 years ago

but I might only want debug to be higher on just one LDAP instance... could still have module specific override?

alan

On 27 April 2017 at 14:25, Arran Cudbard-Bell notifications@github.com wrote:

LDAP would benefit from a global configuration section as well.

@alandekok https://github.com/alandekok What do you think about having

global { ldap { debug_level = 0x000 }

python { path = "whatever" } }

and an extra instantiation callback in the modules?

There's just some stuff which doesn't belong in module instance configs and needs to be set globally.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/FreeRADIUS/freeradius-server/issues/1845#issuecomment-297712374, or mute the thread https://github.com/notifications/unsubscribe-auth/ACE-VSM5S-b3-AbWtxT-aQLiUf_Hqz9dks5r0JddgaJpZM4KxOdx .

alandekok commented 7 years ago

Templates already do that to a large extent, tho people don't use them a lot.

arr2036 commented 7 years ago

@alanbuxey no, and you can't today.

The point is there are some configuration items which map to library level settings, so they shouldn't really be set in module instances. They're global to the server.

alanbuxey commented 7 years ago

I do (use templates). When there are multiple servers in a failover pool and they share common settings they are a very powerful/useful setting :)

alan

On 27 Apr 2017 3:30 pm, "Alan DeKok" notifications@github.com wrote:

Templates already do that to a large extent, tho people don't use them a lot.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/FreeRADIUS/freeradius-server/issues/1845#issuecomment-297730456, or mute the thread https://github.com/notifications/unsubscribe-auth/ACE-VY55CN6rqKEkqyjszCWucWXRKd78ks5r0KZ1gaJpZM4KxOdx .

herwinw commented 7 years ago

Excerpt from eap:

tls-config tls-common {
  private_key_password = whatever
  ...
}

ttls {
  tls = tls-common
 ...
}

Excerpt from rest

tls {
  ca_file = ${certdir}/cacert.pem
  ...
}

authorize {
  tls = ${..tls}
  ...
}

These are just the first two things that came to my mind when thinking about having some global setting and a possibility to override them.

arr2036 commented 7 years ago

Offline discussion.

Decided on a library section which gets passed to the load callback in modules.