Skip to content

Commit

Permalink
Apply upstream refactor to sshkey.c
Browse files Browse the repository at this point in the history
In PR #159 we resolved a massive merge conflict in `sshkey.c` by simply taking the upstream version and removing all PQ SSH key support from the trunk. This broke the build/tests and obviously the ability to use PQ SSH keys. This PR restores PQ SSH key support for "pure" PQ algorithms. A follow-up PR will bring back support for hybrid-PQ. This was done to break up the PRs and do things more incrementally.

At a high-level, there was a major refactor of `sshkey` upstream which moved algorithm implementations to an OOP abstraction via the `sshkey_impl` struct. This allowed them to get rid of all the switch statements, encapsulate algorithm specific code, and standardize on a single factory method to get the right implementation. Some other minor things like the function signatures for sign/verify were updated to accept new optional arguments.

Getting OQS over to this new system was a matter of defining these `sshkey_impl` structs and factoring out the relevant OQS bits to the proper functions. This involved a number of changes/renames/deletions to the templates. I also performed a 3-way merge to pull-in changes that weren't directly impacted by the refactor.

There is no support for hybrid SSH Key support for now. This is to make sure that the baseline refactor is in a good shape before committing to a certain path. To support hybrid, I propose implementing generic hybrid versions of these classes. The only tricky thing is figuring out how to expose the RSA/EC implementations to the `ssh-oqs` file (probably declaring externs?). I considered doing this "on the outside", but that seems to go against what the refactor aimed to do and adds a lot more complexity to the sshkey source than seems necessary.

What's working now?
- `build_openssh.sh` now compiles and installs successfully 🎉
- `make tests` runs through a lot of tests before failing now. This is my next line of inquiry to chase down.
  • Loading branch information
geedo0 committed Jul 15, 2024
1 parent ac7c26b commit d6dc6fe
Show file tree
Hide file tree
Showing 21 changed files with 793 additions and 154 deletions.
14 changes: 7 additions & 7 deletions kexgen.c
Original file line number Diff line number Diff line change
Expand Up @@ -619,31 +619,31 @@ input_kex_gen_init(int type, u_int32_t seq, struct ssh *ssh)
///// OQS_TEMPLATE_FRAGMENT_ADD_INIT_SWITCH_CASES_START
case KEX_KEM_FRODOKEM_640_AES_SHA256:
r = kex_kem_frodokem_640_aes_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_FRODOKEM_976_AES_SHA384:
r = kex_kem_frodokem_976_aes_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_FRODOKEM_1344_AES_SHA512:
r = kex_kem_frodokem_1344_aes_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_FRODOKEM_640_SHAKE_SHA256:
r = kex_kem_frodokem_640_shake_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_FRODOKEM_976_SHAKE_SHA384:
r = kex_kem_frodokem_976_shake_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_FRODOKEM_1344_SHAKE_SHA512:
r = kex_kem_frodokem_1344_shake_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_KYBER_512_SHA256:
r = kex_kem_kyber_512_enc(kex, client_pubkey,
&server_pubkey, &shared_secret);
&server_pubkey, &shared_secret);
break;
case KEX_KEM_KYBER_768_SHA384:
r = kex_kem_kyber_768_enc(kex, client_pubkey,
Expand Down
2 changes: 1 addition & 1 deletion monitor.c
Original file line number Diff line number Diff line change
Expand Up @@ -1775,7 +1775,7 @@ monitor_apply_keystate(struct ssh *ssh, struct monitor *pmonitor)
kex->kex[KEX_KEM_HQC_192_SHA384] = kex_gen_server;
kex->kex[KEX_KEM_HQC_256_SHA512] = kex_gen_server;
#ifdef WITH_OPENSSL
#ifdef OPENSSL_HAS_ECC
#ifdef OPENSSL_HAS_ECC
kex->kex[KEX_KEM_FRODOKEM_640_AES_ECDH_NISTP256_SHA256] = kex_gen_server;
kex->kex[KEX_KEM_FRODOKEM_976_AES_ECDH_NISTP384_SHA384] = kex_gen_server;
kex->kex[KEX_KEM_FRODOKEM_1344_AES_ECDH_NISTP521_SHA512] = kex_gen_server;
Expand Down
Empty file modified oqs-template/generate.py
100644 → 100755
Empty file.
4 changes: 2 additions & 2 deletions oqs-template/ssh-keygen.c/print_resource_records.fragment
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
{%- for sig in config['sigs'] %}
n += do_print_resource_record(pw,
_PATH_HOST_{{ sig['name']|upper }}_KEY_FILE, rr_hostname,
print_generic);
print_generic, opts, nopts);
{%- for alg in sig['mix_with'] %}
n += do_print_resource_record(pw,
_PATH_HOST_{{ alg['name']|upper }}_{{ sig['name']|upper }}_KEY_FILE, rr_hostname,
print_generic);
print_generic, opts, nopts);
{%- endfor %}
{%- endfor %}

87 changes: 83 additions & 4 deletions oqs-template/ssh-oqs.c/define_sig_functions.fragment
Original file line number Diff line number Diff line change
@@ -1,37 +1,116 @@
{%- for sig in config['sigs'] %}
{%- set symbol_base_name = sig['name']|replace('_','') %}
/*---------------------------------------------------
* {{ sig['name']|upper }} METHODS
*---------------------------------------------------
*/
int ssh_{{ sig['name']|replace('_','') }}_sign(const struct sshkey *key,
static int ssh_{{ symbol_base_name }}_generate(struct sshkey *k, int bits)
{
k->oqs_pk_len = oqs_sig_pk_len(k->type);
k->oqs_sk_len = oqs_sig_sk_len(k->type);
if ((k->oqs_pk = malloc(k->oqs_pk_len)) == NULL ||
(k->oqs_sk = malloc(k->oqs_sk_len)) == NULL) {
return SSH_ERR_ALLOC_FAIL;
}
return OQS_SIG_{{ sig['name'] }}_keypair(k->oqs_pk, k->oqs_sk);
}

int ssh_{{ symbol_base_name }}_sign(const struct sshkey *key,
u_char **sigp,
size_t *lenp,
const u_char *data,
size_t datalen,
const char *alg,
const char *sk_provider,
const char *sk_pin,
u_int compat)
{
OQS_SIG *sig = OQS_SIG_new(OQS_SIG_alg_{{ sig['name'] }});
if (sig == NULL) {
return SSH_ERR_ALLOC_FAIL;
}
int r = ssh_generic_sign(sig, "{{ sig['name']|replace('_','') }}", key, sigp, lenp, data, datalen, compat);
int r = ssh_generic_sign(sig, "{{ symbol_base_name }}", key, sigp, lenp, data, datalen, compat);
OQS_SIG_free(sig);
return r;
}
int ssh_{{ sig['name']|replace('_','') }}_verify(const struct sshkey *key,

int ssh_{{ symbol_base_name }}_verify(const struct sshkey *key,
const u_char *signature,
size_t signaturelen,
const u_char *data,
size_t datalen,
const char *alg,
u_int compat)
{
OQS_SIG *sig = OQS_SIG_new(OQS_SIG_alg_{{ sig['name'] }});
if (sig == NULL) {
return SSH_ERR_ALLOC_FAIL;
}
int r = ssh_generic_verify(sig, "{{ sig['name']|replace('_','') }}", key, signature, signaturelen, data, datalen, compat);
int r = ssh_generic_verify(sig, "{{ symbol_base_name }}", key, signature, signaturelen, data, datalen, compat);
OQS_SIG_free(sig);
return r;
}

static const struct sshkey_impl_funcs sshkey_{{ symbol_base_name }}_funcs = {
/* .size = */ ssh_generic_size,
/* .alloc = */ ssh_generic_alloc,
/* .cleanup = */ ssh_generic_cleanup,
/* .equal = */ ssh_generic_equal,
/* .ssh_serialize_public = */ ssh_generic_serialize_public,
/* .ssh_deserialize_public = */ ssh_generic_deserialize_public,
/* .ssh_serialize_private = */ ssh_generic_serialize_private,
/* .ssh_deserialize_private = */ ssh_generic_deserialize_private,
/* .generate = */ ssh_{{ symbol_base_name }}_generate,
/* .copy_public = */ ssh_generic_copy_public,
/* .sign = */ ssh_{{ symbol_base_name }}_sign,
/* .verify = */ ssh_{{ symbol_base_name }}_verify,
};

const struct sshkey_impl sshkey_{{ symbol_base_name }}_impl = {
/* .name = */ "ssh-{{ symbol_base_name }}",
/* .shortname = */ "{{ symbol_base_name|upper }}",
/* .sigalg = */ NULL,
/* .type = */ KEY_{{ sig['name']|upper }},
/* .nid = */ 0,
/* .cert = */ 0,
/* .sigonly = */ 0,
/* .keybits = */ 256, // TODO - What should be here?
/* .funcs = */ &sshkey_{{ symbol_base_name }}_funcs,
};
{%- endfor %}

#ifdef HYBRID_IMPLEMENTATION_EXISTS
// #ifdef WITH_OPENSSL
{%- for sig in config['sigs'] %}
{%- for alg in sig['mix_with'] if alg['rsa'] %}
{%- set symbol_base_name = alg['name']|replace('_','') + '_' + sig['name']|replace('_','') %}
static const struct sshkey_impl_funcs sshkey_{{ symbol_base_name }}_funcs = {
/* .size = */ ssh_generic_size,
/* .alloc = */ ssh_generic_alloc,
/* .cleanup = */ ssh_generic_cleanup,
/* .equal = */ ssh_generic_equal,
/* .ssh_serialize_public = */ ssh_generic_serialize_public,
/* .ssh_deserialize_public = */ ssh_generic_deserialize_public,
/* .ssh_serialize_private = */ ssh_generic_serialize_private,
/* .ssh_deserialize_private = */ ssh_generic_deserialize_private,
/* .generate = */ ssh_{{ symbol_base_name }}_generate,
/* .copy_public = */ ssh_generic_copy_public,
/* .sign = */ ssh_{{ symbol_base_name }}_sign,
/* .verify = */ ssh_{{ symbol_base_name }}_verify,
};

const struct sshkey_impl sshkey_{{ symbol_base_name }}_impl = {
/* .name = */ "ssh-{{ symbol_base_name }}",
/* .shortname = */ "{{ symbol_base_name|upper }}",
/* .sigalg = */ NULL,
/* .type = */ KEY_{{ sig['name']|upper }},
/* .nid = */ 0,
/* .cert = */ 0,
/* .sigonly = */ 0,
/* .keybits = */ 256, // TODO - What should be here?
/* .funcs = */ &sshkey_{{ symbol_base_name }}_funcs,
};
{%- endfor %}
{%- endfor %}
#endif /* WITH_OPENSSL */

8 changes: 8 additions & 0 deletions oqs-template/ssh-oqs.c/return_pk_len.fragment
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{%- for sig in config['sigs'] %}
case KEY_{{ sig['name']|upper }}:
{%- for alg in sig['mix_with'] %}
case KEY_{{ alg['name']|upper }}_{{ sig['name']|upper }}:
{%- endfor -%}
return OQS_SIG_{{ sig['name'] }}_length_public_key;
{%- endfor %}

8 changes: 8 additions & 0 deletions oqs-template/ssh-oqs.c/return_sk_len.fragment
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{%- for sig in config['sigs'] %}
case KEY_{{ sig['name']|upper }}:
{%- for alg in sig['mix_with'] %}
case KEY_{{ alg['name']|upper }}_{{ sig['name']|upper }}:
{%- endfor %}
return OQS_SIG_{{ sig['name'] }}_length_secret_key;
{%- endfor %}

20 changes: 9 additions & 11 deletions oqs-template/sshkey.c/define_keytypes.fragment
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
{%- for sig in config['sigs'] %}
{ "ssh-{{ sig['name']|replace('_','') }}", "{{ sig['name']|replace('_','')|upper }}", NULL,
KEY_{{ sig['name']|upper }}, 0, 0, 0 },
&sshkey_{{ sig['name']|replace('_','') }}_impl,
{%- endfor %}
#ifdef WITH_OPENSSL
#ifdef HYBRID_IMPLEMENTATION_EXISTS
// #ifdef WITH_OPENSSL
{%- for sig in config['sigs'] %}
{%- for alg in sig['mix_with'] if alg['rsa'] %}
{ "ssh-{{ alg['name'] }}-{{ sig['name']|replace('_','') }}", "{{ alg['name']|upper }}_{{ sig['name']|replace('_','')|upper }}", NULL,
KEY_{{ alg['name']|upper }}_{{ sig['name']|upper }}, 0, 0, 0 },
{%- endfor %}
{%- for alg in sig['mix_with'] if alg['rsa'] %}
&sshkey_{{ alg['name']|replace('_','') }}_{{ sig['name']|replace('_','') }}_impl,
{%- endfor %}
{%- endfor %}
#ifdef OPENSSL_HAS_ECC
{%- for sig in config['sigs'] %}
{%- for alg in sig['mix_with'] if not alg['rsa'] %}
{ "ssh-{{ alg['name']|replace('_','-') }}-{{ sig['name']|replace('_','') }}", "{{ alg['name']|upper }}_{{ sig['name']|replace('_','')|upper }}", NULL,
KEY_{{ alg['name']|upper }}_{{ sig['name']|upper }}, {{ alg['openssl_nid'] }}, 0, 0 },
{%- endfor %}
{%- for alg in sig['mix_with'] if not alg['rsa'] %}
&sshkey_{{ alg['name']|replace('_','') }}_{{ sig['name']|replace('_','') }}_impl,
{%- endfor %}
{%- endfor %}
#endif /* OPENSSL_HAS_ECC */
#endif /* WITH_OPENSSL */
Expand Down
20 changes: 20 additions & 0 deletions oqs-template/sshkey.c/extern_key_impls.fragment
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{%- for sig in config['sigs'] %}
extern const struct sshkey_impl sshkey_{{ sig['name']|replace('_','') }}_impl;
{%- endfor %}

#ifdef HYBRID_IMPLEMENTATION_EXISTS
// #ifdef WITH_OPENSSL
{%- for sig in config['sigs'] %}
{%- for alg in sig['mix_with'] if alg['rsa'] %}
extern const struct sshkey_impl sshkey_{{ alg['name']|replace('_','') }}_{{ sig['name']|replace('_','') }}_impl;
{%- endfor %}
{%- endfor %}
#ifdef OPENSSL_HAS_ECC
{%- for sig in config['sigs'] %}
{%- for alg in sig['mix_with'] if not alg['rsa'] %}
extern const struct sshkey_impl sshkey_{{ alg['name']|replace('_','') }}_{{ sig['name']|replace('_','') }}_impl;
{%- endfor %}
{%- endfor %}
#endif /* OPENSSL_HAS_ECC */
#endif /* WITH_OPENSSL */

8 changes: 0 additions & 8 deletions oqs-template/sshkey.c/return_pk_len.fragment

This file was deleted.

8 changes: 0 additions & 8 deletions oqs-template/sshkey.c/return_sk_len.fragment

This file was deleted.

24 changes: 0 additions & 24 deletions oqs-template/sshkey.c/sshkey_generate_switch_keytype.fragment

This file was deleted.

24 changes: 0 additions & 24 deletions oqs-template/sshkey.c/sshkey_sign_switch_keytype.fragment

This file was deleted.

21 changes: 0 additions & 21 deletions oqs-template/sshkey.c/sshkey_verify_switch_keytype.fragment

This file was deleted.

5 changes: 0 additions & 5 deletions oqs-template/sshkey.h/declare_prototypes.fragment

This file was deleted.

Loading

0 comments on commit d6dc6fe

Please sign in to comment.