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

Pavel Reichl preichl at redhat.com
Wed Jun 18 08:33:31 UTC 2014


On Tue, 2014-06-17 at 21:24 +0200, Lukas Slebodnik wrote:
> 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.

OK, makes sense. Updated patch attached.

Thanks,
PR

> 
> Otherwise, test looks very good.
> Thank you for increasing code coverage.
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TESTS-sss_ssh-textual-public-key-format.patch
Type: text/x-patch
Size: 7040 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/a14b2738/attachment.bin>


More information about the sssd-devel mailing list