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

[ransford-stripe]

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!

[belak]

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]

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]

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]

@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]

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

[jzqiu-arista]

Hi team, just wondering if there are any plans to update or revisit this PR?