Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-repair: track and use a lower bound for the time for TLS checks #3781
Conversation
Some checks were not successful
codecov-io
commented
Aug 22, 2017
•
Edited 1 time
-
codecov-io
Aug 25, 2017
-
Aug 25, 2017
Codecov Report
@@ Coverage Diff @@
## master #3781 +/- ##
==========================================
- Coverage 75.73% 75.73% -0.01%
==========================================
Files 409 409
Lines 35345 35388 +43
==========================================
+ Hits 26770 26800 +30
- Misses 6680 6691 +11
- Partials 1895 1897 +2
Continue to review full report at Codecov.
|
pedronis
requested a review
from
niemeyer
Aug 24, 2017
mvo5
approved these changes
Aug 25, 2017
This looks very nice, some nitpicks and suggestions for your consideration inline.
| @@ -180,6 +183,14 @@ func (run *Runner) Fetch(brandID, repairID string, revision int) (repair *assert | ||
| return nil, nil, err | ||
| } | ||
| + moveTimeLowerBound := true |
mvo5
Aug 25, 2017
Collaborator
(nitpick/idea): if we had a run.maybeMoveTimeLowerBound(resp *http.Response) we could just do a unconditional defer run.maybeMoveTimeLowerBound(resp) here and do the check for the return code inside maybeMoveTimeLowerBound() thus being slightly more DRY.
| @@ -139,6 +157,15 @@ func mustParseURL(s string) *url.URL { | ||
| return u | ||
| } | ||
| +func (s *runnerSuite) mockNow(c *C, runner *repair.Runner) (restore func()) { |
mvo5
Aug 25, 2017
Collaborator
Maybe we can call this something more descriptive, i.e. in what way now is mocked. Maybe: mockTimeNowBroken, mockTimeNowReturnsEpoch, simulateBrokenClock or similar? This will make the subsequent tests that use it easier to read.
| repair, aux, err := runner.Fetch("canonical", "2", -1) | ||
| c.Assert(err, IsNil) | ||
| c.Check(repair, NotNil) | ||
| c.Check(aux, HasLen, 0) | ||
| c.Check(repair.BrandID(), Equals, "canonical") | ||
| c.Check(repair.RepairID(), Equals, "2") | ||
| c.Check(repair.Body(), DeepEquals, []byte("script\n")) | ||
| + | ||
| + c.Check(runner.TLSTime().Before(t0), Equals, false) |
mvo5
Aug 25, 2017
Collaborator
Maybe we could make this a tiny bit more descriptive by adding a small helper, something has a name like: s.checkTLSTimeGotUpdate() (or similar) which would do the t0 := time.Now() internally. With the other suggested name change it would be something like:
r := s.simulateTimeNowBroken()
...
c.checkTimeNoLongerBroken()
(feel free to pick better names of course :)
| +var ( | ||
| + defaultTransport *http.Transport = http.DefaultTransport.(*http.Transport) | ||
| +) | ||
| + | ||
| // NewHTTPCLient returns a new http.Client with a LoggedTransport, a | ||
| // Timeout and preservation of range requests across redirects | ||
| func NewHTTPClient(opts *ClientOpts) *http.Client { |
mvo5
Aug 25, 2017
Collaborator
Still a bit mind-boggling that NewHTTPClient() is in logger.go - but nothing to do with this branch :)
Some checks were not successful
|
Thanks a lot for the update, looks even nicer now. |
zyga
approved these changes
Aug 30, 2017
LGTM, one question about storing the lower bound in the state (perhaps I'm missing something and it is already done).
| @@ -318,12 +340,36 @@ func (run *Runner) readState() error { | ||
| return dec.Decode(&run.state) | ||
| } | ||
| +func (run *Runner) moveTimeLowerBound(t time.Time) { | ||
| + if t.After(run.state.TimeLowerBound) { | ||
| + run.state.TimeLowerBound = t.UTC() |
zyga
Aug 30, 2017
Contributor
Nice :-)
Should we store such lower bound in the state persistently, so that we prevent general time rewind attacks (of whatever kind?)
pedronis
and others
added some commits
Aug 30, 2017
Some checks were not successful
chipaca
John Lenton
Some checks were not successful
pedronis commentedAug 22, 2017
This tracks and uses a lower bound for time for TLS checks, as was discussed here:
https://forum.snapcraft.io/t/repair-capability-emergency-fixes/311/33