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

Upgrade to OpenSSH v9.7 #159

Merged
merged 1,789 commits into from
Jul 9, 2024
Merged

Upgrade to OpenSSH v9.7 #159

merged 1,789 commits into from
Jul 9, 2024

Conversation

geedo0
Copy link

@geedo0 geedo0 commented Jun 24, 2024

This is a first pass at resolving all of the merge conflicts between the current tip of OQS-v8 and the V_9_7_P1 tag in upstream OpenSSH (Issue #135).

The merge strategy here differs a bit from previous upstream merges (e.g. PR #106 and PR #121) where all of the changes were squashed and incorporated into a single commit and applied to the trunk. This is a more typical git merge in that we retain both parents and their commit histories. This will make future merges more straightforward by allowing git to notice the shared history and avoid marking these merged commits as conflicting changes. For the maintainers, make sure to merge using the default "Merge Pull Request" button. I tested this on my own fork and seems to be the only one of Github's options that preserves the history.

Here's the git-foo used to script the merge and handle the false positives from the "squash merges".

oqs_tip=OQS-v8
openssh_release=V_9_7_P1
git merge ${openssh_release}
base=`git merge-base ${oqs_tip} ${openssh_release}`
for f in `git diff --name-only --diff-filter=U`; do
  # This fetches all of the commits which touched the file since the merge base
  # Filter out the two commits for the 8.6 and 8.9 merges since they are technically already incorporated
  conflicts=$(git log --oneline ${base}..${oqs_tip} -- $f | ggrep -v -P '(1f58edd59|f058d3168)')
  # Check if we have no OQS-OpenSSH conflicts specific
  if [[ -z ${conflicts} ]]; then
    echo "$f has no conflicts"
    # Resolve the conflict by taking the upstream version of the file
    git checkout --theirs -- $f
    git add $f
  else
    echo "$f has conflicts"
    echo ${conflicts}
    # Send all of the OQS diffs to a file to help resolve the merge conflicts
    for c in `echo ${conflicts} | cut -d ' ' -f1`; do
      git show $c -- $f >> ~/conflicting_diffs.t
    done
  fi
done

For the remaining conflicts, I went through each file one-by-one with this pseudo-algorithm:

  1. Incorporate all changes from both sides that have no direct conflicts.
  2. Look for OQS specific changes with conflicts and apply them as-appropriate.
  3. Take the upstream version for any remaining conflicts.

Callouts from this process:

  • sshkey.c and sshkey.h experienced a major refactor upstream that impacted how OQS modified these files. I simply took the upstream versions for now and plan to address the conflict properly in a separate PR.
  • Kept README.md as-is from OQS and applied changes to README.original.md.
  • Took .depend from upstream, will update in a subsequent commit.
  • version.h retained the 2022-01 datestamp from OQS, will update this when we're ready to stage a release.
  • In ssh-keygen.c the OQS_TEMPLATE_FRAGMENT_PRINT_RESOURCE_RECORDS_START template changed to accept two additional arguments opts and nopts. I added these in manually for now.

To self-check I did the following:

  • Test build by running build_openssh.sh and finding compiler errors.
  • Run git diff HEAD V_9_7_P1 to highlight all the changes and assert that all changes were introduced by OQS alone.

This last process flagged a handful of issues. Mostly around duplicated code blocks from taking them from previous upstream merges and this current merge and git not noticing it. With that out of the way, I'm reasonably confident that this PR is pretty close to upstream v9.7 with only the changes from OQS applied to it.

So after all that, what's working so far? build_openssh.sh will build the project but fail to install with some error about unknown key types. The failure I had locally is the same one reported by the CI job.

What's next?

  • Properly handle the merge conflicts in sshkey.(c|h).
  • Regenerate .depend.
  • Fix the impacted OQS templates and regenerate the source.
  • Cut a new OQS-v9 branch and update version.h.

daztucker and others added 30 commits June 23, 2023 09:49
Hardenedmalloc dropped support for "legacy glibc" versions in their
64dad0a69 so use a newer Ubuntu version for the runner for that test.
ok djm@ dtucker@

OpenBSD-Commit-ID: 3e6d47567b895c7c28855c7bd614e106c987a6d8
by github PR#410, ok deraadt.

OpenBSD-Commit-ID: 0514cd51db3ec60239966622a0d3495b15406ddd
This function is apparently deprecated. Documentation on what is the
supposed replacement is is non-existent, so this follows the approach
glibc used https://sourceware.org/git/?p=glibc.git;a=patch;h=f278835f59

ok dtucker@
OpenBSD-Commit-ID: d0f12af0a5067a756aa707bc39a83fa6f58bf7e5
overflows reported by Yair Mizrahi @ JFrog; feedback/ok millert@

OpenBSD-Commit-ID: 52af085f4e7ef9f9d8423d8c1840a6a88bda90bd
These too are unreachable, but we want the code to be safe regardless of
context. Reported by Yair Mizrahi @ JFrog
OpenBSD-Commit-ID: e7c31034a5434f2ead3579b13a7892960651e6b0
This defines wire formats for optional KRL extensions and implements
parsing of the new submessages. No actual extensions are supported at
this point.

ok markus

OpenBSD-Commit-ID: ae2fcde9a22a9ba7f765bd4f36b3f5901d8c3fa7
When the KRL format was originally defined, it included support for
signing of KRL objects. However, the code to sign KRLs and verify KRL
signatues was never completed in OpenSSH.

Now, some years later, we have SSHSIG support in ssh-keygen that is
more general, well tested and actually works. So this removes the
semi-finished KRL signing/verification support from OpenSSH and
refactors the remaining code to realise the benefit - primarily, we
no longer need to perform multiple parsing passes over KRL objects.

ok markus@

OpenBSD-Commit-ID: 517437bab3d8180f695c775410c052340e038804
This allows matching on the addresses of available network interfaces
and may be used to vary the effective client configuration based on
network location (e.g. to use a ProxyJump when not on a particular
network).

ok markus@

OpenBSD-Commit-ID: cffb6ff9a3803abfc52b5cad0aa190c5e424c139
This adds a ssh_config(5) "Tag" directive and corresponding
"Match tag" predicate that may be used to select blocks of
configuration similar to the pf.conf(5) keywords of the same
name.

ok markus

OpenBSD-Commit-ID: dc08358e70e702b59ac3e591827e5a96141b06a3
valid magic number and not SSH_ERR_MESSAGE_INCOMPLETE; the former is needed
to fall back to text revocation lists in some cases; fixes t-cert-hostkey.

OpenBSD-Commit-ID: 5c670a6c0f027e99b7774ef29f18ba088549c7e1
where it caused merge conflict in -portable for each commit :(

OpenBSD-Commit-ID: 756ebac963df3245258b962e88150ebab9d5fc20
too no code change

OpenBSD-Commit-ID: ef5bf46b57726e4260a63b032b0b5ac3b4fe9cd4
OpenBSD-Commit-ID: 4776ced33b780f1db0b2902faec99312f26a726b
OpenBSD-Commit-ID: 535f5257c779e26c6a662a038d241b017f8cab7c
with that in ssh.1 - reformat usage() to match what "man ssh" does on 80width

OpenBSD-Commit-ID: 5235dd7aa42e5bf90ae54579d519f92fc107036e
OpenBSD-Commit-ID: 9a08ed8dae27d3f38cf280f1b28d4e0ff41a737a
Fixes build breakage on platforms that lack getifaddrs()
that isn't a PKCS#11 provider; from / ok markus@

OpenBSD-Commit-ID: 39532cf18b115881bb4cfaee32084497aadfa05c
libraries to ssh-agent by default.

The old behaviour of allowing remote clients from loading providers
can be restored using `ssh-agent -O allow-remote-pkcs11`.

Detection of local/remote clients requires a ssh(1) that supports
the `[email protected]` extension. Forwarding access to a
ssh-agent socket using non-OpenSSH tools may circumvent this control.

ok markus@

OpenBSD-Commit-ID: 4c2bdf79b214ae7e60cc8c39a45501344fa7bd7c
This checks via nlist(3) that candidate provider libraries contain one
of the symbols that we will require prior to dlopen(), which can cause
a number of side effects, including execution of constructors.

Feedback deraadt; ok markus

OpenBSD-Commit-ID: 1508a5fbd74e329e69a55b56c453c292029aefbe
Make ssh-pkcs11-client start an independent helper for each provider,
providing better isolation between modules and reliability if a single
module misbehaves.

This also implements reference counting of PKCS#11-hosted keys,
allowing ssh-pkcs11-helper subprocesses to be automatically reaped
when no remaining keys reference them. This fixes some bugs we have
that make PKCS11 keys unusable after they have been deleted, e.g.
https://bugzilla.mindrot.org/show_bug.cgi?id=3125

ok markus@

OpenBSD-Commit-ID: 0ce188b14fe271ab0568f4500070d96c5657244e
daztucker and others added 16 commits February 22, 2024 17:59
to the active configuration. This fixes the config parser from erroneously
rejecting cases like:

AuthenticationMethods password
Match User ivy
 AuthenticationMethods any

bz3657 ok markus@

OpenBSD-Commit-ID: 7f196cba634c2a3dba115f3fac3c4635a2199491
spotted by Coverity (CID 438039)

OpenBSD-Commit-ID: 208839699939721f452a4418afc028a9f9d3d8af
discussed with deraadt and dtucker a while ago
Unbreaks "make test" when compiled --without-openssl.

Similar treatment to how we do DSA and ECDSA.
OpenBSD-Commit-ID: 463e4a69eef3426a43a2b922c4e7b2011885d923
found by RASU JSC, reported by Maks Mishin in GHPR#467

OpenBSD-Commit-ID: 97d96a166b1ad4b8d229864a553e3e56d3116860
Use openssl in the directory specified by --with-ssl-dir as long
as it's functional.  Reported by The Doctor.
$TEST_SHELL. Fixes test when run by a user whose login shell is tcsh.
Found by vinschen at redhat.com.

OpenBSD-Regress-ID: f68d79e7f00caa8d216ebe00ee5f0adbb944062a
allowed_signers files with blank lines; reported by Wiktor Kwapisiewicz

OpenBSD-Commit-ID: b3a22a2afd753d70766f34bc7f309c03706b5298
ppoll() bz3670, reported by Ben Hamilton; ok dtucker@

OpenBSD-Commit-ID: e58f18042b86425405ca09e6e9d7dfa1df9f5f7f
Fixes test failures on Solaris 8 reported by Tom G. Christensen
Add LibreSSL 3.9.0, bump older branches to their respective current
releases.
OpenBSD-Commit-ID: 618ececf58b8cdae016b149787af06240f7b0cbc
@baentsch
Copy link
Member

Thanks very much for your work and particularly the explanations, @geedo0 ! So do you indeed prefer to merge this first to the new branch before all CI checks turn green? That feels not quite right, but would surely create a clean git log (upstream merge in this PR, getting OQS integration to work again in the next one), so fine with me unless someone objects.

@geedo0
Copy link
Author

geedo0 commented Jun 25, 2024

So do you indeed prefer to merge this first to the new branch before all CI checks turn green?

Yes, I think it's okay since it's going into the new OQS-v9 branch instead of the stable/working OQS-v8 branch. This way, we can have more targeted PRs that folks can actually scrutinize instead of this 30k LOC behemoth of a PR.

Copy link
Member

@dstebila dstebila left a comment

Choose a reason for hiding this comment

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

I've read through the description of your process, and also skimmed the diff between geedo0:merge-ossh97 and openssh:V_9_7_P1. I'm okay with the idea of merging this into OQS-v9 and then continuing with the next phase of work on that branch.

My only comment would be about whether we should omit the .get_allowed_signers* files from our repository; I'm not sure what the purpose of these files is, but if it's about who's authorized to sign certain things, then we may nto intend to authorize the OpenSSH team to sign things related to our fork.

@geedo0
Copy link
Author

geedo0 commented Jul 9, 2024

My only comment would be about whether we should omit the .get_allowed_signers* files from our repository; I'm not sure what the purpose of these files is, but if it's about who's authorized to sign certain things, then we may nto intend to authorize the OpenSSH team to sign things related to our fork.

Good catch, you got me curious and I looked into it. It looks like you're allowed to use SSH keys to sign commits (instead of GPG keys). This file gives you granular control over the SSH keys authorized to do that. This fork doesn't enforce anything related to commit signing, but it's certainly good hygeine to not pull-in such enforcement from upstream.

This is a first pass at resolving all of the merge conflicts between the current tip of `OQS-v8` and the `V_9_7_P1` tag in upstream OpenSSH.

The merge strategy here differs a bit from previous upstream merges (e.g. PR open-quantum-safe#106 and PR open-quantum-safe#121) where all of the changes were squashed and incorporated into a single commit and applied to the trunk. This is a more typical `git merge` in that we retain both parents and their commit histories. This will make future merges more straightforward by allowing git to notice the shared history and avoid marking these merged commits as conflicting changes.

Here's the git-foo used to script the merge and handle the false positives from the "squash merges".

```
oqs_tip=OQS-v8
openssh_release=V_9_7_P1
git merge ${openssh_release}
base=`git merge-base ${oqs_tip} ${openssh_release}`
for f in `git diff --name-only --diff-filter=U`; do
  # This fetches all of the commits which touched the file since the merge base
  # Filter out the two commits for the 8.6 and 8.9 merges since they are technically already incorporated
  conflicts=$(git log --oneline ${base}..${oqs_tip} -- $f | ggrep -v -P '(1f58edd|f058d3168)')
  # Check if we have no OQS-OpenSSH conflicts specific
  if [[ -z ${conflicts} ]]; then
    echo "$f has no conflicts"
    # Resolve the conflict by taking the upstream version of the file
    git checkout --theirs -- $f
    git add $f
  else
    echo "$f has conflicts"
    echo ${conflicts}
    # Send all of the OQS diffs to a file to help resolve the merge conflicts
    for c in `echo ${conflicts} | cut -d ' ' -f1`; do
      git show $c -- $f >> ~/conflicting_diffs.t
    done
  fi
done
```

For the remaining conflicts, I went through each file one-by-one with this pseudo-algorithm:
1. Incorporate all changes from both sides that have no direct conflicts.
2. Look for OQS specific changes with conflicts and apply them as-appropriate.
3. Take the upstream version for any remaining conflicts.

Callouts from this process:
- `sshkey.c` and `sshkey.h` experienced a major refactor upstream that impacted how OQS modified these files. I simply took the upstream versions for now and plan to address the conflict properly in a separate PR.
- Kept `README.md` as-is from OQS and applied changes to `README.original.md`.
- Took `.depend` from upstream, will update in a subsequent commit.
- `version.h` retained the 2022-01 datestamp from OQS, will update this when we're ready to stage a release.
- In `ssh-keygen.c` the `OQS_TEMPLATE_FRAGMENT_PRINT_RESOURCE_RECORDS_START` template changed to accept two additional arguments `opts` and `nopts`. I added these in manually for now.

To self-check I did the following:
- Test build by running `build_openssh.sh` and finding compiler errors.
- Run `git diff HEAD V_9_7_P1` to highlight all the changes and assert that all changes were introduced by OQS alone.

This last process flagged a handful of issues. Mostly around duplicated code blocks from taking them from previous upstream merges and this current merge and git not noticing it. With that out of the way, I'm reasonably confident that this PR is pretty close to upstream v9.7 with only the changes from OQS applied to it.

So after all that, what's working so far? `build_openssh.sh` will build the project but fail to install with some error about unknown key types.

What's next?
- Properly handle the merge conflicts in `sshkey.(c|h)`.
- Regenerate `.depend`.
- Fix the impacted OQS templates and regenerate the source.
- Cut a new `OQS-v9` branch and update `version.h`.
@geedo0
Copy link
Author

geedo0 commented Jul 9, 2024

I removed the .git_allowed_signers file. As long as you the default "Merge Pull Request" button, that should do what we want and preserve the Git history. This is important as the latest upstream is actually 9.8 now and I need to follow this PR up with yet another upstream merge to keep us up to date.

@dstebila dstebila merged commit ac7c26b into open-quantum-safe:OQS-v9 Jul 9, 2024
0 of 3 checks passed
geedo0 added a commit to geedo0/openssh that referenced this pull request Jul 15, 2024
In PR open-quantum-safe#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.
geedo0 added a commit to geedo0/openssh that referenced this pull request Jul 15, 2024
In PR open-quantum-safe#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.

Signed-off-by: Gerardo Ravago <[email protected]>
geedo0 added a commit to geedo0/openssh that referenced this pull request Jul 15, 2024
In PR open-quantum-safe#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.

Signed-off-by: Gerardo Ravago <[email protected]>
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.