[Feature] KeyCollector not prompted with TouchRequest for PIV operation

[canton7]

Is there an existing issue for this?

• [x] I have searched the existing issues

Current Behavior

When using PIV, for example PivSession.Sign, the KeyCollector will never be called with TouchRequest, even when the operation requires a touch.

PivSession.PerformPrivateKeyOperation knows the touch policy, so it can work out whether a touch will be requested. However, it just doesn't contain any logic to prompt the user to touch the YubiKey.

Expected Behavior

Operations on PivSession which request a touch will call the KeyCollector with a TouchRequest`.

Steps To Reproduce

1. Set a PivTouchPolicy of Once or Always
2. Perform an operation which requires a touch
3. Observe that the KeyCollector is not called with a request of TouchRequest

Version

1.13.0

Version

N/A

Anything else?

No response

[DennisDyallo]

Hi @canton7 This is because TouchRequest is something that only FIDO supports, as it is part of the FIDO specification. This means the YubiKey firmware will notify this state to the SDK. With other YubiKey applications, it's not a part of the spec, so the SDK would have to manually track, guess and account for quite a few edge cases, (e.g. cached touch policy). So since it requires a non trivial amount of work, coupled with the fact that it doesn't provide guartantees in the behaviour, it hasn't been implemented.

The way our own apps handle this is that we wrap our requests with a small timeout timer that if unhandled, the app assumes that the YubiKey needs a touch and notifies the user.

I'm sorry this may not have been the response you're looking for.

[DennisDyallo]

What I will is to add this to our internal issue tracker.

[canton7]

I see, thanks. It would be good to update your documentation, as it makes no mention of this:

• https://docs.yubico.com/yesdk/users-manual/sdk-programming-guide/key-collector.html
• https://docs.yubico.com/yesdk/users-manual/sdk-programming-guide/key-collector-touch.html

Taking the approach you suggest, where you wrap the request in a timeout handler, is tricky, because PivSession.Sign may call the KeyCollector to prompt for a PIN, which will make the call to PivSession.Sign take longer than expected. To get around this, I had to copy/paste the implementation of Sign, and add my own logic around the call to Connection.SendCommand(signCommand);.

You already have logic to work out whether PIN input is required, and whether a PIN has already been supplied: I don't suppose the logic for Touch will be much different?

One complexity is that it doesn't look like a sign command can be cancelled, so you wouldn't be able to provide a SignalUserCancel, but that's probably OK, so long as it's a documented limitation.

[DennisDyallo]

fyi @equijano21 , we should incorporate this into the upcoming docs changes

[canton7]

For what it's worth, my extension method is:

/// <summary>
/// Contains extension methods on <see cref="PivSession"/>
/// </summary>
public static class PivSessionExtensions
{
    /// <summary>
    /// <see cref="PivSession.Sign(byte, ReadOnlyMemory{byte})"/> doesn't have any logic to prompt for touch, see https://github.com/Yubico/Yubico.NET.SDK/issues/229.
    /// This method adds that capability.
    /// </summary>
    public static byte[] SignPromptForTouch(this PivSession session, byte slotNumber, ReadOnlyMemory<byte> dataToSign)
    {
        var slotMetadata = session.GetMetadata(slotNumber);
        bool pinRequired = true;

        // If the metadata says Never, then pinRequired is false.
        // If the metadata says Once, and the PIN is verified, then the
        // PIN is not required.
        // The only other case is Always which means we set the
        // pinRequired to true, but we init that variable to true.
        if (slotMetadata.PinPolicy == PivPinPolicy.Never ||
            (slotMetadata.PinPolicy == PivPinPolicy.Once && session.PinVerified))
        {
            pinRequired = false;
        }

        if (pinRequired)
        {
            // This is the verify method that will throw an exception if the
            // user cancels.
            session.VerifyPin();
        }

        bool promptForTouch = slotMetadata.TouchPolicy != PivTouchPolicy.Never;
        if (promptForTouch)
        {
            session.KeyCollector?.Invoke(new KeyEntryData() { Request = KeyEntryRequest.TouchRequest });
        }

        var signCommand = new AuthenticateSignCommand(dataToSign, slotNumber, slotMetadata.Algorithm);
        var commandToPerformResponse = session.Connection.SendCommand(signCommand);

        if (promptForTouch)
        {
            session.KeyCollector?.Invoke(new KeyEntryData() { Request = KeyEntryRequest.Release });
        }

        if (commandToPerformResponse.Status != ResponseStatus.AuthenticationRequired)
        {
            return commandToPerformResponse.GetData();
        }

        // If we reach this code, the Status is AuthRequired and the problem is touch.
        throw new OperationCanceledException("The response APDU indicates further information or action, such as PIN, management key, or touch, is needed to complete the command.");
    }
}

This isn't perfect: it might show a touch prompt and then immediately hide it. That could be improved by doing what you already do for PIN an tracking whether it's been input for the current session (which I can't do from an extension method). However, I have YubiKeys configured to either always require a touch every time, or never, so this doesn't matter.

[DennisDyallo]

Thanks. Since we have no telemetry, valuable to see some code in the wild.