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

Zeroize secret keys upon drop #277

Merged
merged 4 commits into from
Aug 20, 2022
Merged

Zeroize secret keys upon drop #277

merged 4 commits into from
Aug 20, 2022

Conversation

tzemanovic
Copy link
Member

@tzemanovic tzemanovic commented Aug 4, 2022

moved from anoma/anoma#1166

based on #250

Closes issue #82. It seems the best way to get the memory of the SecretKey byte array to be reset to 0s is to wrap the external secret key structs (ed25519_consensus::SigningKey for example) in a Box pointer when initializing our own SecretKey structs.

I found some inspiration from this blog post, which described behavior I was observing while experimenting with this code.

@tzemanovic
Copy link
Member Author

let's rebase on #250 to check this through new CI

@tzemanovic
Copy link
Member Author

pls update wasm

tzemanovic added a commit that referenced this pull request Aug 5, 2022
@tzemanovic tzemanovic marked this pull request as ready for review August 5, 2022 14:30
Copy link
Member Author

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

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

I cannot approve here as I moved the PR, but I reviewed it and LGTM

tzemanovic added a commit that referenced this pull request Aug 19, 2022
* brent/922/zeroize-secret-keys:
  changelog: add #277
  [ci skip] wasm checksums update
  move zeroize test out of macro (also in advance of incorporating secp256k1)
  make fmt
  wrap SigningKey in a Box pointer when placing into SecretKey struct, test that memory is actually zeroized after dropping SecretKey
  changes to Cargo.toml and Cargo.lock from adding latest version of zeroize crate
  update the changelog config to namada repo
  [ci] wasm checksums update
  ledger: debug log some ABCI++ requests
  ledger: make prepare_proposal and process_proposal stateless
  ledger: refactor tx_queue
  scripts/get_tendermint: update for ABCI++ temp fork release
  test/e2e/ledger: enable ignored tests for ABCI++ workaround
  shell: process transaction when `ProcessProposal` hasn't (non-validator)
  make: remove "*-abci-plus-plus"
  all: remove "ABCI" conditional compilation
  deps: remove ABCI dependencies, use ABCI++ as default
  changelog: add #247
  test/e2e: update assert_success/failure to first consume output
  [ci] improve e2e log upload to add validator logs
@tzemanovic tzemanovic mentioned this pull request Aug 19, 2022
35 tasks
juped pushed a commit that referenced this pull request Aug 19, 2022
@juped juped force-pushed the brent/922/zeroize-secret-keys branch from 32ab54c to 3948c98 Compare August 19, 2022 16:53
@juped juped merged commit 0101aef into main Aug 20, 2022
juped added a commit that referenced this pull request Sep 20, 2022
* main: (118 commits)
  Revert "Update getting-started.md"
  Update getting-started.md
  changelog: add #442
  rustdoc: fix outdated links
  make build-doc: only build rustdoc in this command
  [ci] remove drone
  ci: double the build-and-test timeout
  [ci] download masp paramters
  [ci] improve automation tool, add pls spawn devnet command
  [ci] added dev documentation build, added rust doc build
  I removed the limitation.
  I removed the limitation.
  encoding_spec: rm ":" from fragment links in e.g. `ed25519::PublicKey`
  docs/dev: update encoding spec generation
  [fix] use fetch-depth: 0 on tag pipeline
  Namada 0.7.1
  fix spelling of 'features' in Cargo.toml
  realign cargo/wasm integrity
  changelog: add #278
  test: allow to sign and verify secp256k1
  client: add check on validator consensus key
  shared: optional secp256k1 signing and verification to avoid wasm bloat
  pick scheme for generating validator keys
  must use ed25519 for validator consensus key and node ID
  update rustdoc on PKH
  make fmt
  deps: enable secp256k1 in tendermint-rs
  handle secp256k1 in key_to_tendermint()
  fmt && clippy
  use heliaxdev/libsecp256k1 crate fork as dependency for now
  release: update release.toml for namada repo
  new test for zeroizing secp256k1 keys
  wrap libsecp256k1::SecretKey in a Box within SecretKey struct
  add a test to zeroize secp256k1
  fix bug in supplying args to test_genesis_validators()
  make validator_key_to_json() compatible with ed25519 and secp256k1 keys
  fix bug to prevent generating keys with common SchemeType
  fix bug where we were generating a key with common scheme
  make fmt
  fix unit test test_toml_roundtrip to supply good validator keys
  e2e test_genesis_validators(): make each validator have different key scheme
  fix some comments
  fix bug in supplying keypair to Tendermint
  clean and simplify code in id_from_pk()
  allow CL specification of a specific key scheme for the TxInitValidator
  improve write_tendermint_node_key() to produce the proper json given the underlying key scheme
  convert from common to underlying key type in id_from_pk() when constructing the TendermintNodeId
  change variable names in fns try_to_sk() and try_to_sig() to reduce confusion
  clean up code implementing Serialize/Deserialize, comment on certain implementations
  remove Result layering for id_from_pk
  drop 'Consensus' from SchemeType enum variants
  incorporate options into key generation functions
  command line options for specifying key scheme
  initial commit for supporting secp256k1 keys
  clean and simplify code in id_from_pk()
  allow CL specification of a specific key scheme for the TxInitValidator
  improve write_tendermint_node_key() to produce the proper json given the underlying key scheme
  convert from common to underlying key type in id_from_pk() when constructing the TendermintNodeId
  change variable names in fns try_to_sk() and try_to_sig() to reduce confusion
  clean up code implementing Serialize/Deserialize, comment on certain implementations
  remove Result layering for id_from_pk
  drop 'Consensus' from SchemeType enum variants
  make libsecp256k1 objects public when wrapped within our own Key and Sig objects
  remove clippy::bind_instead_of_map now that we will use wildcard
  incorporate options into key generation functions
  command line options for specifying key scheme
  initial commit for supporting secp256k1 keys
  changelog: add #277
  move zeroize test out of macro (also in advance of incorporating secp256k1)
  wrap SigningKey in a Box pointer when placing into SecretKey struct, test that memory is actually zeroized after dropping SecretKey
  changes to Cargo.toml and Cargo.lock from adding latest version of zeroize crate
  docs: update book's config branch, edit-url and repo links
  docs: add notes about the books
  doc/docs: s/anoma/namada
  re-add `dev` docs section
  update outer readme
  Replace anoma with namada in quickstart
  Update config files
  update links to user guide docs
  update relative paths in dev docs
  changelog: add #322
  docs: move and link to openAPI spec from Ledger RPC
  Apply suggestions from code review
  Storage key regexes should permit any UTF-8 string
  Add invalid storage key error response example
  Add example for getting an account's public key
  A key can contain any ASCII, not just alphabetical characters
  Add specs/openapi.yml
  Add changelog
  .gitignore: make some patterns relative to repo root
  Add changelog
  .gitignore: make some patterns relative to repo root
  change string to valid path
  Initial info on Secp256k1 keys and zeroizing secret keys
  crypto.md: add context for signatures
  crypto.md: remove outdated encoding descriptions
  crypto.md: remove references to closed issue
  Updates changelog
  Removes mention to clap version
  Fixes typos. Updates comment on `clap`
  ...
tzemanovic added a commit that referenced this pull request Sep 23, 2022
* tomas/fix-rustdoc: (116 commits)
  changelog: add #442
  rustdoc: fix outdated links
  make build-doc: only build rustdoc in this command
  [ci] remove drone
  ci: double the build-and-test timeout
  [ci] download masp paramters
  [ci] improve automation tool, add pls spawn devnet command
  [ci] added dev documentation build, added rust doc build
  I removed the limitation.
  I removed the limitation.
  encoding_spec: rm ":" from fragment links in e.g. `ed25519::PublicKey`
  docs/dev: update encoding spec generation
  [fix] use fetch-depth: 0 on tag pipeline
  Namada 0.7.1
  fix spelling of 'features' in Cargo.toml
  realign cargo/wasm integrity
  changelog: add #278
  test: allow to sign and verify secp256k1
  client: add check on validator consensus key
  shared: optional secp256k1 signing and verification to avoid wasm bloat
  pick scheme for generating validator keys
  must use ed25519 for validator consensus key and node ID
  update rustdoc on PKH
  make fmt
  deps: enable secp256k1 in tendermint-rs
  handle secp256k1 in key_to_tendermint()
  fmt && clippy
  use heliaxdev/libsecp256k1 crate fork as dependency for now
  release: update release.toml for namada repo
  new test for zeroizing secp256k1 keys
  wrap libsecp256k1::SecretKey in a Box within SecretKey struct
  add a test to zeroize secp256k1
  fix bug in supplying args to test_genesis_validators()
  make validator_key_to_json() compatible with ed25519 and secp256k1 keys
  fix bug to prevent generating keys with common SchemeType
  fix bug where we were generating a key with common scheme
  make fmt
  fix unit test test_toml_roundtrip to supply good validator keys
  e2e test_genesis_validators(): make each validator have different key scheme
  fix some comments
  fix bug in supplying keypair to Tendermint
  clean and simplify code in id_from_pk()
  allow CL specification of a specific key scheme for the TxInitValidator
  improve write_tendermint_node_key() to produce the proper json given the underlying key scheme
  convert from common to underlying key type in id_from_pk() when constructing the TendermintNodeId
  change variable names in fns try_to_sk() and try_to_sig() to reduce confusion
  clean up code implementing Serialize/Deserialize, comment on certain implementations
  remove Result layering for id_from_pk
  drop 'Consensus' from SchemeType enum variants
  incorporate options into key generation functions
  command line options for specifying key scheme
  initial commit for supporting secp256k1 keys
  clean and simplify code in id_from_pk()
  allow CL specification of a specific key scheme for the TxInitValidator
  improve write_tendermint_node_key() to produce the proper json given the underlying key scheme
  convert from common to underlying key type in id_from_pk() when constructing the TendermintNodeId
  change variable names in fns try_to_sk() and try_to_sig() to reduce confusion
  clean up code implementing Serialize/Deserialize, comment on certain implementations
  remove Result layering for id_from_pk
  drop 'Consensus' from SchemeType enum variants
  make libsecp256k1 objects public when wrapped within our own Key and Sig objects
  remove clippy::bind_instead_of_map now that we will use wildcard
  incorporate options into key generation functions
  command line options for specifying key scheme
  initial commit for supporting secp256k1 keys
  changelog: add #277
  move zeroize test out of macro (also in advance of incorporating secp256k1)
  wrap SigningKey in a Box pointer when placing into SecretKey struct, test that memory is actually zeroized after dropping SecretKey
  changes to Cargo.toml and Cargo.lock from adding latest version of zeroize crate
  docs: update book's config branch, edit-url and repo links
  docs: add notes about the books
  doc/docs: s/anoma/namada
  re-add `dev` docs section
  update outer readme
  Replace anoma with namada in quickstart
  Update config files
  update links to user guide docs
  update relative paths in dev docs
  changelog: add #322
  docs: move and link to openAPI spec from Ledger RPC
  Apply suggestions from code review
  Storage key regexes should permit any UTF-8 string
  Add invalid storage key error response example
  Add example for getting an account's public key
  A key can contain any ASCII, not just alphabetical characters
  Add specs/openapi.yml
  Add changelog
  .gitignore: make some patterns relative to repo root
  Add changelog
  .gitignore: make some patterns relative to repo root
  change string to valid path
  Initial info on Secp256k1 keys and zeroizing secret keys
  crypto.md: add context for signatures
  crypto.md: remove outdated encoding descriptions
  crypto.md: remove references to closed issue
  Updates changelog
  Removes mention to clap version
  Fixes typos. Updates comment on `clap`
  docs/dev: remove PoS spec and link to spec page instead
  changelog: add #1070
  ...
@brentstone brentstone deleted the brent/922/zeroize-secret-keys branch December 5, 2022 01:21
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