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

htpdate fails to read the date header #30

Open
freedge opened this issue May 9, 2023 · 6 comments
Open

htpdate fails to read the date header #30

freedge opened this issue May 9, 2023 · 6 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@freedge
Copy link

freedge commented May 9, 2023

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 twekkel added the bug Something isn't working label May 9, 2023
@twekkel
Copy link
Owner

twekkel commented May 9, 2023

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

twekkel commented May 11, 2023

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

@freedge
Copy link
Author

freedge commented May 11, 2023

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

twekkel commented May 11, 2023

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

twekkel commented May 13, 2023

Some deep digging, thought me a couple of things

  • connection: close will immediately flush server data to htpdate, but that would require a new TCP and TLS handshake for each poll (ineffectieve and relative expensive)
  • connection: keep-alive might require multiple SSL_read actions in a loop as web server doesn't provide the full header in one read... but it you will have the wait in between the the SSL_read actions or else there is no data yet

When the sleep is commented out at line 250

sleep(0.1);
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
Copy link
Owner

twekkel commented May 13, 2023

btw

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

works fine too with the original htpdate version

@twekkel twekkel added the help wanted Extra attention is needed label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants