Prevent import keys from accepting unprefixed ids. #6344

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
3 participants
Contributor

perrito666 commented Sep 28, 2016

Accepting up-prefixed key ids might lead to accidentally importing
unexpected keys.

QA Steps

  • find a working controller
  • run: juju import-ssh-key lp:a-valid-launchpad-key
  • run: juju import-ssh-key gh:a-valid-github-key
  • run: juju import-ssh-key a-valid-launchpad-key
  • the third one should fail, feel free to try combination of those, any one including the third option should fail.

LGTM after fixes applied

cmd/juju/commands/import_sshkeys.go
@@ -74,6 +74,14 @@ func (c *importKeysCommand) Init(args []string) error {
return errors.New("no ssh key id specified")
default:
@natefinch

natefinch Sep 28, 2016

Contributor

Can you drop the switch and just do

if len(args) == 0 {
    return errors.new("no ssh key id specified")
}
// rest of code

The like 4 level deep indenting is ugly a hard to read.

cmd/juju/commands/import_sshkeys.go
@@ -74,6 +74,14 @@ func (c *importKeysCommand) Init(args []string) error {
return errors.New("no ssh key id specified")
default:
c.sshKeyIds = args
+ for _, k := range c.sshKeyIds {
+ switch k[:3] {
@natefinch

natefinch Sep 28, 2016

Contributor

this will panic if len(k) < 3

Prevent import keys from accepting unprefixed ids.
Accepting up-prefixed key ids might lead to accidentally importing
unexpected keys.
Contributor

perrito666 commented Sep 28, 2016

$$merge$$

Contributor

jujubot commented Sep 28, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Sep 28, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9357

Contributor

perrito666 commented Sep 28, 2016

$$merge$$

Contributor

jujubot commented Sep 28, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Sep 28, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9358

Contributor

perrito666 commented Sep 28, 2016

$$merge$$

Contributor

jujubot commented Sep 28, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 12274c3 into juju:master Sep 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment