twekkel / htpdate

HTTP Time protocol
https://www.vervest.org/htp
Other
50 stars 13 forks source link

htpdate fails to read the date header #30

Open freedge opened 1 year ago

freedge commented 1 year ago

if the date header does not appear within the first 1024 bytes of the server first response, then it is not parsed at all:

$ htpdate -q https://www.pool.ntp.org -d
burst: 1 try: 1 when: 500000
www.pool.ntp.org no timestamp
burst: 1 try: 2 when: 500000
www.pool.ntp.org no timestamp
No server suitable for synchronization found

This prevents for example the synchro from https://www.pool.ntp.org, as the date header appears after a bunch of other headers. It would require a bigger buffer and multiple calls to SSL_read to work.

EDIT: in recent versions of htpdate the BUFFERSIZE was bumped to 8192 but there is still a single call to SSL_read so it still fails.

twekkel commented 1 year ago

Yep indeed a bug... SSL_read should be called in a loop, as long as ssl_error == SSL_ERROR_WANT_READ... but that never happens. I'm open for suggestions...

twekkel commented 1 year ago

Please give it a try with this branch https://github.com/twekkel/htpdate/tree/LargeResponseHeader

freedge commented 1 year ago

hi! thanks for the quick response!

so the branch works a bit better. Note the rc return code is incorrect now (it will return the amount of data sent even if the receive fails).

ntpdate is able to parse a date, but if fails just after:

$ ./htpdate -q https://www.pool.ntp.org -d -c
www.pool.ntp.org          443, 11 May 2023 18:08:54 GMT (183 ms) => 5
www.pool.ntp.org no timestamp
when: 562500000, nap: 500000000
No server suitable for synchronization found

in my case my laptop is not yet synchronized, so htpdate loops, and the second call fails as the connection is closed already. So htpdate only works with -p 1

twekkel commented 1 year ago

Using connection: close breaks every new call as it expects that the connection stays alive... so this is not going to be merged as-is. Setting up a new connection with each call (especially HTTPS) is more expensive, but probably more safe... will need some more time to investigate and come up with a solid solution.

twekkel commented 1 year ago

Some deep digging, thought me a couple of things

When the sleep is commented out at line 250 https://github.com/twekkel/htpdate/blob/e29b49069eb3abaa0c566d71192e0babbeb4db7e/htpdate.c#L250 htpdate might work or not.... introducing the sleep however makes htpdate pretty inaccurate. @freedge please give this update branch a try, with and without sleep.

I'm currently inclined to say that https://www.pool.ntp.org is a poor time source for htpdate and probably the current/original implementation is the best way to go forward.

Not closing this issue, as better solutions might be available...

twekkel commented 1 year ago

btw

./htpdate -d https://www.ntppool.org/en/

works fine too with the original htpdate version