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

[Damien Miller]

Hi,

Could you please create a new bug for this? The context is pretty hard to
follow and I'd like something that captures it all so I can show it to
some people who know more about PKCS#11 than I do.

-d

On Fri, 26 Apr 2019, Jakub Jelen wrote:

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