feat: containers ssh command
#10582
feat: containers ssh command
#10582
Conversation
🦋 Changeset detectedLatest commit: e781205 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
This PR is a requirement for SSH for containers After this is merged, we will update this PR |
Wrangler PR: cloudflare/workers-sdk#10582 Signed-off-by: flakey5 <[email protected]>
Wrangler PR: cloudflare/workers-sdk#10582 Signed-off-by: flakey5 <[email protected]>
| ); | ||
| } | ||
|
|
||
| if (key.public_key.toLowerCase().startsWith("ssh-ed25519")) { |
There was a problem hiding this comment.
Not sure if we wanna have this check here, it's not unlikely that we end up supporting more key types and this would make it so you'd need to update wrangler in order to use them
emily-shen
force-pushed
the
flakey5/cc-4634
branch
from
feedecb to
22f6eff
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
flakey5
force-pushed
the
flakey5/cc-4634
branch
2 times, most recently
from
c0a8856 to
0f3402b
Compare
emily-shen
force-pushed
the
flakey5/cc-4634
branch
from
128659d to
9ef10ba
Compare
|
What is the status here? |
I don't believe I have write access to the branch anymore, but this feature shouldn't need any more changes on the container's side of things so this pr should be the only thing left to get this feature added in iirc |
|
@flakey5 did you manage to test this e2e before you left? i'm having trouble getting it to work with various also @vicb - for context @flakey5's internship finished, so I can take over making sure the PR is ready to be merged (that's why I rebased) - there shouldn't be any new changes needed. |
I was able to get it working, |
Signed-off-by: flakey5 <[email protected]>
Co-authored-by: emily-shen <[email protected]> Co-authored-by: Victor Berchet <[email protected]>
Co-authored-by: emily-shen <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Co-authored-by: Nikita Sharma <[email protected]>
Signed-off-by: flakey5 <[email protected]>
emily-shen
force-pushed
the
flakey5/cc-4634
branch
from
9ef10ba to
a29c077
Compare
|
the plan is to merge this mostly as is but hidden as experimental (which i have done here a29c077). |
emily-shen
force-pushed
the
flakey5/cc-4634
branch
from
bcb989b to
65ea1ef
Compare
this code will only ever be used by wrangler and not vite/miniflare so it doesn't have to be in containers shared.
emily-shen
left a comment
emily-shen
left a comment
There was a problem hiding this comment.
approving with my containers hat on but would like a wrangler team approval too.
| .option("cipher", { | ||
| describe: | ||
| "SSH option: Selects the cipher specification for encrypting the session", | ||
| "Sets `ssh -c`: Select the cipher specification for encrypting the session", | ||
| type: "string", | ||
| }) | ||
| .option("log-file", { | ||
| describe: | ||
| "SSH option: Append debug logs to log_file instead of standard error", | ||
| "Sets `ssh -E`: Append debug logs to log_file instead of standard error", | ||
| type: "string", | ||
| }) | ||
| .option("escape-char", { | ||
| describe: | ||
| "SSH option: Sets the escape character for sessions with a pty (default: ‘~’)", | ||
| "Sets `ssh -e`: Set the escape character for sessions with a pty (default: ‘~’)", | ||
| type: "string", | ||
| }) | ||
| .option("config-file", { | ||
| describe: | ||
| "SSH option: Specifies an alternative per-user configuration file", | ||
| "Sets `ssh -F`: Specify an alternative per-user ssh configuration file", | ||
| type: "string", | ||
| }) | ||
| .option("pkcs11", { | ||
| describe: | ||
| "SSH option: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication", | ||
| "`Sets `ssh -I`: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication", | ||
| type: "string", | ||
| }) | ||
| .option("identity-file", { | ||
| describe: | ||
| "SSH option: Selects a file from which the identity (private key) for public key authentication is read", | ||
| "Sets `ssh -i`: Select a file from which the identity (private key) for public key authentication is read", | ||
| type: "string", | ||
| }) | ||
| .option("mac-spec", { | ||
| describe: | ||
| "SSH option: A comma-separated list of MAC (message authentication code) algorithms, specified in order of preference", | ||
| "Sets `ssh -m`: A comma-separated list of MAC (message authentication code) algorithms, specified in order of preference", | ||
| type: "string", | ||
| }) | ||
| .option("option", { | ||
| describe: | ||
| "SSH option: Can be used to give options in the format used in the configuration file", | ||
| "Sets `ssh -o`: Can be used to give options in the format used in the ssh configuration file", | ||
| type: "string", | ||
| }) | ||
| .option("tag", { | ||
| describe: | ||
| "SSH option: Specify a tag name that may be used to select configuration in ssh_config(5)", | ||
| "Sets `ssh -P`: Specify a tag name that may be used to select configuration in ssh_config", | ||
| type: "string", | ||
| }) |
There was a problem hiding this comment.
I think all of these options should accept single character aliases (where possible) that match the flags in SSH, which would make the "Sets `ssh -X`" text redundant. I had planned to do that in a follow up PR.
Up to you if you think it's worth changing now before merging. Personally I'd like to get this merged sooner to unblock other work and fixup minor nits and papercuts in follow ups.
There was a problem hiding this comment.
pasting this here as well posterity: i'm going to merge this pr as is once CI passes, and then you can do a followup PR?
the short flags is what aaron had originally, but some of them conflicted with wrangler's global flags. i think for consistency aaron went with just the full option names for everything. wdyt?
There was a problem hiding this comment.
Hmm I do think at the very least we should provide aliases for -o, -F, and -i, as those are fairly common when using the "real" ssh and it would be nice to support those in our wrapper too.
AFAICT -c and -e are the only conflicting flags, which correspond to --cipher and --escape-char. I'm perfectly ok with not providing aliases for those options since they're relatively more rare. That does add a bit of inconsistency since not all of the options have short flags, but that's also not exactly unusual with CLI tools.
There was a problem hiding this comment.
that's fair, lets add them back in then! idk what the best phrasing is, but the "Sets ssh -o" prefixes was trying to convey that these flags just pass through to the 'real ssh' flags, so it would be good to have something that indicates that still.
7 participants
Closes CC-4634
Adds a
containers sshcommand +sshandauthorized_keysproperties to a container's configuration. This allows for ssh'ing into a container through wrangler.[x] Currently not working since it relies on pending api gateway PRsworks now