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

[Jan Cholasta]

Hi,

the attached patch fixes <https://fedorahosted.org/sssd/ticket/2349>;.

Honza

-- 
Jan Cholasta

[Pavel Reichl]

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. :-(

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?

Thanks,

PR

PS: Also unit test would be nice, but I can do it myself later.

[Jan Cholasta]

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.

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.

>  Thanks,
>
>  PR
>
>  PS: Also unit test would be nice, but I can do it myself later. 
-- 
Jan Cholasta

[Pavel Reichl]

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

[Lukas Slebodnik]

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&gt;
> 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

>  
>  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&gt;
> +
> +    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&quot;;
> + 
It would be good to test ssh key without new line at the end.

LS

[Pavel Reichl]

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&gt;
>  >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. 

>  > 
>  > 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&gt;
>  >+
>  >+    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&quot;;
>  >+
>  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? 

Regards, 

PR

>  
>  LS
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

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&gt;
> > >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(a)redhat.com&gt;
> > >+
> > >+    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&quot;;
> > >+
> > 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

[Pavel Reichl]

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&gt;
>  >> >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&gt;
>  >> >+
>  >> >+    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&quot;;
>  >> >+
>  >> 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

[Jakub Hrozek]

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&gt;
>  > >> >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&gt;
>  > >> >+
>  > >> >+    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&quot;;
>  > >> >+
>  > >> 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?

[Pavel Reichl]

On Tue, 2014-06-17 at 17:33 +0200, Jakub Hrozek wrote:
>  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&gt;
>  > > >> >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&gt;
>  > > >> >+
>  > > >> >+    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&quot;;
>  > > >> >+
>  > > >> 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 
Yes, I ACKed Honza's patch.

[Lukas Slebodnik]

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(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&gt;
> > >> >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&gt;
> > >> >+
> > >> >+    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&quot;;
> > >> >+
> > >> 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 
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(a)redhat.com&gt;
> 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(a)redhat.com&gt;
> +
> +    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(a)sssd.dev.work&quot;;
> +
> +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&quot;;
> +
> +uint8_t key_data_LFLF[] = "ssh-rsa
> AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ
> developer(a)sssd.dev.work\n\n&quot;;
> +
> +uint8_t key_data_CRLF[] = "ssh-rsa
> AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ
> developer(a)sssd.dev.work\r\n&quot;;
> +
> +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(a)sssd.dev.work\n&quot;;
> +
> +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

[Pavel Reichl]

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(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&gt;
>  >> >> >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&gt;
>  >> >> >+
>  >> >> >+    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&quot;;
>  >> >> >+
>  >> >> 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
>  
>  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(a)redhat.com&gt;
>  >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(a)redhat.com&gt;
>  >+
>  >+    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(a)sssd.dev.work&quot;;
>  >+
>  >+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&quot;;
>  >+
>  >+uint8_t key_data_LFLF[] = "ssh-rsa
> AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ
> developer(a)sssd.dev.work\n\n&quot;;
>  >+
>  >+uint8_t key_data_CRLF[] = "ssh-rsa
> AAAAB3NzaC1yc2EAAAADAQABAAABAQDfymad64oZkWa6q3xLXmCt/LfCRnd6yZSDp7UK6Irx5/Dv69dEKK2kBGL9Wfn+3ZDa6ov2XZrBmUthh8KOJvTw72+axox3kcJ5HwOYZCMeKbcr10RNScGuHErA1HhjTY6M9L8d0atVH2QIxw7ZHoVVnTHC4U4+541YfJkNUiOUIj65cFFZm9ULp32ZPrK+j2wW+XZkHhrZeFMlg4x4fe5FocO6ik1eqLxBejo7tMy+1m3R2a795AIguf6vNWeE5aNMd4pcmPcZHb3JOq3ItzE/3lepXD/3wqMt36EqNykBVE7aJj+LVkcEgjP9CDDsg9j9NB+AuWYmIYqrHW/Rg/vJ
> developer(a)sssd.dev.work\r\n&quot;;
>  >+
>  >+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(a)sssd.dev.work\n&quot;;
>  >+
>  >+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(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (18/06/14 10:33), Pavel Reichl wrote:
> 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(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&gt;
> > >> >> >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&gt;
> > >> >> >+
> > >> >> >+    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&quot;;
> > >> >> >+
> > >> >> 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
> > 
> > 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(a)redhat.com&gt;
> > >Date: Thu, 12 Jun 2014 10:06:45 +0100
> > >Subject: [PATCH] TESTS: sss_ssh - textual public key format
> > >
> > >--- 
Thank you.

ACK

LS

[Jakub Hrozek]

On Wed, Jun 18, 2014 at 10:45:48AM +0200, Lukas Slebodnik wrote:
>  ACK
>  
>  LS 
master: b5f61c9b3f5ea79bf319c18ff59394070c04d607
sssd-1-11: 7e277b03e77d8e44e6da17d1ccd50eaf8f85f4ca

[Jakub Hrozek]

On Thu, Jun 12, 2014 at 05:48:57PM +0200, Pavel Reichl wrote:
>  I think this patch fixes the ticket and doesn't break anything. 
>  ACK 
Pushed upstream:
    master: 26510727739c3e8d14f804568e496b7d7b073964 
    sssd-1-11: f5e1bb7c2c8c9c3861f81e2ca40e28b52db87e18