This repository was archived by the owner on Oct 11, 2024. It is now read-only.
Non-verifying monitor #776
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's work in progress addressed some early review comments
simplify monitor Dockerfile (non-opinionated) add argument to 'prepare script' such that the monitor can contact the kt-server if it isn't reachable via localhost add proto messages that proof certain errors occurred early review comments add signing capability move verification into separate function gofmt golint / presubmit.sh regenerate proto with correct dependency versions add script for generating monitor signing key Add comments to exported methods Add "observed at" timestamp, sign the root hash update test
Minor changes verify signature on response resolves google#672 Add log sig verification rebase wip WIP add TODO remove streaming API (simplifies moving to core) Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's work in progress revert local changes, add TODOs simplify monitor Dockerfile (non-opinionated)
Minor changes verify signature on response resolves google#672 Add log sig verification rebase wip WIP add TODO remove streaming API (simplifies moving to core) Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's Monitor service and types: regenerate protos and downgrade grpc-gateway to match trillian's work in progress revert local changes, add TODOs simplify monitor Dockerfile (non-opinionated) regenerate proto with correct dependency versions Add to kubernetes config and deploy script resolves google#672 rebase rebase =gofmt WIP
* move monitor service types into separate file * move verification (wip) into sperate file * generate and use separate priv. key for signing * pass pointer of priv key * unexport verifKeys
* Remove replacer which isn't used anywhere
# Conflicts: # core/client/kt/requests.go # core/mutator/entry/entry.go # core/mutator/entry/entry_test.go # impl/proto/monitor_v1_service/gen.go
# Conflicts: # docker-compose.yml
gdbelvin
reviewed
Aug 21, 2017
cmd/keytransparency-monitor/main.go
Outdated
| glog.Fatalf("Could not read domain info %v:", err) | ||
| } | ||
|
|
||
| srv := monitor.New(mcc, crypto.NewSHA256Signer(key), logTree, mapTree, *pollPeriod) |
Contributor
There was a problem hiding this comment.
Please use a different New function from crypto. NewSHA256Signer is going away.
liamsi
force-pushed
the
non_verifying_monitor
branch
from
d9fc2c8 to
8603214
Compare
liamsi
force-pushed
the
non_verifying_monitor
branch
from
6451c46 to
42a855b
Compare
gdbelvin
reviewed
Aug 22, 2017
core/monitor/verify.go
Outdated
| // Additionally to the response it takes a complete list of mutations. The list | ||
| // of received mutations may differ from those included in the initial response | ||
| // because of the max. page size. | ||
| func (m *Monitor) VerifyResponse(in *ktpb.GetMutationsResponse, allMuts []*ktpb.Mutation) *mopb.GetMonitoringResponse { |
Contributor
There was a problem hiding this comment.
Should this return error?
Contributor
Author
There was a problem hiding this comment.
It might later (in another PR). All "errors" related to failed validation will go into the GetMonitoringResponse
gdbelvin
reviewed
Aug 22, 2017
impl/monitor/monitor.go
Outdated
| // TODO(ismail) use domain info to properly init. the monitor: | ||
| monitor: &cmon.Monitor{}, | ||
| signer: signer, | ||
| proccessedSMRs: make([]*mopb.GetMonitoringResponse, 256), |
Contributor
There was a problem hiding this comment.
Please move non-grpc functions to their own object.
They can be in the same package.
liamsi
force-pushed
the
non_verifying_monitor
branch
from
1fe5a23 to
cdc64de
Compare
liamsi
force-pushed
the
non_verifying_monitor
branch
from
82ad8e2 to
d6333c8
Compare
liamsi
force-pushed
the
non_verifying_monitor
branch
from
d6333c8 to
b881c04
Compare
gdbelvin
approved these changes
Aug 23, 2017
…on_verifying_monitor
This was referenced Aug 23, 2017
liamsi
force-pushed
the
non_verifying_monitor
branch
from
7fa59ef to
153d655
Compare
Codecov Report
@@ Coverage Diff @@
## master #776 +/- ##
=========================================
- Coverage 48.14% 47.04% -1.1%
=========================================
Files 28 32 +4
Lines 2476 2553 +77
=========================================
+ Hits 1192 1201 +9
- Misses 1098 1166 +68
Partials 186 186
Continue to review full report at Codecov.
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains everything a monitor would needs to do besides the verification logic. This PR contains all review comments from: #709
The verification logic comes with a separate PR: #768