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

ssh exit with non-zero status on disabled user #472

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

otubo
Copy link
Contributor

@otubo otubo commented Jul 2, 2020

It is confusing for scripts, where a disabled user has been specified,
that ssh exits with a zero status by default without indication anything
failed.

I think exitting with a non-zero status would make more clear in scripts
and automated setups where things failed, thus making noticing the issue
and debugging easier.

LP: #1170059

Signed-off-by: Eduardo Otubo otubo@redhat.com
Signed-off-by: Aleksandar Kostadinov akostadi@redhat.com

It is confusing for scripts, where a disabled user has been specified,
that ssh exits with a zero status by default without indication anything
failed.

I think exitting with a non-zero status would make more clear in scripts
and automated setups where things failed, thus making noticing the issue
and debugging easier.

Signed-off-by: Eduardo Otubo <otubo@redhat.com>
Signed-off-by: Aleksandar Kostadinov <akostadi@redhat.com>
@otubo
Copy link
Contributor Author

otubo commented Jul 2, 2020

Same as #469, just speeding things up since I already signed CLA. Agreed with original author.

@OddBloke
Copy link
Collaborator

OddBloke commented Jul 2, 2020

This LGTM, but I may be missing an angle on it. @smoser, you introduced this behaviour (9 years ago, admittedly), do you have an opinion on it?

@otubo
Copy link
Contributor Author

otubo commented Jul 3, 2020

Copy link
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

You should mark this as fixes https://bugs.launchpad.net/cloud-init/+bug/1170059 .

My other comment can be seen in https://bugs.launchpad.net/cloud-init/+bug/1170059/comments/1 .

Thats an interesting suggestion.
I don't think there'd be any significant fallout of changing that in trunk.

fwiw, it is configurable, by setting 'disable_root_opts'
default value is:
DISABLE_ROOT_OPTS = ("no-port-forwarding,no-agent-forwarding,"
"no-X11-forwarding,command="echo 'Please login as the user \"$USER\" "
"rather than the user \"root\".';echo;sleep 10"")

cloudinit/ssh_util.py Outdated Show resolved Hide resolved
@OddBloke OddBloke self-assigned this Jul 7, 2020
Copy link
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

@OddBloke Thoughts ?

@@ -44,7 +44,7 @@
DISABLE_USER_OPTS = (
"no-port-forwarding,no-agent-forwarding,"
"no-X11-forwarding,command=\"echo \'Please login as the user \\\"$USER\\\""
" rather than the user \\\"$DISABLE_USER\\\".\';echo;sleep 10\"")
" rather than the user \\\"$DISABLE_USER\\\".\';echo;sleep 10;exit 142\"")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to keep back and forth. I'm fine with the solution, but can we do something like this:

_DISABLE_USER_SSH_EXIT = 142

# caller replaces $USER and $DISABLE_USER
DISABLE_USER_OPTS = (
    "no-port-forwarding,no-agent-forwarding,"
    "no-X11-forwarding,command=\"echo \'Please login as the user \\\"$USER\\\""
    " rather than the user \\\"$DISABLE_USER\\\".\';echo;sleep 10;"
    "exit " + str(_DISABLE_USER_SSH_EXIT) + "\"")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why not. I'll send a final commit.

Copy link
Collaborator

@smoser smoser 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.

Copy link
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

I've tested this locally and it works; thanks!

@OddBloke OddBloke merged commit e161059 into canonical:master Jul 15, 2020
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