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

Fixes #8498 - Add root_pass accessibility to finish scripts #1983

Closed
wants to merge 4 commits into from

Conversation

TJM
Copy link
Member

@TJM TJM commented Nov 26, 2014

This fixes the ability to access the root_pass attribute, and ensures that its loaded. It also includes an updated default kickstart template that actually sets the root password on a "image" based install. (with the assumption that the root password will be set by the provision script otherwise so setting it again would be unnecessary). It also fixes the check to see if the password pasted was already hashed. (probably could use some documentation there)

Since the "finish" script will not get loaded automatically, in order to test this, add the following to a finish script for a centos/rhel/fedora based install.. I am not sure if it works on debian or gentoo based images.

<% if @host.provision_method == 'image' &&  ! root_pass.blank? -%>
# Install the root password
echo 'root:<%= root_pass -%>' | /usr/sbin/chpasswd -e
<% end -%>

This is a resubmit of my previous PR #1982 due to bad commit messages.

@ohadlevy
Copy link
Member

[test]

@dLobatog dLobatog changed the title 8498 Add root_pass accessibility to finish scripts Fixes #8498 - Add root_pass accessibility to finish scripts Nov 26, 2014
@dLobatog
Copy link
Member

@TJM Tests are failing, check out the results in jenkins or run rake test to try it out yourself.

@TJM
Copy link
Member Author

TJM commented Nov 26, 2014

@elobato : The tests are failing directly in the "develop" branch too. I thought that was a "known issue" ?

results from "develop" branch:

(snip)
1166 tests, 3030 assertions, 6 failures, 20 errors, 2 skips
Coverage report generated for Unit Tests to /home/vagrant/foreman/coverage. 11070 / 16838 LOC (65.74%) covered.
The Apipie cache is turned off. Enable it and run apipie:cache rake task to speed up API calls.
ldapname is deprecated and will be removed in a future version
Workaround for RbVmomi may not work as ComputeResource is already loaded: ComputeResource
/home/vagrant/foreman/config/initializers/rbvmomi.rb:20: warning: toplevel constant ComputeResource referenced by RbVmomi::VIM::ComputeResource
(snip .. etc etc etc, pages and pages of errors)

@domcleal
Copy link
Contributor

There aren't any known failures.. those messages don't really show the problem, can you gist the whole lot perhaps? Do check you have organisations+locations enabled in settings.yml, as these are required when running the tests at least (not necessary normally).

Else, click the Details link next to "Job result: FAILURE", click the red "foreman", click the # number next to test_develop_pr_core and it shows a list of failed tests.

@dLobatog
Copy link
Member

@TJM

Here's the link to the failures in this PR test_develop_pr_core
develop is green.

@TJM
Copy link
Member Author

TJM commented Nov 28, 2014

Hi @elobato, Sorry about that. I got some help from @ohadlevy Thursday (morning for me), it turns out that the failures I was seeing were related to me running bundle install --without-postgres --without-libvirt ... in addition to that, you cannot just use the settings.yaml.example like the Contribute page says, you have to edit it and enable locations and organizations. Now that I can get tests to succeed on "develop" I am able to make them fail in the same way that jenkins sees on my branch. SO, now I just need to figure out what is causing it to fail :)

@dLobatog
Copy link
Member

[test]

1 similar comment
@domcleal
Copy link
Contributor

domcleal commented Dec 2, 2014

[test]

@TJM
Copy link
Member Author

TJM commented Dec 2, 2014

So, I am guessing that SQL failure was not related to my code :)

@ohadlevy
Copy link
Member

@TJM would you mind rebasing this PR ? thanks

@TJM
Copy link
Member Author

TJM commented Dec 23, 2014

Hi guys, I think that someone potentially did a git push --force to the "develop" branch, which is what caused this to get all out of whack. It was somewhere around the "rails 3.2.21" stuff. I did have a conflict with the host_managed.rb, which was expected and I fixed that, but I just took the "HEAD" version of the Gemfile because I had not previously changed that.

@dLobatog
Copy link
Member

[test]

@@ -19,6 +20,11 @@ oses:
puppet_enabled = pm_set || @host.params['force-puppet'] && @host.params['force-puppet'] == 'true'
%>

<% if @host.provision_method == 'image' && ! root_pass.blank? -%>
Copy link
Member

Choose a reason for hiding this comment

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

if @host.provision_method == 'image' &&  root_pass.present?

is more readable IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed! I am still learning what methods are available, I hated "not blank" :)

@unorthodoxgeek
Copy link
Member

@ohadlevy - I think this can be merged.
that minor styling issue I commented on is not critical.

@@ -19,6 +20,11 @@ oses:
puppet_enabled = pm_set || @host.params['force-puppet'] && @host.params['force-puppet'] == 'true'
%>

<% if @host.provision_method == 'image' && ! root_pass.blank? -%>
# Install the root password
echo 'root:<%= root_pass -%>' | /usr/sbin/chpasswd -e
Copy link
Member

Choose a reason for hiding this comment

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

since chpasswd is in shadow-utils I assume that's going to work, just wondering if there is a version of the os that this wont work, an alternative solution would be:

echo -e "<%=root_pass %>\n<%= root_pass %>" | passwd --stdin root

Copy link
Member Author

Choose a reason for hiding this comment

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

I am fairly sure that using passwd --stdin is going to expect the actual password, while chpasswd actually is expecting the hashed password, which is why I chose that.

Copy link
Member

Choose a reason for hiding this comment

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

@TJM yep, you are right, scratch that then :)

@ohadlevy
Copy link
Member

[test]

@@ -116,9 +116,9 @@ def crypt_root_pass
end

if unencrypted_pass.present?
self.root_pass = unencrypted_pass.starts_with?('$') ? unencrypted_pass :
self.root_pass = !!(unencrypted_pass.match('^\$\d+\$.+\$.+')) ? unencrypted_pass :
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: this fixes another issue I had with a root password that started with "$" not being hashed. I don't know if there should be a separate bug report for that? (probably)

Copy link
Member Author

Choose a reason for hiding this comment

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

WIll pull this change out, it needs to be its own issue

@dLobatog
Copy link
Member

@TJM 👍 from my side. Please add a few unit tests and we're all set 😄 !

@@ -1,3 +1,4 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

I believe shebangs are not necessary in this kind of template

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this one will go away with rebase

@dLobatog
Copy link
Member

Merged c47be8a , 3 months later.. thanks @TJM! 🙇

@dLobatog dLobatog closed this Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants