Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

fixed #2263, geth keys with ciphertext shorter than 32 bytes#2318

Merged
gavofyork merged 4 commits into
masterfrom
short_ciphertext
Sep 28, 2016
Merged

fixed #2263, geth keys with ciphertext shorter than 32 bytes#2318
gavofyork merged 4 commits into
masterfrom
short_ciphertext

Conversation

@debris
Copy link
Copy Markdown
Collaborator

@debris debris commented Sep 26, 2016

Fixes https://github.com/ethcore/parity/issues/2263

We also need to add additional tests. @NikVolf can you send me keyfile with short ciphertext?

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 26, 2016
rphmeier
rphmeier reviewed Sep 26, 2016
View reviewed changes
Comment thread ethstore/src/json/bytes.rs
use rustc_serialize::hex::{ToHex, FromHex, FromHexError};

#[derive(Debug, PartialEq)]
pub struct Bytes(Vec<u8>);
Copy link
Copy Markdown
Contributor

@rphmeier rphmeier Sep 26, 2016

Choose a reason for hiding this comment

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

this is the third definition of this type afaict (rpc, json, and now here) -- is it possible to be more DRY?

Copy link
Copy Markdown
Collaborator Author

@debris debris Sep 26, 2016

Choose a reason for hiding this comment

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

They share the same name, but they are not the same. json::Bytes deserialization is extremely lenient, rpc::Bytes deserialization allows 0x prefixes. Here, the format is very strict

rphmeier
rphmeier reviewed Sep 26, 2016
View reviewed changes
Comment thread ethstore/src/json/hash.rs Outdated

impl From<&'static str> for $name {
fn from(s: &'static str) -> Self {
s.parse().unwrap()
Copy link
Copy Markdown
Contributor

@rphmeier rphmeier Sep 26, 2016

Choose a reason for hiding this comment

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

.expect("invalid string literal for {}: '{}'", stringify!($name), s) would be a little more helpful

Copy link
Copy Markdown
Collaborator Author

@debris debris Sep 26, 2016

Choose a reason for hiding this comment

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

👍

@gavofyork
Copy link
Copy Markdown
Contributor

gavofyork commented Sep 26, 2016

CI failing.

gavofyork
gavofyork approved these changes Sep 26, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

I'd +1 @rphmeier 's call for DRY but looks good otherwise.

@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Sep 26, 2016

CI failure is unrelated, I restarted it

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 26, 2016

Coverage Status

Coverage decreased (-0.006%) to 85.208% when pulling e586b05 on short_ciphertext into abcdc81 on master.

@gavofyork
Copy link
Copy Markdown
Contributor

gavofyork commented Sep 26, 2016

would be nice to have a test; should be merged for 1.3.2 regardless, but hopefully @NikVolf can get a test in before.

@gavofyork gavofyork added B0-patch A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 26, 2016
@gavofyork gavofyork modified the milestone: 1.3.2 Sep 26, 2016
@NikVolf
Copy link
Copy Markdown
Contributor

NikVolf commented Sep 27, 2016

according to Gustav, test case can be obtained by generating enough keys in geth
just keep creating until you encounter short one or use Gustav script there
https://github.com/ethcore/parity/issues/2263

godep go test -run TestKeyStorePassphrase -count 500

@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Sep 27, 2016

I tried that script, but keys are not printed to the output by default. I asked Gustav for help with that. I will update the pr as soon as he responds :)

@Gustav-Simonsson
Copy link
Copy Markdown

Gustav-Simonsson commented Sep 27, 2016 •
edited
Loading

Added two test vectors for keys with length 31 and 30: https://github.com/ethereum/go-ethereum/pull/3032/files

@NikVolf @debris Yes that test will generate shorter keys if enough iterations a run; how to log them:

-       return json.Marshal(encryptedKeyJSONV3)
+       j, err := json.Marshal(encryptedKeyJSONV3)
+       if len(keyBytes) == 31 {
+               fmt.Printf("31 keyBytes: %x\njson: %s\n", keyBytes, j)
+       }
+       return j, err

at https://github.com/ethereum/go-ethereum/blob/master/accounts/key_store_passphrase.go#L144

@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Sep 28, 2016

tests are failing, I'm working on fix now :)

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Sep 28, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 28, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 28, 2016

Coverage Status

Coverage increased (+0.02%) to 85.207% when pulling 3f1f609 on short_ciphertext into fb92a98 on master.

@gavofyork gavofyork merged commit c0e7220 into master Sep 28, 2016
@gavofyork gavofyork deleted the short_ciphertext branch September 28, 2016 13:47
jacogr added a commit that referenced this pull request Sep 29, 2016
@jacogr
Merge branch 'master' into js
50adb8a 
* master:
  Fixing Delegate Call in JIT (#2378)
  Prioritizing re-imported transactions (#2372)
  Revert #2172, pretty much. (#2387)
  correct sync memory usage calculation (#2385)
  Update gitlab-ci
  Fix the traceAddress field in transaction traces. (#2373)
  Removing extras data from retracted blocks. (#2375)
  fixed #2263, geth keys with ciphertext shorter than 32 bytes (#2318)
  Expanse compatibility (#2369)
  Specify column cache sizes explicitly; default fallback of 2MB (#2358)
  Canonical state cache (master) (#2311)
  make block queue into a more generic verification queue and fix block heap size calculation (#2095)
  Hash Content RPC method (#2355)
  Reorder transaction_by_hash to favour canon search (#2332)
  DIV optimization (#2327)
  Error when deserializing invalid hex (#2339)
  Changed http:// to https:// on some links (#2349)
  add a test
  fix migration system, better errors

# Conflicts:
#	.gitlab-ci.yml
arkpar pushed a commit that referenced this pull request Oct 4, 2016
@debris @arkpar
fixed #2263, geth keys with ciphertext shorter than 32 bytes (#2318)
a0b71a9 
* fixed #2263, geth keys with ciphertext shorter than 32 bytes

* replace unwrap with more helpful expect

* tests for decrypting short secrets
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@rphmeier rphmeier rphmeier left review comments

@gavofyork gavofyork gavofyork approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

1.3.2

Development

Successfully merging this pull request may close these issues.

7 participants

@debris @gavofyork @coveralls @NikVolf @Gustav-Simonsson @rphmeier @arkpar