Merge ~ahasenack/ubuntu/+source/libnfsidmap:bionic-uid-map-krb5-1819197 into ubuntu/+source/libnfsidmap:ubuntu/devel

[Andreas Hasenack]

This fixes a very old bug, reported multiple times in both debian[1] and ubuntu[2].

Testing it might be a bit complicated, I just so happened to have a kerberos + nfsv4 setup at home already, I'll think of an easy way to set something up when the SRU time comes.

There have been two versions of the fix floating around across all these bugs. Compounded with that, there doesn't seem to be an official upstream anymore.

Patch 1[3], which is what I used here, fixes nss.c:strip_domain() to look for the last "@" sign in the username, and strip that off, instead of the first. This is what fixes the case when a username already has an "@" sign, which is the default in sssd. A package from my PPA[4] with this patch was tested by the bug reporter, and confirmed working.

The second patch[5] has that hunk, plus another one to fix nss.c:nss_gss_princ_to_ids() in a similar manner:

- princ_realm = strstr(princ, "@");
+ princ_realm = strrchr(princ, '@');

It's not clear to me when that function is used. I would have to understand that to check what kind of (incorrect) values could be given to the "princ" variable. If it's really just given a principal (which is of the form user@REALM), then both strstr() and strrchr() would yield the same result. One could argue that it wouldn't hurt to always return the last "@" occurrance (and not the first), though.

The most up-do-date release I found for libnfsidmap2 is 0.27[6], and that seems to be a self-appointed version done by Redhat. Citi, which is the site mentioned in d/control, still has only 0.25[7]

0.27 does NOT contain the second fix, just the first.

I emailed Steve D. (from that fedorapeople site 0.27 release) asking who is upstream now, he said he doesn't think there is one.

I would need a lot more investigation to understand when the second patch is needed. So given:
- 0.27 does not contain the second fix
- I found no other reference to the second fix "in the wild", just in these bug reports, and no follow-up confirmation about what is fixed (I specifically asked in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=581199#25)
- the first patch fixes the reported problem scenario
- beta freeze is looming

I'm inclined to upload just with the first patch hunk, and later update it if needed.

1. http://bugs.debian.org/744768, http://bugs.debian.org/581199, http://bugs.debian.org/924425
2. https://bugs.launchpad.net/bugs/1819197, https://bugs.launchpad.net/bugs/1728310, https://bugs.launchpad.net/bugs/584942
3. https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=581199;filename=libnfsidmap_0.20-1_fix_at_sign_user_with_domain.diff;msg=5
4. https://launchpad.net/~ahasenack/+archive/ubuntu/libnfsidmap-krb5-1819197
5. https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=581199;filename=libnfsidmap_0.25-5ubuntu1.debdiff;msg=15
6. https://fedorapeople.org/~steved/libnfsidmap/
7. http://www.citi.umich.edu/projects/nfsv4/linux/

[Andreas Hasenack (ahasenack)]

I did a codesearch in debian (https://codesearch.debian.net/search?q=nfs4_gss_princ_to_ids) for nfs4_gss_princ_to_ids, and the only non-libnfsidmap package that calls it is nfs-utils. Doesn't tell me yet if that function always uses a well formatted principal name.

[Christian Ehrhardt  (paelzer)]

Thank you for the explanation why you picked hunk#1 and not hunk#2 or hunk#1+#2 - that really helped especially given the multitude of bugs involved.
I agree to changing what we know the purpose for any only if needed amend later with the second change.

Also the patch and its integration look good to me.

In the name of the users, thanks for sorting out this old pain point!
+1

The only thing for a coming upload.
This is the bionic branch - the change as proposed (as you know) will need to go to Disco first. Due to Bionic and Cosmic being the same version more expect:
libnfsidmap (0.25-5.1ubuntu0.18.04.1) bionic; urgency=medium
libnfsidmap (0.25-5.1ubuntu0.18.10.1) cosmic; urgency=medium
libnfsidmap (0.25-5.1ubuntu1) disco; urgency=medium

The currently proposed 0.25-5.1ubuntu0.1 will not work that well since you'll have a conflict in Cosmic.

review: Approve

[Andreas Hasenack (ahasenack)]

Oh, good catch on the version number, I forgot I was building for bionic to test at home.

Unmerged commits

b5e53e7... by Andreas Hasenack on 2019-03-15

changelog

a8be1c2... by Andreas Hasenack on 2019-03-15

  * d/p/03-uid-map-krb5.patch: fix uid mapping when sec=krb5 is used
    (LP: #1819197)

diff --git a/debian/changelog b/debian/changelog
index 07f91f1..2230294 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+libnfsidmap (0.25-5.1ubuntu0.1) bionic; urgency=medium
+
+  * d/p/03-uid-map-krb5.patch: fix uid mapping when sec=krb5 is used
+    (LP: #1819197)
+
+ -- Andreas Hasenack <[e-mail]>  Fri, 15 Mar 2019 11:16:24 -0300
+
 libnfsidmap (0.25-5.1) unstable; urgency=medium
 
   * Non-maintainer upload.
diff --git a/debian/patches/03-uid-map-krb5.patch b/debian/patches/03-uid-map-krb5.patch
new file mode 100644
index 0000000..2b07685
--- /dev/null
+++ b/debian/patches/03-uid-map-krb5.patch
@@ -0,0 +1,30 @@
+From 309a89975a50bf53c408233a1bb5b10fd579ca30 Mon Sep 17 00:00:00 2001
+From: "Signed-off-by: Shijoe Panjikkaran" <[e-mail]>
+Date: Wed, 30 Apr 2014 13:19:34 -0400
+Subject: [PATCH] nss: use strrchr() instead of strchr() to get the last
+ occurrence of "@"
+
+Signed-off-by: Shijoe Panjikkaran <[e-mail]>
+Signed-off-by: Steve Dickson <[e-mail]>
+
+Origin: https://github.com/Distrotech/libnfsidmap/commit/309a89975
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=924425
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libnfsidmap/+bug/1819197
+Last-Update: 2019-03-15
+---
+ nss.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/nss.c b/nss.c
+index b2b1227..f8129fe 100644
+--- a/nss.c
++++ b/nss.c
+@@ -135,7 +135,7 @@ static char *strip_domain(const char *name, const char *domain)
+ 	char *l = NULL;
+ 	int len;
+ 
+-	c = strchr(name, '@');
++	c = strrchr(name, '@');
+ 	if (c == NULL && domain != NULL)
+ 		goto out;
+ 	if (c == NULL && domain == NULL) {
diff --git a/debian/patches/series b/debian/patches/series
index 452e741..6a5cb27 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
 01-661215-wrong-double-ldap-check.patch
 02-idmapd.conf.5.patch
+03-uid-map-krb5.patch