Deterministic Keystore by KPrasch · Pull Request #2701 · nucypher/nucypher

KPrasch · 2021-05-19T06:01:18Z

Type of PR:
Feature & Rework

Required reviews:
3

What this does:

NuCypher Keystore

Support restoration of Keystore (formerly keyring) from mnemonic seed phrase
Use HKDF info strings to derive umbral private keys from secret material and info
Reworks keyring functionality to implement deterministic power derivation from secret
Simplified keystore file and metadata format for encrypted seed
Reduces Scrypt derivation difficulty (subject to further change)
Completely decouples ethereum account from keystore identification in favor of stamps
CLI interactions for mnemonic generation and recovery (ursula init & ursula recover, additional follow-up in Backup and restore Ursula config #2682)
Shorten minimum password length to 8 characters

TLS

Derive TLS private key instead of storing it to disk
Removes checksum addresses as TLS certificate pseudonyms and related utilities
Use IP addressing to index certificates and generate filenames
Revert self-signed certificate generation to each startup (needs follow up in [WIP] Update Ursulas certificate #2700)

Rename and reorg

Renames NucyperKeyring to Keystore
Creates new module nucypher.crypto.keystore
Creates new module nucypher.crypto.tls
Deprecates nucypher.crypto.api (moves contents to nuycpher.crypto.utils and nucypher.crypto.tls)
Corrects inaccurate Ursula configuration field checksum_address -> worker_address

Misc.

Index node metadata by stamp instead of checksum address (to accompany decoupling of checksum address from node storages)

Issues fixed/closed:

Closes Use deterministic key derivation to produce Keyring keys #2225
Closes Iteration on current password-based KDF #2654
Reopens Reconsider Embedded Emitters #1754

Notes for reviewers

The new version of keyring (now "keystore") is not presently backwards compatible with older versions of nucypher. If we need to preserve existing private keys, we can first expand the functionality of this PR by resolving #2655 and building a migration utility that implements it. However, given the volume of policies and the standing probationary period, I do not see much added value and suggest that we require all nodes to "re-init" thier nodes generating a fresh keystore and mnemonic.

Why it's needed:

Introduces mechanisms to exercise "Keystore Diligence"
Improves umbral key portability for nucypher characters in support of browser compatibility
Supports https://github.com/nucypher/nucypher/projects/16
Supports https://github.com/nucypher/nucypher/projects/18
Optimize startup authentication

piotr-roslaniec · 2021-05-26T12:49:18Z

nucypher/crypto/keystore.py

+
+def validate_keystore_password(password: str) -> List:
+    """
+    NOTICE: Do not raise inside this function.


Could you expand on this comment a bit? Why we'd like to be careful here, to avoid leaking the password?

This is a fairly dated comment now, but it's still meant to describe two cases:

(Yes) Logging of the local scope's context to sentry, which includes the password.

Usage as a click interactive prompt validator which requires a boolean response.

Logging of the local scope's context to sentry, which includes the password.

Would it make sense to pass around the password in a container class with redefined str/repr, unpacking only right before usage? Or is it an overcomplication?

Would it make sense to pass around the password in a container class with redefined

Hmm not a bad idea, I wonder if this can effectively prevent unintentional transmission... Sounds like something that belongs in the passwords.py module from your recent PR, but perhaps in a follow-up. I'll open an issue about this after the review process advances a bit.

piotr-roslaniec · 2021-05-26T14:56:36Z

nucypher/crypto/utils.py

+    :return: bytes
+    """
+    # TODO: Should we just use os.urandom or avoid the import w/ this?
+    return SYSTEM_RAND.getrandbits(num_bytes * 8).to_bytes(num_bytes, byteorder='big')


os.random would be a bit more concise. For reference, SystemRandom is using os.urandom underneath, and both of those methods are not cross-platform compatible.

I'm somewhat compelled to use the secrets module as a source of randomness for added paranoia. https://docs.python.org/3/library/secrets.html

Switched to the secrets version of SystemRandom.

docs/source/application_development/getting_started.rst

nucypher/cli/actions/auth.py

nucypher/config/base.py

nucypher/crypto/keystore.py

fjarri · 2021-06-16T04:06:00Z

nucypher/crypto/utils.py

    return canonical_address
+
+
+def get_coordinates_as_bytes(point: Union[Point, UmbralPublicKey, SignatureStamp],


I think it was removed in #2699

Hmm, it looks like the function above (canonical_address_from_umbral_key) which uses this function still has calls ans references. Perhaps it's not completely removed in main yet?

canonical_address_from_umbral_key() is also modified to not require it. In fact, in #2612 crypto/utils.py got pretty small. See https://github.com/fjarri/nucypher/blob/7f36abde216d5b1e751e10a95d6f382d7e296474/nucypher/crypto/utils.py

fjarri · 2021-06-16T04:09:08Z

nucypher/crypto/utils.py

+    return digest
+
+
+def ecdsa_sign(message: bytes,


Note: it's being removed in #2612. If I'm not mistaken, it does the same as umbral's signing/verifying functions.

nucypher/cli/actions/auth.py

nucypher/crypto/keystore.py

tests/integration/config/test_keyring_integration.py

tests/unit/crypto/test_keystore.py

nucypher/cli/actions/auth.py

…word enviorment variable.

KPrasch · 2021-06-28T21:41:23Z

Rebased over cc578d9

fjarri · 2021-06-30T21:14:02Z

nucypher/cli/painting/help.py

-    if new_configuration.keyring != NO_KEYRING_ATTACHED:
-        maybe_public_key = bytes(new_configuration.keyring.signing_public_key).hex()
+    if new_configuration.keystore != NO_KEYSTORE_ATTACHED:
+        maybe_public_key = new_configuration.keystore.id


Seems that now it's not a public key at all.

I admit the naming is a bit misleading here, but the Keystore's ID is set to the stamp's hex.

fjarri · 2021-06-30T21:16:52Z

nucypher/crypto/utils.py

+    return signature_is_valid
+
+
+def verify_ecdsa(message: bytes,


What's the difference between this function and Signature.verify()?

Removed ✔️

fjarri · 2021-06-30T21:18:16Z

nucypher/network/nodes.py

 from nucypher.network.middleware import RestMiddleware
 from nucypher.network.protocols import SuspiciousActivity
 from nucypher.utilities.logging import Logger
+from umbral.signing import Signature


Rebase artefact? Should be imported from umbral_adapter.

Thanks. I think I rebased this PR before realizing that I needed to import from umbral adapter. I'll make an update.

fjarri · 2021-06-30T21:18:51Z

nucypher/policy/collections.py

-from cryptography.hazmat.primitives import hashes
 from eth_utils import to_canonical_address, to_checksum_address
-from typing import Optional, Tuple
+from umbral.signing import Signature


Again, needs umbral_adapter here

fjarri · 2021-06-30T21:21:51Z

tests/mock/performance_mocks.py


 class NotAPrivateKey:

+    fake_signature = Signature.from_bytes(


Is it a rebase thing, or it's actually used somewhere?

fjarri · 2021-06-30T21:28:41Z

nucypher/crypto/utils.py

 from eth_keys import KeyAPI as EthKeyAPI
-from typing import Any, Union
+from eth_utils.address import to_checksum_address
+from umbral.signing import Signature


umbral_adapter here

cygnusv

Some preliminary comments, so far looking good!

cygnusv · 2021-07-02T09:54:51Z

nucypher/crypto/api.py

@@ -1,219 +0,0 @@
-"""


Thank goodness, the crypto.api module was too old and unspecific

cygnusv · 2021-07-02T09:57:26Z

nucypher/crypto/keystore.py

+__HKDF_HASH_ALGORITHM = BLAKE2B
+__INFO_BASE = b'NuCypher/'
+_VERIFYING_INFO = __INFO_BASE + b'verify'
+_DECRYPTING_INFO = __INFO_BASE + b'encrypt'


Suggested change

_DECRYPTING_INFO = __INFO_BASE + b'encrypt'

_DECRYPTING_INFO = __INFO_BASE + b'decrypt'

cygnusv · 2021-07-02T09:59:23Z

nucypher/crypto/keystore.py

+# HKDF
+__HKDF_HASH_ALGORITHM = BLAKE2B
+__INFO_BASE = b'NuCypher/'
+_VERIFYING_INFO = __INFO_BASE + b'verify'


Rather than verifying, the angle here should be "signing". This is also more consistent with the Powers namespace (see line 268).

cygnusv · 2021-07-02T10:20:16Z

nucypher/crypto/keystore.py

+    if not salt and not info:
        raise ValueError('Info or salt must be provided.')
    info = info or bytes()
    salt = salt or bytes()


These 2 lines should be dropped. The first one is simply redundant, but the second is explicitly deviating us from the standard. I doubt this poses any security problem, but it's unnecessarily making future interoperability more difficult.
If info or salt are None, simply let HKDF handle that according to the specification.

(Update: it seems this method was dropped anyway in the yesterday's commit, so nothing to see here)

KPrasch

This PR has three approvals - Feel free to continue reviewing but any additional RFCs will be completed in a follow-up PR.

KPrasch added Enhancement New or improved features API labels May 19, 2021

KPrasch requested a review from piotr-roslaniec May 19, 2021 06:01

KPrasch assigned KPrasch and piotr-roslaniec May 19, 2021

piotr-roslaniec reviewed May 26, 2021

View reviewed changes

KPrasch force-pushed the keyring-v2 branch 2 times, most recently from 97e3187 to b2518e2 Compare June 10, 2021 02:14

KPrasch added a commit to KPrasch/nucypher that referenced this pull request Jun 15, 2021

Newsfragment for PR nucypher#2701

31f79f3

KPrasch added a commit to KPrasch/nucypher that referenced this pull request Jun 16, 2021

Newsfragment for PR nucypher#2701

dd3d256

KPrasch force-pushed the keyring-v2 branch from 31f79f3 to dd3d256 Compare June 16, 2021 03:14

KPrasch changed the title ~~[WIP] Deterministic Keystore~~ Deterministic Keystore Jun 16, 2021

KPrasch requested review from cygnusv, derekpierre, fjarri, jMyles and vepkenez June 16, 2021 03:15

KPrasch marked this pull request as ready for review June 16, 2021 03:15