crypto
crypto copied to clipboard
golang
ssh: add ssh partial success for ssh server 2FA
Adds ssh partial success error, and next auth methods call back, When auth methods call back return ErrPartialSuccess, then let client continue use nextAuthMethodsCallback return methods auth farther
I don't know if match RFC or not, Please check
#https://github.com/golang/go/issues/17889 #https://github.com/golang/go/issues/17934
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.
What to do if you already signed the CLA
Individual signers
- It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
Corporate signers
- Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
- The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
- The email used to register you as an authorized contributor must also be attached to your GitHub account.
ℹ️ Googlers: Go here for more info.
This PR (HEAD: 40738d426814df40c4d54d5e7017f7af7725da47) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/175978 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
Message from Gobot Gobot:
Patch Set 1:
Congratulations on opening your first change. Thank you for your contribution!
Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.
Please don’t reply on this GitHub thread. Visit golang.org/cl/175978. After addressing review feedback, remember to publish your drafts!
Message from Benny Siegert:
Patch Set 1: Run-TryBot+1
(3 comments)
Can you please add a test?
Please don’t reply on this GitHub thread. Visit golang.org/cl/175978. After addressing review feedback, remember to publish your drafts!
Message from Gobot Gobot:
Patch Set 1:
TryBots beginning. Status page: https://farmer.golang.org/try?commit=19bc885e
Please don’t reply on this GitHub thread. Visit golang.org/cl/175978. After addressing review feedback, remember to publish your drafts!
Message from Gobot Gobot:
Patch Set 1: TryBot-Result+1
TryBots are happy.
Please don’t reply on this GitHub thread. Visit golang.org/cl/175978. After addressing review feedback, remember to publish your drafts!
This PR (HEAD: ef0d1a6f5b87067803518089d4cbc349777a56bd) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/175978 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
This PR (HEAD: a7099eef26a7fdc97f3ac5f5b2b61f9f136dd16f) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/175978 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
Message from Filippo Valsorda:
Patch Set 3:
#17889 should be turned into a proposal.
Please don’t reply on this GitHub thread. Visit golang.org/cl/175978. After addressing review feedback, remember to publish your drafts!
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.
ℹ️ Googlers: Go here for more info.
This PR (HEAD: 911d13b3bf6ecdf2583a0c3fa131b60ebfd5cdb9) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/175978 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
Here is the way to include 2FA or any other questions you want source code you can follow this project https://github.com/kennylevinsen/sshmux/blob/master/forward.go#L134
// SessionForward performs a regular forward, providing the user with an
// interactive remote host selection if necessary. This forwarding type
// requires agent forwarding in order to work.
func (s *Server) SessionForward(S *session.Session, newChannel ssh.NewChannel, chans <-chan ssh.NewChannel) {
// Okay, we're handling this as a regular session
sshChan, sessReqs, err := newChannel.Accept()
if err != nil {
log.Error(LogFilter, "%v", err)
return
}
stderr := sshChan.Stderr()
term := terminal.NewTerminal(sshChan, "")
check := false
for _, q := range s.Questions {
count := 0
if S.GetSeed() == "" {
continue
}
for count < q.Retry {
count++
term.Write([]byte(q.Question))
line, err := term.ReadLine()
if err != nil {
log.Error("term.ReadLine: %v", err)
sshChan.Close()
return
}
if q.Checker(line, term) {
check = true
break
}
}
if !check {
sshChan.Close()
return
}
}
...
create a []Question struct list under Server struct to follow all your questions
I have found that the solution that uses NextAuthMethodsCallback does not work completely.
In the scenario where we have PublicKey followed by Password auth, there is a bug caused by the fact that the client calls the server twice, firstly to pre-auth the key and then secondly to actually auth. The response from candidate.perms, candidate.result = config.PublicKeyCallback(s, pubKey) is cached so the same result is used both for the pre-auth and for the auth. This breaks if you return ErrPartialSuccess as it fails the pre-auth stage and does not allow the auth chain to continue. And due to the caching you can not discriminate in your callback whether to send a nil error or a PartialSuccess.
I worked around it with this. But I don't know if it is the ideal solution:
--- a/vendor/golang.org/x/crypto/ssh/server.go
+++ b/vendor/golang.org/x/crypto/ssh/server.go
@@ -480,7 +480,7 @@ userAuthLoop:
return nil, parseError(msgUserAuthRequest)
}
- if candidate.result == nil {
+ if candidate.result == nil || candidate.result == ErrPartialSuccess {
okMsg := userAuthPubKeyOkMsg{
Algo: algo,
PubKey: pubKeyData,
@adampointer-form3 The ssh client will try the methods listed by the server, but if you configure NextAuthMethodsCallback as "publickey", then the client will not try "publickey" method until the client get ErrPartialSuccess err. It will not trigger publickey method cached problem.
It has been quite some time, and this functionality is sorely needed -- any chance @LeeEirc could sign the CLA since that seems to be the hangup!
Apologies for two posts but a related question: if this PR is forever dead due to CLA, what is the appropriate next step? Would it be useful for someone else to open a separate PR with @ibuler's code but removing the commit from @LeeEirc? Lee's code makes a fix but, for instance, I could make the same type of fix and sign the CLA to unstick it.
@ibuler I think the issue is that in your PR you accepted a commit from a third party (@LeeEirc ) and that means they have to sign it to. If you know them maybe you could give a nudge?
Otherwise (and I am not a lawyer) I imagine one thing you could do is rebase your PR to remove his commit, and potentially re-implement their portion of the overall PR in your own way?
Hi, @ibuler. I think what @slifty says above is accurate -- the issue is the inclusion of @LeeEirc's commit in your PR, because it means that now @LeeEirc would also be a contributor and would need to sign the CLA.
So our options are:
- Hope that @LeeEirc sees this and signs the CLA too, or
- Remove his commit and replace it with one by you that does basically the same thing. That is actually probably the easiest solution here -- @LeeEirc's commit 8ce6a60537c033a is just a tiny number of lines of difference, and even some of those are whitespace changes that you can just leave out. (IMHO it's questionable whether commit 8ce6a60537c033a is substantial enough to even be subject to copyright licensing at all, really -- but we can't have that discussion with the CLA bot, so it's easiest just to replace the commit.)
This pull request does not work with public-key authentication as pointed out by @adampointer-form3. There should probably be a test to validate that.
The current implementation only allows public-key authentication as the last method. If you try to return ssh.PartialSuccess, it will fail. This is evident in the Go client code where the PartialSuccess field is not checked in the key query request:
https://github.com/golang/crypto/blob/c86fa9a7ed909e2f2a8ab8298254fca727aba16a/ssh/client_auth.go#L378-L379
There are (at least) two other implementations that don't have this issue, #130 and #214. I like the proposed interface of the latter to avoid passing strings around.
