Create roots analyzer #40

RJPercival · 2019-09-24T11:10:51Z

Creates incident reports when the root certificates returned by a CT Log's get-roots endpoint change.

codecov-io · 2019-09-24T11:12:43Z

Codecov Report

Merging #40 into master will increase coverage by 0.86%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   64.94%   65.81%   +0.86%     
==========================================
  Files           8        9       +1     
  Lines         368      430      +62     
==========================================
+ Hits          239      283      +44     
- Misses        115      126      +11     
- Partials       14       21       +7

Impacted Files	Coverage Δ
rootsanalyzer/rootsanalyzer.go	`70.96% <70.96%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6671dc5...e49d05f. Read the comment docs.

RJPercival · 2019-09-24T11:30:31Z

There's probably a better name than "rootsanalyzer" for this - maybe "rootswatcher" or "rootsmonitor"?

RJPercival · 2019-10-21T09:51:52Z

@taknira, will you have time to review this soon?

taknira · 2019-10-21T10:21:35Z

Yup, on it :)

taknira

Have looked at everything except the tests so far - have some comments (mostly tiny things or thinking out loud) to be going on with :)

storage/storage.go

taknira · 2019-10-28T14:41:12Z

storage/storage.go

@@ -41,3 +41,19 @@ type STHWriter interface {
 type RootsWriter interface {
 	WriteRoots(ctx context.Context, l *ctlog.Log, roots []*x509.Certificate, receivedAt time.Time) error
 }
+
+// RootSetID uniquely identifies a specific set of certificates, regardless of their order.
+type RootSetID string


You had a bunch of reasons for making this a string instead of []byte - what were they again?

As a string, it can be used as the key in a map. This is helpful since that's exactly what it is - a key for referring to a set of certificates. If I make it a []byte, I have to cast it to string whenever I used it with a map. It is also trivially comparable with other RootSetIDs as a string. However, there aren't any significant obstacles to making it a []byte so I'd be fine with changing it, if you've got strong reasons for doing so? The only one I can think of is that it's not really a printable string, so []byte is semantically more accurate.

I'm relaxed, I've been going back and forth on this in my head. It looks like the only map place it's used atm is for testing, and so that shouldn't be the sole reason for it to be a string, but I also don't have strong reasons why it should be []byte, so whichever is fine. I think in my head we'd be calculating it by doing a bunch of hashing, so that felt bytey, but then having the ID as []byte might cause problems if we want to use it as part of a primary key in a DB, depending on the db type. So author's choice - I leave it up to you :)

storage/testonly/roots.go

taknira · 2019-10-28T15:39:47Z

rootsanalyzer/rootsanalyzer.go

+					glog.Errorf("rootsanalyzer: %s", err)
+					return
+				}
+				addedCerts, removedCerts := diffRootSets(oldRoots, newRoots)


A thought, that can be done in a different PR if you agree with it - it might be useful to also store the diffs alongside the root sets or RootSetIDs or something, so that that changes are stored somewhere that's not just in the incident DB? Just a thought. Depends on your db schema of course, but having a list of rootSetIDs in the order that we saw them (or something), and storing any change (or maybe a representation of the change if the change could be too hefty) between that one and the previous one might be useful? WDYT?

@Mercurrent once you're familiar with this stuff it'd be good to get your take on this too!

That's something I planned to have in the design doc, but haven't actually run into a need for so far. @Mercurrent may wish to add this when it becomes necessary.

taknira · 2019-10-28T15:47:45Z

storage/storage.go

@@ -41,3 +41,19 @@ type STHWriter interface {
 type RootsWriter interface {
 	WriteRoots(ctx context.Context, l *ctlog.Log, roots []*x509.Certificate, receivedAt time.Time) error


Does WriteRoots need some sort of comment or potential future tweaking to either comment on the fact it should create or take as an argument a RootSetID, now that this concept has been introduced? It may be that (again, in a diff PR) the Roots Getter sorts and de-dupes the set of roots it gets, and calculates the RootSetID before calling WriteRoots, or something.... This is thinking about where the interface line should be drawn - I'm pro making as much of the useful functionality sit above the interface line as possible, at least in cases where we have a clear idea of how it should work. WDYT?

I was letting the RootsWriter implementation handle calculating the RootSetID, since it's arguably implementation-specific. The RootSetID isn't defined by Monologue to be anything but an opaque, deterministic identifier. I could make WriteRoots() return the RootSetID, which makes this a bit more obvious?

rootsanalyzer/rootsanalyzer.go

taknira

Aaaand looked at the tests :)

rootsanalyzer/rootsanalyzer_test.go

taknira · 2019-10-28T16:48:10Z

rootsanalyzer/rootsanalyzer.go

+{{ end }}{{ end }}{{ if gt (len .RemovedCerts) 0 }}
+Certificates removed ({{ len .RemovedCerts }}):
+{{ range .RemovedCerts }}{{ .Subject }}
+{{ end }}{{ end }}`))


If only the subject of the certs will be going in to the incident reports, it's almost definitely worth storing the complete cert diff data along side the RootSetID or something (as mentioned in another comment), just in case there end up being roots with the same subject field, but different keys, or something? It'd be good to have the full diff info somewhere I think?

I could include the full diff in here; I can imagine that being useful.

For now, I've just included the SHA256 hash of the cert, which makes it clear if something about the cert has changed (if it's in both the added and removed sections). That hash can be thrown into https://crt.sh to get the full cert details. I think the full diff could be deferred for a future PR.

Yup, SHA256 hash should do the trick for now :)

rootsanalyzer/rootsanalyzer.go

taknira · 2020-01-29T13:16:14Z

rootsanalyzer/rootsanalyzer.go

+					glog.Errorf("rootsanalyzer: %s", err)
+					return
+				}
+				addedCerts, removedCerts := diffRootSets(oldRoots, newRoots)


@Mercurrent once you're familiar with this stuff it'd be good to get your take on this too!

taknira · 2020-01-29T13:26:26Z

rootsanalyzer/rootsanalyzer.go

+{{ end }}{{ end }}{{ if gt (len .RemovedCerts) 0 }}
+Certificates removed ({{ len .RemovedCerts }}):
+{{ range .RemovedCerts }}{{ .Subject }}
+{{ end }}{{ end }}`))


Yup, SHA256 hash should do the trick for now :)

rootsanalyzer/rootsanalyzer.go

taknira · 2020-01-29T14:02:02Z

storage/storage.go

@@ -41,3 +41,19 @@ type STHWriter interface {
 type RootsWriter interface {
 	WriteRoots(ctx context.Context, l *ctlog.Log, roots []*x509.Certificate, receivedAt time.Time) error
 }
+
+// RootSetID uniquely identifies a specific set of certificates, regardless of their order.
+type RootSetID string


I'm relaxed, I've been going back and forth on this in my head. It looks like the only map place it's used atm is for testing, and so that shouldn't be the sole reason for it to be a string, but I also don't have strong reasons why it should be []byte, so whichever is fine. I think in my head we'd be calculating it by doing a bunch of hashing, so that felt bytey, but then having the ID as []byte might cause problems if we want to use it as part of a primary key in a DB, depending on the db type. So author's choice - I leave it up to you :)

storage/testonly/doc.go

taknira · 2020-01-29T14:13:51Z

There are just a couple of small things left to tweak (logStr, testonly doc comment location) so approving so you can go ahead and merge once those are done.

Creates incident reports when the root certificates return by a CT Log's get-roots endpoint change.

RJPercival added the enhancement New feature or request label Sep 24, 2019

RJPercival requested a review from taknira September 24, 2019 11:10

googlebot added the cla: yes https://cla.developers.google.com/ label Sep 24, 2019

RJPercival added this to In progress in Minimum Viable Monitor Sep 24, 2019

RJPercival mentioned this pull request Sep 24, 2019

MySQL storage implementation #41

Open

taknira reviewed Oct 28, 2019

View reviewed changes

golangcibot reviewed Jan 22, 2020

View reviewed changes

rootsanalyzer/rootsanalyzer.go Outdated Show resolved Hide resolved

RJPercival force-pushed the roots_analyzer branch from 704e6dc to c149aaa Compare January 22, 2020 13:09

RJPercival requested a review from taknira January 22, 2020 13:37

taknira reviewed Jan 29, 2020

View reviewed changes

taknira approved these changes Jan 29, 2020

View reviewed changes

RJPercival force-pushed the roots_analyzer branch from fe03d39 to 51bc7d8 Compare January 29, 2020 17:05

Create rootsanalyzer

e49d05f

Creates incident reports when the root certificates return by a CT Log's get-roots endpoint change.

RJPercival force-pushed the roots_analyzer branch from 51bc7d8 to e49d05f Compare January 29, 2020 17:15

RJPercival merged commit 0d4f685 into google:master Jan 29, 2020

Minimum Viable Monitor automation moved this from In progress to Done Jan 29, 2020

RJPercival deleted the roots_analyzer branch January 29, 2020 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create roots analyzer #40

Create roots analyzer #40

RJPercival commented Sep 24, 2019 •

edited

Loading

codecov-io commented Sep 24, 2019 •

edited

Loading

RJPercival commented Sep 24, 2019

RJPercival commented Oct 21, 2019

taknira commented Oct 21, 2019

taknira left a comment

taknira Oct 28, 2019

RJPercival Jan 6, 2020

taknira Jan 29, 2020

taknira Oct 28, 2019

taknira Jan 29, 2020

RJPercival Jan 29, 2020

taknira Oct 28, 2019

RJPercival Jan 22, 2020

taknira left a comment

taknira Oct 28, 2019

RJPercival Jan 22, 2020

RJPercival Jan 29, 2020

taknira Jan 29, 2020

taknira Jan 29, 2020

taknira Jan 29, 2020

taknira Jan 29, 2020

taknira commented Jan 29, 2020

		@@ -41,3 +41,19 @@ type STHWriter interface {
		type RootsWriter interface {
		WriteRoots(ctx context.Context, l ctlog.Log, roots []x509.Certificate, receivedAt time.Time) error

Create roots analyzer #40

Create roots analyzer #40

Conversation

RJPercival commented Sep 24, 2019 • edited Loading

codecov-io commented Sep 24, 2019 • edited Loading

Codecov Report

RJPercival commented Sep 24, 2019

RJPercival commented Oct 21, 2019

taknira commented Oct 21, 2019

taknira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taknira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taknira commented Jan 29, 2020

RJPercival commented Sep 24, 2019 •

edited

Loading

codecov-io commented Sep 24, 2019 •

edited

Loading