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

sshd test #38

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

sshd test #38

wants to merge 6 commits into from

Conversation

till
Copy link

@till till commented Jun 18, 2023

  • Chore: upgrade go to 1.19
  • Chore: update actions
  • Update: allow push to certain branches
  • Chore: address feedback from CR
  • Refactor: move key to its own struct
  • Update: callback for repositories

till added 5 commits June 16, 2023 17:30
This extends MasterOnly and makes it configurable. The idea is to
set arbitrary branch names which are allowed to be pushed to and
otherwise return an error.

Resolves: sosedoff#33
- renamed AllowedBranches to AllowedRefs
- use t.Run() in tests
This is to be able to re-use the code when you e.g. supply your own
listener for the sshd server and the code skips all the internal
setup.
@till
Copy link
Author

till commented Jun 18, 2023

@sosedoff Hey, I can rebase this after #34 — I did a few more things and leave comments inline. Let me know if you'd like to include this.

"golang.org/x/crypto/ssh"
)

type Key struct {
Copy link
Author

Choose a reason for hiding this comment

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

Mostly extracted this as supplying your own listener makes the code inaccessible.

sshconfig *ssh.ServerConfig
config *Config
PublicKeyLookupFunc func(string) (*PublicKey, error)
ReposForKeyLookupFunc func(*PublicKey) ([]string, error)
Copy link
Author

Choose a reason for hiding this comment

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

Added another callback, the idea is to be able to do access control. It seemed like, right now, once a key has been authenticated via PublicKeyLookupFunc, there's little I can do for ACL in the code.

}
}

return &ssh.Permissions{
Copy link
Author

Choose a reason for hiding this comment

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

The biggest change here is that I am putting everything available into the extension, so the consumer can use it downstream.

}

go ssh.DiscardRequests(reqs)
go s.handleConnection(keyId, chans)
go s.handleConnection(exts, chans)
Copy link
Author

Choose a reason for hiding this comment

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

Injects everything into handleConnection to make the context available to the hooks.

ssh.go Outdated
cmd.Env = append(os.Environ(), "GITKIT_KEY="+keyID)

envVariables := os.Environ()
envVariables = append(envVariables, "GITKIT_CURRENT_REPOSITORY="+gitcmd.Repo)
Copy link
Author

Choose a reason for hiding this comment

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

Add current repository to the hook (this is new). I just found it to be convenient to know when the receiver executes.

// append data via ssh.Permissions.Extensions
for k, v := range exts {
log.Println("k=" + k + ", v=" + v)
envVariables = append(envVariables, "GITKIT_"+strings.ToUpper(k)+"="+v)
Copy link
Author

Choose a reason for hiding this comment

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

This line exposes everything from the extensions on to the receiver via GITKIT_ variables. There's also a small BC break in here, before the key was in GITKIT_KEY, now it's GITKIT_KEYID. I can work around it though if you feel strongly about it.

@@ -249,6 +220,10 @@ func (s *SSH) setup() error {
return fmt.Errorf("public key lookup func is not provided")
}

if s.ReposForKeyLookupFunc == nil {
log.Println("no repository callback, an authorized user may access any repositories")
Copy link
Author

Choose a reason for hiding this comment

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

Message to let the user know that once authenticated, anything goes. I think this is inline with how it currently works. Unless you overwrite a bunch of things and supply your own ssh config.

@@ -80,7 +76,7 @@ func execCommand(cmdname string, args ...string) (string, string, error) {
return string(bufOut), string(bufErr), err
}

func (s *SSH) handleConnection(keyID string, chans <-chan ssh.NewChannel) {
func (s *SSH) handleConnection(exts map[string]string, chans <-chan ssh.NewChannel) {
Copy link
Author

Choose a reason for hiding this comment

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

In theory, and not sure what your preference is... but somewhere in handleConnection, I think it should also do authz for the repositories. But one can also do it in the git-receive hook later. But that seems a bit odd.

- adds a callback for ACL (to return the list of repositories for the current user)
- adds all public key data to extensions
- exposes the extensions via environment variables (GITKIT_) to the hook
@till
Copy link
Author

till commented Jun 25, 2023

@sosedoff Lmk if you want this, I kinda gave up on this as the server is too much work. Started using this one instead: https://github.com/charmbracelet/wish

Had too many issues authenticating, e.g. with agent forwarding, every key is validated and it will consistently fail. And then there are a bunch of other issues that would need to be implemented.

Would probably refactor everything in gitkit to use it if you want. Otherwise, it's okay if you'd rather not.

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.

1 participant