FreeRADIUS / freeradius-server

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

[defect]: Port time parsing from v4 to v3.2.x #5323

Open alandekok opened 5 months ago

alandekok commented 5 months ago

What type of defect/bug is this?

Unexpected behaviour (obvious or verified by project member)

How can the issue be reproduced?

In v4, src/lib/util/time.c has a function fr_unix_time_from_str(). This function mirrors the v3 function fr_get_time().

However, v4 has been updated to add a call to strptime(), which handles time zones. v3 is missing that.

We don't want to port the v4 "parse RFC3339 date" code. But we do want to be able to parse time zones.

Log output from the FreeRADIUS daemon

Config:

update request {
    Event-Timestamp := "May 16 2024 21:32:59 CEST"
}

which gives output:

(0)       update request {
(0)         Event-Timestamp := "May 16 2024 21:32:59 EDT"

Whoops. :( Time zone is ignored.



### Relevant log output from client utilities

_No response_

### Backtrace from LLDB or GDB

_No response_
rygl commented 2 months ago

Hi, any updates on that?

alandekok commented 2 months ago

@rygl We're happy to accept patches.

alandekok commented 2 months ago

Just looking at this now, v3 does call strptime() in src/lib/util/value.c. And the only call to the old fr_get_time() function is from that code in src/lib/util/value.c.

The call to strptime() was added in 2020. So if your system can't parse Event-Timestamp := "May 16 2024 21:32:59 EDT", then it's likely that you're running a very old version of the server.

rygl commented 2 months ago

I am affraid that not:

#freeradius -v
radiusd: FreeRADIUS Version 3.2.3, for host x86_64-pc-linux-gnu, built on Mar 24 2024 at 21:35:44
FreeRADIUS Version 3.2.3

The same issue is with 3.2.1. I am not sure if it makes any difference - the the issue is that update req. with: Event-Timestamp = "May 16 2024 21:32:59 CEST"

gives ouptut to detail file:

Event-Timestamp = "May 16 2024 22:32:59 CEST"

I am ready to provide more details or debugs if it helps.

alandekok commented 2 months ago

From the strptime() documentation on OSX:

     The %Z format specifier only accepts time zone abbreviations of the local time zone, or the value "GMT".  This
     limitation is because of ambiguity due to of the over loading of time zone abbreviations.  One such example is
     EST which is both Eastern Standard Time and Eastern Australia Summer Time.

I suspect the same is true on other systems. In short, parsing time zones is difficult to impossible.

If the time zone being parsed is the local time zone, then it will work. Otherwise, it's essentially an impossible problem to solve.

rygl commented 2 months ago

I am sorry, what you mean if you say "local time zone here"? My local tz is CEST, req. is coming from CEST... nevertheles I understand the problem with ambiguous TZ abreviations.

Is it worth to test freeRadius v4 in my case? I just process accounting req., no auth logic.

alandekok commented 2 months ago

If your local time zone is CEST and the date being given is CEST, then it should work. If it doesn't, I suspect it's a bug in strptime(), and not in FreeRADIUS.

You can test v4, which should have the same behavior, because it also uses strptime().

In order to address this issue in v4, we have changed the default date printing to RFC 3339 format. That format does not use words for months, and uses only numbers for time zone offsets. As such, that format is unambiguous, and doesn't have any of these problems.