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

proxy: add support for Smartcard authentication #6633

Closed
wants to merge 6 commits into from

Conversation

sumit-bose
Copy link
Contributor

No description provided.

src/confdb/confdb.c Outdated Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/confdb/confdb.c Outdated Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
@alexey-tikhonov alexey-tikhonov self-assigned this Mar 21, 2023
@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Mar 21, 2023
@alexey-tikhonov
Copy link
Member

Hi @sumit-bose,

could you please rebase this PR (since #6672 was merged)?

src/confdb/confdb.c Outdated Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
@alexey-tikhonov alexey-tikhonov added backport-to-stable and removed no-backport This should go to target branch only. labels May 5, 2023
@alexey-tikhonov
Copy link
Member

(targets 2.9.1+)

@alexey-tikhonov
Copy link
Member

Hi @sumit-bose,

is this unblocked now, as local auth policy was introduced in d019132 ?

@sumit-bose sumit-bose changed the title [WIP POC] proxy: add support for Smartcard authentication proxy: add support for Smartcard authentication Sep 1, 2023
src/confdb/confdb.c Outdated Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
Copy link
Member

@pbrezina pbrezina 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. See comments inline, otherwise code-wise ack. Is there any way how can I test it without having a smartcard?

src/providers/proxy/proxy_init.c Outdated Show resolved Hide resolved
src/tests/intg/test_pam_responder.py Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved

ret = confdb_get_string(be_ctx->cdb, NULL, be_ctx->conf_path,
CONFDB_DOMAIN_LOCAL_AUTH_POLICY,
"match", &local_policy);
Copy link
Member

@alexey-tikhonov alexey-tikhonov Sep 16, 2023

Choose a reason for hiding this comment

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

$ grep -rn \"match\" *
responder/pam/pamsrv_cmd.c:943:                            "match", &local_policy);
responder/pam/pamsrv_cmd.c:954:    } else if (strcasecmp(local_policy, "match") == 0) {
responder/pam/pamsrv_cmd.c:1232:              && strcasecmp(local_policy, "match") == 0) {

-- "match", "only", etc begs for a define, imo...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

I meant in the entire code base, not in 'proxy provider' code locally...
But it can be done in another PR.

@alexey-tikhonov
Copy link
Member

Is there any way how can I test it without having a smartcard?

@spoore1, IIRC, in the past we briefly mentioned an option to run existing "smart card auth of local user" tests against "proxy" provider and a copr build from this PR.
Did you have a chance to consider this?

@sumit-bose
Copy link
Contributor Author

Hi @spoore1,

can you run your tests again with the latest version?

If proxy_pam_target is used to configure an authentication method and the local password policy does not allow Smartcard authentication you should see a password prompt even if a Smartcard is present. Now password authentication can be used with the configured target.

If in the same configuration local Smartcard authentication is allowed then Smartcard authentication is preferred and you should see a PIN prompt.

I think this behavior should match the expected behavior with the given configuration. Please let me know if you think if some aspects should be explained more explicitly in the man page?

bye,
Sumit

@spoore1
Copy link
Contributor

spoore1 commented Oct 11, 2023

Hi @spoore1,

can you run your tests again with the latest version?

If proxy_pam_target is used to configure an authentication method and the local password policy does not allow Smartcard authentication you should see a password prompt even if a Smartcard is present. Now password authentication can be used with the configured target.

If in the same configuration local Smartcard authentication is allowed then Smartcard authentication is preferred and you should see a PIN prompt.

I think this behavior should match the expected behavior with the given configuration. Please let me know if you think if some aspects should be explained more explicitly in the man page?

bye, Sumit

@sumit-bose I don't see the error anymore and it does appear to honor the different settings for local_auth_policy with a smart card inserted (using virtual for this test). Also, my only question about the man page wording is if local_auth_policy overrides proxy_pam_target if both are specified like I did in a few tests below?

[root@fedora38-pr6633 sssd]# rpm -q sssd
sssd-9.pr6633-03316.fc38.x86_64

###########################################################################
[root@fedora38-pr6633 sssd]# grep "domain/nssfiles" -A4 /etc/sssd/sssd.conf
[domain/nssfiles]
id_provider = proxy 
local_auth_policy = only
proxy_lib_name = files
[root@fedora38-pr6633 sssd]# su - localuser1 -c 'su - localuser1 -c whoami'
PIN for localuser1:
localuser1

###########################################################################
[root@fedora38-pr6633 sssd]# grep "domain/nssfiles" -A4 /etc/sssd/sssd.conf
[domain/nssfiles]
id_provider = proxy
proxy_lib_name = files
proxy_pam_target = sssd-shadowutils
[root@fedora38-pr6633 sssd]# su - localuser1 -c 'su - localuser1 -c whoami'
Password: 
localuser1

###########################################################################
[root@fedora38-pr6633 sssd]# grep "domain/nssfiles" -A4 /etc/sssd/sssd.conf
[domain/nssfiles]
id_provider = proxy
proxy_lib_name = files
proxy_pam_target = sssd-shadowutils
local_auth_policy = only
[root@fedora38-pr6633 sssd]# su - localuser1 -c 'su - localuser1 -c whoami'
PIN for localuser1:
localuser1

###########################################################################
[root@fedora38-pr6633 sssd]# grep "domain/nssfiles" -A4 /etc/sssd/sssd.conf
[domain/nssfiles]
id_provider = proxy
proxy_lib_name = files
proxy_pam_target = sssd-shadowutils
local_auth_policy = enable:passkey
[root@fedora38-pr6633 sssd]# su - localuser1 -c 'su - localuser1 -c whoami'
Password: 
localuser1

###########################################################################
[root@fedora38-pr6633 sssd]# grep "domain/nssfiles" -A4 /etc/sssd/sssd.conf
[domain/nssfiles]
id_provider = proxy
proxy_lib_name = files
proxy_pam_target = sssd-shadowutils 
local_auth_policy = enable:passkey, enable:smartcard
[root@fedora38-pr6633 sssd]# su - localuser1 -c 'su - localuser1 -c whoami'
PIN for localuser1:
localuser1

@sumit-bose
Copy link
Contributor Author

Hi,

thanks for testing. What about to add a paragraph like

Please note that if local Smartcard authentication is enabled and a Smartcard is present,
Smartcard authentication will be preferred over the authentication methods supported
by the backend. I.e. there will be a PIN prompt instead of e.g. a password prompt. 

to the local_auth_policy item of man sssd.conf?

bye,
Sumit

@spoore1
Copy link
Contributor

spoore1 commented Oct 11, 2023

@sumit-bose Adding that to the man page for local_auth_policy does make the expected behavior clear. And the wording looks good to me.

Thanks

@sumit-bose
Copy link
Contributor Author

@sumit-bose Adding that to the man page for local_auth_policy does make the expected behavior clear. And the wording looks good to me.

Thanks

Hi,

thanks, I added the paragraph in the latest version.

bye,
Sumit

src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
@alexey-tikhonov
Copy link
Member

@sumit-bose, could you please also add RN to the commit description?

To be able to do local Smartcard authenticate the backend must be able
to map a certificate to a user based on the provided mapping rules.

With this patch the proxy provider is able to handle the certificate
mapping rules and users handled by the proxy provider can be configured
for Smartcard authentication. Besides the mapping rule local Smartcard
authentication should be enable with the 'local_auth_policy' option in
the backend and with 'pam_cert_auth' in the PAM responder.

:relnote: The proxy provider is now able to handle certificate mapping and
  matching rules and users handled by the proxy provider can be
  configured for local Smartcard authentication. Besides the mapping rule
  local Smartcard authentication should be enable with the 'local_auth_policy'
  option in the backend and with 'pam_cert_auth' in the PAM responder.
The main use case of this NSS module is to run proxy provider tests with
cwrap's nss-wrapper.  The proxy provider loads the NSS modules directly
with dlopen() and is not using glibc's NSS mechanism. Since nss-wrapper
just wraps the standard glibc calls and does not provide an NSS module
on its own we have to use this workaround to make proxy provider work
with nss-wrapper.

DO NOT USE THIS IN /etc/nsswitch.conf, it will cause an infinite loop.
This patch replaces the deprecated files provider in the PAM responder
tests with the proxy provider. The straight-forward replacement would be
'proxy_lib_name = files' to use libnss_files.so.2 with the proxy
provider. But the tests are using nss-wrapper which wraps the plain
glibc calls. Because of this the test is using a dedicated NSS module to
work with nss-wrapper.
With this new boolean options the backends calling
confdb_certmap_to_sysdb() can indicate if the certificate mapping rules
should be applied for local users or not, which currently means LDAP
based mapping with a search filter string.
All Smartcard authentication related tests are run now with the proxy
provider and the deprecated files provider. If the files provider will
be removed the tests can be removed by reverting this patch.
SSSD currently assumed that PAM modules configured for the proxy auth
provider expect passwords as input. If a Smartcard is present during the
authentication, but local Smartcard authentication is not enabled, the
user should see a password prompt.
@sumit-bose
Copy link
Contributor Author

@sumit-bose, could you please also add RN to the commit description?

done

src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
src/tests/intg/nss_call.c Show resolved Hide resolved
@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Oct 13, 2023
@alexey-tikhonov
Copy link
Member

@sumit-bose, could you please also add RN to the commit description?

done

Thank you.

@pbrezina
Copy link
Member

Pushed PR: #6633

  • master
    • 4d475e4 - intg: add proxy auth with fallback test
    • f5f8030 - intg: use file and proxy provider in PAM responder test
    • 8952f6d - confdb: add new option for confdb_certmap_to_sysdb()
    • 54f5589 - intg: replace files with proxy provider in PAM responder test
    • ffd4674 - intg: add NSS module for nss-wrapper support
    • c386992 - proxy: add support for certificate mapping rules
  • sssd-2-9
    • 04b6a22 - intg: add proxy auth with fallback test
    • 7668ed6 - intg: use file and proxy provider in PAM responder test
    • 25a913e - confdb: add new option for confdb_certmap_to_sysdb()
    • d364914 - intg: replace files with proxy provider in PAM responder test
    • 351aab9 - intg: add NSS module for nss-wrapper support
    • 42face7 - proxy: add support for certificate mapping rules

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Oct 16, 2023
@pbrezina pbrezina closed this Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants