Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
alandekok opened this issue May 20, 2024 · 7 comments
Open

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

alandekok opened this issue May 20, 2024 · 7 comments
Labels
defect category: a defect or misbehaviour v3.2.x meta: relates to the v3.2.x branch

Comments

@alandekok
Copy link
Member

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_
@alandekok alandekok added defect category: a defect or misbehaviour v3.2.x meta: relates to the v3.2.x branch labels May 20, 2024
@rygl
Copy link

rygl commented Aug 22, 2024

Hi, any updates on that?

@alandekok
Copy link
Member Author

@rygl We're happy to accept patches.

@alandekok
Copy link
Member Author

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
Copy link

rygl commented Aug 23, 2024

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
Copy link
Member Author

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
Copy link

rygl commented Aug 23, 2024

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect category: a defect or misbehaviour v3.2.x meta: relates to the v3.2.x branch
Projects
None yet
Development

No branches or pull requests

2 participants