Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

hc-dpki cli for DeepKey #2117

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

hc-dpki cli for DeepKey #2117

wants to merge 10 commits into from

Conversation

zo-el
Copy link
Member

@zo-el zo-el commented Feb 17, 2020

PR summary

Adds some new commands to the CLI tool for managing DPKI keys.

  • hc dpki genroot creates a new random root seed
  • hc dpki keygen is similar to hc keygen but will use a dpki root seed mnemonic so that keys can be recovered
  • hc dpki genrevoke/genauth produce revocation/auth seeds from which keys can be derived.
  • hc dpki sign use a revocation/auth seed to produce a key then sign a message (which can be an agent key). This is the process by which keys can be authorized and revoked

Each time a mnemonic is produced it is optionally encrypted with a passphrase

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@zippy
Copy link
Member

zippy commented Feb 20, 2020

@zo-el I notice a bunch of tests are breaking, would like to review this after tests pass...

@zo-el zo-el changed the title hc-dpki cli for DeepKey [WIP]: hc-dpki cli for DeepKey Feb 20, 2020
@zo-el
Copy link
Member Author

zo-el commented Feb 20, 2020

@zo-el I notice a bunch of tests are breaking, would like to review this after tests pass...

Yes @zippy, this is still work in progress. I apologise.

@zo-el zo-el changed the title [WIP]: hc-dpki cli for DeepKey hc-dpki cli for DeepKey Feb 24, 2020
@zo-el
Copy link
Member Author

zo-el commented Feb 26, 2020

@zippy this PR is ready for review

@zo-el zo-el added the NEEDS REVIEW This PR requires peer code review label Feb 27, 2020
quiet,
);
keygen_standalone(keystore_passphrase)?
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed a few panics in here even though we are returning an HCResult, is this because we cannot recover from them if they happen?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, these expects should be replaced with ?

.read_line(&mut input)
.expect("Could not read from stdin");
match input.as_str() {
"Y\n" => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care about lowercase/uppercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not Really

&mut self.inner.buf,
)?;
Ok(KeyBundle::new_from_seed_buf(&mut ref_seed_buf)?)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably generalize these two structs with an enum.

enum Seed
{ Auth,Revocation}

Copy link
Contributor

@StaticallyTypedAnxiety StaticallyTypedAnxiety left a comment

Choose a reason for hiding this comment

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

Looks good but had a few questions about the number of expects we have in here

Copy link
Member

@maackle maackle left a comment

Choose a reason for hiding this comment

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

I agree with @AshantiMutinta's comments

quiet,
);
keygen_standalone(keystore_passphrase)?
};
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, these expects should be replaced with ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NEEDS REVIEW This PR requires peer code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants