Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Hybrid SSH Keys #161

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Implement Hybrid SSH Keys #161

merged 1 commit into from
Aug 13, 2024

Conversation

geedo0
Copy link

@geedo0 geedo0 commented Jul 23, 2024

This finishes the work in PR #160 which applied the upstream sshkey.c refactor to the OQS fork by adding support for hybrid SSH keys. More importantly, this brings the OQS-v9 branch up to parity with OQS-v8 in terms of supported algorithms and functionality. Therefore, we can do more in depth and thorough validation to increase confidence in cutting over to this newer branch.

Speaking to the code changes for hybrid SSH key support, this works by adding logic to ssh-oqs which branches on hybrid SSH key implementations to handle the classical portion of the key and combine it with the PQ portion as-appropriate. The main trick is to introduce a small lookup table for the RSA/ECDSA implementation and exposing the symbols to ssh-oqs via an extern declaration. One notable oddity is that upstream OpenSSH multiplexes the underlying EC curves by placing a generic implementation behind the P-256 struct and allowing the implementation to fork based on the bits or key->type parameters. Depending on the context, this is how sshkey does things so I followed their convention.

Related to issue #135

Testing

Asserted that Circle CI jobs pass. These tests run through a subset of the OpenSSH unit tests that have been documented to pass against the OQS fork and skip tests that depend on missing/broken functionality. This demonstrates internal consistency and parity with the testing bar set by OQS-v8.

Performed interop testing between OQS-v8 and OQS-v9 to assert that we have no regressions from pulling in 2 years of upstream changes and re-implementing PQ+Hybrid SSH Keys. This was done by modifying try_connection.py which tests all PQ+Hybrid signatures and key exchanges by connecting the built SSH client to the SSHD server and explicitly specifying each algorithm. By adding CLI flags to override this test to use an SSH or SSHD binary from somewhere else, we can perform thorough interop testing between an OQS-v8 server and OQS-v9 client or vice versa. Detailed process/commands outlined below.

# Clone and build OQS-v8 in a different location
git clone [email protected]:open-quantum-safe/openssh.git oqs-openssh-clean
cd oqs-openssh-clean
git checkout OQS-v8
./oqs-scripts/clone_liboqs.sh
./oqs-scripts/build_liboqs.sh
./oqs-scripts/build_openssh.sh

# Assert that all key exchange and signature algorithms interop between v8 and v9
## OQS-v9 ssh client connecting to an OQS-v8 sshd server
python3 oqs-test/try_connection.py --sshd `readlink -f ../oqs-openssh-clean/sshd` doall
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-rsa3072-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp256-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp521-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-dilithium2.
...

## OQS-v8 ssh client connecting to an OQS-v9 sshd server
python3 oqs-test/try_connection.py --ssh `readlink -f ../oqs-openssh-clean/ssh` doall
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-rsa3072-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp256-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp521-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-dilithium2.
...

@geedo0 geedo0 force-pushed the hybrid branch 4 times, most recently from faf03d7 to ad933c6 Compare July 23, 2024 16:02
@geedo0 geedo0 marked this pull request as ready for review July 23, 2024 17:02
ssh-oqs.c Show resolved Hide resolved
@geedo0 geedo0 requested a review from dstebila July 26, 2024 17:51
@geedo0
Copy link
Author

geedo0 commented Jul 30, 2024

@christianpaquin @baentsch @SWilson4 - Requesting your reviews if you have the time (don't have permissions to request directly in the PR).

/* .nid = */ {{ alg['openssl_nid'] }},
/* .cert = */ 0,
/* .sigonly = */ 0,
/* .keybits = */ 256, // TODO - What should be here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look quite finished, does it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call-out and reminder. I looked at this briefly and couldn't work out exactly what it's used for. All I know is that it's not strictly required to be accurate/populated. Seems vestigial or at best a rarely used fallback option when more specific strategies fail. I'll take a closer look at it now that we're closer to done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used as a fallback if impl->size() is NULL here. For OQS, this is never the case so keybits should be set to 0 (like it is for ECDSA and RSA).

ssh-oqs.c Show resolved Hide resolved
ssh-oqs.c Show resolved Hide resolved
ssh-oqs.c Show resolved Hide resolved
ssh-oqs.c Show resolved Hide resolved
impl = &sshkey_ecdsa_nistp256_impl;
break;
}
return impl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I understand why you don't do specific "...->func != NULL" checks: These two function sets are static and always guaranteed to be "OK" in this sense. May be sensible to add a comment here, just in case someone in the future adds another "impl" operating differently.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits. Thanks for the work and the good explanations as to what you did and tested, @geedo0 !

@geedo0 geedo0 force-pushed the hybrid branch 2 times, most recently from 7624075 to 4f822a2 Compare August 6, 2024 17:56
This finishes the work in PR open-quantum-safe#160 which applied the upstream `sshkey.c` refactor to the OQS fork by adding support for hybrid SSH keys. More importantly, this brings the `OQS-v9` branch up to parity with `OQS-v8` in terms of supported algorithms and functionality. Therefore, we can do more in depth and thorough validation to increase confidence in cutting over to this newer branch.

Speaking to the code changes for hybrid SSH key support, this works by adding logic to `ssh-oqs` which branches on hybrid SSH key implementations to handle the classical portion of the key and combine it with the PQ portion as-appropriate. The main trick is to introduce a small lookup table for the RSA/ECDSA implementation and exposing the symbols to `ssh-oqs` via an extern declaration. One notable oddity is that upstream OpenSSH multiplexes the underlying EC curves by placing a generic implementation behind the P-256 struct and allowing the implementation to fork based on the `bits` or `key->type` parameters. Depending on the context, this is how `sshkey` does things so I followed their convention.

Related to issue open-quantum-safe#135

Asserted that Circle CI jobs pass. These tests run through a subset of the OpenSSH unit tests that have been documented to pass against the OQS fork and skip tests that depend on missing/broken functionality. This demonstrates internal consistency and parity with the testing bar set by `OQS-v8`.

Performed interop testing between `OQS-v8` and `OQS-v9` to assert that we have no regressions from pulling in 2 years of upstream changes and re-implementing PQ+Hybrid SSH Keys. This was done by modifying `try_connection.py` which tests all PQ+Hybrid signatures and key exchanges by connecting the built SSH client to the SSHD server and explicitly specifying each algorithm. By adding CLI flags to override this test to use an SSH or SSHD binary from somewhere else, we can perform thorough interop testing between an `OQS-v8` server and `OQS-v9` client or vice versa. Detailed process/commands outlined below.

```
git clone [email protected]:open-quantum-safe/openssh.git oqs-openssh-clean
cd oqs-openssh-clean
git checkout OQS-v8
./oqs-scripts/clone_liboqs.sh
./oqs-scripts/build_liboqs.sh
./oqs-scripts/build_openssh.sh

python3 oqs-test/try_connection.py --sshd `readlink -f ../oqs-openssh-clean/sshd` doall
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-rsa3072-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp256-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp521-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-dilithium2.
...

python3 oqs-test/try_connection.py --ssh `readlink -f ../oqs-openssh-clean/ssh` doall
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-rsa3072-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp256-falcon512.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-ecdsa-nistp521-falcon1024.
Success! Key Exchange Algorithm: frodokem-640-aes-sha256. Signature Algorithm: ssh-dilithium2.
...
```

Signed-off-by: gcr <[email protected]>
@geedo0 geedo0 merged commit 063384d into open-quantum-safe:OQS-v9 Aug 13, 2024
3 checks passed
@geedo0 geedo0 deleted the hybrid branch August 13, 2024 17:01
@geedo0 geedo0 mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants