Yubico / yubikey-val

YubiKey OTP validation server in PHP
https://developers.yubico.com/yubikey-val
BSD 2-Clause "Simplified" License
130 stars 43 forks source link

Possible oops with sync pools #40

Closed scottsakai closed 7 years ago

scottsakai commented 8 years ago

Hi, I found what appears to be an error in the verification code when operating with a sync pool. In short, it looks like some peers may get ignored, which is bad if the ignored peers happen to return replayed counter values.

I've implemented what appears to be a fix, and enabled reasonable behavior for two-peer pools (where authentication doesn't break because one of the two peers is down).

Also added an example cron script to keep ykval-queue running.

dainnilsson commented 8 years ago

There's a few things going on here, which might not be obvious at first glance. First off, we queue sync requests to all server in the sync pool: https://github.com/Yubico/yubikey-val/blob/master/ykval-verify.php#L373

Then, if the configured sync level requires immediate sync (>0), we attempt to sync with all servers in parallel. However, once we've obtained a sufficient number of replies, we cancel the rest. This behavior is consistent regardless of sync level, as "a sufficient number of requests" for sl = 0 is 0, so no requests are even issued. Any additional servers in the pool that have not been synced at this point (all of them in case of sl=0) will remain in the sync queue database at this point, and will get synced shortly by the sync daemon. The already synced servers (if any) will have been removed from the queue at this point.

The reason we don't simply sync with all servers is that we want to return a response to the caller as soon as possible, and PHP's architecture does not allow us to continue processing the remaining requests once we send a response.

As for the ykval-cron script, this is already taken care of by the init script available in the Debian package: https://github.com/Yubico/yubikey-val-dpkg/blob/master/debian/yubikey-val.ykval-queue.init

thorduri commented 7 years ago

@dainnilsson Given your comment, close this PR ?