Regression regarding the PIN prompts for PKCS#11 (Was: Call for testing: OpenSSH 8.0)

[Jakub Jelen]

On Wed, 2019-04-24 at 14:09 +0200, Jakub Jelen wrote:
> On Sat, 2019-04-06 at 03:20 +1100, Damien Miller wrote:
> > On Fri, 5 Apr 2019, Jakub Jelen wrote:
> > 
> > > There is also changed semantics of the ssh-keygen when listing
> > > keys
> > > from PKCS#11 modules. In the past, it was not needed to enter a
> > > PIN
> > > for
> > > this, but now.
> > > 
> > > At least, it is not consistent with a comment in the function
> > > pkcs11_open_session(), which says
> > > 
> > >  727  * if pin == NULL we delay login until key use
> > > 
> > > Being logged in before listing keys prevents bug #2430, but as a
> > > side
> > > effect, even the ssh can not list keys before login and if the
> > > configuration contains a PKCS#11 module, the user is always
> > > prompted
> > > for a PIN, which is not very user friendly.
> > > 
> > > I see this is a regression and the bug #2430 should get solved as
> > > proposed in the patches (will need some tweaks after the big
> > > refactoring).
> > 
> > We'll take a look at this (and the other things you just reported)
> > after the release is done.
> 
> Release is out with this regression. Is there any progress on this?
> The
> simplest thing how to reproduce is by extending the agent-pkcs11
> regress testsuite with the following line, which previously worked
> fine, but now asks for a pin:
> 
> ${SSHKEYGEN} -D ${TEST_SSH_PKCS11}
> 
> Is this on a radar or should I create a new bug? I am using keys from
> PKCS#11 all the time and this prevents me from updating to the newer
> version.

Hello there,
digging a bit in the git history, it looks like the regression was
introduced by the commit 7a7fdca [1] authored by markus@, which is
trying to fix a crash introduced by 41923ce [2]. That looks like also
my fault that I preliminary approved this change probably without
proper testing. Certainly the [2] is wrong -- there needs to be a way
to process session_open function without calling to the C_Login and
CKF_LOGIN_REQUIRED should not stay in the way (see the comments in the
bug #2652).

Actually I think both of the commits should get reverted since they are
not addressing any problem, but just breaking the default use cases.
The underlying problem of the bug #2652 is bug #2430 (still not
addressed even though several patches were proposed).

The attached patch is basically the revert that I am going to carry
downstream to have the PKCS#11 working and I recommend to fix this also
in openssh upstream before other people will start using this and
complaining. I would be also happy to help with solving the underlying
problem since there are indeed other users interested in that per the
bug reports.

[1] https://github.com/openssh/openssh-portable/commit/7a7fdca
[2] https://github.com/openssh/openssh-portable/commit/41923ce
[3] 
http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html

Regards,
-- 
Jakub Jelen
Senior Software Engineer
Security Technologies
Red Hat, Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openssh_revert.patch
Type: text/x-patch
Size: 2136 bytes
Desc: not available
URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20190426/dfdb1336/attachment-0001.bin>