Whirr
  1. Whirr
  2. WHIRR-161

Check that both SSH keys belong to the same pair

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.3.0
    • Component/s: core
    • Labels:
      None

      Description

      Check that the private and public keys belong to the same pair (via, e.g. http://s.apache.org/MNY or by en/decrypting a dummy message).

      1. WHIRR-161.patch
        93 kB
        Andrei Savu
      2. WHIRR-161.patch
        10 kB
        Andrei Savu
      3. WHIRR-161.patch
        10 kB
        Andrei Savu

        Issue Links

          Activity

          Hide
          Andrei Savu added a comment -

          I need some help on this one. I have posted a question on stackoverflow [1]. The Java CE documentation is really hard to use.

          [1] http://stackoverflow.com/questions/4428236

          Got any suggestions for a cross-platform pure Java solution?

          Show
          Andrei Savu added a comment - I need some help on this one. I have posted a question on stackoverflow [1] . The Java CE documentation is really hard to use. [1] http://stackoverflow.com/questions/4428236 Got any suggestions for a cross-platform pure Java solution?
          Hide
          Tom White added a comment -

          I had a quick look at this too and realized it was non-trivial! The OpenSAML code has precisely the right function, but I hesitate to add a big dependency like this.

          In fact, the problem we saw was that the public key didn't match because a user had changed whirr.private-key-file but not whirr.public-key-file. With the change in WHIRR-160 which makes the public key default to private key + ".pub" (rather than {{$

          {sys:user.home}

          /.ssh/id_rsa_rackspace.pub}}), perhaps this isn't such a problem now?

          Show
          Tom White added a comment - I had a quick look at this too and realized it was non-trivial! The OpenSAML code has precisely the right function, but I hesitate to add a big dependency like this. In fact, the problem we saw was that the public key didn't match because a user had changed whirr.private-key-file but not whirr.public-key-file. With the change in WHIRR-160 which makes the public key default to private key + ".pub" (rather than {{$ {sys:user.home} /.ssh/id_rsa_rackspace.pub}}), perhaps this isn't such a problem now?
          Hide
          Andrei Savu added a comment -

          I think I can avoid adding OpenSAML as a dependency by extracting only the relevant code. After taking a quick look I believe that the only dependency that needs to be added is not-yet-commons-ssl.jar (org.apache.commons.ssl). Is this acceptable?

          I will give it a try tomorrow. I understand having really strict validation it's not mandatory but it would be a nice addition.

          Show
          Andrei Savu added a comment - I think I can avoid adding OpenSAML as a dependency by extracting only the relevant code. After taking a quick look I believe that the only dependency that needs to be added is not-yet-commons-ssl.jar (org.apache.commons.ssl). Is this acceptable? I will give it a try tomorrow. I understand having really strict validation it's not mandatory but it would be a nice addition.
          Hide
          Tom White added a comment -

          > ... the only dependency that needs to be added is not-yet-commons-ssl.jar (org.apache.commons.ssl). Is this acceptable?

          Hmm, this uses the Apache namespace, but is not an ASF project as far as I can tell. You could send a question to Apache's legal-discuss list (http://www.apache.org/foundation/mailinglists.html#foundation-legal) to get a definitive answer.

          Show
          Tom White added a comment - > ... the only dependency that needs to be added is not-yet-commons-ssl.jar (org.apache.commons.ssl). Is this acceptable? Hmm, this uses the Apache namespace, but is not an ASF project as far as I can tell. You could send a question to Apache's legal-discuss list ( http://www.apache.org/foundation/mailinglists.html#foundation-legal ) to get a definitive answer.
          Hide
          Andrei Savu added a comment -

          I've attached a patch that checks that both keys belong to the same key pair.

          I have edited core/pom.xml and added not-yet-commons-ssl.jar as a dependency.

          It should be safe to use from a license point of view:
          [1] http://juliusdavies.ca/commons-ssl/index.html
          [2] http://wiki.apache.org/incubator/CommonsSSLProposal

          I have also used a class placed in the public domain for Base64 encoding:
          [3] http://iharder.sourceforge.net/current/java/base64/

          Show
          Andrei Savu added a comment - I've attached a patch that checks that both keys belong to the same key pair. I have edited core/pom.xml and added not-yet-commons-ssl.jar as a dependency. It should be safe to use from a license point of view: [1] http://juliusdavies.ca/commons-ssl/index.html [2] http://wiki.apache.org/incubator/CommonsSSLProposal I have also used a class placed in the public domain for Base64 encoding: [3] http://iharder.sourceforge.net/current/java/base64/
          Hide
          Tom White added a comment -
          Show
          Tom White added a comment - Apache James uses not-yet-commons-ssl, so there's a precedent ( http://svn.apache.org/viewvc/james/jdkim/trunk/pom.xml?view=markup ). Can you use Apache Commons Codec for the Base64 encoding? http://commons.apache.org/codec/apidocs/org/apache/commons/codec/binary/Base64.html Does this code only support RSA keys? Should we generalize to support DSA keys too? Or at least update the docs to spell out what is supported (I would suggest a note in src/site/confluence/quick-start-guide.confluence). Nits: the indentation should be two spaces, and star imports should be avoided.
          Hide
          Andrei Savu added a comment -

          I will change the patch to use commons-codec for Base64 encoding.

          > Does this code only support RSA keys? Should we generalize to support DSA keys too?

          It supports only RSA keys but this limitation was already in the code base. Should we keep things the way they are? I will add a note to confluence explaining the restriction.

          > Nits: the indentation should be two spaces, and star imports should be avoided.

          Ok. I will fix that.

          Show
          Andrei Savu added a comment - I will change the patch to use commons-codec for Base64 encoding. > Does this code only support RSA keys? Should we generalize to support DSA keys too? It supports only RSA keys but this limitation was already in the code base. Should we keep things the way they are? I will add a note to confluence explaining the restriction. > Nits: the indentation should be two spaces, and star imports should be avoided. Ok. I will fix that.
          Hide
          Andrei Savu added a comment -

          Fixed all the issues. It should be ready to be committed.

          Show
          Andrei Savu added a comment - Fixed all the issues. It should be ready to be committed.
          Hide
          Andrei Savu added a comment -

          I suggest that we should open another JIRA for accepting DSA key pairs if needed.

          Show
          Andrei Savu added a comment - I suggest that we should open another JIRA for accepting DSA key pairs if needed.
          Hide
          Tom White added a comment -

          Thanks for the new patch. It's getting close now.

          • KeyPair#sameKeyPair swallows the exception. We should at least log it.
          • KeyPairTest should use assertThat rather than assert (since the latter depends on whether asserts are enabled).
          • The indentation and start import still need fixing.

          Have you manually tested this with a keypair generated with ssh-keygen?

          +1 to moving DSA key support to another issue.

          Show
          Tom White added a comment - Thanks for the new patch. It's getting close now. KeyPair#sameKeyPair swallows the exception. We should at least log it. KeyPairTest should use assertThat rather than assert (since the latter depends on whether asserts are enabled). The indentation and start import still need fixing. Have you manually tested this with a keypair generated with ssh-keygen? +1 to moving DSA key support to another issue.
          Hide
          Andrei Savu added a comment -

          Thanks for reviewing. I will fix the remaining issues as soon as possible.

          > Have you manually tested this with a keypair generated with ssh-keygen?

          I have successfully started a 3-node ZooKeeper cluster using a version of whirr with this patch and ~/.ssh/id_rsa as a private-key-file. I have been able to login to all the nodes using the standard ssh client. I'm pretty confident this works for any valid SSH RSA key.

          Show
          Andrei Savu added a comment - Thanks for reviewing. I will fix the remaining issues as soon as possible. > Have you manually tested this with a keypair generated with ssh-keygen? I have successfully started a 3-node ZooKeeper cluster using a version of whirr with this patch and ~/.ssh/id_rsa as a private-key-file. I have been able to login to all the nodes using the standard ssh client. I'm pretty confident this works for any valid SSH RSA key.
          Hide
          Andrei Savu added a comment -

          Tom, this should be it. I'm moving on to WHIRR-164

          Show
          Andrei Savu added a comment - Tom, this should be it. I'm moving on to WHIRR-164
          Hide
          Tom White added a comment -

          I've just committed this. Thanks Andrei!

          (I manually tested the patch with a valid keypair and then with an invalid keypair. I also made a minor change to formatting, and put the new dependency version number in the top-level POM.)

          Show
          Tom White added a comment - I've just committed this. Thanks Andrei! (I manually tested the patch with a valid keypair and then with an invalid keypair. I also made a minor change to formatting, and put the new dependency version number in the top-level POM.)
          Hide
          Andrei Savu added a comment -

          Great! Tom, thanks for reviewing.

          Show
          Andrei Savu added a comment - Great! Tom, thanks for reviewing.

            People

            • Assignee:
              Andrei Savu
              Reporter:
              Andrei Savu
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development