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

Latest brew install needs extended sudo rights #109

Closed
wants to merge 1 commit into from

Conversation

Sauraus
Copy link

@Sauraus Sauraus commented Oct 5, 2016

This fixes: #105

@kameghamegha
Copy link

Hey, @Sauraus, have you completed the DCO step? https://blog.chef.io/2016/09/19/introducing-developer-certificate-of-origin/ Here is some more info.

@bdangit
Copy link

bdangit commented Nov 18, 2016

would really love this fix right now. 👍

user homebrew_owner
not_if { ::File.exist? '/usr/local/bin/brew' }
begin
template '/etc/sudoers.d/homebrew_sudo' do

Choose a reason for hiding this comment

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

Fails if /etc/sudoers.d doesn't exist.

Something like...

...

directory '/etc/sudoers.d' do
  action :nothing
end

template '/etc/sudoers.d/homebrew_sudo' do
  notifies :create, 'directory[/etc/sudoers.d]', :before
  ...

...might help/fix it

Copy link
Author

Choose a reason for hiding this comment

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

/etc/sudoers.d can only be missing if someone messed around with the system, not sure we need to account for people's tinkering with system paths.

Choose a reason for hiding this comment

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

Tried running the patch on a fresh install of Mavericks.
/etc/sudoers.d didn't seem to exist beforehand.

Other /etc directories like /etc/ssh don't exist either.

Copy link
Author

Choose a reason for hiding this comment

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

Mavericks... isn't that a little out dated and thus insecure to be used for anything?

But I do see your point, I've only done testing on 'new' OS X versions.

Choose a reason for hiding this comment

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

I'm with you that it's older. However, chef-dk still supports 10.9. Plus the metadata.rb file doesn't have any macOS version restrictions currently.

@Sauraus Sauraus force-pushed the master branch 2 times, most recently from 77956db to 94e9d36 Compare December 9, 2016 01:03
@discentem
Copy link

Why has this not been merged?

@andrew-cc
Copy link

@Sauraus There are a bunch of failures on CI from Rubocop—perhaps getting those fixed will move this along to getting merged 🙂

@andrew-cc
Copy link

Additional errors I’m getting with this change applied onto 3.0.0:

    ==> Searching online for the Command Line Tools
    ==> /usr/bin/sudo /usr/bin/touch /tmp/.com.apple.dt.CommandLineTools.installondemand.in-progress
    ==> Installing Command Line Tools (macOS Sierra version 10.12) for Xcode-8.2
    ==> /usr/bin/sudo /usr/sbin/softwareupdate -i Command\ Line\ Tools\ (macOS\ Sierra\ version\ 10.12)\ for\ Xcode-8.2
    Software Update Tool
    Copyright 2002-2015 Apple Inc.


    Downloading Command Line Tools (macOS Sierra version 10.12) for Xcode
    Downloaded Command Line Tools (macOS Sierra version 10.12) for Xcode
    Installing Command Line Tools (macOS Sierra version 10.12) for Xcode
    Done with Command Line Tools (macOS Sierra version 10.12) for Xcode
    Done.
    ==> /usr/bin/sudo /bin/rm -f /tmp/.com.apple.dt.CommandLineTools.installondemand.in-progress
    ==> /usr/bin/sudo /usr/bin/xcode-select --switch /Library/Developer/CommandLineTools
    STDERR: sudo: no tty present and no askpass program specified
    Failed during: /usr/bin/sudo /usr/bin/xcode-select --switch /Library/Developer/CommandLineTools
    ---- End output of /opt/workstation/.chef/local-mode-cache/cache/homebrew_go < /dev/null ----
    Ran /opt/workstation/.chef/local-mode-cache/cache/homebrew_go < /dev/null returned 1

    Resource Declaration:
    ---------------------
    # In /opt/workstation/.chef/local-mode-cache/cache/cookbooks/homebrew/recipes/default.rb

     49:   execute 'install homebrew' do
     50:     command "#{homebrew_go} < /dev/null"
     51:     environment lazy { { 'HOME' => ::Dir.home(homebrew_owner), 'USER' => homebrew_owner } }
     52:     user homebrew_owner
     53:     not_if { ::File.exist? '/usr/local/bin/brew' }
     54:   end
     55: ensure

After which I added /usr/bin/xcode-select to the list of commands, and the Homebrew install worked (though a later install of a Homebrew cask failed with sudo issues).

@Sauraus Sauraus force-pushed the master branch 2 times, most recently from e65c5ad to 288cb95 Compare January 20, 2017 02:03
@Sauraus
Copy link
Author

Sauraus commented Jan 20, 2017

I just ran several kitchen tests on my local mac and they all converged just fine, not a single fault to be found.

@discentem
Copy link

@Sauraus Did you run rubocop linting though? This is what the failed build indicates.

@Sauraus
Copy link
Author

Sauraus commented Jan 20, 2017

@discentem I did but I wasn't going to break the code just to satisfy rubocop BTW. When I run rubocop locally I get 16 offenses and not 6.

Inspecting 24 files
.C.C.C.C..W...C........C

Offenses:

Gemfile:10:1: C: Gem rake should appear before tomlrb in their gem group.
gem 'rake'
^^^^^^^^^^
Gemfile:12:1: C: Gem community_cookbook_releaser should appear before stove in their gem group.
gem 'community_cookbook_releaser'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Rakefile:24:23: C: Avoid comma after the last item of a hash.
        progress: true,
                      ^
libraries/homebrew_mixin.rb:22:1: C: Missing top-level class documentation comment.
class Chef12HomebrewUser
^^^^^
libraries/homebrew_mixin.rb:42:14: C: Align the parameters of a method call if they span more than one line.
             "Homebrew owner is 'root' which is not supported. " \ ...
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
providers/cask.rb:46:1: C: Use alias instead of alias_method at the top level.
alias_method :action_cask, :action_install
^^^^^^^^^^^^
providers/cask.rb:47:1: C: Use alias instead of alias_method at the top level.
alias_method :action_uncask, :action_uninstall
^^^^^^^^^^^^
recipes/default.rb:43:14: W: (...) interpreted as grouped expression.
    variables (
             ^
recipes/default.rb:44:12: C: Avoid using {...} for multi-line blocks.
      lazy {
           ^
recipes/default.rb:45:11: C: Use the new Ruby 1.9 hash syntax.
        { :user => homebrew_owner, :hostname => node['hostname'],
          ^^^^^^^^
recipes/default.rb:45:36: C: Use the new Ruby 1.9 hash syntax.
        { :user => homebrew_owner, :hostname => node['hostname'],
                                   ^^^^^^^^^^^^
recipes/default.rb:46:11: C: Use the new Ruby 1.9 hash syntax.
          :commands => node['homebrew']['sudo']['commands'] }
          ^^^^^^^^^^^^
recipes/default.rb:72:121: C: Line is too long. [151/120]
  only_if { shell_out('/usr/local/bin/brew analytics state', user: homebrew_owner).stdout.include?('enabled') != node['homebrew']['enable-analytics'] }
                                                                                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
resources/cask.rb:5:3: C: Align the parameters of a method call if they span more than one line.
  name_attribute: true, ...
  ^^^^^^^^^^^^^^^^^^^^^
resources/cask.rb:10:3: C: Align the parameters of a method call if they span more than one line.
  kind_of: String
  ^^^^^^^^^^^^^^^
test/integration/default/default_spec.rb:10:23: C: Use 0o for octal literals.
  it { should be_mode 0755 }
                      ^^^^

24 files inspected, 16 offenses detected

@Sauraus
Copy link
Author

Sauraus commented Jan 20, 2017

@tas50 I've fixed the rubocop warnings, but the spec tests are something entirely different IMO.

@andrew-cc
Copy link

FWIW, as Sauraus seemed to see, we went the route of just installing Homebrew under its own system user which can do passwordless sudo. Then a “real” user just does e.g. sudo -Hu homebrew brew install whatever, and then we can just set the chef homebrew cookbook user attribute to “homebrew”.

@Sauraus
Copy link
Author

Sauraus commented Jan 21, 2017

@andrew-cc saw that yes, still scary to having a user with limitless sudo ;)

@andrew-cc
Copy link

Since it’s a user that can’t be logged-in to, some other user must use sudo to become them. It’s perhaps a shame in some sense that Homebrew isn’t designed like any other major *nix package manager. In some sense it’s “safer” having everything owned by a different user since it prevents arbitrary user software from modifying /usr/local since it doesn’t have perms.

@Sauraus
Copy link
Author

Sauraus commented Jan 23, 2017

Agree with your sentiment.

PS. Don't say any of that to the Homebrew maintainers @Brantone has experience with that...

@aramprice
Copy link

@Sauraus the spec failures are likely related to this issue #114.

@tas50
Copy link
Contributor

tas50 commented Jul 24, 2018

I'm going to close this out at this point because this just isn't an appropriate solution for most users. We try to implement the path of least surprise at Chef and giving blanket sudo rights is not going to be expected or desired by a good chunk of our userbase.

@tas50 tas50 closed this Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homebrew need sudoers right without password when using chef as service
8 participants