[SSSD] [PATCH] SSH: Allow newline at the end of public key values in LDAP

Lukas Slebodnik lslebodn at redhat.com
Fri Jun 13 10:40:14 UTC 2014


On (13/06/14 11:26), Pavel Reichl wrote:
>On Thu, 2014-06-12 at 18:51 +0200, Lukas Slebodnik wrote:
>> On (12/06/14 17:48), Pavel Reichl wrote:
>> >On Wed, 2014-06-11 at 16:57 +0200, Jan Cholasta wrote:
>> >> On 5.6.2014 16:34, Pavel Reichl wrote:
>> >> > On Wed, 2014-06-04 at 17:05 +0200, Jan Cholasta wrote:
>> >> >> Hi,
>> >> >>
>> >> >> the attached patch fixes <https://fedorahosted.org/sssd/ticket/2349>.
>> >> >>
>> >> >> Honza
>> >> >>
>> >> >> _______________________________________________
>> >> >> sssd-devel mailing list
>> >> >> sssd-devel at lists.fedorahosted.org
>> >> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> >> >
>> >> > Hello Honza,
>> >> >
>> >> > I'm little confused about this ticket - reporter complained that it's
>> >> > not allowed to have newlines *anywhere* in the public key string, not
>> >> > even at the end. But you added support for newlines only at the end of
>> >> > the file. Does is it make any sense to have newlines at the public key
>> >> > at all? I was not able to quickly find relevant RFC. :-(
>> >>
>> >> My mistake, I was the one who suggested the title of the ticket to the
>> >> reporter. Newline should indeed not be allowed anywhere in the string
>> >> except at the end.
>> >
>> >OK then, I was thinking of updating trac ticket description but I then
>> >noticed it's linked to bugzilla so I'll I just leave it as it is now.
>> >
>> >>
>> >> I'm afraid there is no RFC, I used OpenSSH source code as reference.
>> >>
>> >> >
>> >> > I also noticed that key is denied if it contains '\r'. From top of my
>> >> > head I would assume that if public key was generated on Windows or Mac
>> >> > that the sequence <CR><LF> or <CR> would be used to mark eol - so we
>> >> > should tolerate them to. Should not we?
>> >>
>> >> Maybe, I don't know. OpenSSH uses only '\n' as line separator in
>> >> authorized_keys.
>> >
>> >I think this patch fixes the ticket and doesn't break anything.
>> >ACK
>> >
>> >>
>> >> >
>> >> > Thanks,
>> >> >
>> >> > PR
>> >> >
>> >> > PS: Also unit test would be nice, but I can do it myself later.
>> >Could you please have a look at attached unit test?
>> >
>> >If you like it, you are very welcome to extend it.
>> >
>> >Thanks,
>> >
>> >PR
>>
>> >From 500d96c5e2d29ac42e329a3e92c4cd954ef850b9 Mon Sep 17 00:00:00 2001
>> >From: Pavel Reichl <preichl at redhat.com>
>> >Date: Thu, 12 Jun 2014 10:06:45 +0100
>> >Subject: [PATCH] TESTS: sss_ssh - textual public key format
>> >
>> >---
>> > Makefile.am                     |  14 +++++-
>> > src/tests/cmocka/test_sss_ssh.c | 108 ++++++++++++++++++++++++++++++++++++++++
>> > 2 files changed, 121 insertions(+), 1 deletion(-)
>> > create mode 100644 src/tests/cmocka/test_sss_ssh.c
>> >
>> >diff --git a/Makefile.am b/Makefile.am
>> >index 98c0d139f59922c886fc29ab1fdae6fcfa9a657d..b9e9113b788b1031bf024cd73242f72ab71d37fe 100644
>> >--- a/Makefile.am
>> >+++ b/Makefile.am
>> >@@ -188,7 +188,8 @@ if HAVE_CMOCKA
>> >         responder-get-domains-tests \
>> >         sbus-internal-tests \
>> >         sss_sifp-tests \
>> >-        test_search_bases
>> >+        test_search_bases \
>> >+        test_sss_ssh
>>
>> The function sss_ssh_format_pubkey is part of libsss_util.so.
>> I would prefer to extend test test_utils.
>>
>> sh-4.2$ nm --defined-only .libs/libsss_util.so | grep sss_ssh_format_pubkey
>> 0000000000037d30 T sss_ssh_format_pubkey
>>
>
>I guess I could move it there, but what's the benefit?  Will we gain
>some significant performance benefit? I don't think so.
>
>It may be nice to have all util functionality tested at one place, but
>on the other hand the test_utils.c is already one of the biggest files
>among tests and sooner or later might get splitted anyway.
>
>I think that ssh code deserves its own unit test and its testing
>coverage should be expanded in the future.
We have 6 times bigger test
sh-4.2$ ls -lSn ./src/tests/cmocka/*.c ./src/tests/*.c | head -n 7
-rw-rw-r--. 1 1000 1000 184071 Jun  6 16:54 ./src/tests/sysdb-tests.c
-rw-rw-r--. 1 1000 1000  63895 Jun  6 16:54 ./src/tests/cmocka/test_sss_sifp.c
-rw-rw-r--. 1 1000 1000  55306 Jun  6 16:54 ./src/tests/cmocka/test_nss_srv.c
-rw-rw-r--. 1 1000 1000  52625 Jun  6 16:54 ./src/tests/sbus_codegen_tests.c
-rw-rw-r--. 1 1000 1000  51592 Jun  9 13:23 ./src/tests/sbus_codegen_tests_generated.c
-rw-rw-r--. 1 1000 1000  33509 Jun  6 16:54 ./src/tests/util-tests.c
-rw-rw-r--. 1 1000 1000  30918 Jun  6 16:54 ./src/tests/cmocka/test_utils.c

and as you can see libsss_util is already tested by two different test
 (check-based and cmocka-based)

If you prefer to have test in separate module a compromise can be
to build test_sss_ssh.c as part of test_utils

>
>> >
>> > if BUILD_IFP
>> > non_interactive_cmocka_based_tests += ifp_tests
>> >@@ -1667,6 +1668,17 @@ sss_nss_idmap_tests_LDADD = \
>> >     $(CMOCKA_LIBS) \
>> >     libsss_nss_idmap.la
>> >
>> >+test_sss_ssh_SOURCES = \
>> >+    src/tests/cmocka/test_sss_ssh.c
>> >+test_sss_ssh_CFLAGS = \
>> >+    $(AM_CFLAGS)
>> >+test_sss_ssh_LDADD = \
>> >+    $(CMOCKA_LIBS) \
>> >+    libsss_util.la \
>> >+    libsss_crypt.la \
>> >+    libsss_debug.la \
>> >+    libsss_test_common.la
>> >+
>> > EXTRA_dyndns_tests_DEPENDENCIES = \
>> >      $(ldblib_LTLIBRARIES)
>> > dyndns_tests_SOURCES = \
>> >diff --git a/src/tests/cmocka/test_sss_ssh.c b/src/tests/cmocka/test_sss_ssh.c
>> >new file mode 100644
>> >index 0000000000000000000000000000000000000000..11e7914f26362c611e65d4617b398dc3a2871be0
>> >--- /dev/null
>> >+++ b/src/tests/cmocka/test_sss_ssh.c
>> >@@ -0,0 +1,108 @@
>> >+/*
>> >+    Authors:
>> >+        Pavel Reichl  <preichl at redhat.com>
>> >+
>> >+    Copyright (C) 2014 Red Hat
>> >+
>> >+    Test for the NSS Responder ID-SID mapping interface
>> >+
>> >+    This program is free software; you can redistribute it and/or modify
>> >+    it under the terms of the GNU General Public License as published by
>> >+    the Free Software Foundation; either version 3 of the License, or
>> >+    (at your option) any later version.
>> >+
>> >+    This program is distributed in the hope that it will be useful,
>> >+    but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >+    GNU General Public License for more details.
>> >+
>> >+    You should have received a copy of the GNU General Public License
>> >+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> >+*/
>> >+
>> >+#include "util/util.h"
>> >+#include "util/sss_ssh.h"
>> >+#include "tests/cmocka/common_mock.h"
>> >+
>> >+uint8_t key_data_LF[] = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ developer at sssd.dev.work\n";
>> >+
>> It would be good to test ssh key without new line at the end.
>
>Yes, and I hope I do so, please see pkey variable and especially how the
>length is computed.
>
>>     /* ignore trailling '\n' '\0' */
>>     pkey.data_len = sizeof(key_data_LF) - 2;
>
>Is it OK, Lukas? Or would you prefer if I added new key with omitted '\n' in its data?
>
It is good, but it was not clear from test. I think with different data it
would be more readable.

You can also use different kind of structure initialisation.
It can improve readability of test as well.
    struct sss_ssh_pubkey pkey = {
        .data = key_data_LF,
        .data_len = sizeof(key_data_LF) - 2
    };

You can also remove trailing CR_LF from key_data_CR_somewhere. I didn't notice
CR in the middle of text in first time and thought it is the same string.
Maybe it would be better to have CR nearer to the begginning.
   e.g. "ssh-rsa AA\rAAB3Nz...

LS



More information about the sssd-devel mailing list