Hello, I've written PKCS#11 support to OpenSSH to allow it to work with standard smartcards. I got some very positive responses from users, and I think it is ready to go into portage. It works with or without X.509 support, I use it to access gentoo resources... :) I've tried to touch as little up-stream as I could, so there should be no conflict with normal openssh operation. More details can be found at: http://alon.barlev.googlepages.com/openssh-pkcs11 I will be happy if you include this. Putting me a maintainer to every issue caused by this patch. Best Regards, Alon Bar-Lev.
Created attachment 100108 [details, diff] openssh-4.4_p1-r4.ebuild.diff
i'd like to cut down on openssh patches, not expand ... have you tried submitting this upstream ?
Yes. Damien Miller is very interested in this one, but he had no time (the last year) to review it properly. It is important patch since it allows OpenSSH to work with almost all smartcards, since almost all support PKCS#11 interface. It even make the use of OpenSC safer, by not remembering the PIN if card is removed, and prompt the user for card insert and removal (If you use OpenSC PKCS#11). The code used here is the same as I've merged into OpenVPN, Upcoming KDE-QCA, my recent gnupg-pkcs11-scd. X509 patch is also very important one, but OpenSSH community have no plans to integrate it... PKCS#11 and X509 enabled OpenSSH is very powerful! Please consider adding, I will handle all defects resulting this change.
Hello SpanKY, I see you re-added SecurID patches. SecurID is not really a smartcard, so adding this for smartcard USE is not really correct. There is a difference between the internal OpenSC support of OpenSSH, which can be called smartcard, although not generic as the PKCS#11 support I introduce, and a password generator such as SecurID. Please consider adding a different use flag for SecurID patch. Thanks!
i poked through your patch and looks like there are a few logic quirks in the configure script ssh_pkcs11_x509 is only defined, not used SSH_PKCS11_X509_DISABLED depends only on ssh_x509, not ssh_x509 and ssh_pkcs11
Thanks for looking into it. ssh_pkcs11_x509 is defined so you can test in other patches if PKCS#11 support exists or not. So it meant to be used by others. I had this problem when I tried to detect if x509 patch was applied. I asked Roumen to put such for his too. Regarding SSH_PKCS11_X509_DISABLED, it is used only if PKCS#11 is enabled... But I will modify my code to test for if PKCS#11 is also enabled. I am working on releasing pkcs11-helper as a standalone library as part of OpenSC http://www.opensc-project.org/pkcs11-helper/ I will release it soon, it will make this patch much smaller, also it will be used by OpenVPN, qca-pkcs11 and gnupg-pkcs11-scd and I am wokring with others. Regards,
ok, lemme know when you have the newer/smaller one done, and i'll merge that into our tree
Created attachment 105538 [details, diff] openssh-4.5_p1.ebuild.diff Hi! Here is the latest version. It is much more smaller, since it depends on pkcs11-helper. Regards,
it all looks fine except for the change to ssh-agent.c ... why are these hunks needed: @@ -1064,7 +1234,7 @@ main(int ac, char **av) s_flag++; break; case 'd': - if (d_flag) + if (d_flag > 3) usage(); d_flag++; break; @@ -1167,7 +1337,7 @@ main(int ac, char **av) * the socket data. The child continues as the authentication agent. */ if (d_flag) { - log_init(__progname, SYSLOG_LEVEL_DEBUG1, SYSLOG_FACILITY_AUTH, 1); + log_init(__progname, SYSLOG_LEVEL_DEBUG1+d_flag-1, SYSLOG_FACILITY_AUTH, 1); format = c_flag ? "setenv %s %s;\n" : "%s=%s; export %s;\n"; printf(format, SSH_AUTHSOCKET_ENV_NAME, socket_name, SSH_AUTHSOCKET_ENV_NAME);
(In reply to comment #9) > it all looks fine except for the change to ssh-agent.c ... why are > these hunks needed: Thank you again for reviewing! This is needed because there is no way to turn on debugging level in current implementation of ssh-agent... (Specifying -d -d -d) Because I am adding a new functionality I need a better logging... This was taken from ssh it-self and do not break current implementation.
vapier, please either move the SecurID patch to USE=securid or similar (NOT USE=smartcard) or just remove the SecurID patch from our openssh ebuilds. I am currently unable to use OpenSC support with the latest openssh ebuilds. Thanks!
got an updated version for 4.7_p1 ?
Created attachment 132152 [details, diff] openssh-4.7_p1-r1.ebuild.diff Sure! But I having trouble with keywording the pkcs11-helper at bug#160285. I think you can complete the missing keywords... :) Next, I need to start stabilization cycle... But nobody can estimate how long these cycles takes at Gentoo dimention.
Created attachment 132218 [details, diff] openssh-4.7_p1-r2.ebuild.diff
Created attachment 141317 [details, diff] openssh-4.7_p1-r3.ebuild.diff
Created attachment 146787 [details, diff] openssh-4.7_p1-r20.ebuild.diff
Created attachment 148108 [details, diff] openssh-4.9_p1.ebuild.diff
Comment on attachment 148108 [details, diff] openssh-4.9_p1.ebuild.diff this bug is about pkcs11 only ... keep x509 and anything else out of it
also, the pks pkg needs to go stable before i'll add it to openssh depend
Created attachment 148710 [details, diff] openssh-5.0_p1.ebuild.diff Hi! Someone added pkcs11-helper dependency for openssh in portage. This should be removed as long as USE flag is not added. Thanks!
hrm, it got added by accident while testing
Alon: want to update this for openssh-5.1 assuming that your upstream patches are still good?
Created attachment 174528 [details, diff] openssh-5.1_p1-r2.ebuild.diff Sure.
the only issue i see is the EPATCH_OPTS ... why do you need to force -p1 ?
-p1 is forced because the patch adds some new files, if I don't use this, it will create the first path component for the files located in the root. Is there any other solution?
no that's fine ... next time i get around to a revbump, i'll add the patchset (or whoever revbumps openssh next time) i'm assuming you've opened standard bugs upstream to try and get it merged
Yes, I opened a bug for this [1]. As for any new feature that does not come from core developers, no exiting news. Upstream wish to add a feature for multi-agent into openssh, but without modifying the protocol to be more generic. This has two main issues: 1. Users that do not wish to use the agent or cannot (console mode) will not be able to use the new features. 2. The current protocol forces the tools to understand each agent command, so new commands and features cannot be used. The OpenSC project will soon drop the support for libopensc which is used only by this application. All other known applications moved to PKCS#11. So I guess I will maintain this as external patch for a while. Thanks! [1] https://bugzilla.mindrot.org/show_bug.cgi?id=1371
added to openssh-5.2_p1, but disabled since your patchset doesnt apply :)
Created attachment 183040 [details, diff] openssh-5.2_p1.ebuild.diff Thank you!! I rebased.
why not build the pkcs files conditionally in the Makefile ? then you wouldnt need to wrap the files in '#ifdef ENABLE_PKCS11' ... some functions (like _pkcs11_ssh_pin_prompt) calls strlen() on the same unchanged string. gcc will not optimize that away, so you should cache it yourself: size_t phraselen; ... phraselen = strlen(passphrase); free() will gladly accept NULL pointers, so there is no need for constructs like these: if (foo != NULL) free(foo);
Thank you for your feedback. (In reply to comment #30) > why not build the pkcs files conditionally in the Makefile ? then you wouldnt > need to wrap the files in '#ifdef ENABLE_PKCS11' ... As no other component is conditioned in the Makefile, I thought it is a bad idea to break the convension used. I've duplicated the SMARTCARD usage. > some functions (like _pkcs11_ssh_pin_prompt) calls strlen() on the same > unchanged string. gcc will not optimize that away, so you should cache it > yourself: > size_t phraselen; > ... > phraselen = strlen(passphrase); This saldom happens... So optimization was not taken into account. I will modify this next time I touch the code. > free() will gladly accept NULL pointers, so there is no need for constructs > like these: > if (foo != NULL) > free(foo); As far as I know it is not the case in every compiler. Also upstream is using xfree wrapper, and if you look at xmalloc.c you see the following: void xfree(void *ptr) { if (ptr == NULL) fatal("xfree: NULL pointer given as argument"); free(ptr); } Thanks!
free(NULL) is a C library issue, not compiler. POSIX requires free(NULL) to work. even the gnulib guys (who are portable to every friggin platform) requires free(NULL) to work and it hasnt been an issue for them yet. that said, i can see why the openssh guys would mark free(NULL) as a bug as they see it as a potential indicator that other pieces of code are broken somehow.
