Skip to content

possible fix for #95 deprecated option UseLogin#103

Closed
MrSecure wants to merge 1 commit into
dev-sec:masterfrom
MrSecure:adjust_uselogin_logic
Closed

possible fix for #95 deprecated option UseLogin#103
MrSecure wants to merge 1 commit into
dev-sec:masterfrom
MrSecure:adjust_uselogin_logic

Conversation

@MrSecure
Copy link
Copy Markdown

@MrSecure MrSecure commented May 2, 2018

In my testing this fixes the deprecation of the UseLogin option in more recent OpenSSH versions.
Additionally, it should detect the insecure configuration as far back as OpenSSH 3.0.2 (release notes link below state that UseLogin was NOT ENABLED by default at that time)

https://www.openssh.com/txt/release-3.0.2

Copy link
Copy Markdown
Member

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you @MrSecure for this improvement

@atomic111
Copy link
Copy Markdown
Member

@MrSecure thanks for your PR, it would be nice to check for no and for nil in an describe.one block. Can you adjust this please?

@chris-rock
Copy link
Copy Markdown
Member

chris-rock commented May 4, 2018

Thank you @atomic111 you are right, it should be

descibe.one do
  describe sshd_config do
    its('UseLogin') { should eq 'no' }
  end
  describe sshd_config do
    its('UseLogin') { should eq nil }
  end
end

then. This prevent that all other values then nil and no are invalid, while not yes allows a lot of other values.

Copy link
Copy Markdown
Member

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

As requested in my comment, I think we should make the control a little bit more narrow in its allowed values

@atomic111
Copy link
Copy Markdown
Member

@MrSecure can you please add this small change?

@artem-sidorenko
Copy link
Copy Markdown
Member

@MrSecure @chris-rock @atomic111 we can now do it depending on the ssh version, see #110

@chris-rock
Copy link
Copy Markdown
Member

@artem-sidorenko That would be a great solution

@artem-sidorenko artem-sidorenko self-assigned this Aug 3, 2018
@artem-sidorenko
Copy link
Copy Markdown
Member

closing this in favour of #114

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