feat: fetch SSH key from Coder if required #174

johnstcn · 2024-05-03T16:31:59Z

Relates to #31

Depends on #173

Modifies the SSH logic introduced in #170 with an extra step to fetch the SSH key from Coder.

There is currently a race condition between envbuilder starting and the workspace build being marked as 'completed'. To work around this, I added some backoff to fetching the SSH key using the agent token.

Demo:

envbuilder-ssh-auth-coder.mov

johnstcn · 2024-05-03T18:24:42Z

git.go

+ if errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusUnauthorized {
+ // Retry, as this may just mean that the workspace build has not yet
+ // completed.
+ log(codersdk.LogLevelInfo, "#1: 🕐 Backing off as the workspace build has not yet completed...")
+ return err
+ }
+ close(signerChan)


review: I'm open to adding more cases here, but keeping the retry scope small for now.

johnstcn · 2024-05-03T18:25:00Z

git.go

+// keyFingerprint returns the md5 checksum of the public key of signer.
+func keyFingerprint(s gossh.Signer) string {
+ h := md5.New()
+ h.Write(s.PublicKey().Marshal())
+ return fmt.Sprintf("%x", h.Sum(nil))
+}


This is an unrelated change, but I figured it would be nice to see

johnstcn · 2024-05-03T18:25:36Z

git.go

+ signerChan := make(chan gossh.Signer, 1)
+ eb := backoff.NewExponentialBackOff()
+ eb.MaxElapsedTime = 0
+ eb.MaxInterval = time.Minute


review: should this maybe be a configurable knob?

matifali · 2024-05-03T18:37:04Z

Is it related to #92?

kylecarbs · 2024-05-03T18:38:07Z

git.go

+ if signer == nil && options.CoderAgentURL != "" && options.CoderAgentToken != "" {
+ options.Logger(codersdk.LogLevelInfo, "#1: 🔑 Fetching key from %s!", options.CoderAgentURL)
+ fetchCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
+ s, err := FetchCoderSSHKeyRetry(fetchCtx, options.Logger, options.CoderAgentURL, options.CoderAgentToken)
+ if err == nil {
+ signer = s
+ options.Logger(codersdk.LogLevelInfo, "#1: 🔑 Fetched %s key %s !", signer.PublicKey().Type(), keyFingerprint(signer)[:8])
+ } else {
+ options.Logger(codersdk.LogLevelInfo, "#1: ❌ Failed to fetch SSH key from %s: %w", options.CoderAgentURL, err)
 }


Deeply integrating the hot-path code with Coder feels incorrect to me.

I don't believe the envbuilder package should have any knowledge of the Coder agent or Coder URL. Prior to the CLI change, it didn't. The command-line simply logged to Coder as a sink if those vars were provided, and I think implementing this similarly abstractly would be an improvement.

We want people outside of the Coder ecosystem to use envbuilder. By integrating the codebase so heavily into Coder, we alienate that possibility.

My preference would be to allow passing SSH options to envbuilder, and also not provide Coder options directly to envbuilder.

How about instead exposing some 'lifecycle hooks' inside envbuilder?
For example, we could expose a 'pre-clone' hook that executes a given script before cloning the git repository. For this use-case, that script could curl | jq to get the git private key. It would require modifying the envbuilder image, but I think this provides the decoupling and flexibility you're looking for.

Adopting "lifecycle hooks" is a reasonable concept, we may argue where they should be scripts or just shell commands. Maybe we can implement them similarly to Git Hooks?

Any thoughts as to what kind of hook, except pre-clone, we might need in envbuilder? Running scripts is generally already addressable by devcontainer.json.

Yeah, I think you're right that another data source for the coder_user is good, but shouldn't block this effort.

Mentioning that in coder/terraform-provider-coder#217

If we added a new data source to the coder provider, I believe we can address this similarly to what @kylecarbs suggested:

container { env = { GIT_SSH_KEY: basy64(data.coder_workspace.owner_ssh_key) } }

👍 That's certainly a valid path forward.

Based on what was implemented in #170, there would be a few extra steps:

Create a secret containing the user's private key

Mount that secret into the container under a well-known path

Reference that path in the container's environment

Will open a separate PR for this.

@matifali it's funny you should mention that, it was my initial thought as well, but then I got lazy 😄. Perhaps we could move this to a separate issue in https://github.com/coder/terraform-provider-coder?

Already moved. See
coder/terraform-provider-coder#219

nwrkbiz · 2024-05-05T10:20:53Z

Awesome, this is was I am waiting for since a while 🙏🏻

mtojek

I'm happy with exploring the idea for hooks unless #31 is bleeding hard (urgent).

mtojek · 2024-05-06T08:24:10Z

git.go

+ if signer == nil && options.CoderAgentURL != "" && options.CoderAgentToken != "" {
+ options.Logger(codersdk.LogLevelInfo, "#1: 🔑 Fetching key from %s!", options.CoderAgentURL)
+ fetchCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
+ s, err := FetchCoderSSHKeyRetry(fetchCtx, options.Logger, options.CoderAgentURL, options.CoderAgentToken)
+ if err == nil {
+ signer = s
+ options.Logger(codersdk.LogLevelInfo, "#1: 🔑 Fetched %s key %s !", signer.PublicKey().Type(), keyFingerprint(signer)[:8])
+ } else {
+ options.Logger(codersdk.LogLevelInfo, "#1: ❌ Failed to fetch SSH key from %s: %w", options.CoderAgentURL, err)
 }


Adopting "lifecycle hooks" is a reasonable concept, we may argue where they should be scripts or just shell commands. Maybe we can implement them similarly to Git Hooks?

johnstcn · 2024-05-09T08:11:36Z

Thanks @mafredri @matifali for your excellent input and comments.

The overwhelming consensus here is to avoid a direct integration and instead allow injecting a Coder user's SSH private key into the envbuilder container. There should be no further changes in envbuilder required to support this. Closing this PR out in favour of coder/terraform-provider-coder#219

johnstcn mentioned this pull request May 3, 2024

SSH repositories fail to configure with coder #31

Closed

johnstcn marked this pull request as draft May 3, 2024 17:10

Base automatically changed from cj/fix-coder-url to main May 3, 2024 17:21

johnstcn added 4 commits May 3, 2024 17:58

feat: fetch SSH key from Coder if required

6d76aca

fix up after rebase

efd51b6

update README

b1ba280

retry fetching agent ssh key

19eed21

johnstcn force-pushed the cj/coder-ssh branch from 9e13995 to 19eed21 Compare May 3, 2024 17:58

johnstcn marked this pull request as ready for review May 3, 2024 18:21

johnstcn self-assigned this May 3, 2024

johnstcn requested review from mafredri, dannykopping, mtojek and BrunoQuaresma May 3, 2024 18:22

johnstcn commented May 3, 2024

View reviewed changes

kylecarbs reviewed May 3, 2024

View reviewed changes

mtojek reviewed May 6, 2024

View reviewed changes

matifali mentioned this pull request May 6, 2024

Add coder_workspace_owner data source coder/terraform-provider-coder#219

Closed

bpmct mentioned this pull request May 6, 2024

Coder Terraform Provider v1.0 Docs coder/terraform-provider-coder#217

Closed

29 tasks

johnstcn closed this May 9, 2024

johnstcn mentioned this pull request Sep 2, 2024

Allow passing Git SSH key as an environment variable #333

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fetch SSH key from Coder if required #174

feat: fetch SSH key from Coder if required #174

johnstcn commented May 3, 2024 •

edited

Loading

johnstcn May 3, 2024

johnstcn May 3, 2024

johnstcn May 3, 2024

matifali commented May 3, 2024 •

edited

Loading

kylecarbs May 3, 2024

kylecarbs May 3, 2024

johnstcn May 3, 2024 •

edited

Loading

mtojek May 6, 2024

mafredri May 6, 2024

bpmct May 6, 2024

bpmct May 6, 2024

johnstcn May 7, 2024

mafredri May 7, 2024

matifali May 8, 2024

nwrkbiz commented May 5, 2024

mtojek left a comment

mtojek May 6, 2024

johnstcn commented May 9, 2024

feat: fetch SSH key from Coder if required #174

feat: fetch SSH key from Coder if required #174

Conversation

johnstcn commented May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matifali commented May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwrkbiz commented May 5, 2024

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn commented May 9, 2024

johnstcn commented May 3, 2024 •

edited

Loading

matifali commented May 3, 2024 •

edited

Loading

johnstcn May 3, 2024 •

edited

Loading