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

new access option: --force-password <HASH>, to only try one specific password #256

Merged
merged 4 commits into from
Dec 9, 2021

Conversation

madchrist
Copy link
Contributor

This is like --force-key, but for passwords

When this option is set, instead of autologin trying all the passwords until one works, only the one matching the hash is tried
If no password matches the hash then it aborts

This works for both personal accesses and group accesses

@madchrist
Copy link
Contributor Author

madchrist commented Oct 26, 2021

I used a new test file (341-selfaccesses-force-password.sh) because I didn't want to put so much tests into one of the existing files
But I don't know if I should use ${capabilities[mfa]} / ${capabilities[pamtester]} or something similar

@madchrist madchrist force-pushed the force-password branch 2 times, most recently from 6fc732c to 2f238d1 Compare October 27, 2021 11:32
@madchrist
Copy link
Contributor Author

Fixed a few perl-critic/tidy issues

@madchrist
Copy link
Contributor Author

Fixed a couple wrong plugin names in the doc

@speed47 speed47 added the tests:full Launch full tests through GitHub Actions label Oct 27, 2021
@speed47
Copy link
Collaborator

speed47 commented Oct 28, 2021

Hey @madchrist , thanks for this PR.

I used a new test file (341-selfaccesses-force-password.sh) because I didn't want to put so much tests into one of the existing files

Good move, this way you can test it easily without running all the other tests:

./tests/functional/docker/docker_build_and_run_tests.sh debian10 --module=341-selfaccesses-force-password.sh

FYI the options you can pass in addition to the OS are actually the same options than the ones that can be seen in ./tests/functional/launch_tests_on_instance.sh --help. I need to document the dev process, sorry you had to find all this out by yourself :/

But I don't know if I should use ${capabilities[mfa]} / ${capabilities[pamtester]} or something similar

That's exactly why the OpenSUSE tests are failing, as MFA is not supported on it, you have to skip these tests. You can add this on the bottom of 341-selfaccesses-force-password.sh:

if [ "${capabilities[mfa]}" = 1 ] || [ "${capabilities[mfa-password]}" = 1 ]; then
    testsuite_selfaccesses_force_password
fi

And the tests should pass!

@madchrist
Copy link
Contributor Author

Ok, that makes sense, thanks for the tips

The new option doesn't need MFA, so it should work on opensuse
The test on the other hand uses MFA, so I think I will modify it to not need MFA anymore
That way everything will support opensuse (and other non-MFA systems)

@speed47
Copy link
Collaborator

speed47 commented Oct 28, 2021

The new option doesn't need MFA, so it should work on opensuse The test on the other hand uses MFA, so I think I will modify it to not need MFA anymore That way everything will support opensuse (and other non-MFA systems)

Indeed, that would be perfect.

@madchrist
Copy link
Contributor Author

madchrist commented Nov 2, 2021

I updated the test to handle non-mfa systems
(and rebased)

@madchrist
Copy link
Contributor Author

Small fix for opensuse15.2

Copy link
Collaborator

@speed47 speed47 left a comment

Choose a reason for hiding this comment

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

Looks mostly ok, just a few suggestions before merging!

As a side note, we might want in a future version to support more parameters types to --force-password, instead of the raw hash (which we might not want to appear in logs, for example). We could support an index number (0 = main password, N>0 = Nth fallback password as you've done internally), or even an id representing the hash of a password, as we do for e.g. egress ssh keys (see the output of selfListEgressKeys). We can do this later in another PR though, this one is probably large enough!

Comment on lines 164 to 172
elsif ($hash =~ /^\$1\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'md5crypt', hash => $hash});
}
elsif ($hash =~ /^\$5\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'sha256crypt', hash => $hash});
}
elsif ($hash =~ /^\$6\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'sha512crypt', hash => $hash});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

as we've just verified the hash validity, we can now untaint it and pass it up so that our caller can get the untainted version:

Suggested change
elsif ($hash =~ /^\$1\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'md5crypt', hash => $hash});
}
elsif ($hash =~ /^\$5\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'sha256crypt', hash => $hash});
}
elsif ($hash =~ /^\$6\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+$/) {
return R('OK', value => {type => 'sha512crypt', hash => $hash});
}
elsif ($hash =~ /^(\$1\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+)$/) {
return R('OK', value => {type => 'md5crypt', hash => $1});
}
elsif ($hash =~ /^(\$5\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+)$/) {
return R('OK', value => {type => 'sha256crypt', hash => $1});
}
elsif ($hash =~ /^(\$6\$[a-zA-Z0-9]+\$[a-zA-Z0-9\.\/]+)$/) {
return R('OK', value => {type => 'sha512crypt', hash => $1});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

# fetch checksums for a1|g1's second and third egress passwords
success ${mode}_listpass $a0 --osh $list_pass_plugin $target
json .error_code OK .command $list_pass_plugin
password2_sha256=$(get_json | jq '.value[1].hashes.sha256crypt' | sed -e 's/"//g')
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can just use jq -r here, to get the raw output, instead of sed'ing out the "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

bin/shell/osh.pl Outdated
if (
$grant->{'forcePassword'}
&& ( ($userPasswordContext eq 'self' && $grant->{'type'} eq 'personal')
|| ($userPasswordContext eq 'group' && $grant->{'type'} eq 'group-member' && $grant->{'group'} eq $userPasswordClue))
Copy link
Collaborator

Choose a reason for hiding this comment

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

A user can also be a group guest (i.e. have access to the group credentials, such as egress keys and passwords, but not automatically be granted the whole servers/device list, just a subset of those), in which case we should also enter this if.
It can be checked with $grant->{'type'} eq 'group-guest'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While fixing this I noticed the guest grant does not contain the extra infos (the forced password)
To fix this, when fetching the guest grant, I replace it with the matching group grant

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hah, actually you can't do that, it'll have adverse effects, this is because a guest access can carry more information that the member access, for a given server, such as a TTL expiration. e.g. you can have an ACL in the group like this:
127.0.0.0/8 port-any user-any
And a guest group access for some account with:
127.1.2.3 port 22 user root TTL=1d

Access will be allowed for the guest (as long as the TTL has not expired) because this guest ACL matches at least one of the regular group ACL. This also means that if the 127.0.0.0/8 ACL is removed from the group, the guest account will no longer have access to 127.1.2.3 through it, because even if this ACL is declared for their guest access, it no longer matches any ACL of the group itself.

In other words, guest group accesses may be more specific that the group ACL.

What this means for the force-password feature is that this should also be declared (if needed) when declaring the group guest access. Side note: the force-key feature has the same issue, as you can't use --force-key with groupAddGuestAccess, there is no way to force a key in guest accesses. We should fix that by adding the --force-key and --force-password options to group{Add,Del}GuestAccess. As this PR has already a lot of changes, I suggest doing it in a later one to avoid working on this one for 6 months :)

So in the meantime, so we can go forward, can you revert your change to is_access_granted by restoring groupGuest and groupLegacy vars instead of groupMember?

Note to self: I'll also add a test case that ensure such code change can no longer pass all the tests!

@@ -780,14 +786,14 @@ sub is_access_granted {

# the guy must have a guest access but the group itself must also still have access
if ($grantedGuest && $grantedGroup) {
push @grants, {type => 'group-guest', group => $shortGroup, %{$grantedGuest->value}};
push @grants, {type => 'group-guest', group => $shortGroup, %{$grantedGroup->value}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
push @grants, {type => 'group-guest', group => $shortGroup, %{$grantedGroup->value}};
push @grants, {type => 'group-guest', group => $shortGroup, %{$grantedGuest->value}};

osh_debug("is_access_granted: adding grantedGuest to grants because is guest and group has access");
}

# special legacy case; we also check if account has a legacy access for ip AND that the group ALSO has access to this ip
if ($grantedLegacy && $grantedGroup) {
osh_debug("is_access_granted: adding grantedLegacy to grants because legacy not null and group has access");
push @grants, {type => 'group-guest-legacy', group => $shortGroup, %{$grantedLegacy->value}};
push @grants, {type => 'group-guest-legacy', group => $shortGroup, %{$grantedGroup->value}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
push @grants, {type => 'group-guest-legacy', group => $shortGroup, %{$grantedGroup->value}};
push @grants, {type => 'group-guest-legacy', group => $shortGroup, %{$grantedLegacy->value}};

)
{

# fetch the hashes of the main password and all its fallbacks
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# fetch the hashes of the main password and all its fallbacks
# FIXME: force-password and force-key don't work yet for guest accesses, see #256
# fetch the hashes of the main password and all its fallbacks

@speed47
Copy link
Collaborator

speed47 commented Dec 2, 2021

Suggested changes above effectively revert the changes so that we have a working force-password without side effect, and a note that it's not implemented yet for group guests (see conversation above)

@madchrist
Copy link
Contributor Author

Guest access works a little differently than I though, so I removed guest support for now as suggested
I'll make another PR for it once this one goes through

speed47
speed47 previously approved these changes Dec 8, 2021
bin/shell/osh.pl Outdated
# FIXME: force-password and force-key don't work yet for guest accesses, see #256
# fetch the hashes of the main password and all its fallbacks
my $fnrethashes;
if ($userPasswordContext eq 'self') { $fnrethashes = OVH::Bastion::get_hashes_list(context => 'account', account => $userPasswordClue); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

May you please but these blocks using many lines to keep the style consistent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note: perltidy doesn't seem to have a config option to force multi-statement one-liners to be split against multiple lines, unfortunately. That's why it went undetected/unmodified. However, if only one \n is added to such one-liner, between two statements, then perltidy will correctly do the job of re-arranging the whole block correctly, one statement per line.

In other words you can just add a \n on each of those lines after any semi-colon, run perltidy, and you should be good to go.

@speed47
Copy link
Collaborator

speed47 commented Dec 9, 2021

OpenSUSE failure just happened also on another PR, they seem to have changed something around their fping binary, which doesn't seem to work well inside a Docker when not called from root. I'll go ahead and merge, this problem has nothing to do with this PR's code. It'll be investigated separately.

@speed47 speed47 merged commit 98c1c79 into ovh:master Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests:full Launch full tests through GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants