On Wed, Apr 29, 2015 at 05:40:37PM +0200, Lukas Slebodnik wrote:
On (22/04/15 10:24), Sumit Bose wrote:
>On Mon, Apr 06, 2015 at 08:58:16PM +0200, Sumit Bose wrote:
>> Hi,
>>
>> this series of patches tries to add special prompting in the PAM dialog
>> for 2-Factor-Authentication (2FA) and use it where suitable. As
>> discussed on
>>
https://fedorahosted.org/sssd/wiki/DesignDocs/PAMConversationForOTP if
>> long-term password and one-time component are entered in a single prompt
>> SSSD is in general not able to split them and use the long-term password
>> of offline authentication or some user keyrings. With this patch-set an
>> IPA user with configured OTP token will be prompted to enter the two
>> factors separately (if the login application supports it) and the first
>> factor (long-term password) will be save into the cache for offline
>> authentication if it is long enough.
>>
>> Two new internals are needed for this. A special authentication token
>> type for 2FA and a PAM pre-auth request to check if the given user user
>> a standard password or 2FA/OTP.
>>
>> The first two patches enhance the authtok module to handle 2FA.
>> Patches 4-6 add the pre-auth request to PAM client, responder and IPA
>> backend. Patches 7-12 add the prompting, the minimal length check and
>> the caching of the first factor. Finally tests for the PAM responder are
>> added which should test existing and new behaviour.
>>
>
>Please find attached a new version of the patch set which contains a few
>fixes which should silence a couple of compiler warnings.
>
>bye,
>Sumit
There are some coding style issues and clang warning
It shoudl be fixed with attached patches.
I wanted to simplyfy your job :-)
Thank you very much for the review and the fixes, I included most of
your comments. For the few I skipped please find an explanation below.
>+
>+ fd = open(PAM_PREAUTH_INDICATOR, O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW,
>+ 0644);
Can we use syscall "creat" or do we need special flags?
creat() uses O_CREAT|O_WRONLY|O_TRUNC, I found O_EXCL and O_NOFOLLOW
important hence I used open()
>+ if (fd < 0) {
>+ if (errno != EEXIST) {
>+ DEBUG(SSSDBG_OP_FAILURE,
>+ "Failed to create preauth indicator file [%s].\n",
>+ PAM_PREAUTH_INDICATOR);
>+ ret = EOK;
>+ goto done;
>+ }
>+
>+ DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Preauth indicator file [%s] already exists. " \
>+ "Maybe it is left after an unplanned exit. Continuing.\n",
>+ PAM_PREAUTH_INDICATOR);
>+ } else {
>+ close(fd);
>+ }
>+
...
>+ ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_TYPE,
authtok_type);
>+ if (ret) goto fail;
"enum sss_authtok_type" is stored as long but read as int
sh$ $git grep SYSDB_CACHEDPWD_TYPE
src/db/sysdb.h-#define SYSDB_CACHEDPWD "cachedPassword"
src/db/sysdb.h:#define SYSDB_CACHEDPWD_TYPE "cachedPasswordType"
--
src/db/sysdb_ops.c-
src/db/sysdb_ops.c: ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_TYPE,
authtok_type);
--
src/db/sysdb_ops.c- cached_authtok_type = ldb_msg_find_attr_as_uint(ldb_msg,
src/db/sysdb_ops.c:
SYSDB_CACHEDPWD_TYPE,
--
src/db/sysdb_ops.c- "accountExpires",
SYSDB_FAILED_LOGIN_ATTEMPTS,
src/db/sysdb_ops.c: SYSDB_LAST_FAILED_LOGIN,
SYSDB_CACHEDPWD_TYPE,
--
src/tests/sysdb-tests.c- struct ldb_result *res;
src/tests/sysdb-tests.c: const char *attrs[] = { SYSDB_CACHEDPWD_TYPE,
SYSDB_CACHEDPWD_FA2_LEN,
--
src/tests/sysdb-tests.c-
src/tests/sysdb-tests.c: val = ldb_msg_find_attr_as_int(res->msgs[0],
SYSDB_CACHEDPWD_TYPE, 0);
--
src/tests/sysdb-tests.c-
src/tests/sysdb-tests.c: val = ldb_msg_find_attr_as_int(res->msgs[0],
SYSDB_CACHEDPWD_TYPE, 0);
C standard does not say exactly the size of enum, but int should be enough
and type should be the same for read and store
unfortunately the overlap between the ldb attr and sysdb attr calls are
quite minimal. ldb has int, unsigned int, int64_t and uint64_t while in
sysdb we have long and uint32_t. So we have to add a new sysdb call, I'm
just wondering what would be the most suitable one for enums. It looks
like the expression that defines the value must be int, so maybe int
might be the most useful choice. Do you have any suggestions here?
New version attached.
bye,
Sumit