On Fri, Jun 13, 2014 at 02:30:28PM +0200, 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(a)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(a)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(a)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(a)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(a)lists.fedorahosted.org
> >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> New patch attached.
>
> Regards,
> PR
Is anyone reviewing the unit test? Shall I take a look? Do you consider
Honza's patch acked?
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel