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

go/oasis-test-runner/oasis: Add a keymanager replication test #2843

Merged
merged 9 commits into from
May 5, 2020

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented Apr 15, 2020

  • Add a basic replication test.
    • Spin up a replica.
    • Explicitly query the replica or something.
  • Fix bugs.
    • Oh my god, the key manager worker host doesn't support enclave rpc.
    • Key manager client should support km->km.
    • Fix the key manager grpc access control.

@Yawning Yawning added c:testing Category: testing c:key management Category: key management labels Apr 15, 2020
@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch 9 times, most recently from 902f2b0 to 7a1c23d Compare April 17, 2020 12:37
@kostko kostko linked an issue Apr 20, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #2843 into master will decrease coverage by 0.02%.
The diff coverage is 66.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2843      +/-   ##
==========================================
- Coverage   67.60%   67.57%   -0.03%     
==========================================
  Files         349      350       +1     
  Lines       33796    33960     +164     
==========================================
+ Hits        22849    22950     +101     
- Misses       8009     8051      +42     
- Partials     2938     2959      +21     
Impacted Files Coverage Δ
go/keymanager/api/grpc.go 50.98% <39.47%> (-33.64%) ⬇️
go/worker/keymanager/init.go 71.42% <58.33%> (-1.46%) ⬇️
go/keymanager/client/client.go 82.56% <62.50%> (+7.56%) ⬆️
go/worker/keymanager/watcher.go 65.71% <65.71%> (ø)
go/worker/keymanager/handler.go 67.92% <71.42%> (+6.38%) ⬆️
go/worker/keymanager/worker.go 64.58% <95.65%> (+6.07%) ⬆️
go/consensus/tendermint/keymanager/keymanager.go 86.74% <100.00%> (ø)
go/keymanager/api/api.go 70.00% <100.00%> (+1.03%) ⬆️
go/oasis-node/cmd/node/node.go 54.06% <100.00%> (+0.11%) ⬆️
go/worker/common/runtime_host.go 66.18% <100.00%> (+0.74%) ⬆️
... and 37 more

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 e83231b...5378569. Read the comment docs.

@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch 2 times, most recently from 22dbf25 to 7a1fa12 Compare April 23, 2020 12:01
go/worker/keymanager/handler.go Outdated Show resolved Hide resolved
go/worker/keymanager/handler.go Outdated Show resolved Hide resolved
@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch 14 times, most recently from 56785c9 to 2687d40 Compare April 30, 2020 11:35
@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch 2 times, most recently from fc084c0 to 63d39f3 Compare May 4, 2020 11:03
@Yawning
Copy link
Contributor Author

Yawning commented May 4, 2020

Well, replication works. The rust side simple encryption test client will panic when the query goes to the replica, but that's an orthogonal problem. Either this doesn't retry, or the retry logic is busted.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: RpcFailure(RpcStatus { status: Unknown, details: Some("worker/keymanager: error from runtime: decrypt error") })', tests/clients/simple-keyvalue-enc/src/main.rs:99:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@Yawning Yawning marked this pull request as ready for review May 4, 2020 11:08
@Yawning Yawning changed the title w1p: go/oasis-test-runner/oasis: Add a keymanager replication test go/oasis-test-runner/oasis: Add a keymanager replication test May 4, 2020
@kostko
Copy link
Member

kostko commented May 4, 2020

The rust side simple encryption test client will panic when the query goes to the replica, but that's an orthogonal problem. Either this doesn't retry, or the retry logic is busted

Rust part of the retry mechanism should work and is also covered in unit tests. Currently the Rust client will only retry three times though so depending on the situation this may not be enough. It could also be a problem with the round robin node selection policy.

@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch 6 times, most recently from 2a96f63 to 8e0b3c6 Compare May 5, 2020 06:18
matevz
matevz previously requested changes May 5, 2020
go/oasis-test-runner/oasis/keymanager.go Outdated Show resolved Hide resolved
go/oasis-test-runner/oasis/keymanager.go Outdated Show resolved Hide resolved
Yawning added 6 commits May 5, 2020 07:53
Access control forbidding replication may be more secure, but is not all
that useful.
This fixture stuff is over-complication for what ostensibly was a KISS
test harness, that will become even more of an overcomplicated nightmare
once a poor damned soul makes policies specify more than just the
keymanager runtime ID and serial number.

Hopefully someone that's not me gets to have that particular honor.
It is likely prudent to bind the persisted master secret to the runtime
ID.  This change does so by including the key manager runtime ID as the
AAD when sealing the master secret.

This is backward incompatible with all current key manager instances as
the existing persisted master secret will not decrypt.
@Yawning Yawning force-pushed the yawning/feature/km-replication-test branch from 3085711 to 5378569 Compare May 5, 2020 07:54
@Yawning Yawning merged commit a408af7 into master May 5, 2020
@Yawning Yawning deleted the yawning/feature/km-replication-test branch May 5, 2020 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:key management Category: key management c:testing Category: testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests for key manager replication
3 participants