-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Card origin_devexp_286 - Added ssh key-based access to private git repository #1926
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| "use strict"; | ||
|
|
||
| describe("CreateController", function(){ | ||
| var controller, form; | ||
| var $scope = { | ||
| projectTemplates: {}, | ||
| openshiftTemplates: {}, | ||
| templatesByTag: {} | ||
| }; | ||
|
|
||
| beforeEach(function(){ | ||
| inject(function(_$controller_){ | ||
| // The injector unwraps the underscores (_) from around the parameter names when matching | ||
| controller = _$controller_("CreateController", { | ||
| $scope: $scope, | ||
| DataService: { | ||
| list: function(templates){} | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
| it("valid http URL", function(){ | ||
| var match = 'http://example.com/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid http URL, without http part", function(){ | ||
| var match = 'example.com/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
|
|
||
| it("valid http URL with user and password", function(){ | ||
| var match = 'http://user:[email protected]/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid http URL with port", function(){ | ||
| var match = 'http://example.com:8080/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid http URL with port, user and password", function(){ | ||
| var match = 'http://user:[email protected]:8080/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid https URL", function(){ | ||
| var match = 'https://example.com/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid ftp URL", function(){ | ||
| var match = 'ftp://example.com/dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("valid git+ssh URL", function(){ | ||
| var match = '[email protected]:dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("invalid git+ssh URL (double @@)", function(){ | ||
| var match = 'git@@example.com:dir1/dir2'.match($scope.sourceURLPattern) | ||
| expect(match).toBeNull(); | ||
| }); | ||
|
|
||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,19 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "io/ioutil" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
|
|
||
| "github.com/fsouza/go-dockerclient" | ||
| "github.com/golang/glog" | ||
| "github.com/openshift/origin/pkg/api/latest" | ||
| "github.com/openshift/origin/pkg/build/api" | ||
| bld "github.com/openshift/origin/pkg/build/builder" | ||
| "github.com/openshift/origin/pkg/build/builder/cmd/dockercfg" | ||
| "github.com/openshift/origin/pkg/build/builder/cmd/scmauth" | ||
| dockerutil "github.com/openshift/origin/pkg/cmd/util/docker" | ||
| image "github.com/openshift/origin/pkg/image/api" | ||
| ) | ||
|
|
@@ -19,14 +24,17 @@ const DockerCfgFile = ".dockercfg" | |
| type builder interface { | ||
| Build() error | ||
| } | ||
|
|
||
| type factoryFunc func( | ||
| client bld.DockerClient, | ||
| dockerSocket string, | ||
| authConfig docker.AuthConfiguration, | ||
| authPresent bool, | ||
| build *api.Build) builder | ||
|
|
||
| func run(builderFactory factoryFunc) { | ||
| // run is responsible for preparing environment for actual build. | ||
| // It accepts factoryFunc and an ordered array of SCMAuths. | ||
| func run(builderFactory factoryFunc, scmAuths []scmauth.SCMAuth) { | ||
| client, endpoint, err := dockerutil.NewHelper().GetClient() | ||
| if err != nil { | ||
| glog.Fatalf("Error obtaining docker client: %v", err) | ||
|
|
@@ -57,6 +65,11 @@ func run(builderFactory factoryFunc) { | |
| dockercfg.PullAuthType, | ||
| ) | ||
| } | ||
| if len(build.Parameters.Source.SourceSecretName) > 0 { | ||
| if err := setupSourceSecret(build.Parameters.Source.SourceSecretName, scmAuths); err != nil { | ||
| glog.Fatalf("Cannot setup secret file for accessing private repository: %v", err) | ||
| } | ||
| } | ||
| b := builderFactory(client, endpoint, authcfg, authPresent, &build) | ||
| if err = b.Build(); err != nil { | ||
| glog.Fatalf("Build error: %v", err) | ||
|
|
@@ -67,16 +80,83 @@ func run(builderFactory factoryFunc) { | |
|
|
||
| } | ||
|
|
||
| // fixSecretPermissions loweres access permissions to very low acceptable level | ||
| // TODO: this method should be removed as soon as secrets permissions are fixed upstream | ||
| func fixSecretPermissions() error { | ||
| secretTmpDir, err := ioutil.TempDir("", "tmpsecret") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cmd := exec.Command("cp", "-R", ".", secretTmpDir) | ||
| cmd.Dir = os.Getenv("SOURCE_SECRET_PATH") | ||
| if err := cmd.Run(); err != nil { | ||
| return err | ||
| } | ||
| secretFiles, err := ioutil.ReadDir(secretTmpDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, file := range secretFiles { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use exec.Command here as well? You can then avoid listing files using Go and just "chmod -R 0600 dir"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was faster than chmod magic, just to change contents permissions. And passing |
||
| if err := os.Chmod(filepath.Join(secretTmpDir, file.Name()), 0600); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| os.Setenv("SOURCE_SECRET_PATH", secretTmpDir) | ||
| return nil | ||
| } | ||
|
|
||
| func setupSourceSecret(sourceSecretName string, scmAuths []scmauth.SCMAuth) error { | ||
| fixSecretPermissions() | ||
| sourceSecretDir := os.Getenv("SOURCE_SECRET_PATH") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you getting this via env var when you have the value stored already in 'secretTmpDir' ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The TODO part will be removed as soon as secret permissions will be fixed. Then when removing the code between TODO's you wouldn't have to touch the rest of the code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @soltysh I will still remove this as it is confusing now. Move the os.Getenv() into comment with TODO.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also move the 'fixing' part into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a viable option. |
||
| files, err := ioutil.ReadDir(sourceSecretDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| found := false | ||
|
|
||
| SCMAuthLoop: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this necessary? can't this be done by a simple loop?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm iterating through two lists (files and secrets), without it break would just break the inner loop.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @soltysh you can have helper var that helps you break the outer loop
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the labeled break cleaner?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @soltysh it is harder to read the code ;-) |
||
| for _, scmAuth := range scmAuths { | ||
| glog.V(3).Infof("Checking for '%s' in secret '%s'", scmAuth.Name(), sourceSecretName) | ||
| for _, file := range files { | ||
| if file.Name() == scmAuth.Name() { | ||
| glog.Infof("Using '%s' from secret '%s'", scmAuth.Name(), sourceSecretName) | ||
| if err := scmAuth.Setup(sourceSecretDir); err != nil { | ||
| glog.Warningf("Error setting up '%s': %v", scmAuth.Name(), err) | ||
| continue | ||
| } | ||
| found = true | ||
| break SCMAuthLoop | ||
| } | ||
| } | ||
| } | ||
| if !found { | ||
| return fmt.Errorf("the provided secret '%s' did not have any of the supported keys %v", | ||
| sourceSecretName, getSCMNames(scmAuths)) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func getSCMNames(scmAuths []scmauth.SCMAuth) string { | ||
| var names string | ||
| for _, scmAuth := range scmAuths { | ||
| if len(names) > 0 { | ||
| names += ", " | ||
| } | ||
| names += scmAuth.Name() | ||
| } | ||
| return names | ||
| } | ||
|
|
||
| // RunDockerBuild creates a docker builder and runs its build | ||
| func RunDockerBuild() { | ||
| run(func(client bld.DockerClient, sock string, auth docker.AuthConfiguration, present bool, build *api.Build) builder { | ||
| return bld.NewDockerBuilder(client, auth, present, build) | ||
| }) | ||
| }, []scmauth.SCMAuth{&scmauth.SSHPrivateKey{}}) | ||
| } | ||
|
|
||
| // RunSTIBuild creates a STI builder and runs its build | ||
| func RunSTIBuild() { | ||
| run(func(client bld.DockerClient, sock string, auth docker.AuthConfiguration, present bool, build *api.Build) builder { | ||
| return bld.NewSTIBuilder(client, sock, auth, present, build) | ||
| }) | ||
| }, []scmauth.SCMAuth{&scmauth.SSHPrivateKey{}}) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package scmauth | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something inside me is telling me that should package should live elsewhere ("pkg/util/scm" ?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, generally speaking this will be one of those things we'll externalize when abstracting scm access, IMHO.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we want to externalize it, then there is no point on baking it here. externalizing this from its own package living in 'pkg/util' will be much easier ;-) also the naming seems wrong to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open for naming propositions, I suck at it 😉 As for location of that package, since this is tightly connected with builds, I've decided to put it here. As for externalization, I'm wondering how useful these auth methods will be for other SCM. I know mercurial supports SSH keys, but the setup might be different. If so then packages could be named then "pkg/util/scm/git/auth/" |
||
|
|
||
| // SCMAuth is an interface implemented by different authentication providers | ||
| // which are responsible for setting up the credentials to be used when accessing | ||
| // private repository. | ||
| type SCMAuth interface { | ||
| Name() string | ||
| Setup(baseDir string) error | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package scmauth | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be "pkg/util/scm/auth/key.go" |
||
|
|
||
| import ( | ||
| "io/ioutil" | ||
| "os" | ||
| "path/filepath" | ||
| ) | ||
|
|
||
| const SSHPrivateKeyMethodName = "ssh-privatekey" | ||
|
|
||
| // SSHPrivateKey implements SCMAuth interface for using SSH private keys. | ||
| type SSHPrivateKey struct{} | ||
|
|
||
| // Setup creates a wrapper script for SSH command to be able to use the provided | ||
| // SSH key while accessing private repository. | ||
| func (_ SSHPrivateKey) Setup(baseDir string) error { | ||
| script, err := ioutil.TempFile("", "gitssh") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer script.Close() | ||
| if err := script.Chmod(0711); err != nil { | ||
| return err | ||
| } | ||
| if _, err := script.WriteString("#!/bin/sh\nssh -i " + | ||
| filepath.Join(baseDir, SSHPrivateKeyMethodName) + | ||
| " -o StrictHostKeyChecking=false \"$@\"\n"); err != nil { | ||
| return err | ||
| } | ||
| // set environment variable to tell git to use the SSH wrapper | ||
| if err := os.Setenv("GIT_SSH", script.Name()); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Name returns the name of this auth method. | ||
| func (_ SSHPrivateKey) Name() string { | ||
| return SSHPrivateKeyMethodName | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more doc about what constitutes a valid secret today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I should explain how to setup a secret over here, it'll only pollute build docs with unnecessary details, which should be covered in secrets. Of course there will be some more information around that topic in our docs, but I don't think it's necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not looking for major detail, but "secret must be an SSH key for git auth" or something... our API turns out to be our docs frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.