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

Added an optional feature to compare the local time with a set of trusted ntp servers #143

Merged
merged 10 commits into from
Nov 15, 2022

Conversation

kommendorkapten
Copy link
Member

If the local time is diffing more than a configured threshold, the service shuts down itself.

Signed-off-by: Fredrik Skogman [email protected]

Release Note

  • Optional comparison of service's local time with a set of ntp servers, to make sure local time is not drifting.

ntp servers. If the local time is diffing more than a configured threshold,
the service shuts down itself.

Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
@kommendorkapten kommendorkapten requested a review from a team as a code owner November 11, 2022 12:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #143 (0fcb494) into main (af55c02) will decrease coverage by 4.14%.
The diff coverage is 27.61%.

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   53.79%   49.64%   -4.15%     
==========================================
  Files          15       18       +3     
  Lines         712      846     +134     
==========================================
+ Hits          383      420      +37     
- Misses        285      382      +97     
  Partials       44       44              
Impacted Files Coverage Δ
pkg/ntpmonitor/config.go 0.00% <0.00%> (ø)
pkg/ntpmonitor/ntpmonitor.go 14.00% <14.00%> (ø)
pkg/ntpmonitor/randomchoice.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kommendorkapten kommendorkapten changed the title Added an optionl feature to compare the local time with a set of trusted ntp servers Added an optional feature to compare the local time with a set of trusted ntp servers Nov 11, 2022
@kommendorkapten
Copy link
Member Author

The motivation behind this PR is that for a TSA it's extremely important that we are using a time source that's accurate. Just relying on the host's time can be dangerous, as NTP may not be enabled, or the configured NTP server is not correct. Having the process itself verifying its time with external thirdparty servers is defence in depth, and following the "trust but verify" philosophy.

Signed-off-by: Fredrik Skogman <[email protected]>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we discussed in #133 was using NTP servers that provided NTS. I'm not sure if there are enough, but something to consider, it would be great if we could secure the NTP connections

cmd/timestamp-server/app/serve.go Outdated Show resolved Hide resolved
ntpsync.yaml Outdated Show resolved Hide resolved
cmd/timestamp-server/app/serve.go Outdated Show resolved Hide resolved
pkg/ntpmonitor/ntpmonitor.go Outdated Show resolved Hide resolved
cmd/timestamp-server/app/serve.go Outdated Show resolved Hide resolved
pkg/ntpmonitor/ntpmonitor.go Show resolved Hide resolved
// RandomChoice returns a random selection of n items from the slic s.
// The choice is made using a PSEUDO RANDOM selection.
// If n is greater than len(s), an empty slice is returned.
func RandomChoice[T any](s []T, n int) []T {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooooo how fancy, using generics :) I love it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah it's the first generics code I wrote, and actually felt natural to make this function like this :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the first generics code I've seen in sigstore repos!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailblazer 😏

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great, just a few nits!

cmd/timestamp-server/app/serve.go Outdated Show resolved Hide resolved
pkg/ntpmonitor/config.go Show resolved Hide resolved
pkg/ntpmonitor/ntpmonitor.go Show resolved Hide resolved
ntpsync.yaml Outdated Show resolved Hide resolved
pkg/ntpmonitor/ntpmonitor.go Show resolved Hide resolved
@haydentherapper haydentherapper merged commit 30de875 into sigstore:main Nov 15, 2022
@kommendorkapten kommendorkapten deleted the ntp-monitor branch November 16, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants