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

Make Context conform to "x/crypto/ssh".ConnMetadata interface #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ransford-stripe
Copy link

@ransford-stripe ransford-stripe commented Oct 21, 2019

This package's Context interface is almost, but not quite, identical to x/crypto/ssh's ConnMetadata interface. Seeing how similar they are, I wonder if the differences are unintentional.

I'm pretty sure anyone implementing certificate-based user authentication will run into this interface mismatch while trying to plumb connection details through to a CertChecker, because code like the simple example below fails to typecheck:

    // gossh refers to x/crypto/ssh
    // ssh refers to gliderlabs/ssh

    checker := gossh.CertChecker{
        IsUserAuthority: func(pub gossh.PublicKey) bool {
            // our own logic to check whether pub is acceptable
            return true
        }
    }

    func isUserCertOK(ctx ssh.Context, key ssh.PublicKey) bool {
        if cert, ok := key.(*gossh.Certificate); ok {
            // ctx must satisfy ConnMetadata or else this fails to compile
            _, err := checker.Authenticate(ctx, cert)
            return err == nil
        }
        // return isNonCertificateSshKeyOK(ctx, key)
    }

    sshServer := &ssh.Server{
        // ...
        PublicKeyHandler: isUserCertOK,
    }

This PR slightly changes the Context interface so that an sshContext conforms to ConnMetadata. Specifically:

  • Make ClientVersion(), ServerVersion(), and SessionID() return []byte instead of string
  • Add a type assertion to context_test.go

This is a breaking change, so I'll leave it to the maintainers to decide if and when to merge.

Thanks for this great library!

It's useful for this package's sshContext to implement ConnMetadata when you
want to authenticate user certificates from a PublicKeyHandler.

Somewhat realistic example of user certificate checking after this patch:

    checker := gossh.CertChecker{
        IsUserAuthority: func(pub gossh.PublicKey) bool {
            // our own logic to check whether pub is acceptable
            return true
        }
    }

    func isUserCertOk(ctx ssh.Context, key ssh.PublicKey) bool {
        if cert, ok := key.(*gossh.Certificate); ok {
            _, err := checker.Authenticate(ctx, cert)
        }
    }

    sshServer := &ssh.Server{
        // ...
        PublicKeyHandler: isUserCertOK,
    }
@ransford-stripe ransford-stripe changed the title Make Context implement "x/crypto/ssh".ConnMetadata interface Make Context conform to "x/crypto/ssh".ConnMetadata interface Oct 21, 2019
@belak
Copy link
Collaborator

belak commented Oct 21, 2019

Oof, yeah. I'm not sure what to do about this - lots of the things in this library were done for convenience and I assume the []byte to string was intentional, but @progrium may have a better idea.

Good call-out and solid PR, but you're right that it is a breaking change...

@ransford-stripe
Copy link
Author

Better to have a robust user base and to wring hands about breaking changes than to have no users at all!

FWIW, here is a minimal workaround that wraps this package's Context (as it exists in the latest master) just enough to pass as ConnMetadata:

type gContext ssh.Context
type gContextWrapper struct{ gContext }
func (s *gContextWrapper) ClientVersion() []byte { return []byte(s.gContext.ClientVersion()) }
func (s *gContextWrapper) ServerVersion() []byte { return []byte(s.gContext.ServerVersion()) }
func (s *gContextWrapper) SessionID() []byte {
	dec, _ := hex.DecodeString(s.gContext.SessionID())
	return dec
}
// now when you need a ConnMetadata but have only a ssh.Context, you can call:
// wctx := &gContextWrapper{ctx}
// _, err := checker.Authenticate(wctx, cert)

@shazow
Copy link
Member

shazow commented Oct 22, 2019

One upside of returning a []byte rather than a string is it makes it clearer that the data is not necessarily display-safe. It's easy to inject all kinds of escape codes and whatnot in there, so sanitization is required.

@ransford-stripe
Copy link
Author

@belak this seems like it would pair nicely with #122 in a breaking-changes release tagged 0.3. @progrium did you have an opinion on this PR per Kaleb's comment?

@belak
Copy link
Collaborator

belak commented Dec 16, 2019

Yes, that seems reasonable. I’ll try to take another look later today.

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.

3 participants