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.

Sign up for GitHub

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

Jump to bottom

pki: make openssl database non-writable for others #558

Merged
merged 1 commit into from Jul 27, 2022
Merged

pki: make openssl database non-writable for others #558

merged 1 commit into from Jul 27, 2022

Conversation

michalskrivanek
Copy link
Member

let's keep these files non-writable to any random user

Bug-Url: https://bugzilla.redhat.com/2088446

@michalskrivanek michalskrivanek mentioned this pull request Jul 26, 2022
Closed
@michalskrivanek michalskrivanek force-pushed the openssl-db branch 7 times, most recently from 75b3573 to ee9ab16 Compare July 26, 2022 11:15
@michalskrivanek
Copy link
Member Author

/ost

@michalskrivanek michalskrivanek force-pushed the openssl-db branch 2 times, most recently from 2f8b462 to abcb0c3 Compare July 26, 2022 13:20
michalskrivanek
michalskrivanek commented Jul 26, 2022
View reviewed changes
packaging/bin/pki-common.sh.in Outdated
@@ -31,6 +31,7 @@ common_restore_perms() {
# of these files, so we have to reset
# our defaults
chown --reference="${pkidir}" "${pkidir}"/serial.txt* "${pkidir}"/database.txt* "${pkidir}"/.rnd* > /dev/null 2>&1
chmod go-wx "${pkidir}"/serial.txt* "${pkidir}"/database.txt* "${pkidir}"/.rnd* || die "Cannot set files permissions"
Copy link
Member Author

Choose a reason for hiding this comment

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

so with umask this shouldn't actually be needed

@michalskrivanek michalskrivanek force-pushed the openssl-db branch 3 times, most recently from 6a58b27 to 667b1d1 Compare July 26, 2022 14:13
@mz-pdm
Copy link
Member

mz-pdm commented Jul 26, 2022

How about the other pki-* scripts -- shouldn't the umask be set there too?

@michalskrivanek
Copy link
Member Author

How about the other pki-* scripts -- shouldn't the umask be set there too?
AFAICT the ones that are called from engine-setup are all right, it's just ansible that calls it with 000

mz-pdm
mz-pdm reviewed Jul 26, 2022
View reviewed changes
Copy link
Member

@mz-pdm mz-pdm left a comment

Choose a reason for hiding this comment

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

AFAICT the ones that are called from engine-setup are all right, it's just ansible that calls it with 000

I see, OK.

packaging/bin/pki-enroll-openssh-cert.sh Outdated Show resolved Hide resolved
@michalskrivanek
Copy link
Member Author

michalskrivanek commented Jul 27, 2022 •
edited

this works on DISA STIG enabled system too, but the default umask there is 077 and so the created files are "more open" than before, e.g.:
-rw-------. 1 root root 1419 Jul 27 08:11 1000.pem
-rw-r--r--. 1 root root 5442 Jul 27 08:11 1001.pem

@michalskrivanek
Copy link
Member Author

@mz-pdm do you think it's worth fixing?
I can only think of following the default umask value by getting it from /etc/profile, e.g. using
$( . /etc/profile 2>/dev/null; command umask)

@mz-pdm
Copy link
Member

mz-pdm commented Jul 27, 2022

You can get the current umask by simply running umask without arguments. And yes, I think it's worth fixing -- while we should get the permissions right eventually, on the DISA STIG enabled systems it's good to be protected against occasional mistakes, e.g. when working with intermediate files in our code or in the tools we use.

Such a change would require verifying that we set permissions on all the resulting files explicitly (with chmod) and don't end up with too restrictive permissions on some of the files. Maybe comparing permissions in OST with (while using the stricter umask in the system) and without this patch?

@mz-pdm
Copy link
Member

mz-pdm commented Jul 27, 2022

You can get the current umask by simply running umask without arguments.

Ah, yes, perhaps not in Ansible? Even on those systems? If so then I would say it's a bug in Ansible.

@mz-pdm
Copy link
Member

mz-pdm commented Jul 27, 2022

I can only think of following the default umask value by getting it from /etc/profile, e.g. using
$( . /etc/profile 2>/dev/null; command umask)

I think a better way would be sh -l -c umask. This should work even if Ansible sets its own umask.

@michalskrivanek michalskrivanek force-pushed the openssl-db branch 2 times, most recently from 0495570 to 492c68f Compare July 27, 2022 11:11
mz-pdm
mz-pdm approved these changes Jul 27, 2022
View reviewed changes
packaging/bin/pki-enroll-openssh-cert.sh Outdated
@@ -32,6 +32,7 @@ sign() {
}
trap cleanup 0
cat "${PKIDIR}/private/ca.pem" > "${TMPCA}"
umask "$(/bin/sh -l -c umask)"
Copy link
Member

Choose a reason for hiding this comment

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

If there is no umask set explicitly in shell login scripts (/etc/profile, ...) then the umask is inherited from the calling environment. It shouldn't be a problem in RHEL-based systems though.

@michalskrivanek michalskrivanek force-pushed the openssl-db branch 2 times, most recently from 72fea37 to ac94678 Compare July 27, 2022 12:07
@michalskrivanek
pki: make certificates only readable to group and others
c8174a2 
even though we copy these to hosts it's not a good idea to allow them to
be overwritten by a random user. Openssl database files are also always
recreated, umask should take care of all of that.
Ansible does not use default umask from OS so we need to explicitly set
it. We can use login shell to figure out the effective command-line umask
value.
@michalskrivanek michalskrivanek merged commit 3e0b763 into oVirt:master Jul 27, 2022
@michalskrivanek michalskrivanek deleted the openssl-db branch July 27, 2022 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mz-pdm mz-pdm mz-pdm approved these changes

@ahadas ahadas Awaiting requested review from ahadas ahadas is a code owner

@bennyz bennyz Awaiting requested review from bennyz bennyz is a code owner

@emesika emesika Awaiting requested review from emesika emesika is a code owner

@mwperina mwperina Awaiting requested review from mwperina mwperina is a code owner

@oliel oliel Awaiting requested review from oliel oliel is a code owner

@sgratch sgratch Awaiting requested review from sgratch sgratch is a code owner

@smelamud smelamud Awaiting requested review from smelamud smelamud is a code owner

@didib didib Awaiting requested review from didib didib is a code owner

@ljelinkova ljelinkova Awaiting requested review from ljelinkova ljelinkova is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@michalskrivanek @mz-pdm