Merge ~ahasenack/ubuntu/+source/sssd:bionic-sssd-max-id-1807246 into ubuntu/+source/sssd:ubuntu/bionic-devel

[Andreas Hasenack]

Bileto ticket and ppa, with green dep8 tests: https://bileto.ubuntu.com/#/ticket/3594

Upstream fix for bug #1807246. Since sssd always requires a somewhat complicated setup to test, I decided to also include the DEP8 tests we have added to the package in cosmic and later. To that end, I added a bionic task to #1793882 and included them here. I believe this gives more confidence in this particular update for an LTS, and also future updates of sssd.

I debated if I should pick just the one-liner that is the real fix, or the whole upstream commit. I chose the latter, since it applied cleanly, but one could argue that the SRU spirit means I should have picked the one-liner. I can see it getting in the way of future patches because of unecessary noise, but at the same time it is closer to upstream, which is where future patches are developed.

The bug has the SRU template filled out, and the testing instructions can be followed there.

[Robie Basak (racb)]

dep8 tests are now identical to those on the development release

The new fix-id-out-of-range-lookup.patch is identical to the upstream commit (except for added dep3 headers)

Version string in changelog correct, no collisions

Changelog message looks good with correct bug references.

No other changes.

SRU information prepared and complete

Whitespace changes are great but I agree they're trivial enough that it's cleaner to do this than diverge from the upstream patch. Thank you for explaining your reasoning on this.

The patch itself looks fine.

Upload and SRU +1

review: Approve

[Andreas Hasenack (ahasenack)]

Tagged and uploaded, thanks.

diff --git a/debian/changelog b/debian/changelog
index d8193ce..9557805 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,14 @@
+sssd (1.16.1-1ubuntu1.1) bionic; urgency=medium
+
+  * d/p/fix-id-out-of-range-lookup.patch: CACHE_REQ: Do not fail the domain
+    locator plugin if ID outside the domain range is looked up.  Thanks to
+    Jakub Hrozek <[e-mail]>. (LP: #1807246)
+  * d/t/common-tests, d/t/control, d/t/ldap-user-group-krb5-auth,
+    d/t/ldap-user-group-ldap-auth, d/t/login.exp, d/t/util: add DEP8
+    tests for kerberos and LDAP (LP: #1793882)
+
+ -- Andreas Hasenack <[e-mail]>  Wed, 16 Jan 2019 13:58:03 -0200
+
 sssd (1.16.1-1ubuntu1) bionic; urgency=medium
 
   * Merge from Debian. (LP: #1754746)
diff --git a/debian/patches/fix-id-out-of-range-lookup.patch b/debian/patches/fix-id-out-of-range-lookup.patch
 new file mode 100644
index 0000000..404479f
--- /dev/null
+++ b/debian/patches/fix-id-out-of-range-lookup.patch
@@ -0,0 +1,117 @@
+From 2952de740f2ec1da9cbd682fb1d9219e5370e6a1 Mon Sep 17 00:00:00 2001
+From: Jakub Hrozek <[e-mail]>
+Date: May 03 2018 20:02:44 +0000
+Subject: CACHE_REQ: Do not fail the domain locator plugin if ID outside the domain range is looked up
+
+
+A fix for upstream bug #3569 and the domain-locator feature were both
+developed in the context of the same upstream version and therefore
+touched the same code, but the domain locator did not account for the
+ERR_ID_OUTSIDE_RANGE error code.
+
+Therefore lookups for IDs that are outside the range for the domain
+caused the whole lookup to fail instead of carrying on to the next
+domain.
+
+This patch just handles ERR_ID_OUTSIDE_RANGE the same way as if the ID
+was not found at all. Also some whitespace errors are fixed.
+
+Resolves:
+https://pagure.io/SSSD/sssd/issue/3728
+
+Reviewed-by: Fabiano Fidêncio <[e-mail]>
+Origin: upstream, https://pagure.io/SSSD/sssd/c/2952de7
+Bug: https://pagure.io/SSSD/sssd/issue/3728
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1807246
+Last-Update: 2019-01-15
+---
+
+diff --git a/src/responder/common/cache_req/cache_req.c b/src/responder/common/cache_req/cache_req.c
+index 134688b..28b5633 100644
+--- a/src/responder/common/cache_req/cache_req.c
++++ b/src/responder/common/cache_req/cache_req.c
+@@ -523,6 +523,7 @@ static void cache_req_locate_dom_cache_done(struct tevent_req *subreq)
+         DEBUG(SSSDBG_TRACE_INTERNAL, "Result found in the cache\n");
+         tevent_req_done(req);
+         return;
++    case ERR_ID_OUTSIDE_RANGE:
+     case ENOENT:
+         /* Not cached and locator was requested, run the locator
+          * DP request plugin
+diff --git a/src/responder/common/cache_req/plugins/cache_req_common.c b/src/responder/common/cache_req/plugins/cache_req_common.c
+index 2404168..d19ca89 100644
+--- a/src/responder/common/cache_req/plugins/cache_req_common.c
++++ b/src/responder/common/cache_req/plugins/cache_req_common.c
+@@ -27,7 +27,7 @@
+ #include "responder/common/cache_req/cache_req_plugin.h"
+ 
+ errno_t cache_req_idminmax_check(struct cache_req_data *data,
+-	                         struct sss_domain_info *domain)
++                                 struct sss_domain_info *domain)
+ {
+    if (((domain->id_min != 0) && (data->id < domain->id_min)) ||
+        ((domain->id_max != 0) && (data->id > domain->id_max))) {
+diff --git a/src/responder/common/cache_req/plugins/cache_req_group_by_id.c b/src/responder/common/cache_req/plugins/cache_req_group_by_id.c
+index 3fb8103..e0c6b65 100644
+--- a/src/responder/common/cache_req/plugins/cache_req_group_by_id.c
++++ b/src/responder/common/cache_req/plugins/cache_req_group_by_id.c
+@@ -85,7 +85,7 @@ cache_req_group_by_id_lookup(TALLOC_CTX *mem_ctx,
+ 
+     ret = cache_req_idminmax_check(data, domain);
+     if (ret != EOK) {
+-	return ret;
++        return ret;
+     }
+     return sysdb_getgrgid_with_views(mem_ctx, domain, data->id, _result);
+ }
+diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
+index 252d89d..45d71b8 100644
+--- a/src/tests/cmocka/test_responder_cache_req.c
++++ b/src/tests/cmocka/test_responder_cache_req.c
+@@ -1827,6 +1827,37 @@ void test_group_by_id_multiple_domains_notfound(void **state)
+     assert_true(test_ctx->dp_called);
+ }
+ 
++void test_group_by_id_multiple_domains_outside_id_range(void **state)
++{
++    struct cache_req_test_ctx *test_ctx = NULL;
++    struct sss_domain_info *domain = NULL;
++    struct sss_domain_info *domain_a = NULL;
++
++    test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
++
++    domain_a = find_domain_by_name(test_ctx->tctx->dom,
++                                   "responder_cache_req_test_a", true);
++    assert_non_null(domain_a);
++    domain_a->id_min = 1;
++    domain_a->id_max = 100;
++
++    /* Setup group. */
++    domain = find_domain_by_name(test_ctx->tctx->dom,
++                                 "responder_cache_req_test_d", true);
++    assert_non_null(domain);
++    prepare_group(domain, &groups[0], 1000, time(NULL));
++
++    /* Mock values. */
++    will_return_always(__wrap_sss_dp_get_account_send, test_ctx);
++    will_return_always(sss_dp_req_recv, 0);
++    will_return_always(sss_dp_get_account_domain_recv, ERR_GET_ACCT_DOM_NOT_SUPPORTED);
++
++    /* Test. */
++    run_group_by_id(test_ctx, NULL, 0, ERR_OK);
++    assert_true(test_ctx->dp_called);
++    check_group(test_ctx, &groups[0], domain);
++}
++
+ void test_group_by_id_multiple_domains_locator_cache_valid(void **state)
+ {
+     struct cache_req_test_ctx *test_ctx = NULL;
+@@ -3970,6 +4001,7 @@ int main(int argc, const char *argv[])
+         new_single_domain_test(group_by_id_missing_notfound),
+         new_multi_domain_test(group_by_id_multiple_domains_found),
+         new_multi_domain_test(group_by_id_multiple_domains_notfound),
++        new_multi_domain_test(group_by_id_multiple_domains_outside_id_range),
+ 
+         new_multi_domain_test(group_by_id_multiple_domains_locator_cache_valid),
+         new_multi_domain_test(group_by_id_multiple_domains_locator_cache_expired),
+
diff --git a/debian/patches/series b/debian/patches/series
index a82d1b4..ca3afd4 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
 #placeholder
+fix-id-out-of-range-lookup.patch
diff --git a/debian/tests/common-tests b/debian/tests/common-tests
 new file mode 100644
index 0000000..1bb8e1a
--- /dev/null
+++ b/debian/tests/common-tests
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+run_common_tests() {
+    echo "Assert local user databases do not have our LDAP test data"
+    check_local_user "${ldap_user}"
+    check_local_group "${ldap_user}"
+    check_local_group "${ldap_group}"
+
+    echo "The LDAP user is known to the system via getent"
+    check_getent_user "${ldap_user}"
+
+    echo "The LDAP user's private group is known to the system via getent"
+    check_getent_group "${ldap_user}"
+
+    echo "The LDAP group ${ldap_group} is known to the system via getent"
+    check_getent_group "${ldap_group}"
+
+    echo "The id(1) command can resolve the group membership of the LDAP user"
+    #$ id -Gn testuser1
+    #testuser1 ldapusers
+    output=$(id -Gn ${ldap_user})
+    # XXX couldn't find a better way to make this comparison using just /bin/sh
+    if [ "${output}" != "${ldap_user} ${ldap_group}" ]; then
+        if [ "${output}" != "${ldap_group} ${ldap_user}" ]; then
+            die "Output doesn't match expected group membership: ${output}"
+        fi
+    fi
+}
diff --git a/debian/tests/control b/debian/tests/control
 new file mode 100644
index 0000000..93ed50a
--- /dev/null
+++ b/debian/tests/control
@@ -0,0 +1,7 @@
+Tests: ldap-user-group-ldap-auth
+Depends: @, slapd, ldap-utils, openssl, expect, lsb-release
+Restrictions: isolation-container, needs-root, allow-stderr
+
+Tests: ldap-user-group-krb5-auth
+Depends: @, slapd, ldap-utils, openssl, expect, lsb-release, krb5-user, krb5-admin-server, krb5-kdc
+Restrictions: isolation-container, needs-root, allow-stderr
diff --git a/debian/tests/ldap-user-group-krb5-auth b/debian/tests/ldap-user-group-krb5-auth
 new file mode 100644
index 0000000..e50382e
--- /dev/null
+++ b/debian/tests/ldap-user-group-krb5-auth
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+set -ex
+
+. debian/tests/util
+. debian/tests/common-tests
+
+mydomain="example.com"
+myhostname="ldap.${mydomain}"
+mysuffix="dc=example,dc=com"
+myrealm="EXAMPLE.COM"
+admin_dn="cn=admin,${mysuffix}"
+admin_pw="secret"
+ldap_user="testuser1"
+ldap_user_pw="testuser1secret"
+kerberos_principal_pw="testuser1kerberos"
+ldap_group="ldapusers"
+
+adjust_hostname "${myhostname}"
+reconfigure_slapd
+generate_certs "${myhostname}"
+enable_ldap_ssl
+populate_ldap_rfc2307
+create_realm "${myrealm}" "${myhostname}"
+create_krb_principal "${ldap_user}" "${kerberos_principal_pw}"
+configure_sssd_ldap_rfc2307_krb5_auth
+enable_pam_mkhomedir
+
+# tests begin here
+run_common_tests
+
+# login works with the kerneros password
+echo "The Kerberos principal can login on a terminal"
+kdestroy > /dev/null 2>&1 || /bin/true
+/usr/bin/expect -f debian/tests/login.exp "${ldap_user}" "${kerberos_principal_pw}" "${ldap_user}"@"${myrealm}"
diff --git a/debian/tests/ldap-user-group-ldap-auth b/debian/tests/ldap-user-group-ldap-auth
 new file mode 100644
index 0000000..c25cff0
--- /dev/null
+++ b/debian/tests/ldap-user-group-ldap-auth
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+set -ex
+
+. debian/tests/util
+. debian/tests/common-tests
+
+mydomain="example.com"
+myhostname="ldap.${mydomain}"
+mysuffix="dc=example,dc=com"
+admin_dn="cn=admin,${mysuffix}"
+admin_pw="secret"
+ldap_user="testuser1"
+ldap_user_pw="testuser1secret"
+ldap_group="ldapusers"
+
+adjust_hostname "${myhostname}"
+reconfigure_slapd
+generate_certs "${myhostname}"
+enable_ldap_ssl
+populate_ldap_rfc2307
+configure_sssd_ldap_rfc2307
+enable_pam_mkhomedir
+
+# tests begin here
+run_common_tests
+
+echo "The LDAP user can login on a terminal"
+/usr/bin/expect -f debian/tests/login.exp "${ldap_user}" "${ldap_user_pw}"
diff --git a/debian/tests/login.exp b/debian/tests/login.exp
 new file mode 100644
index 0000000..63c25ab
--- /dev/null
+++ b/debian/tests/login.exp
@@ -0,0 +1,74 @@
+#!/usr/bin/expect
+
+set timeout 10
+set user [lindex $argv 0]
+set password [lindex $argv 1]
+set principal [lindex $argv 2]
+
+set distribution [exec "lsb_release" "-is"]
+
+if { $distribution == "Ubuntu" } {
+    set welcome "Welcome to"
+} elseif { $distribution == "Debian" } {
+    set welcome "Debian GNU/Linux comes"
+} else {
+    puts "Unsupported linux distribution $distribution"
+    exit 1
+}
+
+spawn login
+expect "login:"
+send "$user\r"
+expect "Password:"
+send "$password\r"
+expect {
+  timeout
+  {
+    puts "Expect error: timeout after password\r\r"
+    exit 1
+  }
+  "Login incorrect"
+  {
+    puts "Expect error: incorrect credentials\r\r"
+    exit 1
+  }
+  "$welcome"
+}
+expect {
+  timeout
+  {
+    puts "Expect error: timeout waiting for prompt\r\r"
+    exit 1
+  }
+  "$ "
+}
+send "id -un\r"
+expect {
+  timeout
+  {
+    puts "Expect error: timeout waiting for 'id' result\r\r"
+    exit 1
+  }
+  "$user"
+}
+expect {
+  timeout
+  {
+    puts "Expect error: timeout waiting for prompt\r\r"
+    exit 1
+  }
+  "$ "
+}
+if { $principal != "" } {
+  send "klist\r"
+  expect {
+    timeout
+    {
+      puts "Expect error: timeout waiting for klist output\r\r"
+      exit 1
+    }
+    "Default principal: $principal"
+  }
+}
+send "logout\r"
+exit 0
diff --git a/debian/tests/util b/debian/tests/util
 new file mode 100644
index 0000000..80f0bea
--- /dev/null
+++ b/debian/tests/util
@@ -0,0 +1,259 @@
+#!/bin/sh
+
+reconfigure_slapd() {
+    debconf-set-selections << EOF
+slapd	slapd/domain	string	${mydomain}
+slapd	shared/organization	string	${mydomain}
+slapd	slapd/password1	password	${admin_pw}
+slapd	slapd/password2	password	${admin_pw}
+EOF
+    rm -rf /var/backups/*slapd* /var/backups/unknown*ldapdb
+    dpkg-reconfigure -fnoninteractive -pcritical slapd
+}
+
+die() {
+    echo "ERROR"
+    echo "$@"
+    exit 1
+}
+
+enable_pam_mkhomedir() {
+    if ! grep -qE "^session.*pam_mkhomedir\.so" /etc/pam.d/common-session; then
+        echo "session optional pam_mkhomedir.so" >> /etc/pam.d/common-session
+    fi
+}
+
+adjust_hostname() {
+    local myhostname="$1"
+
+    echo "${myhostname}" > /etc/hostname
+    hostname "${myhostname}"
+    if ! grep -qE "${myhostname}" /etc/hosts; then
+        # just so it's resolvable
+        echo "127.0.1.10 ${myhostname}" >> /etc/hosts
+    fi
+}
+
+generate_certs() {
+    local cn="$1"
+    local cert="/etc/ldap/server.pem"
+    local key="/etc/ldap/server.key"
+    local cnf="/etc/ldap/openssl.cnf"
+
+    cat > "$cnf" <<EOF
+RANDFILE                = /dev/urandom
+[ req ]
+default_bits            = 1024
+default_keyfile         = privkey.pem
+distinguished_name      = req_distinguished_name
+prompt                  = no
+policy                  = policy_anything
+
+[ req_distinguished_name ]
+commonName              = ${cn}
+EOF
+    openssl req -new -x509 -nodes -out "$cert" -keyout "$key" -config "$cnf"
+    chmod 0640 "$key"
+    chgrp openldap "$key"
+    if [ ! -f "$cert" ]; then
+        echo "ERROR, failed to generate certificate for ldap test"
+        exit 1
+    fi
+    if [ ! -f "$key" ]; then
+        echo "ERROR, failed to generate key for ldap test"
+        exit 1
+    fi
+}
+
+enable_ldap_ssl() {
+    cat > /etc/ldap/ldap.conf <<EOF
+BASE ${mysuffix}
+URI ldap://${myhostname}
+TLS_CACERT /etc/ldap/server.pem
+EOF
+    {
+        cat <<EOF
+dn: cn=config
+add: olcTLSCACertificateFile
+olcTLSCACertificateFile: /etc/ldap/server.pem
+-
+add: olcTLSCertificateFile
+olcTLSCertificateFile: /etc/ldap/server.pem
+-
+add: olcTLSCertificateKeyFile
+olcTLSCertificateKeyFile: /etc/ldap/server.key
+EOF
+    } | ldapmodify -H ldapi:/// -Y EXTERNAL -Q 
+}
+
+populate_ldap_rfc2307() {
+    {
+        cat <<EOF
+dn: ou=People,${mysuffix}
+ou: People
+objectClass: organizationalUnit
+
+dn: ou=Group,${mysuffix}
+ou: Group
+objectClass: organizationalUnit
+
+dn: uid=${ldap_user},ou=People,${mysuffix}
+uid: ${ldap_user}
+objectClass: inetOrgPerson
+objectClass: posixAccount
+cn: ${ldap_user}
+sn: ${ldap_user}
+givenName: ${ldap_user}
+mail: ${ldap_user}@${mydomain}
+userPassword: ${ldap_user_pw}
+uidNumber: 10001
+gidNumber: 10001
+loginShell: /bin/bash
+homeDirectory: /home/${ldap_user}
+
+dn: cn=${ldap_user},ou=Group,${mysuffix}
+cn: ${ldap_user}
+objectClass: posixGroup
+gidNumber: 10001
+memberUid: ${ldap_user}
+
+dn: cn=${ldap_group},ou=Group,${mysuffix}
+cn: ${ldap_group}
+objectClass: posixGroup
+gidNumber: 10100
+memberUid: ${ldap_user}
+EOF
+    } | ldapadd -x -D "${admin_dn}" -w "${admin_pw}"
+}
+
+configure_sssd_ldap_rfc2307_krb5_auth() {
+    cat > /etc/sssd/sssd.conf <<EOF
+[sssd]
+config_file_version = 2
+services = nss, pam
+domains = LDAP
+
+[domain/LDAP]
+id_provider = ldap
+ldap_uri = ldap://${myhostname}
+auth_provider = krb5
+krb5_server = ${myhostname}
+krb5_realm = ${myrealm}
+cache_credentials = True
+ldap_search_base = ${mysuffix}
+EOF
+    chmod 0600 /etc/sssd/sssd.conf
+    systemctl restart sssd
+}
+
+configure_sssd_ldap_rfc2307() {
+    cat > /etc/sssd/sssd.conf <<EOF
+[sssd]
+config_file_version = 2
+services = nss, pam
+domains = LDAP
+
+[domain/LDAP]
+id_provider = ldap
+ldap_uri = ldap://${myhostname}
+cache_credentials = True
+ldap_search_base = ${mysuffix}
+EOF
+    chmod 0600 /etc/sssd/sssd.conf
+    systemctl restart sssd
+}
+
+check_local_user() {
+    local local_user="$1"
+
+    if grep -q "^${local_user}" /etc/passwd; then
+        die "Found ${local_user} in /etc/passwd"
+    fi
+}
+
+check_local_group() {
+    local local_group="$1"
+
+    if grep -q "^${local_group}" /etc/group; then
+        die "Found ${local_group} in /etc/group"
+    fi
+}
+
+check_getent_user() {
+    local getent_user="$1"
+    local output
+
+    output=$(getent passwd ${getent_user})
+    if [ -z "${output}" ]; then
+        die "${getent_user} not found via getent passwd"
+    fi
+}
+
+check_getent_group() {
+    local getent_group="$1"
+    local output
+
+    output=$(getent group ${getent_group})
+    if [ -z "${output}" ]; then
+        die "${getent_group} not found via getent group"
+    fi
+}
+
+create_realm() {
+    local realm_name="$1"
+    local kerberos_server="$2"
+
+    # start fresh
+    rm -rf /var/lib/krb5kdc/*
+    rm -rf /etc/krb5kdc/*
+    rm -f /etc/krb5.keytab
+
+    # setup some defaults
+    cat > /etc/krb5kdc/kdc.conf <<EOF
+[kdcdefaults]
+    kdc_ports = 750,88
+[realms]
+    ${realm_name} = {
+	    database_name = /var/lib/krb5kdc/principal
+	    admin_keytab = FILE:/etc/krb5kdc/kadm5.keytab
+	    acl_file = /etc/krb5kdc/kadm5.acl
+	    key_stash_file = /etc/krb5kdc/stash
+	    kdc_ports = 750,88
+	    max_life = 10h 0m 0s
+	    max_renewable_life = 7d 0h 0m 0s
+	    master_key_type = des3-hmac-sha1
+	    #supported_enctypes = aes256-cts:normal aes128-cts:normal
+	    default_principal_flags = +preauth
+    }
+EOF
+
+    cat > /etc/krb5.conf <<EOF
+[libdefaults]
+    default_realm = ${realm_name}
+    kdc_timesync = 1
+    ccache_type = 4
+    forwardable = true
+    proxiable = true
+    fcc-mit-ticketflags = true
+[realms]
+	${realm_name} = {
+		kdc = ${kerberos_server}
+		admin_server = ${kerberos_server}
+	}
+EOF
+    echo "# */admin *" > /etc/krb5kdc/kadm5.acl
+
+    # create the realm
+    kdb5_util create -s -P secretpassword
+
+    # restart services
+    systemctl restart krb5-kdc.service krb5-admin-server.service
+}
+
+create_krb_principal() {
+    local principal="$1"
+    local password="$2"
+
+    kadmin.local -q "addprinc -pw ${password} ${principal}"
+}
+