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

DLPX-86537 CIS: sudoers configuration #498

Merged

Conversation

dbshah12
Copy link
Contributor

@dbshah12 dbshah12 commented Sep 30, 2024

Problems

  • This setting specifies the presence of 'use_pty' setting in /etc/sudoers and /etc/sudoers.d/ file. If set, sudo will run the command in a pseudo-pty. Attackers can run a malicious program using sudo which would fork a background process that remains even when the main program has finished executing. This setting should be configured according to the needs of the business.

  • This setting specifies the presence of sudo log file on the system. A sudo log file simplifies auditing of sudo commands. Sudo provides users with temporarily elevated privileges to perform operations. And if it is enabled, creating an audit log of exactly what was run (and who ran it) is essential to reporting. This setting should be configured according to the needs of the business.

Solutions

  • Edit /etc/sudoers.d/delphix and add below 2 lines in it to get logs of sudo commands:
Defaults use_pty
Defaults logfile=/var/log/sudo.log
  • Add new /etc/logrotate.d/sudo-log for log rotation.

Testing

Manual

  • Checked the below details on the New machine with this change and upgraded DE:
    • These logs are not available on normal machines.
Screenshot 2024-10-04 at 5 11 24 PM

@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch 3 times, most recently from 6423546 to 096ff6b Compare October 1, 2024 11:44
@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch from 096ff6b to fe3b417 Compare October 1, 2024 15:04
@dbshah12 dbshah12 self-assigned this Oct 3, 2024
@dbshah12 dbshah12 added the S & I Security & Infra Team label Oct 3, 2024
@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch from bb04111 to fe3b417 Compare October 3, 2024 08:26
@dbshah12 dbshah12 marked this pull request as ready for review October 3, 2024 13:30
Copy link
Contributor

@sebroy sebroy left a comment

Choose a reason for hiding this comment

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

We should not be making these changes using ansible. The idea of the "*.d" directories is that they can contain configuration fragments as files. You can simply include the files you want in the delphix-platform repo. For example, see how we already provide such a configuration fragment in the repo already in a file called delphix. We can do the same for logrotate configuration fragments.

@dbshah12
Copy link
Contributor Author

dbshah12 commented Oct 3, 2024

We should not be making these changes using ansible. The idea of the "*.d" directories is that they can contain configuration fragments as files. You can simply include the files you want in the delphix-platform repo. For example, see how we already provide such a configuration fragment in the repo already in a file called delphix. We can do the same for logrotate configuration fragments.

Ohh yeah, I wasn't aware of it, I used delphix file to set below 2

Defaults use_pty
Defaults logfile='/var/log/sudo.log'

And also added a log rotator, @sebroy thanks for suggesting, can you please re-review? I will do testing again once, the pre-push completes.

@dbshah12 dbshah12 requested a review from sebroy October 3, 2024 14:31
@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch from 8ee46c4 to 82be5d4 Compare October 4, 2024 04:13
@dbshah12
Copy link
Contributor Author

dbshah12 commented Oct 4, 2024

We should not be making these changes using ansible. The idea of the "*.d" directories is that they can contain configuration fragments as files. You can simply include the files you want in the delphix-platform repo. For example, see how we already provide such a configuration fragment in the repo already in a file called delphix. We can do the same for logrotate configuration fragments.

Ohh yeah, I wasn't aware of it, I used delphix file to set below 2

Defaults use_pty
Defaults logfile='/var/log/sudo.log'

And also added a log rotator, @sebroy thanks for suggesting, can you please re-review? I will do testing again once, the pre-push completes.

@sebroy Completed the normal and upgrade testing, things are working as expected, can you please re-review?

Copy link
Contributor

@sebroy sebroy left a comment

Choose a reason for hiding this comment

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

one last nit: we should be consistent with copyright blocks. In this case, one file has a copyright block, and the other does not. Either they should both have one, or neither should have one. IMO configuration files are not copyrightable code, and should not have a copyright block. As such, I suggest removing the copyright block from the sudoers.d file to make things consistent.

@dbshah12
Copy link
Contributor Author

dbshah12 commented Oct 4, 2024

one last nit: we should be consistent with copyright blocks. In this case, one file has a copyright block, and the other does not. Either they should both have one, or neither should have one. IMO configuration files are not copyrightable code, and should not have a copyright block. As such, I suggest removing the copyright block from the sudoers.d file to make things consistent.

Yes, that's a good idea. However, I was thinking about something else:

I searched for the files with # Licensed under the Apache License, and all of them include a copyright notice. Should we consider adding this instead of removing it?

@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch from 82be5d4 to 51daad5 Compare October 4, 2024 14:42
Copy link
Contributor

@nealquigley nealquigley left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have strong feelings nor expertise in copyrights but per your comment, it seems like adding the same sort of block to the new file would be more consistent with the rest of the codebase than removing it from the preexisting file.

@dbshah12
Copy link
Contributor Author

dbshah12 commented Oct 7, 2024

LGTM. I don't have strong feelings nor expertise in copyrights but per your comment, it seems like adding the same sort of block to the new file would be more consistent with the rest of the codebase than removing it from the preexisting file.

  • Yeah, I agree with you on consistency but as @sebroy suggested configuration files are not copyrightable code so I removed it.

@dbshah12 dbshah12 force-pushed the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch from 3dfb61c to 69ee2b7 Compare October 7, 2024 03:14
@dbshah12 dbshah12 merged commit 9c308bb into develop Oct 7, 2024
15 checks passed
@dbshah12 dbshah12 deleted the dlpx/pr/dbshah12/8461e8c0-eab3-4996-a255-9eb1bcf8d765 branch October 7, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S & I Security & Infra Team
Development

Successfully merging this pull request may close these issues.

3 participants