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

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 17 19:24:21 UTC 2014


On (13/06/14 14:30), Pavel Reichl wrote:
>On Fri, 2014-06-13 at 12:40 +0200, Lukas Slebodnik wrote:
>> 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
>
>OK, let it be compromise.
>
>> 
>> >
>> >> >
>> >> > 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.
>OK
>
>> 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
>>     };
>
>I think you are actually right about this.
>
>> You can also remove trailing CR_LF from key_data_CR_somewhere. I didn't notice
>Yeah, copy&paste mistake, thanks for noticing.
>
>> 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...
>OK
>> 
>> LS
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>New patch attached.
>
>Regards,
>PR

Patch works fine. (almost ACK :-)
I have just little nittpick which will improve readability of test.

>From 7bd5b1d8e1ab5bd38a28525de761bcaea594261c 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                     |  4 +-
> src/tests/cmocka/test_sss_ssh.c | 99 +++++++++++++++++++++++++++++++++++++++++
> src/tests/cmocka/test_utils.c   |  3 ++
> 3 files changed, 105 insertions(+), 1 deletion(-)
> create mode 100644 src/tests/cmocka/test_sss_ssh.c
>
>diff --git a/Makefile.am b/Makefile.am
>index 98c0d139f59922c886fc29ab1fdae6fcfa9a657d..b23ab8f883b77d9e8ce9c118aec49de23252fb65 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -1736,7 +1736,9 @@ test_ipa_idmap_LDADD = \
>     libsss_test_common.la
> 
> test_utils_SOURCES = \
>-    src/tests/cmocka/test_utils.c
>+    src/tests/cmocka/test_utils.c \
>+    src/tests/cmocka/test_sss_ssh.c
>+
> test_utils_CFLAGS = \
>     $(AM_CFLAGS)
> test_utils_LDADD = \
>diff --git a/src/tests/cmocka/test_sss_ssh.c b/src/tests/cmocka/test_sss_ssh.c
>new file mode 100644
>index 0000000000000000000000000000000000000000..9f45b7e388636cf7155c7749da82fe977ca58748
>--- /dev/null
>+++ b/src/tests/cmocka/test_sss_ssh.c
>@@ -0,0 +1,99 @@
>+/*
>+    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_noLF[] = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ developer at sssd.dev.work";
>+
>+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";
>+
>+uint8_t key_data_LFLF[] = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ developer at sssd.dev.work\n\n";
>+
>+uint8_t key_data_CRLF[] = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ developer at sssd.dev.work\r\n";
>+
>+uint8_t key_data_CR_somewhere[] = "ssh-rsa AA\rAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wf+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ developer at sssd.dev.work\n";
>+
>+void test_textual_public_key(void **state)
>+{
>+    TALLOC_CTX *mem_ctx;
>+    errno_t ret;
>+    char *res;
>+
>+    struct sss_ssh_pubkey pkey_null = {
>+        .data = key_data_noLF,
>+        .data_len = sizeof(key_data_noLF)
>+    };
>+
>+    struct sss_ssh_pubkey pkey = {
>+        .data = key_data_noLF,
>+        .data_len = sizeof(key_data_noLF) - 1 /* ignore trailling '\0' */
>+    };
>+
>+    struct sss_ssh_pubkey pkey_LF = {
>+        .data = key_data_LF,
>+        .data_len = sizeof(key_data_LF) - 1 /* ignore trailling '\0' */
>+    };
>+
>+    struct sss_ssh_pubkey pkey_LFLF = {
>+        .data = key_data_LFLF,
>+        .data_len = sizeof(key_data_LFLF) - 1 /* ignore trailling '\0' */
>+    };
>+
>+    struct sss_ssh_pubkey pkey_CRLF = {
>+        .data = key_data_CRLF,
>+        .data_len = sizeof(key_data_CRLF) - 1 /* ignore trailling '\0' */
>+    };
>+
>+    struct sss_ssh_pubkey pkey_CR_somewhere = {
>+        .data = key_data_CR_somewhere,
>+        .data_len = sizeof(key_data_CR_somewhere) - 1 /* ignore traill. '\0' */
>+    };
>+
>+    mem_ctx = talloc_new(NULL);
>+    assert_non_null(mem_ctx);
>+    check_leaks_push(mem_ctx);
>+
>+    ret = sss_ssh_format_pubkey(mem_ctx, &pkey, &res);
>+    assert_int_equal(ret, EOK);
>+    talloc_free(res);
>+
>+    ret = sss_ssh_format_pubkey(mem_ctx, &pkey_LF, &res);
>+    assert_int_equal(ret, EOK);
>+    talloc_free(res);
>+
>+    ret = sss_ssh_format_pubkey(mem_ctx, &pkey_LFLF, &res);
>+    assert_int_equal(ret, EINVAL);
>+
>+    ret = sss_ssh_format_pubkey(mem_ctx, &pkey_null, &res);
                                            ^^^^^^^^^
                From this name, it looks like ".data" is NULL.
                which is not true.
IMO, better name would be
                   a) pkey_null_terminated.
                   b) pkey_with_trailling_nul_character

Yes, proposed names are long. But in unit test, I prefer to have long
self-descriptive names. Unit test should be "additional documentation"
to tested function. Good unit test should tell you more than doxygen comments
or the source code itself.

Otherwise, test looks very good.
Thank you for increasing code coverage.

LS



More information about the sssd-devel mailing list