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

Authorized keys custom path #172

Merged
merged 7 commits into from
Apr 18, 2017
Merged

Conversation

lubomir-kacalek
Copy link
Contributor

Hi,
this propposed change allow to have configured a custom authorized key file path for an ssh server.
Thanks.
Lubomir Kacalek

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73e8996 on lubomir-kacalek:master into d4dc236 on dev-sec:master.

Copy link
Member

@artem-sidorenko artem-sidorenko left a comment

Choose a reason for hiding this comment

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

@lubomir-kacalek hey Lubos :) Thanks for the PR!

@@ -96,6 +96,11 @@ MaxStartups 10:30:100
# Enable public key authentication
PubkeyAuthentication yes

<% if @node['ssh-hardening']['ssh']['server']['authorized_keys_path'] != nil %>
Copy link
Member

Choose a reason for hiding this comment

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

you do not need != nil here

@artem-sidorenko
Copy link
Member

@lubomir-kacalek I would like to release the next minor version after #173 is merged, do you think you will be able to update this PR in the next days, so we can include it in the release?

@artem-sidorenko
Copy link
Member

@lubomir-kacalek as discussed, the tests might look like this (in the spec/recipes/server_spec.rb)

describe 'customized AuthorizedKeysFile option' do
  context 'without customized AuthorizedKeysFile' do
      cached(:chef_run) do
        ChefSpec::ServerRunner.new.converge(described_recipe)
      end

      it 'does not have AuthorizedKeysFile configured' do
        expect(chef_run).not_to render_file('/etc/ssh/sshd_config').
          with_content(/^AuthorizedKeysFile/)
      end
  end

  context 'with customized AuthorizedKeysFile' do
      cached(:chef_run) do
        ChefSpec::ServerRunner.new do |node|
          node.normal['ssh-hardening']['ssh']['server']['authorized_keys_path'] = '/some/authorizedkeysfile'
        end.converge(described_recipe)
      end

      it 'has AuthorizedKeysFile configured' do
        expect(chef_run).to render_file('/etc/ssh/sshd_config').
          with_content(/^AuthorizedKeysFile /some/authorizedkeysfile/)
      end
  end
end

@artem-sidorenko
Copy link
Member

@lubomir-kacalek its also possible to have it a bit simplier:

with_content('AuthorizedKeysFile /some/authorizedkeysfile')

instead of

with_content(/^AuthorizedKeysFile /some/authorizedkeysfile/)

and

with_content('AuthorizedKeysFile')

instead of

with_content(/^AuthorizedKeysFile/)

@atomic111
Copy link
Member

@lubomir-kacalek can you please sign your pr?

@artem-sidorenko
Copy link
Member

@lubomir-kacalek @atomic111 means to use the sign-off, e.g. git commit -s (see the man page for details)

Signed-off-by: Lubomir Kacalek <[email protected]>
@lubomir-kacalek
Copy link
Contributor Author

Hi @artem-sidorenko,
required changes has been pushed to my fork. Thank you in advance for support with testing part :-)

@atomic111 : New commit has been signed off as well.

Best regards,

   Lubos

@artem-sidorenko
Copy link
Member

@lubomir-kacalek I looks better, I resolved the conflicts to master

Can you please make the rubocop happy? There are some offenses

@artem-sidorenko
Copy link
Member

@atomic111 any remarks to this PR?

I would like to merge it when rubocop is made happy and release the 2.1 of ssh-hardening

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 71b4f50 on lubomir-kacalek:master into eaf6c11 on dev-sec:master.

@artem-sidorenko artem-sidorenko merged commit 2e89e52 into dev-sec:master Apr 18, 2017
@artem-sidorenko
Copy link
Member

@lubomir-kacalek thank you!

2.1 will be released today/tomorrow

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.

4 participants