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

Reask password when client enters the wrong password to use a private key file #3384

Merged
merged 11 commits into from Sep 18, 2019

Conversation

@wu-s-john
Copy link
Contributor

commented Sep 9, 2019

Better handling errors for errors using secrets library
Removed the Or_error type in password because it was not needed anymore

Thank you for contributing to Coda! Please see CONTRIBUTING.md if you haven't
yet.

Explain how you tested your changes here:

I tried using the following command:

src/_build/default/app/cli/src/coda.exe daemon -propose-key /Users/johnwu/Desktop/priv_key

And inputted an incorrect password. As a result, I was asked to input a correct password.

Checklist:

  • Tests were added for the new behavior
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:
Better handling errors for errors using secrets library
Removed the Or_error type in password because it was not needed anymore
@wu-s-john wu-s-john requested review from cmr and imeckler as code owners Sep 9, 2019
| Incorrect_password_or_corrupted_privkey ->
raise Incorrect_password_or_corrupted_privkey

let curropted_privkey error = Error (Corrupted_privkey error)

This comment has been minimized.

Copy link
@Schmavery

Schmavery Sep 9, 2019

Contributor

corrupted maybe

| Error (Unix.Unix_error ((EACCES as e), _, _)) ->
Deferred.Or_error.errorf "could not mkdir -p %s: %s\n" dn (message e)
Deferred.return @@ Privkey_error.corrupted_privkey
@@ Error.createf !"could not mkdir -p %s: %s\n" dn (message e)

This comment has been minimized.

Copy link
@cmr

cmr Sep 12, 2019

Contributor

you don't need the ! if you aren't using any of the ppx_custom_printf extensions right?

@cmr
cmr approved these changes Sep 12, 2019
@@ -119,12 +122,12 @@ let%test_unit "successful roundtrip" =
~trials:4
~f:(fun (password, plaintext) ->
let enc = encrypt ~password:(Bytes.copy password) ~plaintext in
let dec = decrypt enc ~password |> Or_error.ok_exn in
let dec = Option.value_exn (decrypt enc ~password |> Result.ok) in

This comment has been minimized.

Copy link
@cmr

cmr Sep 12, 2019

Contributor

why is this preferable?

This comment has been minimized.

Copy link
@wu-s-john

wu-s-john Sep 13, 2019

Author Contributor

decrypt does not return an Or_error.t anymore. The type is some ('a, [Variant of errors]) Result.t.

This comment has been minimized.

Copy link
@wu-s-john

wu-s-john Sep 13, 2019

Author Contributor

So that is why we have to use this way.

This comment has been minimized.

Copy link
@bkase

bkase Sep 17, 2019

Contributor

Is there not a Result.ok_exn?

wu-s-john added 2 commits Sep 16, 2019
Copy link
Contributor

left a comment

Small stuff, otherwise looks good!

| Corrupted_privkey of Error.t
| Incorrect_password_or_corrupted_privkey
| Cannot_open_file of string
| Parent_directory_does_not_exist of string

This comment has been minimized.

Copy link
@bkase

bkase Sep 17, 2019

Contributor

Can you make the exception:

exception Privkey_exn of t

and that way we don't duplicate the code too much here

@@ -119,12 +122,12 @@ let%test_unit "successful roundtrip" =
~trials:4
~f:(fun (password, plaintext) ->
let enc = encrypt ~password:(Bytes.copy password) ~plaintext in
let dec = decrypt enc ~password |> Or_error.ok_exn in
let dec = Option.value_exn (decrypt enc ~password |> Result.ok) in

This comment has been minimized.

Copy link
@bkase

bkase Sep 17, 2019

Contributor

Is there not a Result.ok_exn?

@@ -24,4 +26,4 @@ end
val encrypt : password:Bytes.t -> plaintext:Bytes.t -> t

(** Decrypt some bytes with a password *)
val decrypt : password:Bytes.t -> t -> Bytes.t Core.Or_error.t
val decrypt : password:Bytes.t -> t -> (Bytes.t, Privkey_error.t) Result.t

This comment has been minimized.

Copy link
@bkase

bkase Sep 17, 2019

Contributor

Typically it's nice to put errors behind a polymorphic variant so they compose better: see https://keleshev.com/composable-error-handling-in-ocaml

@wu-s-john

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Result.ok_exn only works if the error type is exn. However, our error type is our own custom polymorphic variant.

@bkase
bkase approved these changes Sep 18, 2019
Copy link
Contributor

left a comment

Looks good!

mergify bot added 2 commits Sep 18, 2019
@mergify mergify bot merged commit ae7893a into develop Sep 18, 2019
19 of 22 checks passed
19 of 22 checks passed
ci/circleci: build-wallet Your tests failed on CircleCI
Details
ci/circleci: test--test_postake_holy_grail Your tests failed on CircleCI
Details
ci/circleci: build-macos CircleCI is running your tests
Details
Rule: automatically merge approved PRs with the ready-to-merge label (merge) The pull request has been merged automatically
Details
Summary 1 rule matches
Details
ci/circleci: build-artifacts--testnet_postake_medium_curves Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test--fake_hash Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_bootstrap Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_catchup Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_delegation Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_txns Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_three_proposers Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_txns Your tests passed on CircleCI!
Details
ci/circleci: test-unit--dev Your tests passed on CircleCI!
Details
ci/circleci: test-unit--test_postake_snarkless_unittest Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details
@mergify mergify bot deleted the fix/handled-exeption-wrong-password branch Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.