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

cmd: harden the security of privkey field in config show #3141

Merged
merged 3 commits into from
Sep 4, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Aug 29, 2016

also add test for lower case config replace.

@Kubuxu Kubuxu added the need/review Needs a review label Aug 29, 2016
@Kubuxu Kubuxu added this to the ipfs-0.4.3-rc4 milestone Aug 29, 2016
@Kubuxu Kubuxu added the status/in-progress In progress label Aug 29, 2016
@Kubuxu Kubuxu changed the base branch from master to version/0.4.3-rc4 August 29, 2016 18:55
@Kubuxu Kubuxu added need/review Needs a review and removed need/review Needs a review status/in-progress In progress labels Aug 29, 2016
@whyrusleeping
Copy link
Member

@Kubuxu I would do this more like: https://gist.github.com/whyrusleeping/ec7e22a5f331bf9f1309f38aa366a343

Just to be a bit more generic about scrubbing this, and making sure we don't have issues in other cases (such as "Identify" having its case changed)

return
}

delete(idmap, "PrivKey")
privKeyKey := "" // make sure we both find the name of privkey and we delete it
for key, _ := range idmap {
Copy link
Member

Choose a reason for hiding this comment

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

  • and idmap will be nil if cfg["Identity"] doesnt work and there is cfg["identity"] instead.
  • As we discussed, this is turning out to be a mess. easier to move the key out.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is nil then command fails, which fails the sharness tests.

@whyrusleeping
Copy link
Member

@Kubuxu wanna review this when you get a chance?

@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 3, 2016

LGTM, we might want to add test time assertions using reflection or selectors on default config that those selectors are up to date.

And two of my commits need to be squshed.

Kubuxu and others added 3 commits September 4, 2016 06:50
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping
Copy link
Member

Alright, this looks good to me. 🚢 🚄 🚅 🚜 🚆

@whyrusleeping whyrusleeping merged commit 8803a76 into version/0.4.3-rc4 Sep 4, 2016
@whyrusleeping whyrusleeping deleted the feat/cmd/config-priv branch September 4, 2016 15:27
@jbenet jbenet mentioned this pull request Sep 4, 2016
58 tasks
for _, k := range key[:len(key)-1] {
foundk, val, ok := find(cur, k)
if !ok {
return fmt.Errorf("failed to find specified key")
Copy link
Member

Choose a reason for hiding this comment

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

wont this error out all the way to the user, if the user provides a config without an Identity.PrivKey value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants