| Summary: | server time check is way too strict (<5 sec drift) | ||
|---|---|---|---|
| Product: | Gentoo Infrastructure | Reporter: | SpanKY <vapier> |
| Component: | Git | Assignee: | Gentoo Infrastructure <infra-bugs> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | hydrapolic, ionen, polynomial-c |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
|
Description
SpanKY
2021-10-21 23:00:08 UTC
I've tried to improve the clarity of the checks and their output: https://gitweb.gentoo.org/infra/githooks.git/diff/local/require-signed-push?h=require-signed-push-clarity&id=require-signed-push-clarity&id2=master There are two checks: - The entire round-trip should take less than 60 seconds (including the developer interacting with their signing token or GPG) - If the developer's system generates the developer-side timestamp signing process (before any hit to GPG or tokens), if the clock is wrong or the system is heavily loaded, it'll trip the error if it's out by more than 5 seconds. The 60 second round-trip timing is to reduce chances of MITM in 2FA. If the developer's clock is wrong, then author timestamps, commit timestamps and the push timestamps will all be wrong (likely in the same way). Commits that are accepted, and in a linear history, may appear to have time jumping around (e.g. your clock is behind the time of the previous commit). That said, maybe 5 seconds is too strict? please suggest an alternate value? 30 seconds? (In reply to Robin Johnson from comment #1) > If the developer's clock is wrong, then author timestamps, commit timestamps > and the push timestamps will all be wrong (likely in the same way). i get that, but a slew of seconds seems like unnecessary churn > Commits that are accepted, and in a linear history, may appear to have time > jumping around (e.g. your clock is behind the time of the previous commit). err, we're talking about git here. timestamps are at best a recommendation. merges, rebases, saved branch work, etc... already shows a history that does not look linear at all. so we've already lost this battle, and it's not really useful in the first place. > That said, maybe 5 seconds is too strict? > please suggest an alternate value? 30 seconds? didn't i suggest bumping it to 30 sec already ? or am i misunderstanding ? I would argue if it is something actually worth addressing. If you can push you do have network access, if you drop something that have smarts for NTP sync like net-misc/chrony then most likely you will never have drift that goes above 1s anyway. Install it, start it, and forget the problem. (In reply to Piotr Karbowski from comment #3) > I would argue if it is something actually worth addressing. If you can push > you do have network access, if you drop something that have smarts for NTP > sync like net-misc/chrony then most likely you will never have drift that > goes above 1s anyway. Install it, start it, and forget the problem. Part of the issue is that triggering this check is not necessarily due to clock drift. If the push operation itself took a while (for whatever reason, such as high cpu load, or a pre-push hook with `sleep 5`), it'll mistakenly think it's clock drift. But as I've been told there's no way to tell the difference. I found it hard to believe so I tested it and you're right, pre-push hook will detail it. In that case, it is worth addressing or even removing as a whole. As far as I'm concerned 10s would be enough for bit of leeway, although the suggested 30s would give the freedom to use pre-push hooks with "less" worries (if not straight up removing). Right now I rely on a "git push" wrapper (for pkgcheck scan --commits and other stuff, similar to pkgdev push) and only have actual pre-push hook do minimal sanity checks (like stopping me if I forgot to use the wrapper). The wrapper is something I formerly didn't use but added after I pushed to gentoo for the first time being hit by the ntp check. Not that I can't live with 5s, but I do feel it's too strict in a situation where it can't be an accurate check for clock drift. ionen:
Thank you for mentioning pre-push as a contributing factor.
Expensive checks should NOT be included in that, because of the timing of when pre-push is run.
From the git hooks documentation:
> The pre-push hook runs during git push, after the remote refs have been updated but before any objects have been transferred. It receives the name and location of the remote as parameters, and a list of to-be-updated refs through stdin. You can use it to validate a set of ref updates before a push occurs (a non-zero exit code will abort the push).
The "during" is the problem here. "pre-push" really should have been calling "during-push", and free up the name to do stuff BEFORE it contacts the server (which would be based on the last known state of the server, rather than latest state).
Reproduction example using pre-push hook of "sleep 5" $ git commit --allow-empty -s -S -m 'bump' [master 82bdf8b] bump $ GIT_TRACE_PACKET=1 GIT_TRACE=1 git push 14:40:43.545004 git.c:455 trace: built-in: git push 14:40:43.545704 run-command.c:666 trace: run_command: unset GIT_PREFIX; ssh git@git.gentoo.org 'git-receive-pack '\''/proj/test.git'\''' 14:40:44.149301 pkt-line.c:80 packet: push< e57099f12b10a55218c70140365d72abc9e3a0ee refs/heads/master\0report-status report-status-v2 delete-refs side-band-64k quiet atomic ofs-delta push-cert=1635111644-b4ed17ed4a9c95d58c11c8a7577c2844d1ae1b3b object-format=sha1 agent=git/2.32.0 14:40:44.149330 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2 14:40:44.149337 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass10 14:40:44.149343 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass11 14:40:44.149350 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass12 14:40:44.149355 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass13 14:40:44.149361 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass14 14:40:44.149367 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass15 14:40:44.149374 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass16 14:40:44.149380 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass17 14:40:44.149387 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass18 14:40:44.149394 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass19 14:40:44.149400 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass2 14:40:44.149406 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass20 14:40:44.149412 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass3 14:40:44.149418 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass4 14:40:44.149423 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass5 14:40:44.149429 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass6 14:40:44.149435 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass7 14:40:44.149441 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass8 14:40:44.149447 pkt-line.c:80 packet: push< f9f519be9722735e5b27176df870019d46b132ec refs/heads/sandbox/robbat2-pass9 14:40:44.149453 pkt-line.c:80 packet: push< d7fd89a672231d17ddbcf1641a4a979996fcf738 refs/heads/sandbox/soap 14:40:44.149459 pkt-line.c:80 packet: push< 658359ef656a7331aba84ac5fdc83da8c3207429 refs/heads/sandbox/soap2 14:40:44.149465 pkt-line.c:80 packet: push< d964e5ba061f1f4307cc7603b127f3f13cf5bdb5 refs/heads/test 14:40:44.149470 pkt-line.c:80 packet: push< d4170b6f0af529ef32efbf73744568feba6af0fb refs/push-certs 14:40:44.149476 pkt-line.c:80 packet: push< dd8249afa700ad36d5e58a062d235db199d9d36c refs/tags/INITIAL-annotated 14:40:44.149482 pkt-line.c:80 packet: push< 11deece4c52aa027595f4e11fe4ee4c5f9da8f03 refs/tags/INITIAL-unannotated 14:40:44.149488 pkt-line.c:80 packet: push< 0000 14:40:44.149806 run-command.c:666 trace: run_command: .git/hooks/pre-push origin git+ssh://git@git.gentoo.org/proj/test.git pre-push sleep: 2021-10-24T14:40:44,152335837-07:00 pre-push sleep: 2021-10-24T14:40:49,154765428-07:00 14:40:49.155259 run-command.c:666 trace: run_command: gpg --status-fd=2 -bsau 0xEE05E6F6A48F6136 14:40:49.250098 pkt-line.c:80 packet: push> push-cert\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.33.1 14:40:49.250113 pkt-line.c:80 packet: push> certificate version 0.1 14:40:49.250119 pkt-line.c:80 packet: push> pusher 0xEE05E6F6A48F6136 1635111649 -0700 14:40:49.250124 pkt-line.c:80 packet: push> pushee git+ssh://git.gentoo.org/proj/test.git 14:40:49.250131 pkt-line.c:80 packet: push> nonce 1635111644-b4ed17ed4a9c95d58c11c8a7577c2844d1ae1b3b 14:40:49.250136 pkt-line.c:80 packet: push> 14:40:49.250140 pkt-line.c:80 packet: push> e57099f12b10a55218c70140365d72abc9e3a0ee 82bdf8b6f6fe067c1895a135f8b93624449ac576 refs/heads/master 14:40:49.250145 pkt-line.c:80 packet: push> -----BEGIN PGP SIGNATURE----- 14:40:49.250150 pkt-line.c:80 packet: push> 14:40:49.250154 pkt-line.c:80 packet: push> iQKTBAABCgB9FiEEveu2pS8Vb98xaNkRGTlfI8WIJsQFAmF10uFfFIAAAAAALgAo 14:40:49.250159 pkt-line.c:80 packet: push> aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldEJE 14:40:49.250163 pkt-line.c:80 packet: push> RUJCNkE1MkYxNTZGREYzMTY4RDkxMTE5Mzk1RjIzQzU4ODI2QzQACgkQGTlfI8WI 14:40:49.250168 pkt-line.c:80 packet: push> JsSSuRAAiZVrCWD/fnsZDDjNviif5mlW2Vun9UHXAqvxEJXbbwErHWuKKWhf4tUh 14:40:49.250173 pkt-line.c:80 packet: push> rDo55FpP0VYuXVbvpBMM6Gr4Kmjl3CBEgiLLZ5aissFj1J5VvfP/W/HGuazDTWfX 14:40:49.250177 pkt-line.c:80 packet: push> /4uQIq9yrcXZRj9FVpprYg5wvbW7x9vlh67BKij77wLWdN/U9bvFUEvxD2jQ8Xc6 14:40:49.250183 pkt-line.c:80 packet: push> TnKk6BVuW3wVRgBuLdKDUNiobG+dx1+PgcfPMs0wOKTZt6g5+kYypGX7jCha7UMk 14:40:49.250188 pkt-line.c:80 packet: push> 1U/2pzelGWMPPIih4doNiTnKhweVWkAyRg7UzjTV+5xDCx62FjWePf4TYgwcojKO 14:40:49.250192 pkt-line.c:80 packet: push> ZpcLYWmKcFmPb3GsVlbtUSOGkQNylEOrd09zgBSbErkVuCuBZO73q/8dMIoIoRXD 14:40:49.250196 pkt-line.c:80 packet: push> wE5ssQE1AMv8AsYqIDaoJU7GtYP8RO8ygOznw6UnwhM3/KlWTwhjPrUIESHjTeWD 14:40:49.250200 pkt-line.c:80 packet: push> o2YQzwDYE4nkC+s4mmIu3QJTp9WIuw5JcONJeTKzh6HoQd7DANr9spIEva19jxlW 14:40:49.250205 pkt-line.c:80 packet: push> A0nPuZroemrO37vGOKGz5kFv6kMPftW+CRgWn55iBuY4RhOJInx3EftiP5b2ssRu 14:40:49.250209 pkt-line.c:80 packet: push> uYh0ir0xQLxTVI+uBQdAoLJXdidYg9yjG/DOHxCUh0Y/xDpQKJqk4BgTmwhdJt+1 14:40:49.250213 pkt-line.c:80 packet: push> csIBhWY0vj7xxrZLTA0zkYYME/rJnTBJheMqkRircK4UyOIYfrA= 14:40:49.250217 pkt-line.c:80 packet: push> =2wdK 14:40:49.250222 pkt-line.c:80 packet: push> -----END PGP SIGNATURE----- 14:40:49.250227 pkt-line.c:80 packet: push> push-cert-end 14:40:49.250239 pkt-line.c:80 packet: push> 0000 14:40:49.250323 run-command.c:666 trace: run_command: git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress 14:40:49.251290 git.c:455 trace: built-in: git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress Enumerating objects: 1, done. Counting objects: 100% (1/1), done. Writing objects: 100% (1/1), 963 bytes | 963.00 KiB/s, done. Total 1 (delta 0), reused 0 (delta 0), pack-reused 0 14:40:49.555835 pkt-line.c:80 packet: sideband< \2Your system clock is off by 5 seconds (limit 5)Run NTP, rebase your commits as needed, and push again. remote: Your system clock is off by 5 seconds (limit 5) remote: Run NTP, rebase your commits as needed, and push again. 14:40:49.556081 pkt-line.c:80 packet: sideband< \1000eunpack ok0033ng refs/heads/master pre-receive hook declined0000 14:40:49.556117 pkt-line.c:80 packet: push< unpack ok 14:40:49.556134 pkt-line.c:80 packet: push< ng refs/heads/master pre-receive hook declined 14:40:49.556142 pkt-line.c:80 packet: push< 0000 14:40:49.557273 pkt-line.c:80 packet: sideband< 0000 To git+ssh://git.gentoo.org/proj/test.git ! [remote rejected] master -> master (pre-receive hook declined) error: failed to push some refs to 'git+ssh://git.gentoo.org/proj/test.git' anyway, does anybody have objections to the other changes I proposed? https://gitweb.gentoo.org/infra/githooks.git/diff/local/require-signed-push?h=require-signed-push-clarity&id=require-signed-push-clarity&id2=master w/ a increase to 30 seconds. (In reply to Robin Johnson from comment #9) looks like there's syntax errors in there. the short funcs are missing ;. -silent_die() { exit 1 } +silent_die() { exit 1; } -die() { echo "$@" >&2; silent_die } +die() { echo "$@" >&2; silent_die; } can we get the fix deployed please ? it's annoying to have to keep repushing. pushed to puppet, please verify in 2 hours. i haven't seen issues so far, but it didn't always hit me (for obvious reasons). i'll assume it's fixed, thanks! i just got this today. running `while sleep 0.5; do date; done` in the terminal with https://www.time.gov/ in a browser tab shows my clock pretty much in step. $ git push --signed Enumerating objects: 16, done. Counting objects: 100% (16/16), done. Delta compression using up to 4 threads Compressing objects: 100% (12/12), done. Writing objects: 100% (12/12), 3.55 KiB | 3.55 MiB/s, done. Total 12 (delta 9), reused 0 (delta 0), pack-reused 0 remote: Push certificate time is too skew (sign-nonce). remote: It's possible your system clock is off by up to 7 seconds vs limit 5 remote: Run NTP, pull & rebase your commits if needed, and push again. remote: ---cut-here--- remote: certificate version 0.1 remote: pusher 0x774DFDAD9E3EB0E71030966D4071D71BD4C4C25F 1644960626 -0500 remote: pushee git+ssh://git.gentoo.org/proj/portage.git remote: nonce 1644960619-e86e8a726c74086f7911b85f3bc5c49abe31bd2a remote: remote: 1327fa9f829e8670c65ff35b9b0bda446991f7ed 937227f72d4e6eb057bb53f793686fe721f52ac2 refs/heads/master remote: -----BEGIN PGP SIGNATURE----- remote: remote: iQIzBAABCAAdFiEEsileu1rx5F93Ey8ovqU1qa4qUYgFAmIMG3IACgkQvqU1qa4q remote: UYjI6A//WVDO81Q/Ul3s2c+bFYE4wgSZhR+D9Si4EmWEp/aeU/tIS7mntBx2fxN4 remote: yFahKBa4Ioj6gvkZDINgg5hSyjFg9Su+tvxKW8ZBhsDeioIfi2DaGV8NZPCPU7Lu remote: DSzt6DhWtblHcSVSQUtuoE9kbLI06E6UpzkpkXsnPhJDh/HMwQozTgm8CbFIE0b8 remote: RCsGaM4MMX4OqXCFQ5fkhV/dH2ukajbVWlXXk9SMGL16/MMjT0LPNTvYpB0j/w/Z remote: pEIk5l1Iaa1xljGro6Sz87IoWuHaWyErk2ePGSho/d+WkYcrcq4QLI2GI5xTuFZC remote: 7zjjDvcaPHRE3lKCq3hG0pZeMfuZi3YlBIQBsIHSknWvH/WwPmi217hYoAEOMIi1 remote: aDriJqT3FLBK3Yu4OPvm3XLxF7M9tcncBT9w8exuLFu8WnEDw/J/ce0TJ4VCC2ZZ remote: GFtPkEE1BQYMvZYpCAzwxOYxN63zpWNgCM6kG+iWSKVS7DQyJ2PyTaaWTn8hoEOx remote: foefQz8U5lp8drSCR0KwiylzRH4FKxdlVSNYufMsqUPk3wcs6WpJ43CnKEgJbA9D remote: QPkgZDL+IU7WJJpDi+DjZw6MpV5mSF/fVMBj2ZXxBvE91NwV0Du8xpFU2jPnA9sO remote: m/jWnIkh7DDnPWb9r83xho/8IBRonfQqyA1haTHjVTy3yHiF5oo= remote: =k0mL remote: -----END PGP SIGNATURE----- remote: ---cut-here--- remote: Time issues during git-push (In reply to SpanKY from comment #14) > i just got this today. running `while sleep 0.5; do date; done` in the > terminal with https://www.time.gov/ in a browser tab shows my clock pretty > much in step. > > $ git push --signed > Enumerating objects: 16, done. > Counting objects: 100% (16/16), done. > Delta compression using up to 4 threads > Compressing objects: 100% (12/12), done. > Writing objects: 100% (12/12), 3.55 KiB | 3.55 MiB/s, done. > Total 12 (delta 9), reused 0 (delta 0), pack-reused 0 > remote: Push certificate time is too skew (sign-nonce). > remote: It's possible your system clock is off by up to 7 seconds vs limit 5 > remote: Run NTP, pull & rebase your commits if needed, and push again. > remote: ---cut-here--- > remote: certificate version 0.1 > remote: pusher 0x774DFDAD9E3EB0E71030966D4071D71BD4C4C25F 1644960626 -0500 > remote: pushee git+ssh://git.gentoo.org/proj/portage.git > remote: nonce 1644960619-e86e8a726c74086f7911b85f3bc5c49abe31bd2a > remote: > remote: 1327fa9f829e8670c65ff35b9b0bda446991f7ed > 937227f72d4e6eb057bb53f793686fe721f52ac2 refs/heads/master > remote: -----BEGIN PGP SIGNATURE----- > remote: > remote: iQIzBAABCAAdFiEEsileu1rx5F93Ey8ovqU1qa4qUYgFAmIMG3IACgkQvqU1qa4q > remote: UYjI6A//WVDO81Q/Ul3s2c+bFYE4wgSZhR+D9Si4EmWEp/aeU/tIS7mntBx2fxN4 > remote: yFahKBa4Ioj6gvkZDINgg5hSyjFg9Su+tvxKW8ZBhsDeioIfi2DaGV8NZPCPU7Lu > remote: DSzt6DhWtblHcSVSQUtuoE9kbLI06E6UpzkpkXsnPhJDh/HMwQozTgm8CbFIE0b8 > remote: RCsGaM4MMX4OqXCFQ5fkhV/dH2ukajbVWlXXk9SMGL16/MMjT0LPNTvYpB0j/w/Z > remote: pEIk5l1Iaa1xljGro6Sz87IoWuHaWyErk2ePGSho/d+WkYcrcq4QLI2GI5xTuFZC > remote: 7zjjDvcaPHRE3lKCq3hG0pZeMfuZi3YlBIQBsIHSknWvH/WwPmi217hYoAEOMIi1 > remote: aDriJqT3FLBK3Yu4OPvm3XLxF7M9tcncBT9w8exuLFu8WnEDw/J/ce0TJ4VCC2ZZ > remote: GFtPkEE1BQYMvZYpCAzwxOYxN63zpWNgCM6kG+iWSKVS7DQyJ2PyTaaWTn8hoEOx > remote: foefQz8U5lp8drSCR0KwiylzRH4FKxdlVSNYufMsqUPk3wcs6WpJ43CnKEgJbA9D > remote: QPkgZDL+IU7WJJpDi+DjZw6MpV5mSF/fVMBj2ZXxBvE91NwV0Du8xpFU2jPnA9sO > remote: m/jWnIkh7DDnPWb9r83xho/8IBRonfQqyA1haTHjVTy3yHiF5oo= > remote: =k0mL > remote: -----END PGP SIGNATURE----- > remote: ---cut-here--- > remote: Time issues during git-push Thanks, we updated it to 30s. -A |
