Skip to content

tools: disallow root login by default - #17863

Merged
martinpitt merged 2 commits into
cockpit-project:mainfrom
jelly:disable-root-login
Nov 16, 2022
Merged

tools: disallow root login by default#17863
martinpitt merged 2 commits into
cockpit-project:mainfrom
jelly:disable-root-login

Conversation

@jelly

@jelly jelly commented Nov 1, 2022 •
edited by martinpitt
Loading

Copy link
Copy Markdown
Member

Disallow root login by default

On all operating systems, logging in as root with a username/password via SSH has been disallowed for a long time. Cockpit now also disallows it by default on new installations, but still allows it on upgrades for backwards compatibility. You can configure this in /etc/cockpit/disallowed-users.

@jelly
jelly temporarily deployed to cockpit-dist November 1, 2022 10:32 Inactive
@jelly

jelly commented Nov 1, 2022

Copy link
Copy Markdown
Member Author

A bunch of test failures as expected:

TestConnection.testWsPackage

And tests which try to login as root.

@jelly

jelly commented Nov 2, 2022

Copy link
Copy Markdown
Member Author

This needs a new test which tests our upgrade behaviour and the default behaviour of not allowing a root login.

@jelly

jelly commented Nov 2, 2022

Copy link
Copy Markdown
Member Author

testWsPackage fails with:

[root@fedora-36-127-0-0-2-2201 ~]# rpm --verify cockpit-ws
.M.......  g /etc/cockpit/disallowed-users

@jelly
jelly force-pushed the disable-root-login branch from 4a2785a to 16ad2b4 Compare November 2, 2022 15:25
@jelly
jelly temporarily deployed to cockpit-dist November 2, 2022 15:30 Inactive
@jelly

jelly commented Nov 3, 2022

Copy link
Copy Markdown
Member Author

To test upgrading we have the current installed packages in our prepared virtual machine in /var/tmp/build/, so to test if an upgrade does not add a the new file:

  1. verify the file is there
  2. remove the file
  3. upgrade using --force
  4. verify the file is not there

jelly
jelly commented Nov 3, 2022
View reviewed changes
Comment thread test/common/testlib.py Outdated
jelly
jelly commented Nov 3, 2022
View reviewed changes
Comment thread tools/cockpit.spec Outdated
@jelly
jelly force-pushed the disable-root-login branch from 16ad2b4 to b599e0e Compare November 3, 2022 10:18
@jelly
jelly temporarily deployed to cockpit-dist November 3, 2022 10:24 Inactive
@jelly
jelly force-pushed the disable-root-login branch from b599e0e to f4f5240 Compare November 4, 2022 10:15
@jelly
jelly temporarily deployed to cockpit-dist November 4, 2022 10:21 Inactive
@jelly
jelly force-pushed the disable-root-login branch from f4f5240 to 0b5b7ad Compare November 4, 2022 15:11
@jelly
jelly temporarily deployed to cockpit-dist November 4, 2022 15:18 Inactive
@garrett garrett mentioned this pull request Nov 7, 2022
Closed
@jelly
jelly force-pushed the disable-root-login branch from 0b5b7ad to dc92bbe Compare November 8, 2022 10:40
@jelly
jelly temporarily deployed to cockpit-dist November 8, 2022 10:45 Inactive
@jelly
jelly force-pushed the disable-root-login branch from dc92bbe to f59edfd Compare November 8, 2022 13:02
@jelly
jelly temporarily deployed to cockpit-dist November 8, 2022 13:07 Inactive
@jelly
jelly force-pushed the disable-root-login branch from f59edfd to e6032b9 Compare November 8, 2022 20:34
@jelly
jelly temporarily deployed to cockpit-dist November 8, 2022 20:41 Inactive
@jelly
jelly force-pushed the disable-root-login branch from e6032b9 to 73df2e8 Compare November 8, 2022 21:28
@jelly
jelly temporarily deployed to cockpit-dist November 8, 2022 21:34 Inactive
@jelly
jelly temporarily deployed to cockpit-dist November 9, 2022 10:25 Inactive
@jelly

jelly commented Nov 9, 2022

Copy link
Copy Markdown
Member Author

Ok great... centos-8-stream on packit upgrades cockpit so no file is created. We need to work around this in tests..

@jelly
jelly force-pushed the disable-root-login branch from 37138d0 to dc0ddc2 Compare November 9, 2022 11:30
@jelly
jelly temporarily deployed to cockpit-dist November 9, 2022 11:36 Inactive
@jelly
jelly force-pushed the disable-root-login branch from dc0ddc2 to 700a802 Compare November 9, 2022 13:52
@jelly
jelly temporarily deployed to cockpit-dist November 9, 2022 13:57 Inactive
@jelly
jelly force-pushed the disable-root-login branch from 700a802 to 70c4572 Compare November 9, 2022 15:25
@jelly
jelly temporarily deployed to cockpit-dist November 9, 2022 15:30 Inactive
@jelly
jelly marked this pull request as ready for review November 9, 2022 16:17
@jelly
jelly requested review from martinpitt and marusak November 14, 2022 09:31
martinpitt
martinpitt approved these changes Nov 14, 2022
View reviewed changes

@martinpitt martinpitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dankjewel! Needs some cleanup and rebasing, but I agree to the approach. Please add a release note.

Comment thread test/common/testlib.py Outdated
Comment thread test/verify/check-connection Outdated
Comment thread tools/arch/cockpit.install Outdated
Comment thread tools/arch/cockpit.install Outdated
Comment thread tools/cockpit.debian.pam Outdated
Comment thread test/common/testlib.py Outdated
Comment thread tools/arch/cockpit.pam Outdated
Comment thread tools/cockpit.spec Outdated
Comment thread test/image-prepare Outdated
Comment thread test/browser/browser.sh Outdated
martinpitt
martinpitt requested changes Nov 14, 2022
View reviewed changes

@martinpitt martinpitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Err, I meant to say "request changes"..

@jelly
test: erase rpms before installing on rhel8
ba4e060 
Unlike other images, RHEL 8 did not erase the cockpit packages before
installing. This is inconsistent with our other images and we only need
to retain our already installed cockpit pacakges when testing on
distropkg.
@jelly
jelly force-pushed the disable-root-login branch from 70c4572 to de60e1b Compare November 14, 2022 12:54
@jelly
jelly requested a review from martinpitt November 14, 2022 12:57
@jelly
jelly temporarily deployed to cockpit-dist November 14, 2022 13:01 Inactive
@martinpitt

martinpitt commented Nov 14, 2022

Copy link
Copy Markdown
Member

arch failed with

==> Validating source files with sha256sums...
    cockpit-279.15.ge35c9668e.tar.xz ... Skipped
    cockpit.pam ... FAILED
    cockpit-ws.sysuser.conf ... Passed
    cockpit-wsinstance.sysuser.conf ... Passed
==> ERROR: One or more files did not pass the validity check!

That's related to that magic SHA sum update? Does that require an image refresh?

martinpitt
martinpitt requested changes Nov 14, 2022
View reviewed changes

@martinpitt martinpitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Very close now, only a missing chmod. That arch failure doesn't look like a flake at first sight?

Comment thread test/verify/check-static-login Outdated
Comment thread tools/cockpit.spec Outdated
@jelly
jelly force-pushed the disable-root-login branch from de60e1b to b3149e7 Compare November 14, 2022 14:22
@jelly
tools: disallow root login by default
928ee68 
On all operating systems logging in as root with a username/password via
ssh is now disallowed. Cockpit now also disallows it by default when
installing it, but still allows it when upgrading your Cockpit.
@jelly
jelly force-pushed the disable-root-login branch from b3149e7 to 928ee68 Compare November 14, 2022 14:23
@jelly

jelly commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

arch failed with

==> Validating source files with sha256sums...
    cockpit-279.15.ge35c9668e.tar.xz ... Skipped
    cockpit.pam ... FAILED
    cockpit-ws.sysuser.conf ... Passed
    cockpit-wsinstance.sysuser.conf ... Passed
==> ERROR: One or more files did not pass the validity check!

That's related to that magic SHA sum update? Does that require an image refresh?

Nope, that's validating the file I changed, I guess we should skip checksums for those files.

@jelly
jelly temporarily deployed to cockpit-dist November 14, 2022 14:28 Inactive
@jelly
jelly requested a review from martinpitt November 15, 2022 09:11
martinpitt
martinpitt approved these changes Nov 16, 2022
View reviewed changes

@martinpitt martinpitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dankjewel!

(FTR, it's really cool that we cover upgrades in our test suite!)

@martinpitt

martinpitt commented Nov 16, 2022

Copy link
Copy Markdown
Member

@jelly : I tweaked the release note a bit, WDYT?

@martinpitt
martinpitt merged commit 3774a9a into cockpit-project:main Nov 16, 2022
@jelly
jelly deleted the disable-root-login branch November 16, 2022 09:20
@jelly

jelly commented Nov 16, 2022

Copy link
Copy Markdown
Member Author

@jelly : I tweaked the release note a bit, WDYT?

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinpitt martinpitt martinpitt approved these changes
@marusak marusak Awaiting requested review from marusak

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jelly @martinpitt @garrett