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

Create the automatic key when specified with -i #9881

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

cmbrose
Copy link
Member

@cmbrose cmbrose commented Nov 5, 2024

Fixes: #9817

Fixes the flow with gh cs ssh --config where the automatic key is specified, but the subsequent ssh command does not actually generate the key.

@cmbrose cmbrose requested review from a team as code owners November 5, 2024 22:31
@cmbrose cmbrose requested a review from williammartin November 5, 2024 22:31
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Nov 5, 2024
@liskin
Copy link

liskin commented Nov 6, 2024

Hm, I just tried this and I'm still getting the same error :-(

$ /tmp/cli/bin/gh cs ssh -c alarming-crypt-vq9p499742wgxq --stdio -- -i codespaces.auto
error getting ssh server details: failed to read public key file: open codespaces.auto.pub: no such file or directory

(But I'll try and dig deeper and figure out what's happening. Haven't done much Go before so it'll be fun.)

@liskin
Copy link

liskin commented Nov 6, 2024

Right, it doesn't work because the private key argument to -i, as well as the output of automaticPrivateKeyPath(sshContext) are relative. If I cd ~/.ssh first, then it all works, but that's obviously not how it's meant to be used. I think we should probably use

func (c *Context) SshDir() (string, error) {
at a few more places rather than just

cli/pkg/ssh/ssh_keys.go

Lines 16 to 17 in b4c221d

type Context struct {
ConfigDir string
which is normally empty.

@cmbrose
Copy link
Member Author

cmbrose commented Nov 6, 2024

Nice catch @liskin - that was a bug from some refactoring I did for the tests actually. Fixed in 940560a

@liskin
Copy link

liskin commented Nov 6, 2024

Nice catch @liskin - that was a bug from some refactoring I did for the tests actually. Fixed in 940560a

Yep, that fixes it. (But the CI seems not to like it :-))

Copy link
Contributor

@dmgardiner25 dmgardiner25 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this up!

Copy link
Member

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Everything looks good! Made a minor change around newly added code comment but only thing.

}

// NewContextForTests creates a new `ssh.Context` with internal properties set to the
// specified values. It should only be used to testing to inject test-specific setup.
Copy link
Member

Choose a reason for hiding this comment

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

Minor readability improvement

Suggested change
// specified values. It should only be used to testing to inject test-specific setup.
// specified values. It should only be used to inject test-specific setup.

Copy link
Member

Choose a reason for hiding this comment

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

I made and pushed this up rather than block merging changes waiting for someone else to do it. 🙇

Comment on lines +192 to +195
sshDir, err := opts.sshContext.SshDir()
if err != nil {
t.Errorf("Could not load ssh config dir: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Not something that has to be fixed, but I wonder if code like this could be simplified as:

Suggested change
sshDir, err := opts.sshContext.SshDir()
if err != nil {
t.Errorf("Could not load ssh config dir: %v", err)
}
sshDir, err := opts.sshContext.SshDir()
require.NoErrorf(t, err, "Could not load ssh config dir")

Comment on lines +112 to +115
sshDir, err := opts.sshContext.SshDir()
if err != nil {
t.Errorf("Could not load ssh config dir: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sshDir, err := opts.sshContext.SshDir()
if err != nil {
t.Errorf("Could not load ssh config dir: %v", err)
}
sshDir, err := opts.sshContext.SshDir()
require.NoErrorf(t, err, "Could not load ssh config dir")

@andyfeller andyfeller enabled auto-merge November 6, 2024 21:15
@andyfeller andyfeller merged commit e356c69 into trunk Nov 6, 2024
16 checks passed
@andyfeller andyfeller deleted the cmbrose/cs-config-autokey branch November 6, 2024 21:24
@bmgzz

This comment was marked as spam.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Nov 21, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://github.com/cli/cli) | minor | `v2.59.0` -> `v2.62.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>cli/cli (cli/cli)</summary>

### [`v2.62.0`](https://github.com/cli/cli/releases/tag/v2.62.0): GitHub CLI 2.62.0

[Compare Source](cli/cli@v2.61.0...v2.62.0)

#### What's Changed

-   Update monotonic verification logic and testing by [@&#8203;malancas](https://github.com/malancas) in cli/cli#9856
-   Check extension for latest version when executed by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9866
-   Shorten extension release checking from 3s to 1s by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9914
-   Mention GitHub CLI team on discussion issues by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9920

**Full Changelog**: cli/cli@v2.61.0...v2.62.0

#### Security

-   A security vulnerability has been identified in GitHub CLI that could allow remote code execution (RCE) when users connect to a malicious Codespace SSH server and use the `gh codespace ssh` or `gh codespace logs` commands.

    For more information, see GHSA-p2h2-3vg9-4p87

#### GitHub CLI notifies users about latest extension upgrades

Similar to the notification of latest `gh` releases, the `v2.62.0` version of GitHub CLI will notify users about latest extension upgrades when the extension is used:

```shell
$ gh ado2gh
...

A new release of ado2gh is available: 1.7.0 → 1.8.0
To upgrade, run: gh extension upgrade ado2gh --force
https://github.com/github/gh-ado2gh
```

##### Why does this matter?

This removes a common pain point of extension authors as they have had to reverse engineer and implement a similar mechanism within their extensions directly.

With this quality of life improvement, there are 2 big benefits:

1.  Extension authors will hopefully see increased adoption of newer releases while having lower bar to maintaining their extensions.
2.  GitHub CLI users will have greater awareness of new features, bug fixes, and security fixes to the extensions used.

##### What do you need to do?

Extension authors should review their extensions and consider removing any custom logic previously implemented to notify users of new releases.

### [`v2.61.0`](https://github.com/cli/cli/releases/tag/v2.61.0): GitHub CLI 2.61.0

[Compare Source](cli/cli@v2.60.1...v2.61.0)

#### Ensure users understand consequences before making repository visibility changes

In `v2.61.0`, `gh repo edit` command has been enhanced to inform users about [consequences of changing visibility](https://gh.io/setting-repository-visibility) and ensure users are intentional before making irreversible changes:

1.  Interactive `gh repo edit` visibility change requires confirmation when changing from `public`, `private`, or `internal`
2.  Non-interactive `gh repo edit --visibility` change requires new `--accept-visibility-change-consequences` flag to confirm
3.  New content to inform users of consequences
    -   Incorporate [GitHub Docs content](https://gh.io/setting-repository-visibility) into help usage and interactive `gh repo edit` experience
    -   Expanded help usage to call out most concerning consequences
    -   Display repository star and watcher counts to understand impact before confirming

#### What's Changed

-   Add acceptance test for `project` command by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#9816
-   Add comprehensive testscript for `gh ruleset` by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9815
-   Add comprehensive testscript for gh ext commandset by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9810
-   Require visibility confirmation in `gh repo edit` by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9845
-   Clean up skipped online tests for `gh attestation verify` by [@&#8203;malancas](https://github.com/malancas) in cli/cli#9838
-   `gh attestation verify` should only verify provenance attestations by default by [@&#8203;malancas](https://github.com/malancas) in cli/cli#9825
-   Set `dnf5` commands as default by [@&#8203;its-miroma](https://github.com/its-miroma) in cli/cli#9844
-   Fix verbiage for deleting workflow runs by [@&#8203;akx](https://github.com/akx) in cli/cli#9876
-   Bump github.com/creack/pty from 1.1.23 to 1.1.24 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9862
-   `gh attestation verify` policy enforcement refactor by [@&#8203;malancas](https://github.com/malancas) in cli/cli#9848
-   Simplify Sigstore verification result handling in `gh attestation verify` by [@&#8203;malancas](https://github.com/malancas) in cli/cli#9877
-   Print empty array for `gh cache list` when `--json` is provided by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9883
-   Bump actions/attest-build-provenance from 1.4.3 to 1.4.4 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9884
-   Create the automatic key when specified with -i by [@&#8203;cmbrose](https://github.com/cmbrose) in cli/cli#9881
-   fix: `gh pr create -w`  ignore template flag by [@&#8203;nilvng](https://github.com/nilvng) in cli/cli#9863

#### New Contributors

-   [@&#8203;akx](https://github.com/akx) made their first contribution in cli/cli#9876
-   [@&#8203;nilvng](https://github.com/nilvng) made their first contribution in cli/cli#9863

**Full Changelog**: cli/cli@v2.60.1...v2.61.0

### [`v2.60.1`](https://github.com/cli/cli/releases/tag/v2.60.1): GitHub CLI 2.60.1

[Compare Source](cli/cli@v2.60.0...v2.60.1)

This is a small patch release to fix installing `gh` via `go install` which was broken with v2.60.0.

#### What's Changed

-   Update testscript to use hard fork by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9821

**Full Changelog**: cli/cli@v2.60.0...v2.60.1

### [`v2.60.0`](https://github.com/cli/cli/releases/tag/v2.60.0): GitHub CLI 2.60.0

[Compare Source](cli/cli@v2.59.0...v2.60.0)

#### What's Changed

-   Add ArchivedAt field by [@&#8203;tsukasaI](https://github.com/tsukasaI) in cli/cli#9790
-   Include startedAt, completedAt in run steps data by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9774
-   Adjust environment help for host and tokens by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9809
-   Add handling of empty titles for Issues and MRs by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#9701
-   `LiveSigstoreVerifier.Verify` should error if no attestations are present by [@&#8203;phillmv](https://github.com/phillmv) in cli/cli#9742
-   `gh at verify` retries fetching attestations if it receives a 5xx by [@&#8203;phillmv](https://github.com/phillmv) in cli/cli#9797
-   Prevent local extension installations with invalid names and conflicts with core commands and other extensions by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9794
-   Rewrite a sentence in CONTRIBUTING.md by [@&#8203;muzimuzhi](https://github.com/muzimuzhi) in cli/cli#9772
-   Use new GitHub preview terms in `working-with-us.md` by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9800
-   Use new GitHub previews terminology in attestation commands' help docs by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9799
-   Clarify in README that `gh` is supported on GitHub Enterprise Cloud by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9805
-   build(deps): bump github.com/gabriel-vasile/mimetype from 1.4.5 to 1.4.6 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#9752

##### Acceptance Test Changes

-   Add acceptance tests for `workflow`, `run`, and `cache` commands by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9766
-   Add basic `api` acceptance tests by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9770
-   Add acceptance tests for `release` commands by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9771
-   Add acceptance tests for `org` and `ssh-key` commands by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9812
-   Add acceptance tests for `gh auth` commands by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#9787
-   Add acceptance tests for `repo` commands by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#9783
-   Add acceptance tests for `search` command by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#9786
-   Add acceptance tests for `variable` commands by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#978
-   Add testscripts for gpg-key and label commands by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9811
-   Use forked testscript for token redaction by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9804
-   Add acceptance tests for `secret` commands by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9782
-   Note token redaction in Acceptance test README by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#9813

#### New Contributors

-   [@&#8203;tsukasaI](https://github.com/tsukasaI) made their first contribution in cli/cli#9790

**Full Changelog**: cli/cli@v2.59.0...v2.60.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gh codespace ssh --config references non-existing ssh key
6 participants