Polish up the PuTTYgen user interface for ECC key types.

Jacob pointed out that a free-text field for entering a key size in
bits is all very well for key types where we actually _can_ generate a
key to a size of your choice, but less useful for key types where
there are only three (or one) legal values for the field, especially
if we don't _say_ what they are.

So I've revamped the UI a bit: now, in ECDSA mode, you get a dropdown
list selector showing the available elliptic curves (and they're even
named, rather than just given by bit count), and in ED25519 mode even
that disappears. The curve selector for ECDSA and the bits selector
for RSA/DSA are independent controls, so each one remembers its last
known value even while temporarily hidden in favour of the other.

The actual generation function still expects a bit count rather than
an actual curve or algorithm ID, so the easiest way to actually
arrange to populate the drop-down list was to have an array of bit
counts exposed by sshecc.c. That's a bit ugly, but there we go.

One small functional change: if you enter an absurdly low value into
the RSA/DSA bit count box (under 256), PuTTYgen used to give a warning
and reset it to 256. Now it resets it to the default key length of
2048, basically because I was touching that code anyway to change a
variable name and just couldn't bring myself to leave it in a state
where it intentionally chose such an utterly useless key size. Of
course this doesn't prevent generation of 256-bit keys if someone
still really wants one - it just means they don't get one selected as
the result of a typo.

diff --git a/ssh.h b/ssh.h
index 75aad70ba82beddc788da304ef1c4250c6d6b1de..7a7e8576e011963b88fe8c15a6c781d319bd1c62 100644 (file)
--- a/ssh.h
+++ b/ssh.h
@@ -155,6 +155,7 @@ struct ec_curve {
 const struct ssh_signkey *ec_alg_by_oid(int len, const void *oid,
                                         const struct ec_curve **curve);
 const unsigned char *ec_alg_oid(const struct ssh_signkey *alg, int *oidlen);
+extern const int ec_nist_curve_lengths[], n_ec_nist_curve_lengths;
 const int ec_nist_alg_and_curve_by_bits(int bits,
                                         const struct ec_curve **curve,
                                         const struct ssh_signkey **alg);

diff --git a/sshecc.c b/sshecc.c
index d62c9b96ef6dd42ed460a56a9aac27a1d0ce1b17..46e6fadbec500c077e886ff01c64f074f8b56139 100644 (file)
--- a/sshecc.c
+++ b/sshecc.c
@@ -2937,6 +2937,9 @@ const unsigned char *ec_alg_oid(const struct ssh_signkey *alg,
     return extra->oid;
 }
 
+const int ec_nist_curve_lengths[] = { 256, 384, 521 };
+const int n_ec_nist_curve_lengths = lenof(ec_nist_curve_lengths);
+
 const int ec_nist_alg_and_curve_by_bits(int bits,
                                         const struct ec_curve **curve,
                                         const struct ssh_signkey **alg)

diff --git a/windows/winpgen.c b/windows/winpgen.c
index db55145c15561afe65a0252a80b8ed0228d32287..ad3da837a7e7e8b159eafef8154368b427a7c255 100644 (file)
--- a/windows/winpgen.c
+++ b/windows/winpgen.c
@@ -21,7 +21,8 @@
 
 #define WM_DONEKEY (WM_APP + 1)
 
-#define DEFAULT_KEYSIZE 2048
+#define DEFAULT_KEY_BITS 2048
+#define DEFAULT_CURVE_INDEX 0
 
 static char *cmdline_keyfile = NULL;
 
@@ -332,7 +333,8 @@ typedef enum {RSA, DSA, ECDSA, ED25519} keytype;
 struct rsa_key_thread_params {
     HWND progressbar;                 /* notify this with progress */
     HWND dialog;                      /* notify this on completion */
-    int keysize;                      /* bits in key */
+    int key_bits;                     /* bits in key modulus (RSA, DSA) */
+    int curve_bits;                    /* bits in elliptic curve (ECDSA) */
     keytype keytype;
     union {
         struct RSAKey *key;
@@ -350,13 +352,13 @@ static DWORD WINAPI generate_rsa_key_thread(void *param)
     progress_update(&prog, PROGFN_INITIALISE, 0, 0);
 
     if (params->keytype == DSA)
-       dsa_generate(params->dsskey, params->keysize, progress_update, &prog);
+       dsa_generate(params->dsskey, params->key_bits, progress_update, &prog);
     else if (params->keytype == ECDSA)
-        ec_generate(params->eckey, params->keysize, progress_update, &prog);
+        ec_generate(params->eckey, params->curve_bits, progress_update, &prog);
     else if (params->keytype == ED25519)
-        ec_edgenerate(params->eckey, params->keysize, progress_update, &prog);
+        ec_edgenerate(params->eckey, 256, progress_update, &prog);
     else
-       rsa_generate(params->key, params->keysize, progress_update, &prog);
+       rsa_generate(params->key, params->key_bits, progress_update, &prog);
 
     PostMessage(params->dialog, WM_DONEKEY, 0, 0);
 
@@ -369,7 +371,7 @@ struct MainDlgState {
     int generation_thread_exists;
     int key_exists;
     int entropy_got, entropy_required, entropy_size;
-    int keysize;
+    int key_bits, curve_bits;
     int ssh2;
     keytype keytype;
     char **commentptr;                /* points to key.comment or ssh2key.comment */
@@ -450,6 +452,8 @@ enum {
     IDC_TYPESTATIC, IDC_KEYSSH1, IDC_KEYSSH2RSA, IDC_KEYSSH2DSA,
     IDC_KEYSSH2ECDSA, IDC_KEYSSH2ED25519,
     IDC_BITSSTATIC, IDC_BITS,
+    IDC_CURVESTATIC, IDC_CURVE,
+    IDC_NOTHINGSTATIC,
     IDC_ABOUT,
     IDC_GIVEHELP,
     IDC_IMPORT,
@@ -584,6 +588,47 @@ void ui_set_state(HWND hwnd, struct MainDlgState *state, int status)
     }
 }
 
+/*
+ * Helper functions to set the key type, taking care of keeping the
+ * menu and radio button selections in sync and also showing/hiding
+ * the appropriate size/curve control for the current key type.
+ */
+void ui_update_key_type_ctrls(HWND hwnd)
+{
+    enum { BITS, CURVE, NOTHING } which;
+    static const int bits_ids[] = {
+        IDC_BITSSTATIC, IDC_BITS, 0
+    };
+    static const int curve_ids[] = {
+        IDC_CURVESTATIC, IDC_CURVE, 0
+    };
+    static const int nothing_ids[] = {
+        IDC_NOTHINGSTATIC, 0
+    };
+
+    if (IsDlgButtonChecked(hwnd, IDC_KEYSSH1) ||
+        IsDlgButtonChecked(hwnd, IDC_KEYSSH2RSA) ||
+        IsDlgButtonChecked(hwnd, IDC_KEYSSH2DSA)) {
+        which = BITS;
+    } else if (IsDlgButtonChecked(hwnd, IDC_KEYSSH2ECDSA)) {
+        which = CURVE;
+    } else {
+        /* ED25519 implicitly only supports one curve */
+        which = NOTHING;
+    }
+
+    hidemany(hwnd, bits_ids, which != BITS);
+    hidemany(hwnd, curve_ids, which != CURVE);
+    hidemany(hwnd, nothing_ids, which != NOTHING);
+}
+void ui_set_key_type(HWND hwnd, struct MainDlgState *state, int button)
+{
+    CheckRadioButton(hwnd, IDC_KEYSSH1, IDC_KEYSSH2ED25519, button);
+    CheckMenuRadioItem(state->keymenu, IDC_KEYSSH1, IDC_KEYSSH2ED25519,
+                       button, MF_BYCOMMAND);
+    ui_update_key_type_ctrls(hwnd);
+}
+
 void load_key_file(HWND hwnd, struct MainDlgState *state,
                   Filename *filename, int was_import_cmd)
 {
@@ -852,8 +897,9 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
 
        {
            struct ctlpos cp, cp2;
+            int ymax;
 
-           /* Accelerators used: acglops1rbde */
+           /* Accelerators used: acglops1rbvde */
 
            ctlposinit(&cp, hwnd, 4, 4, 4);
            beginbox(&cp, "Key", IDC_BOX_KEY);
@@ -894,14 +940,38 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
                       "ED&25519", IDC_KEYSSH2ED25519,
                      "SSH-&1 (RSA)", IDC_KEYSSH1,
                       NULL);
-           staticedit(&cp, "Number of &bits in a generated key:",
+            cp2 = cp;
+           staticedit(&cp2, "Number of &bits in a generated key:",
                       IDC_BITSSTATIC, IDC_BITS, 20);
+            ymax = cp2.ypos;
+            cp2 = cp;
+           staticddl(&cp2, "Cur&ve to use for generating this key:",
+                      IDC_CURVESTATIC, IDC_CURVE, 20);
+            SendDlgItemMessage(hwnd, IDC_CURVE, CB_RESETCONTENT, 0, 0);
+            {
+                int i, bits;
+                const struct ec_curve *curve;
+                const struct ssh_signkey *alg;
+
+                for (i = 0; i < n_ec_nist_curve_lengths; i++) {
+                    bits = ec_nist_curve_lengths[i];
+                    ec_nist_alg_and_curve_by_bits(bits, &curve, &alg);
+                    SendDlgItemMessage(hwnd, IDC_CURVE, CB_ADDSTRING, 0,
+                                       (LPARAM)curve->textname);
+                }
+            }
+            ymax = ymax > cp2.ypos ? ymax : cp2.ypos;
+            cp2 = cp;
+           statictext(&cp2, "(nothing to configure for this key type)",
+                      1, IDC_NOTHINGSTATIC);
+            ymax = ymax > cp2.ypos ? ymax : cp2.ypos;
+            cp.ypos = ymax;
            endbox(&cp);
        }
-        CheckRadioButton(hwnd, IDC_KEYSSH1, IDC_KEYSSH2ECDSA, IDC_KEYSSH2RSA);
-        CheckMenuRadioItem(state->keymenu, IDC_KEYSSH1, IDC_KEYSSH2ECDSA,
-                          IDC_KEYSSH2RSA, MF_BYCOMMAND);
-       SetDlgItemInt(hwnd, IDC_BITS, DEFAULT_KEYSIZE, FALSE);
+        ui_set_key_type(hwnd, state, IDC_KEYSSH2RSA);
+       SetDlgItemInt(hwnd, IDC_BITS, DEFAULT_KEY_BITS, FALSE);
+       SendDlgItemMessage(hwnd, IDC_CURVE, CB_SETCURSEL,
+                           DEFAULT_CURVE_INDEX, 0);
 
        /*
         * Initially, hide the progress bar and the key display,
@@ -949,7 +1019,8 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
                params = snew(struct rsa_key_thread_params);
                params->progressbar = GetDlgItem(hwnd, IDC_PROGRESS);
                params->dialog = hwnd;
-               params->keysize = state->keysize;
+               params->key_bits = state->key_bits;
+               params->curve_bits = state->curve_bits;
                 params->keytype = state->keytype;
                params->key = &state->key;
                params->dsskey = &state->dsskey;
@@ -976,13 +1047,7 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
            {
                state = (struct MainDlgState *)
                    GetWindowLongPtr(hwnd, GWLP_USERDATA);
-               if (!IsDlgButtonChecked(hwnd, LOWORD(wParam)))
-                   CheckRadioButton(hwnd,
-                                     IDC_KEYSSH1, IDC_KEYSSH2ED25519,
-                                    LOWORD(wParam));
-               CheckMenuRadioItem(state->keymenu,
-                                   IDC_KEYSSH1, IDC_KEYSSH2ED25519,
-                                  LOWORD(wParam), MF_BYCOMMAND);
+                ui_set_key_type(hwnd, state, LOWORD(wParam));
            }
            break;
          case IDC_QUIT:
@@ -1029,9 +1094,16 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
                (struct MainDlgState *) GetWindowLongPtr(hwnd, GWLP_USERDATA);
            if (!state->generation_thread_exists) {
                BOOL ok;
-               state->keysize = GetDlgItemInt(hwnd, IDC_BITS, &ok, FALSE);
+               state->key_bits = GetDlgItemInt(hwnd, IDC_BITS, &ok, FALSE);
                if (!ok)
-                   state->keysize = DEFAULT_KEYSIZE;
+                   state->key_bits = DEFAULT_KEY_BITS;
+                {
+                    int curveindex = SendDlgItemMessage(hwnd, IDC_CURVE,
+                                                        CB_GETCURSEL, 0, 0);
+                    assert(curveindex >= 0);
+                    assert(curveindex < n_ec_nist_curve_lengths);
+                    state->curve_bits = ec_nist_curve_lengths[curveindex];
+                }
                /* If we ever introduce a new key type, check it here! */
                state->ssh2 = !IsDlgButtonChecked(hwnd, IDC_KEYSSH1);
                 state->keytype = RSA;
@@ -1042,44 +1114,20 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
                 } else if (IsDlgButtonChecked(hwnd, IDC_KEYSSH2ED25519)) {
                     state->keytype = ED25519;
                 }
-               if (state->keysize < 256) {
-                   int ret = MessageBox(hwnd,
-                                        "PuTTYgen will not generate a key"
-                                        " smaller than 256 bits.\n"
-                                        "Key length reset to 256. Continue?",
-                                        "PuTTYgen Warning",
+               if ((state->keytype == RSA || state->keytype == DSA) &&
+                    state->key_bits < 256) {
+                    char *message = dupprintf
+                        ("PuTTYgen will not generate a key smaller than 256"
+                         " bits.\nKey length reset to default %d. Continue?",
+                         DEFAULT_KEY_BITS);
+                   int ret = MessageBox(hwnd, message, "PuTTYgen Warning",
                                         MB_ICONWARNING | MB_OKCANCEL);
+                    sfree(message);
                    if (ret != IDOK)
                        break;
-                   state->keysize = 256;
-                   SetDlgItemInt(hwnd, IDC_BITS, 256, FALSE);
+                   state->key_bits = DEFAULT_KEY_BITS;
+                   SetDlgItemInt(hwnd, IDC_BITS, DEFAULT_KEY_BITS, FALSE);
                }
-                if (state->keytype == ECDSA && !(state->keysize == 256 ||
-                                                 state->keysize == 384 ||
-                                                 state->keysize == 521)) {
-                    int ret = MessageBox(hwnd,
-                                         "Only 256, 384 and 521 bit elliptic"
-                                         " curves are supported.\n"
-                                         "Key length reset to 256. Continue?",
-                                         "PuTTYgen Warning",
-                                         MB_ICONWARNING | MB_OKCANCEL);
-                    if (ret != IDOK)
-                        break;
-                    state->keysize = 256;
-                    SetDlgItemInt(hwnd, IDC_BITS, 256, FALSE);
-                }
-                if (state->keytype == ED25519 && state->keysize != 256) {
-                    int ret = MessageBox(hwnd,
-                                         "Only 256 bit Edwards elliptic"
-                                         " curves are supported.\n"
-                                         "Key length reset to 256. Continue?",
-                                         "PuTTYgen Warning",
-                                         MB_ICONWARNING | MB_OKCANCEL);
-                    if (ret != IDOK)
-                        break;
-                    state->keysize = 256;
-                    SetDlgItemInt(hwnd, IDC_BITS, 256, FALSE);
-                }
                ui_set_state(hwnd, state, 1);
                SetDlgItemText(hwnd, IDC_GENERATING, entropy_msg);
                state->key_exists = FALSE;
@@ -1098,7 +1146,13 @@ static INT_PTR CALLBACK MainDlgProc(HWND hwnd, UINT msg,
                 * so with 2 bits per mouse movement we expect 2
                 * bits every 2 words.
                 */
-               state->entropy_required = (state->keysize / 2) * 2;
+               if (state->keytype == RSA || state->keytype == DSA)
+                    state->entropy_required = (state->key_bits / 2) * 2;
+               else if (state->keytype == ECDSA)
+                    state->entropy_required = (state->curve_bits / 2) * 2;
+                else
+                    state->entropy_required = 256;
+
                state->entropy_got = 0;
                state->entropy_size = (state->entropy_required *
                                       sizeof(unsigned));