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

Add force second factor #6533

Closed
wants to merge 1 commit into from

Conversation

huckabeec
Copy link

In our use case, we have 2FA configured using SSSD with Kerberos/FAST and OTP configured on the KDCs. A secondary principal is created for each user that holds their OTP credentials.

Also, in our configuration, ssh prompts for both the First and Second factor.

What we found was that someone configured for OTP could bypass the 2FA requirement for ssh if they entered their primary principal password as the first factor due to how the pam_sss code allows an empty second factor. In a perfect world the primary principal would be scrambled/removed when 2FA is setup but we are in a transition state.

This pull request adds a 'force_second_factor' option that if set requires the second factor to be provided if and only if the PAM service being used is sshd. A NULL or missing second factor results in a return of PAM_CRED_INSUFFICIENT.

Hopefully this is useful to someone else as well.

@alexey-tikhonov
Copy link
Member

Hi @huckabeec,

sorry this PR got somehow lost. Could you please rebase on top of latest 'master' branch?

@alexey-tikhonov alexey-tikhonov added Waiting for review no-backport This should go to target branch only. labels Mar 27, 2023
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Why do you enable it only for ssh? Wouldn't it be useful for other services?

On top of that, do you mind squashing all the changes in one commit?

Comment on lines 263 to 264
all in the first factor, set this option. A NULL or
empty second factor will cause login to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the indentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Now there's a different indentation problem. I think it's because you are mixing tabs and spaces.

src/sss_client/pam_sss.c Outdated Show resolved Hide resolved
@sumit-bose
Copy link
Contributor

Hi,

I wonder if it wouldn't be easier to have this as new prompting configuration option for 2fa prompting? See the PROMPTING CONFIGURATION SECTION of man sssd.conf for details. With this you do not have to tweak the PAM configuration and can specify services other than sshd.

bye,
Sumit

@huckabeec
Copy link
Author

Why do you enable it only for ssh? Wouldn't it be useful for other services?

ssh was the only service called out in the original pam code where it made an exception for if i saw the first factor with data but the second factor null. I haven't experienced any other services that would take advantage of this.

On top of that, do you mind squashing all the changes in one commit?

I can try - my git fu is not strong

@huckabeec
Copy link
Author

Hi,

I wonder if it wouldn't be easier to have this as new prompting configuration option for 2fa prompting? See the PROMPTING CONFIGURATION SECTION of man sssd.conf for details. With this you do not have to tweak the PAM configuration and can specify services other than sshd.

bye, Sumit

The prompting capability doesn't address the core issue - the PAM module allows an ssh login to have only one factor specified no matter what the prompting setup is. It has code that cals out that specific use case, so the patch addresses that specific case where we don't want that behavior.

@ikerexxe
Copy link
Contributor

I can try - my git fu is not strong

https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History may help you

Fix typo

Fix parenthesis

Fix indentation
@huckabeec
Copy link
Author

I can try - my git fu is not strong

https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History may help you

That was indeed very helpful - thank you!

@sumit-bose
Copy link
Contributor

Hi,
I wonder if it wouldn't be easier to have this as new prompting configuration option for 2fa prompting? See the PROMPTING CONFIGURATION SECTION of man sssd.conf for details. With this you do not have to tweak the PAM configuration and can specify services other than sshd.
bye, Sumit

The prompting capability doesn't address the core issue - the PAM module allows an ssh login to have only one factor specified no matter what the prompting setup is. It has code that cals out that specific use case, so the patch addresses that specific case where we don't want that behavior.

Hi,

yes, the actual check will happen in pam_sss but my point is where the option to trigger the check is set. PAM configurations are somewhat fragile and I think it might be better to not add more options to pam_sss iof it can be avoided. The prompting option are send to pam_sss from SSSD's PAM responder, so you can trigger the same check with those as well.

bye,
Sumit

@huckabeec
Copy link
Author

Hi,
I wonder if it wouldn't be easier to have this as new prompting configuration option for 2fa prompting? See the PROMPTING CONFIGURATION SECTION of man sssd.conf for details. With this you do not have to tweak the PAM configuration and can specify services other than sshd.
bye, Sumit

The prompting capability doesn't address the core issue - the PAM module allows an ssh login to have only one factor specified no matter what the prompting setup is. It has code that cals out that specific use case, so the patch addresses that specific case where we don't want that behavior.

Hi,

yes, the actual check will happen in pam_sss but my point is where the option to trigger the check is set. PAM configurations are somewhat fragile and I think it might be better to not add more options to pam_sss iof it can be avoided. The prompting option are send to pam_sss from SSSD's PAM responder, so you can trigger the same check with those as well.

bye, Sumit

My initial goal with creating this patch was to limit the changes to just the pam code so it would be more maintainable for us if the changes were never accepted upstream. If someone else wants to create a larger patch set that implements the same effect by different means I obviously have no objections, but that might be outside of my abilities.

@sumit-bose
Copy link
Contributor

Hi,

I have created a proof-of-concept patch with my suggestion to handle this with a sssd.conf prompting option at sumit-bose@d2b0d53 . I wonder if you can check if this works for you as well.

Thanks.

bye,
Sumit

@huckabeec
Copy link
Author

Hi,

I have created a proof-of-concept patch with my suggestion to handle this with a sssd.conf prompting option at sumit-bose@d2b0d53 . I wonder if you can check if this works for you as well.

Thanks.

bye, Sumit

I’ll try this as soon as possible.

@sumit-bose
Copy link
Contributor

Hi,
I have created a proof-of-concept patch with my suggestion to handle this with a sssd.conf prompting option at sumit-bose@d2b0d53 . I wonder if you can check if this works for you as well.
Thanks.
bye, Sumit

I’ll try this as soon as possible.

Thanks

@ikerexxe
Copy link
Contributor

@huckabeec were you able to test Sumit's proposal?

@alexey-tikhonov
Copy link
Member

Please re-open in case of any update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes requested no-backport This should go to target branch only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants