NoMoreFood / putty-cac

Windows Secure Shell Client With Support For Smart Cards, Certificates, & FIDO Keys
474 stars 43 forks source link

Request: Add FIDO2 Display Name Textbox #117

Closed Aetherinox closed 9 months ago

Aetherinox commented 10 months ago

Is their any way we can add a new field to the FIDO Tools section which allows us to pick our own Display Name for the fido key? Currently, if you import to a Yubikey, the display name is PuTTY FIDO User which is coming from code\cert\cert_fido.c

#define FIDO_KEY_USERNAME L"PuTTY FIDO User"

I'm referring to the settings under Connection -> SSH -> Certificate -> FIDO Tools

image

And then just updating code\cert\cert_fido.c

tUserInfo.pwszDisplayName = FIDO_KEY_USERNAME;
tUserInfo.pwszName = FIDO_KEY_USERNAME;
tUserInfo.cbId = wcslen(FIDO_KEY_USERNAME) * 2;
tUserInfo.pbId = (PBYTE)FIDO_KEY_USERNAME;

Seems small, but it would be a damn Christmas gift to get this.

NoMoreFood commented 10 months ago

I will take a look this weekend.

Aetherinox commented 10 months ago

Thanks. I can develop in numerous languages, but my skills in C are very amateur.

I went through the code, added the new textfield and implemented the change on my own test build, and thus far, it is working great. So I'll throw what I did here if you want to use any. But I'd look over the code, as I said, I'm not advanced in this as I am in C#, Lua, etc. I also haven't taken a look at the FIDO Import functionality to see if there's anything that can be done there.

image

The code works great on build, but the only action I've messed with is code\cert\cert_fido.c -> fido_create_key


#define FIDO_MAX_DISPLAY_LEN 32

BOOL fido_create_key(LPCSTR szAlgName, LPCTSTR szDisplayName, LPCSTR szApplication, BOOL bResidentKey, BOOL bUserVerification)
{
    WCHAR szDisplayNameUnicode[FIDO_MAX_DISPLAY_LEN];

    if (MultiByteToWideChar(CP_UTF8, 0, szDisplayName, -1, szDisplayNameUnicode, _countof(szDisplayNameUnicode)) == 0) return FALSE;

    [...]

    WEBAUTHN_USER_ENTITY_INFORMATION tUserInfo = { WEBAUTHN_USER_ENTITY_INFORMATION_CURRENT_VERSION };
    tUserInfo.pwszDisplayName = szDisplayNameUnicode;
    tUserInfo.pwszName = szDisplayNameUnicode;
    tUserInfo.cbId = lstrlen(szDisplayName) * 2;
    tUserInfo.pbId = (PBYTE)szDisplayName;
    tUserInfo.pwszIcon = NULL;
}

I haven't looked at the docs to see what the max char is for FIDO_MAX_DISPLAY_LEN, so I just set it to 32. Most usages are just username or email address of the account.

Then at the top of cert_fido.c, the original declaration can be removed:

#define FIDO_KEY_USERNAME L"PuTTY FIDO User"




Then code\config.c

struct fido_data {
    dlgcontrol* fido_create_key_button, * fido_delete_key_button, * fido_import_key_button, * fido_import_ssh_button,
        * fido_clear_key_button, * fido_algo_combobox, * fido_display_text, * fido_app_text, * fido_verification_radio, * fido_resident_radio;
};

void fido_event_handler(dlgcontrol* ctrl, dlgparam* dlg, void* data, int event)
{

    [...]

    // display name
    char* szDisplayName = dlg_editbox_get(fidod->fido_display_text, dlg);
    if ((szDisplayName != NULL) && (szDisplayName[0] == '\0')) {
        MessageBoxW(NULL, L"Display name cannot be empty",
            L"FIDO Key Creation Failed", MB_SYSTEMMODAL | MB_ICONERROR | MB_OK);
        return;
    }

    [...]

    // attempt to create key
    if (fido_create_key(szAlgId, szDisplayName, szAppId, bResidentKey, bVerificationRequired))
    {
        [...]
    }

    [...]

    // default text for display name
    if (ctrl == fidod->fido_display_text && event == EVENT_REFRESH)
    {
        dlg_editbox_set(ctrl, dlg, "PuTTY-CAC SSH");
    }
}




And code\cert\cert_common.h

EXTERN BOOL fido_create_key(LPCSTR szAlgName, LPCTSTR szDisplayName, LPCSTR szApplication, BOOL bResidentKey, BOOL bUserVerification);




Sure you can come up with it as well in probably a better fashion. But this just gives a little detail to the request.

Generating FIDO keys using ssh-keygen also are plagued with the problem of being unable to define a username that can be used in Yubikey Authenticator, with a lot of complaints by the userbase.

This will definitely make life easier in doing something openssh cant at the moment.

Thanks

Aetherinox commented 10 months ago

I threw the changes here for now just to track them (if you need or want them)

NoMoreFood commented 10 months ago

@Aetherinox Your code was pretty good. I made some minor modifications and rebuilt everything here: https://github.com/NoMoreFood/putty-cac/tree/master/binaries. Please test it out.

Aetherinox commented 10 months ago

Nice to know I didn't screw it up too bad. Tested it out, worked perfect.

image

Also tested it with Touch + PIN, and a custom Display name. All is perfect.

Awesome job, thanks! OpenSSH was supposed to have this feature, but apparently it's broke right now and a bunch of people are complaining about it defaulting to "openssh".

And PuTTY-CAC just makes it a heck of a lot easier.

NoMoreFood commented 10 months ago

Thanks for the quick feedback. Expect an "official" release sometime this coming week if no issues are encountered during other testing.

Aetherinox commented 10 months ago

You're welcome. Appreciate the work. This is perfect.

I've got 8 Yubikeys I have to create FIDO resident keys for, so this just made my life a hell of a lot easier. I can finally finish my FIDO ssh keys.

Thanks again. Super appreciated.

NoMoreFood commented 9 months ago

Published as pre-release (same binaries).

Aetherinox commented 9 months ago

Thanks a bunch. This makes life just a bit easier.