-
Notifications
You must be signed in to change notification settings - Fork 162
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
verifier: Make acceptable TS range configurable #3003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)
go/lib/infra/modules/trust/signhelper.go, line 37 at r1 (raw file):
MaxPldAge = 2 * time.Second // MaxInFuture indicates the maximum time a timestamp may be in the future. MaxInFuture = 1 * time.Second
Just time.Second
is less stuff.
go/lib/infra/modules/trust/signhelper.go, line 202 at r1 (raw file):
now := time.Now() ts := sign.Time() diff := now.Sub(ts)
Use a varname to invert the comparison and make it more readable:
timeInFuture := -diff
if timeInFuture > v.tsRange.MaxInFuture {
// etc
go/lib/infra/modules/trust/signhelper.go, line 202 at r1 (raw file):
now := time.Now() ts := sign.Time() diff := now.Sub(ts)
s/diff
/signatureAge
maybe?
go/lib/infra/modules/trust/signhelper_test.go, line 85 at r1 (raw file):
} sign := proto.NewSignS(proto.SignType_ed25519, src.Pack()) sign.SetTimestamp(time.Now().Add(test.TSDiff))
Just throwing this out there as food for thought for the future: Plugging a clock interface in packages that deal with time would allow us to "freeze the clock" in tests, making them 100% deterministic.
This PR makes the acceptable timestamp range for signatures configurable. Additionally, allow small time diffs in the future to allow for clock drift. fixes scionproto#3002
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @scrye)
go/lib/infra/modules/trust/signhelper.go, line 202 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Use a varname to invert the comparison and make it more readable:
timeInFuture := -diff if timeInFuture > v.tsRange.MaxInFuture { // etc
Done.
go/lib/infra/modules/trust/signhelper.go, line 202 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
s/
diff
/signatureAge
maybe?
Done.
go/lib/infra/modules/trust/signhelper_test.go, line 85 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Just throwing this out there as food for thought for the future: Plugging a clock interface in packages that deal with time would allow us to "freeze the clock" in tests, making them 100% deterministic.
Hmm. we could also provide now as an argument.
Not sure which turns out better tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
go/lib/infra/modules/trust/signhelper_test.go, line 85 at r1 (raw file):
Previously, Oncilla wrote…
Hmm. we could also provide now as an argument.
Not sure which turns out better tbh.
It looks fine now, let's keep it as is.
This PR makes the acceptable timestamp range for signatures
configurable.
Additionally, allow small time diffs in the future to allow for clock
drift.
fixes #3002
This change is