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

feat: add env var for ssh private key #396

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Conversation

DanielleMaywood
Copy link
Contributor

Closes #333

Allow passing a base64 encoded ssh private key to ENVBUILDER_GIT_SSH_PRIVATE_KEY_BASE64 and using this for git authentication.

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I think we should also have an integration test to confirm that this works as expected. Perhaps also validating the behavior when both file and base64 is given.

@@ -273,6 +290,17 @@ func SetupRepoAuth(logf func(string, ...any), options *options.Options) transpor
}
}

// If no path was provided, fall back to the environment variable
if options.GitSSHPrivateKeyBase64 != "" {
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth thinking about what the behavior should be if both key path and key base64 is given. Do we exit immediately (invalid input)? Do we prefer one over the other? How do we inform the user?

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I think we should error if both are provided. Removes any ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with erroring, have made that change 👍

Flag: "git-ssh-private-key-base64",
Env: WithEnvPrefix("GIT_SSH_PRIVATE_KEY_BASE64"),
Value: serpent.StringOf(&o.GitSSHPrivateKeyBase64),
Description: "SSH private key to be used for Git authentication.",
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
Description: "SSH private key to be used for Git authentication.",
Description: "Base64 encoded SSH private key to be used for Git authentication. This option takes precedence over the private key path option.",

The "This option takes ..." is just there for completeness, may be rewritten depending on how the final implementation turns out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we error on having both provided, do we document this in both of the descriptions?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we should be explicit that both options are mutually exclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have documented this now 👍

@johnstcn
Copy link
Member

I think we should also have an integration test to confirm that this works as expected. Perhaps also validating the behavior when both file and base64 is given.

There's a test SSH server implementation you can probably re-use here: https://github.com/coder/terraform-provider-envbuilder/blob/main/internal/provider/git_test.go#L85

@mafredri
Copy link
Member

There's a test SSH server implementation you can probably re-use here: coder/terraform-provider-envbuilder@main/internal/provider/git_test.go#L85

Yep, that should work. I believe only the PublicKeyHandler implementation needs to be updated to reject/accept the key.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Once we have an integration test, this will be good to go! 👍

@DanielleMaywood
Copy link
Contributor Author

I've added an integration test. It works similar to the existing tests in git_test.go. Rather than checking it has worked without error (as the current git ssh server impl doesn't appear to work properly), it instead checks that the error is one that comes after auth has ran.

envbuilder/git/git_test.go

Lines 266 to 268 in 58ac15f

// TODO: ideally, we want to test the entire cloning flow.
// For now, this indicates successful ssh key auth.
require.ErrorContains(t, err, "repository not found")

@DanielleMaywood DanielleMaywood merged commit 08bdb8d into main Oct 25, 2024
4 checks passed
@DanielleMaywood DanielleMaywood deleted the dm-git-ssh-key-env branch October 25, 2024 14:05
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.

Allow passing Git SSH key as an environment variable
3 participants