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

Support OpenSSH private key format for Git SSH authentication #3103

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions internal/cmd/cli/apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ func TestAddAuthToOpts(t *testing.T) {
expectedOpts: &apply.Options{},
expectedErr: errorNotFound,
},
"Auth contains values from OpenSSH private key when provided": {
apply: Apply{SSHPrivateKeyFile: "openssh_private_key_file"},
expectedOpts: &apply.Options{Auth: bundlereader.Auth{SSHPrivateKey: []byte("openssh_private_key_content")}},
expectedErr: nil,
},
}

for name, test := range tests {
Expand Down Expand Up @@ -92,6 +97,8 @@ func mockReadFile(name string) ([]byte, error) {
return []byte(caCerts_content), nil
case sshPrivateKey_file:
return []byte(sshPrivateKey_content), nil
case "openssh_private_key_file":
return []byte("openssh_private_key_content"), nil
}

return nil, errorNotFound
Expand Down
7 changes: 6 additions & 1 deletion internal/cmd/cli/gitcloner/cloner.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
giturls "github.com/rancher/fleet/pkg/git-urls"
"github.com/sirupsen/logrus"
"golang.org/x/crypto/ssh"
"golang.org/x/crypto/ssh/agent"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we'd need this new import 🤔

)

const defaultBranch = "master"
Expand Down Expand Up @@ -130,7 +131,11 @@ func createAuthFromOpts(opts *GitCloner) (transport.AuthMethod, error) {
}
auth, err := gossh.NewPublicKeys(gitURL.User.Username(), privateKey, "")
if err != nil {
return nil, err
// Try to parse as OpenSSH private key
auth, err = gossh.NewPublicKeys(gitURL.User.Username(), privateKey, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why retry the exact same operation as the one that has just failed?

if err != nil {
return nil, err
}
}
if opts.KnownHostsFile != "" {
knownHosts, err := readFile(opts.KnownHostsFile)
Expand Down
30 changes: 30 additions & 0 deletions internal/cmd/cli/gitcloner/cloner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ silO6We5JggtPJICaCCpVawmIJIx3pWMjB+StXfJHoilknkb+ecQF+ofFsUqPb9r
Rn4jGwVFnYAeVq4tj3ECQQCyeMeCprz5AQ8HSd16Asd3zhv7N7olpb4XMIP6YZXy
udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
-----END RSA PRIVATE KEY-----`
opensshPrivateKeyFile = "opensshFile"
//nolint:gosec // it's only test data
opensshPrivateKeyFileContent = `-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS
1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQTi1hnnqv3tZiv8x+O1HkHjLXYfjRM+
o+8aeXACDXFdqT1oALLsoeXbpjL9UZAta69KZlnpAqDJ0dnOPj5ZUaq0AAAAsLX33E2199
xNAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOLWGeeq/e1mK/zH
47UeQeMtdh+NEz6j7xp5cAINcV2pPWgAsuyh5dumMv1RkC1rr0pmWekCoMnR2c4+PllRqr
QAAAAhAOZAKlM42hgAOsRnvRk/wp1mYy+raMO2p05D9BaLcD7oAAAAEXJvb3RAOTQ0YmM1
OTIyYTI4AQIDBAUG
-----END OPENSSH PRIVATE KEY-----`
)
var (
pathCalled string
Expand All @@ -42,6 +53,7 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
)

sshAuth, _ := gossh.NewPublicKeys("git", []byte(sshPrivateKeyFileContent), "")
opensshAuth, _ := gossh.NewPublicKeys("git", []byte(opensshPrivateKeyFileContent), "")
sshKeyComparer := cmp.Comparer(func(x, y gossh.PublicKeys) bool {
return x.User == y.User &&
x.Signer.PublicKey().Type() == y.Signer.PublicKey().Type() &&
Expand All @@ -64,6 +76,9 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
if name == sshPrivateKeyFile {
return []byte(sshPrivateKeyFileContent), nil
}
if name == opensshPrivateKeyFile {
return []byte(opensshPrivateKeyFileContent), nil
}
return nil, errors.New("file not found")
}
defer func() {
Expand Down Expand Up @@ -123,6 +138,21 @@ udiSlDctMM/X3ZM2JN5M1rtAJ2WR3ZQtmWbOjZAbG2Eq
RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
},
},
"branch openssh auth": {
opts: &GitCloner{
Repo: "ssh://git@localhost/test/test-repo",
Path: "path",
Branch: "master",
SSHPrivateKeyFile: opensshPrivateKeyFile,
},
expectedCloneOpts: &git.CloneOptions{
URL: "ssh://git@localhost/test/test-repo",
SingleBranch: true,
ReferenceName: "master",
Auth: opensshAuth,
RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
},
},
"password file does not exist": {
opts: &GitCloner{
Repo: "repo",
Expand Down
9 changes: 7 additions & 2 deletions internal/cmd/cli/gitcloner/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type GitCloner struct {
SSHPrivateKeyFile string
InsecureSkipTLS bool
KnownHostsFile string
OAuthToken string
}

var opts *GitCloner
Expand All @@ -27,7 +28,10 @@ func NewCmd(gitCloner CloneGit) *cobra.Command {
cmd := &cobra.Command{
Use: "gitcloner [REPO] [PATH]",
Short: "Clones a git repository",
Args: cobra.ExactArgs(2),
Long: `The gitcloner command clones a git repository to a specified path.
It supports various authentication methods, including basic auth, SSH private keys, and OAuth tokens.
You can also specify a default branch other than "master" if none is provided.`,
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
opts.Repo = args[0]
opts.Path = args[1]
Expand All @@ -36,14 +40,15 @@ func NewCmd(gitCloner CloneGit) *cobra.Command {
},
}
opts = &GitCloner{}
cmd.Flags().StringVarP(&opts.Branch, "branch", "b", "", "git branch")
cmd.Flags().StringVarP(&opts.Branch, "branch", "b", "", "git branch (default is 'master')")
cmd.Flags().StringVar(&opts.Revision, "revision", "", "git revision")
cmd.Flags().StringVar(&opts.CABundleFile, "ca-bundle-file", "", "CA bundle file")
cmd.Flags().StringVarP(&opts.Username, "username", "u", "", "user name for basic auth")
cmd.Flags().StringVar(&opts.PasswordFile, "password-file", "", "password file for basic auth")
cmd.Flags().StringVar(&opts.SSHPrivateKeyFile, "ssh-private-key-file", "", "ssh private key file path")
cmd.Flags().BoolVar(&opts.InsecureSkipTLS, "insecure-skip-tls", false, "do not verify tls certificates")
cmd.Flags().StringVar(&opts.KnownHostsFile, "known-hosts-file", "", "known hosts file")
cmd.Flags().StringVar(&opts.OAuthToken, "oauth-token", "", "OAuth token for authentication")
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this option used?


return cmd
}
63 changes: 63 additions & 0 deletions internal/cmd/cli/gitcloner/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,69 @@ func TestArgsAreSet(t *testing.T) {
}
}

func TestArgsAreSetWithOpenSSHKey(t *testing.T) {
mock := &clonerMock{}
cmd := NewCmd(mock)
cmd.SetArgs([]string{"test-repo", "test-path", "--branch", "master", "--revision", "v0.1.0", "--ca-bundle-file", "caFile", "--username", "user",
"--password-file", "passwordFile", "--ssh-private-key-file", "opensshFile", "--insecure-skip-tls", "--known-hosts-file", "knownFile"})
err := cmd.Execute()
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if mock.opts.Repo != "test-repo" {
t.Fatalf("expected repo test-repo, got %v", mock.opts.Repo)
}
if mock.opts.Path != "test-path" {
t.Fatalf("expected path test-path, got %v", mock.opts.Path)
}
if mock.opts.Branch != "master" {
t.Fatalf("expected branch master, got %v", mock.opts.Branch)
}
if mock.opts.Revision != "v0.1.0" {
t.Fatalf("expected revision v0.1.0, got %v", mock.opts.Revision)
}
if mock.opts.CABundleFile != "caFile" {
t.Fatalf("expected CABundleFile caFile, got %v", mock.opts.CABundleFile)
}
if mock.opts.Username != "user" {
t.Fatalf("expected Username user, got %v", mock.opts.Username)
}
if mock.opts.PasswordFile != "passwordFile" {
t.Fatalf("expected PasswordFile passwordFile, got %v", mock.opts.PasswordFile)
}
if !mock.opts.InsecureSkipTLS {
t.Fatalf("expected InsecureSkipTLS to be true")
}
if mock.opts.KnownHostsFile != "knownFile" {
t.Fatalf("expected KnownHostsFile knownFile, got %v", mock.opts.KnownHostsFile)
}
if mock.opts.SSHPrivateKeyFile != "opensshFile" {
t.Fatalf("expected SSHPrivateKeyFile opensshFile, got %v", mock.opts.SSHPrivateKeyFile)
}
}

func TestArgsAreSetWithOAuthToken(t *testing.T) {
mock := &clonerMock{}
cmd := NewCmd(mock)
cmd.SetArgs([]string{"test-repo", "test-path", "--branch", "main", "--oauth-token", "test-token"})
err := cmd.Execute()
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if mock.opts.Repo != "test-repo" {
t.Fatalf("expected repo test-repo, got %v", mock.opts.Repo)
}
if mock.opts.Path != "test-path" {
t.Fatalf("expected path test-path, got %v", mock.opts.Path)
}
if mock.opts.Branch != "main" {
t.Fatalf("expected branch main, got %v", mock.opts.Branch)
}
if mock.opts.OAuthToken != "test-token" {
t.Fatalf("expected OAuthToken test-token, got %v", mock.opts.OAuthToken)
}
}

type clonerMock struct {
opts *GitCloner
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/git/netutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ func GetAuthFromSecret(url string, creds *corev1.Secret) (transport.AuthMethod,
}
auth, err := gossh.NewPublicKeys(gitURL.User.Username(), creds.Data[corev1.SSHAuthPrivateKey], "")
if err != nil {
return nil, err
// Try to parse as OpenSSH private key
auth, err = gossh.NewPublicKeys(gitURL.User.Username(), creds.Data[corev1.SSHAuthPrivateKey], "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark as in cloner.go: Why retry the exact same operation as the one that has just failed?

if err != nil {
return nil, err
}
}
if creds.Data["known_hosts"] != nil {
auth.HostKeyCallback, err = newCreateKnownHosts(creds.Data["known_hosts"])
Expand Down
30 changes: 30 additions & 0 deletions pkg/git/netutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,36 @@ YcwLYudAztZeA/A4aM5Y0MA6PlNIeoHohuMkSZNOBcvkNEWdzGBpKb34yLfMarNm
Expect(err.Error()).To(ContainSubstring("knownhosts: missing host pattern"))
})
})

Context("SSH auth with valid OpenSSH private key and no known_hosts", func() {
var privateKey = []byte(`-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS
1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQTi1hnnqv3tZiv8x+O1HkHjLXYfjRM+
o+8aeXACDXFdqT1oALLsoeXbpjL9UZAta69KZlnpAqDJ0dnOPj5ZUaq0AAAAsLX33E2199
xNAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOLWGeeq/e1mK/zH
47UeQeMtdh+NEz6j7xp5cAINcV2pPWgAsuyh5dumMv1RkC1rr0pmWekCoMnR2c4+PllRqr
QAAAAhAOZAKlM42hgAOsRnvRk/wp1mYy+raMO2p05D9BaLcD7oAAAAEXJvb3RAOTQ0YmM1
OTIyYTI4AQIDBAUG
-----END OPENSSH PRIVATE KEY-----`)
BeforeEach(func() {
secret = &corev1.Secret{
Type: corev1.SecretTypeSSHAuth,
Data: map[string][]byte{
corev1.SSHAuthPrivateKey: privateKey,
},
}
})
It("returns no error and the ssh auth", func() {
auth, err := git.GetAuthFromSecret("[email protected]:rancher/fleet.git", secret)
Expect(err).ToNot(HaveOccurred())
expectedSigner, err := ssh.ParsePrivateKey(privateKey)
Expect(err).ToNot(HaveOccurred())
pk, ok := auth.(*gossh.PublicKeys)
Expect(ok).To(BeTrue())
Expect(pk.User).To(Equal("git"))
Expect(pk.Signer).To(Equal(expectedSigner))
})
})
})

var _ = Describe("git's GetHTTPClientFromSecret tests", func() {
Expand Down