Encrypt ldap bind password when queuing to MiqQueue

[jvlcek]

https://bugzilla.redhat.com/show_bug.cgi?id=1297576

This PR encrypts the Ldap bind password when placing it on the MiqQueue in Args
and decrypts it when popping the message off the MiqQueue. This ensures unencrypted
passwords are not in the DB and also prevents them from being logged.

[jvlcek]

@abellotti Please review and share any suggestions for where the encrypt and decrypt methods should live if not where I have them. I could imagine their might be other possibilities for where they should be but there was other if ldap logic in the current location.

Also the v2 encrypted test data might be better generated instead of hard coded but I an not sure the best way to generate them. As long as the encrypted and decrypted test data will always match the solution I have should work fine.
Thank you!
JoeV

[chessbyte]

where is this config variable defined?

[djberg96]

@chessbyte It's a reader, line 28.

[jvlcek]

@chrisarcand and @jrafanie Thank you for the spec simplification suggestions. I like this approach much better. 👍
Merge if appropriate.
Thank you! JoeV

[chrisarcand]

Hmm I'm not sure what's going on here. This could easily be a false positive as without any of the preceding lines called_encrypt_ldap_password could be true.

[chrisarcand]

Indeed, line 174 means this test always passes.

You should be doing some form of expect(subject).to receive(:encrypt_ldap_password)

I'm also not sure what double(:id => :return_value) does. Does RSpec have a :return_value? Even if it does, I'm not sure what you're trying to do in 172 or 173 (the latter would theoretically 'allow' MiqQueue something that it already does normally ;) )

[chrisarcand]

I see you mean :return_value as a dummy value, derp. In this case I'd personally use an Integer on line 172 (whatever, though) but more importantly you needn't give line 173 a block, allow(MiqQueue).to receive(:put) will work just fine 👌

I suggest the following:

    context "when using LDAP" do
      let(:config) { super().merge(:ldap_role => true) }

      before do
        allow(MiqQueue).to receive(:put)
      end

      it "encrypts password for queuing" do
        expect(subject).to receive(:authorize_queue)
        authenticate
      end
    end

Whatcha think?

[jvlcek]

@chrisarcand Thank you again for the help!
@jrafanie I believe this is ready to merge.

[chrisarcand]

Not quite, @jvlcek ! You forgot to do the same thing here :)

[jvlcek]

D'oh! Thanks @chrisarcand!

[miq-bot]

Checked commit jvlcek@56258a2 with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
4 files checked, 0 offenses detected
Everything looks good. 👍

[chrisarcand]

👍 🎉

[jrafanie]

Are the empty parens required in the super() @chrisarcand?

[jvlcek]

@jrafanie They seem to be. I just tried running the spec without them and it fails. :) The super() is found throughout the spec.

[jvlcek]

@jrafanie I uncovered the reason.

super without arguments will invoke the superclass method with the original params. To pass no params the empty parens are needed.

[chrisarcand]

Yes, you need them if you want to call that way.

Not worth changing here but FWIW I avoid this by using another helper to change contexts when you're this far down the nesting, ie:

Instead of:

describe Thing do
  let(:credentials) { { username: "asdf", password: "qwerty" } }

  context "default do
    (test that uses "qwerty" password)
  end

  context "using a different password" do
    let(:credentials) { super().merge(password: "a different one") }
    (test that uses "a different one")
  end
end

I do

describe Thing do
  let(:credentials) { { username: "asdf", password: password } }
  let(:password) { "qwerty" }

  context "default do
    (test that uses "qwerty" password)
  end

  context "using a different password" do
    let(:password) { "a different one" }
    (test that uses "a different one")
  end
end

¯_(ツ)_/¯

[jrafanie]

Thanks @jvlcek, I had a hunch that super() was used to discard the args but it's not obvious why, maybe because the block arg is different... who knows. ✨

Either way, I prefer @chrisarcand's latter example above, but yeah, it's fine for this PR.

[jrafanie]

🍰

[chrisarcand]

🏆