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

Implement the hacluster password verify gatherer #70

Merged
merged 3 commits into from
Aug 18, 2022

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Aug 4, 2022

This gatherer checks if the provided password is the correct one for the hacluster user.

As stated in the code:

By now, this gatherer works with a fixed user: hacluster
It checks if the provider password in the fact Argument field matches with the user password, being:
- true: the provided password is hacluster user password
- false: the provided password does not match the hacluster password
In order to make the checked user variable, the arguments field should be converted to some more
advanced struct to allow getting more data

By now, there is not any need to check the password for other users. The aim of this one is to basically check that the hacluster user password has been changes from the default one, so the cluster doesn't have such a big security hole

Find some more information about the implementation details and how passwords work in linux systems using the hashing way here:
https://linux-audit.com/password-security-with-linux-etc-shadow-file/

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Thanks Xabi, great stuff!
I shared with you some thoughts about the gatherer and related DSL.
Let's hear also from the rest of the team 🚀

@arbulu89 arbulu89 force-pushed the password-gatherer branch 2 times, most recently from e6fa5fc to cf06346 Compare August 9, 2022 12:31
@rtorrero
Copy link
Contributor

I'm a bit late to the party but I'm not entirely convinced that flexibility here is the key value to go for. I'd rather have the gatherer to check exclusively for a well-known pre-established password. e.g. user: hacluster, password:linux or whatever the known defaults are.

Sending user:password combinations to the gatherer is indeed more flexible and allows to reuse this gatherer for other things, but I'd seek for security over flexibility here.

As off for the code, if we still decide to go with this approach: LGTM

@nelsonkopliku
Copy link
Member

@rtorrero you mean something like this option?

facts:
    - name: my_hacluster_pass_has_changed
      gatherer: hacluster_password_changed

that encapsulates username and related default password.

That was also my thought, let's hear also from the rest of the team.

@nelsonkopliku nelsonkopliku force-pushed the password-gatherer branch 2 times, most recently from af7e21b to da8a35f Compare August 17, 2022 16:31
@nelsonkopliku
Copy link
Member

I took the liberty to pick this up and:

  • modified gatherer constructor to inject an executor
  • updated tests

At this stage of current gatherer's implementation we are able to tell whether a username:password combination is correct (returns true|false)

The involved check here is 1.5.2 The hacluster user password has been changed from the default value linux and expressing that via DSL and this gatherer, would look like this

# 1.5.2 The `hacluster` user password has been changed from the default value `linux`
facts:
  - name: default_hacluster_password
    gatherer: verify_password
    argument: "hacluster:linux"
expectations:
  - name: hacluster_password_has_been_changed
    expect: default_hacluster_password == false

If this is fine, let's go ahead, otherwise we'd need to change the gatherer's logic.

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@nelsonkopliku nelsonkopliku merged commit edd3048 into main Aug 18, 2022
@rtorrero
Copy link
Contributor

@rtorrero you mean something like this option?

facts:
    - name: my_hacluster_pass_has_changed
      gatherer: hacluster_password_changed

that encapsulates username and related default password.

That was also my thought, let's hear also from the rest of the team.

Yes (sorry for the late reply)

@stefanotorresi stefanotorresi deleted the password-gatherer branch October 3, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

5 participants