xlat expansion of absent VPs
In processing a request that does not contain Called-Station-Id, I noticed that absent VPs now get expanded to an underscore instead of the usual empty string: xlat aprint 0 xlat aprint 1 xlat aprint 0 xlat aprint 3 expand attr Called-Station-Id --> '_' [...] (14) sql : expand: 'INSERT INTO radacct (acct_session_id, acct_unique_id, flow_id, username, realm, inner_id, nas_ip_address, acct_start_time, last_update, called_station_id, calling_station_id, bs_sector_id, framed_ip_address, acct_status) VALUES(NULLIF('%{Acct-Session-Id}', ''), NULLIF('%{Acct-Unique-Session-Id}', ''), COALESCE(NULLIF('%{WiMAX-PDFID}', ''), '0')::smallint, '%{SQL-User-Name}', '%{Realm}', '%{SQL-User-Name}' || '@' || '%{Realm}', NULLIF('%{NAS-IP-Address}', '')::inet, COALESCE(NULLIF('%{Event-Timestamp}', ''), '%S')::timestamptz, COALESCE(NULLIF('%{Event-Timestamp}', ''), '%S')::timestamptz, NULLIF('%{Called-Station-Id}', '')::inet, NULLIF('%{Calling-Station-Id}', '')::macaddr, COALESCE(parse_bssid(NULLIF('%{WiMAX-BS-Id}', '')), '000000000000'), COALESCE(NULLIF('%{Framed-IP-Address}', ''), '0.0.0.0')::inet, 1);' -> 'INSERT INTO radacct (acct_session_id, acct_unique_id, flow_id, username, realm, inner_id, nas_ip_address, acct_start_time, last_update, called_station_id, calling_station_id, bs_sector_id, framed_ip_address, acct_status) VALUES(NULLIF('13031C4B', ''), NULLIF('ebc3aef1f75ee8853b33a08e42dd364f', ''), COALESCE(NULLIF('0', ''), '0')::smallint, 'test', 'fr3', 'test' || '@' || 'fr3', NULLIF('172.20.10.121', '')::inet, COALESCE(NULLIF('Jun 15 2013 16:32:46 CEST', ''), '2013-06-15 16:30:54')::timestamptz, COALESCE(NULLIF('Jun 15 2013 16:32:46 CEST', ''), '2013-06-15 16:30:54')::timestamptz, NULLIF('_', '')::inet, NULLIF('0C4C399C58F7', '')::macaddr, COALESCE(parse_bssid(NULLIF('0x303030303230303630313030', '')), '000000000000'), COALESCE(NULLIF('109.236.16.222', ''), '0.0.0.0')::inet, 1);' rlm_sql (sql): Executing query: 'INSERT INTO radacct (acct_session_id, acct_unique_id, flow_id, username, realm, inner_id, nas_ip_address, acct_start_time, last_update, called_station_id, calling_station_id, bs_sector_id, framed_ip_address, acct_status) VALUES(NULLIF('13031C4B', ''), NULLIF('ebc3aef1f75ee8853b33a08e42dd364f', ''), COALESCE(NULLIF('0', ''), '0')::smallint, 'test', 'fr3', 'test' || '@' || 'fr3', NULLIF('172.20.10.121', '')::inet, COALESCE(NULLIF('Jun 15 2013 16:32:46 CEST', ''), '2013-06-15 16:30:54')::timestamptz, COALESCE(NULLIF('Jun 15 2013 16:32:46 CEST', ''), '2013-06-15 16:30:54')::timestamptz, NULLIF('_', '')::inet, NULLIF('0C4C399C58F7', '')::macaddr, COALESCE(parse_bssid(NULLIF('0x303030303230303630313030', '')), '000000000000'), COALESCE(NULLIF('109.236.16.222', ''), '0.0.0.0')::inet, 1);' rlm_sql_postgresql: Status: PGRES_FATAL_ERROR rlm_sql_postgresql: Error invalid input syntax for type inet: "_" rlm_sql_postgresql: Postgresql Fatal Error: [22P02: INVALID TEXT REPRESENTATION] Occurred!! rlm_sql (sql): Database query error: ERROR: invalid input syntax for type inet: "_" Same thing with a request that doesn't contain User-Name: rad_recv: Status-Server packet from host 172.20.23.31 port 1814, id=27, length=70 Message-Authenticator = 0x6ec50145d350c17813778026ac2f2a4a NAS-Identifier = "Status Check 0. Are you alive?" (18) # Executing section post-auth from file /etc/raddb/sites-enabled/default (18) group post-auth { (18) - entering group post-auth {...} (18) [exec] = noop (18) update reply { LITERAL: %{User-Name} LITERAL: %{User-Name} --> %{User-Name} EXPANSION: %{User-Name} Looking for attribute name in User-Name xlat aprint 3 expand attr User-Name --> '_' (18) expand: '%{User-Name}' -> '_' (18) WiMAX-MN-NAI := "_" LITERAL: %{md5:%{User-Name}} LITERAL: %{md5:%{User-Name}} --> %{md5:%{User-Name}} EXPANSION: %{md5:%{User-Name}} MOD: md5 --> LITERAL: %{User-Name}} LITERAL: %{User-Name}} --> %{User-Name}} EXPANSION: %{User-Name}} Looking for attribute name in User-Name LITERAL: } xlat aprint 2 xlat aprint 3 expand attr User-Name --> '_' expand mod md5 --> '_' -- Stelian Ionescu a.k.a. fe[nl]ix Quidquid latine dictum sit, altum videtur. http://common-lisp.net/project/iolib
Stelian Ionescu wrote:
In processing a request that does not contain Called-Station-Id, I noticed that absent VPs now get expanded to an underscore instead of the usual empty string:
That's how it was in 2.x. Or at least how the code was *supposed* to work. The idea is that a place-holder is better than silently expanding to an empty string. An empty string means that data which is supposed to be 4 columns (for example) is now suddenly 3. That's bad. If you want to force an empty string, use: %{%{Attribute-Name}:-} Which will do what you want. Alan DeKok.
On Mon, 2013-06-17 at 10:53 -0400, Alan DeKok wrote:
Stelian Ionescu wrote:
In processing a request that does not contain Called-Station-Id, I noticed that absent VPs now get expanded to an underscore instead of the usual empty string:
That's how it was in 2.x. Or at least how the code was *supposed* to work.
That's definitely not how it worked in 2.x, and as for what was supposed to happen, it's too late to argue about it now.
The idea is that a place-holder is better than silently expanding to an empty string. An empty string means that data which is supposed to be 4 columns (for example) is now suddenly 3. That's bad.
That's not true, at least not in a SQL statement. SELECT %{FOO}, %{BAR}, %{BAZ} that may expand to «SELECT a, , c» will raise a syntax error. I can't think of a case where the deletion of a token will result in a valid SQL expression. -- Stelian Ionescu a.k.a. fe[nl]ix Quidquid latine dictum sit, altum videtur. http://common-lisp.net/project/iolib
On 17 Jun 2013, at 16:27, Stelian Ionescu <sionescu@cddr.org> wrote:
On Mon, 2013-06-17 at 10:53 -0400, Alan DeKok wrote:
Stelian Ionescu wrote:
In processing a request that does not contain Called-Station-Id, I noticed that absent VPs now get expanded to an underscore instead of the usual empty string:
That's how it was in 2.x. Or at least how the code was *supposed* to work.
That's definitely not how it worked in 2.x, and as for what was supposed to happen, it's too late to argue about it now.
The idea is that a place-holder is better than silently expanding to an empty string. An empty string means that data which is supposed to be 4 columns (for example) is now suddenly 3. That's bad.
No, because all values should be quoted (even integers and timestamps) and a "" or a '' is perfectly acceptable in the SQL world. If users have an issue with it they can do %{%{FOO}:-_}, inserting _ by default is magical, inconsistent and unexpected. Besides, for SQL the proper behaviour would be %{%{FOO}:-NULL}, NULL being a value which is not known, anything else is arguably wrong.
That's not true, at least not in a SQL statement. SELECT %{FOO}, %{BAR}, %{BAZ} that may expand to «SELECT a, , c» will raise a syntax error. I can't think of a case where the deletion of a token will result in a valid SQL expression.
I have to agree. If an attribute doesn't exist then it should expand to "", that's the behaviour i'm used to too. Previously the xlat code wrongly treated zero length expansions as errors, this might of been the original reason for returning a one char expansion. I think some signatures/functions still need updating (namely those of actual xlat functions), but other xlat operations should now return -1 to indicate an error. I certainly updated all calls to radius_xlat and radius_axlat to check for < 0 return codes. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
Arran Cudbard-Bell wrote:
I have to agree. If an attribute doesn't exist then it should expand to "", that's the behaviour i'm used to too.
For me, it's a major security issue. Silently missing an attribute is bad.
Previously the xlat code wrongly treated zero length expansions as errors, this might of been the original reason for returning a one char expansion.
No, the zero-length expansion issue was a temporary problem during the re-write of the new code. Alan DeKok.
On 17 Jun 2013, at 19:19, Alan DeKok <aland@deployingradius.com> wrote:
Arran Cudbard-Bell wrote:
I have to agree. If an attribute doesn't exist then it should expand to "", that's the behaviour i'm used to too.
For me, it's a major security issue. Silently missing an attribute is bad.
I guess for things like command line arguments, yes, it would be. If it's a security issue arguably the entire expansion should fail, and we should force the user to deal with cases where an attribute may be NULL by specifying alternative static values. In a rather contrived case you could have someone using a shell script to verify an unsupported type of hash. They pass /bin/mysuperhash %{sql:SELECT user FROM users WHERE username='%{User-Name}'} %{Hashed-Password-Attribute} Username isn't found. /bin/mysuperhash _ 0x1111111111 Anywhere that wants to expand config:Cleartext-Password would have the same issue. If it's not going to be null string it shouldn't be anything else either. I don't see this as a usability issue because in the majority of cases an '_' isn't going to be what the admin wanted either.
Previously the xlat code wrongly treated zero length expansions as errors, this might of been the original reason for returning a one char expansion.
No, the zero-length expansion issue was a temporary problem during the re-write of the new code.
No, in a bunch of places radius_xlat()! was treated as an error. -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
Arran Cudbard-Bell wrote:
I guess for things like command line arguments, yes, it would be.
If it's a security issue arguably the entire expansion should fail, and we should force the user to deal with cases where an attribute may be NULL by specifying alternative static values.
Perhaps. It's a balance of usability and security.
I don't see this as a usability issue because in the majority of cases an '_' isn't going to be what the admin wanted either.
It's at least a "known wrong" value.
No, in a bunch of places radius_xlat()! was treated as an error.
Ouch. Alan DeKok.
On Mon, Jun 17, 2013 at 02:19:02PM -0400, Alan DeKok wrote:
Arran Cudbard-Bell wrote:
I have to agree. If an attribute doesn't exist then it should expand to "", that's the behaviour i'm used to too.
For me, it's a major security issue. Silently missing an attribute is bad.
Having empty attributes replaced by "_" could be incredibly annoying depending on the circumstance. But then I hope I'd also have the sense to check what I'm doing to make sure that if an attribute did end up empty it was handled correctly. And what happens if an attribute actually has the value "_"? Unlikely I guess, but possible? Couldn't it just be configurable? Something just like xlat_empty_attribute = "_" for default, but could be set to any string? Gives some possible protection by default, but can be overridden if needed in a global way. Cheers Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Specialist, Infrastructure Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
On 17 Jun 2013, at 23:40, Matthew Newton <mcn4@leicester.ac.uk> wrote:
On Mon, Jun 17, 2013 at 02:19:02PM -0400, Alan DeKok wrote:
Arran Cudbard-Bell wrote:
I have to agree. If an attribute doesn't exist then it should expand to "", that's the behaviour i'm used to too.
For me, it's a major security issue. Silently missing an attribute is bad.
Having empty attributes replaced by "_" could be incredibly annoying depending on the circumstance. But then I hope I'd also have the sense to check what I'm doing to make sure that if an attribute did end up empty it was handled correctly.
And what happens if an attribute actually has the value "_"? Unlikely I guess, but possible?
Couldn't it just be configurable? Something just like
xlat_empty_attribute = "_"
for default, but could be set to any string? Gives some possible protection by default, but can be overridden if needed in a global way.
There are only a few places where this is useful for preventing security issues (like rlm_exec and exec.). Other places it's unnecessary and an annoyance. I don't see there's value in setting it globally, but I do see value is specifying it on a call by call basis so that appropriate values can be used for the string being built. -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
Matthew Newton wrote:
Having empty attributes replaced by "_" could be incredibly annoying depending on the circumstance. But then I hope I'd also have the sense to check what I'm doing to make sure that if an attribute did end up empty it was handled correctly.
Not everyone does that. I think being fail-safe is best.
And what happens if an attribute actually has the value "_"? Unlikely I guess, but possible?
It's possible.
Couldn't it just be configurable? Something just like
xlat_empty_attribute = "_"
for default, but could be set to any string? Gives some possible protection by default, but can be overridden if needed in a global way.
The alternative is to fail the expansion entirely, and return an error. That is MUCH preferable to silently expanding to an empty string. I'll let people here decide. Alan DeKok.
On 06/18/2013 02:13 AM, Alan DeKok wrote:
The alternative is to fail the expansion entirely, and return an error.
That is MUCH preferable to silently expanding to an empty string.
I don't understand what the problem with that is; can you elaborate on it for me? In "exec" situations, FreeRADIUS has split the string into argv before it does each xlat, so you don't get a missing argument - you get an empty one, which is legal. In SQL, you get an escaped string surrounded by ''. Sure, if you omit the '' then you're in trouble, but nothing can stop that sort of brokenness (well, SQL param binding, but we don't have that) I don't really like the idea of expanding to a special/magic value, or of aborting on a missing value. Couldn't we have: %{notempty:%{Var}} ...which fails the entire xlat somehow, for variables which *must* be present?
Phil Mayers wrote:
I don't understand what the problem with that is; can you elaborate on it for me?
In general, silently omitting expected values is bad. It means that the string being printed doesn't have the number of fields you want.
In "exec" situations, FreeRADIUS has split the string into argv before it does each xlat, so you don't get a missing argument - you get an empty one, which is legal.
It works there.
In SQL, you get an escaped string surrounded by ''. Sure, if you omit the '' then you're in trouble, but nothing can stop that sort of brokenness (well, SQL param binding, but we don't have that)
It works there.
I don't really like the idea of expanding to a special/magic value, or of aborting on a missing value.
It's safer than (say) the linelog module suddenly printing 4 columns, when you expect 5. That's the worry. Expanding to an empty string *sometimes* works. But in *general*, it's not safe. No one complained in v2, but maybe it's just that no one noticed.
Couldn't we have:
%{notempty:%{Var}}
...which fails the entire xlat somehow, for variables which *must* be present?
Maybe. I'll take a look. For accounting, it seems that the better approach would be to use filtering in the update sections: update request { Acct-Input-Gigawords >= 0 Acct-Output-Gigawords >= 0 ... } So that if attributes are omitted, that "update" section adds them with "0" values. The later code (SQL, etc.) then no longer needs to check: the attributes are always there. Alan DeKok.
On 17/06/2013 16:44, Arran Cudbard-Bell wrote:
The idea is that a place-holder is better than silently expanding to an empty string. An empty string means that data which is supposed to be 4 columns (for example) is now suddenly 3. That's bad. No, because all values should be quoted (even integers and timestamps) and a "" or a '' is perfectly acceptable in the SQL world.
Aside: I think Postgres will reject a quoted string being used in an integer context.
If users have an issue with it they can do %{%{FOO}:-_}, inserting _ by default is magical, inconsistent and unexpected.
Besides, for SQL the proper behaviour would be %{%{FOO}:-NULL}, NULL being a value which is not known, anything else is arguably wrong. Yes, that is the correct way to insert an integer attribute which may or may not be present.
This is easy for integers but not easy for strings, where in theory you want a quoted value if it's present but an unquoted NULL if it is not. In practice, having to deal with NULL and empty string as distinct values is usually a pain, so I always write the empty string for missing attributes. And Oracle forces empty string to NULL in a varchar column. Regards, Brian.
On 18 Jun 2013, at 11:07, Phil Mayers <p.mayers@IMPERIAL.AC.UK> wrote:
On 06/18/2013 07:27 AM, Brian Candler wrote:
Aside: I think Postgres will reject a quoted string being used in an integer context.
Not correct:
db=> create temp table tt (val integer); CREATE TABLE db=> insert into tt values ('1'); INSERT 0 1
Will it converted quoted NULLs 'NULL' to actual NULL values? I'm guessing no? Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 18/06/13 12:08, Arran Cudbard-Bell wrote:
On 18 Jun 2013, at 11:07, Phil Mayers <p.mayers@IMPERIAL.AC.UK> wrote:
On 06/18/2013 07:27 AM, Brian Candler wrote:
Aside: I think Postgres will reject a quoted string being used in an integer context.
Not correct:
db=> create temp table tt (val integer); CREATE TABLE db=> insert into tt values ('1'); INSERT 0 1
Will it converted quoted NULLs 'NULL' to actual NULL values? I'm guessing no?
No. That's what NULLIF is for, and it's widely used for that. TBH we're running into the limitations of using variable interpolation for SQL here; the "right" way to do this would be parameter binding and passing typed data into the SQL drivers, but that's a lot of work for only a very small incremental gain. It's very, very common in SQL for writers of queries to have to use COALESCE, NULLIF, CASE, type coercion and other features to ensure proper data values (or for the SQL DB to infer them). As far as I'm concerned, the *only* thing FreeRADIUS needs to do is properly quote the variables it expands into a SQL query, which it already does.
On 18 Jun 2013, at 12:31, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
On 18/06/13 12:08, Arran Cudbard-Bell wrote:
On 18 Jun 2013, at 11:07, Phil Mayers <p.mayers@IMPERIAL.AC.UK> wrote:
On 06/18/2013 07:27 AM, Brian Candler wrote:
Aside: I think Postgres will reject a quoted string being used in an integer context.
Not correct:
db=> create temp table tt (val integer); CREATE TABLE db=> insert into tt values ('1'); INSERT 0 1
Will it converted quoted NULLs 'NULL' to actual NULL values? I'm guessing no?
No. That's what NULLIF is for, and it's widely used for that.
TBH we're running into the limitations of using variable interpolation for SQL here; the "right" way to do this would be parameter binding and passing typed data into the SQL drivers, but that's a lot of work for only a very small incremental gain.
We discussed that, and it seems like it could be done if we added callbacks for xlat parsing during xlat registration. Then all the pre-prepared statements could be created during startup. The key unknowns with this, is whether a connection is required to the server in order to create the bound statements, and whether it's ok to share them between multiple handles.
It's very, very common in SQL for writers of queries to have to use COALESCE, NULLIF, CASE, type coercion and other features to ensure proper data values (or for the SQL DB to infer them).
OK.
As far as I'm concerned, the *only* thing FreeRADIUS needs to do is properly quote the variables it expands into a SQL query, which it already does.
OK. Then the default substitution should probably only be done for certain modules where it is an issue. On the command line blank arguments do affect programs which use positional arguments as multiple chars of whitespace are condensed. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 18/06/13 13:02, Arran Cudbard-Bell wrote:
We discussed that, and it seems like it could be done if we added callbacks for xlat parsing during xlat registration. Then all the pre-prepared statements could be created during startup.
The key unknowns with this, is whether a connection is required to the server in order to create the bound statements, and whether it's ok to share them between multiple handles.
If I understand what you mean here - things like PREPARE - those sort of server-side statement/param solutions have caveats on some databases. I would not recommend them for this use-case, certainly not in postgres. And yes, in postgres they are per-connection and cannot be shared. [As an example - PREPAREd statements in postgres have different query planning behaviour than "long form" SQL queries, so their performance can vary over the lifetime of the connection even for the same parameters. Even worse, older versions of postgres might always use a generic query plan for PREPAREd statements.] In general, PREPARE is a niche solution. If you want missing attributes to expand to the unquoted null keyword, then I think you'll have to drill down into the xlat to determine whether the attribute is absent or empty. Personally I don't think even that would be desirable - it means string expansions in SQL will work different to the rest of the server. I'd just leave it at "use NULLIF".
As far as I'm concerned, the *only* thing FreeRADIUS needs to do is properly quote the variables it expands into a SQL query, which it already does.
OK. Then the default substitution should probably only be done for certain modules where it is an issue.
I think that would be confusing.
On the command line blank arguments do affect programs which use positional arguments as multiple chars of whitespace are condensed.
You've lost me. On the command line, that's done by the shell, and if people using shell want to preserve whitespace, they must quote appropriately. Conversely FreeRADIUS splits into argv[] before doing xlat, so the actual value of an attribute - whitespace and all - is passed to the execv call. I consider this a good thing - this shell command: /tmp/script -a foo -b '' -c ' bar ' ...is perfectly legal, and not even that uncommon.
As far as I'm concerned, the *only* thing FreeRADIUS needs to do is properly quote the variables it expands into a SQL query, which it already does.
OK. Then the default substitution should probably only be done for certain modules where it is an issue.
I think that would be confusing.
On the command line blank arguments do affect programs which use positional arguments as multiple chars of whitespace are condensed.
You've lost me. On the command line, that's done by the shell, and if people using shell want to preserve whitespace, they must quote appropriately.
Yes, isn't that the security issue were talking about here? Where people don't appropriately quote xlat expansions and then something expands to a zero length string? At least in the context of exec. If it's known that attributes are sometimes not included in the Access-Requests, then they can be marked as optional with %{%{Attr}:-}, if they're not marked as optional then the server is dealing with RADIUS packets the policy/server configuration was not designed to process, and should fail closed. It should not invent values. The RADIUS RFCs only specify a framework for the protocol, the actual protocol is established on an implementation by implementation basis, and is made up of the RADIUS framework and set of known attributes. If the attributes change then the protocol has changed and the server should fail closed. One way to make dealing with this easier is to hack the filter module to verify a minimum set of attributes is present in the request before it goes any further. You'd still need to explicitly mark optional attributes elsewhere, but it'd make debugging easier, and for some attributes you could set default values within the filter. -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 18/06/13 23:56, Arran Cudbard-Bell wrote:
As far as I'm concerned, the *only* thing FreeRADIUS needs to do is properly quote the variables it expands into a SQL query, which it already does.
OK. Then the default substitution should probably only be done for certain modules where it is an issue.
I think that would be confusing.
On the command line blank arguments do affect programs which use positional arguments as multiple chars of whitespace are condensed.
You've lost me. On the command line, that's done by the shell, and if people using shell want to preserve whitespace, they must quote appropriately.
Yes, isn't that the security issue were talking about here? Where people don't appropriately quote xlat expansions and then something expands to a zero length string? At least in the context of exec.
Just to be clear, I don't think this is, or could be, a security issue. It simply generates an empty argument, which as I've said is both legal and not uncommon.
If it's known that attributes are sometimes not included in the Access-Requests, then they can be marked as optional with %{%{Attr}:-}, if they're not marked as optional then the server is dealing with RADIUS packets the policy/server configuration was not designed to process, and should fail closed. It should not invent values.
It's not inventing a value. It's defining a value to which the "absent" attribute expands in a string, in exactly the same way that it defines how attributes of type "octets" expand to 0xHEX or similarly for other types. You seem to be saying that the server should define "absent" attributes should convert the whole xlat into the empty string + error code. I disagree, but to be honest we're going round in circles here, and I don't think this discussion is contributing much. Do what you think best.
On 18/06/2013 11:07, Phil Mayers wrote:
On 06/18/2013 07:27 AM, Brian Candler wrote:
Aside: I think Postgres will reject a quoted string being used in an integer context.
Not correct:
db=> create temp table tt (val integer); CREATE TABLE db=> insert into tt values ('1'); INSERT 0 1
OK, thanks (I don't have a pg instance to hand). I was thinking of something slightly different I dug up a while back. With mysql: select 0 = 'RandomToken' returns 1 (true) postgres correctly rejects this.
participants (6)
-
Alan DeKok -
Arran Cudbard-Bell -
Brian Candler -
Matthew Newton -
Phil Mayers -
Stelian Ionescu