Skip to content

ssh: add support for server side multi-step authentication #130

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

Closed
wants to merge 4 commits into from

Conversation

drakkan
Copy link
Member

@drakkan drakkan commented Apr 9, 2020

  • add ErrPartialSuccess. Authentication callbacks must return this error for
    multi-step authentication when a specific authentication step succeed
  • add PartialSuccessMethods to ConnMetadata interface, it returns the ordered
    list of authentication methods that returned ErrPartialSuccess. It can be used
    inside callbacks to find if a multi-step authentication is done using the
    correct sequence and to return the authentication methods that can continue
  • add NextAuthMethodsCallback, this callback is called when an authentication
    callback returns ErrPartialSuccess or if, after an initial partial success, an
    authentication step fails. It must return the list of authentications methods
    that can continue.

This way an application can define per-user multi-step authentication.

Fixes #17889

- add ErrPartialSuccess. Authentication callbacks must return this error for
multi-step authentication when a specific authentication step succeed
- add PartialSuccessMethods to ConnMetadata interface, it returns the ordered
list of authentication methods that returned ErrPartialSuccess. It can be used
inside callbacks to find if a multi-step authentication is done using the
correct sequence and to return the authentication methods that can continue
- add NextAuthMethodsCallback, this callback is called when an authentication
callback returns ErrPartialSuccess or if, after an initial partial success, an
authentication step fails. It must return the list of authentications methods
that can continue.

This way an application can define per-user multi-step authentication.

Fixes #17889
@gopherbot
Copy link
Contributor

This PR (HEAD: 2aafde1) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/227781 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

If PublicKeyCallback returns ErrPartialSuccess we need to check source address
and update the returned error if this check fails
@gopherbot
Copy link
Contributor

This PR (HEAD: aed7e17) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/227781 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

@gopherbot
Copy link
Contributor

This PR (HEAD: 83446a0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/227781 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

@tt
Copy link

tt commented May 24, 2020

There's a pretty significant security issue in the current implementation.

If PublicKeyCallback returns ErrPartialSuccess, a client will be able to authenticate by presenting any public key without proving that it possess the corresponding private key.

This is because the client can first query if a key is acceptable before it signs anything.

crypto/ssh/server.go

Lines 577 to 586 in 2aafde1

if candidate.result == nil {
okMsg := userAuthPubKeyOkMsg{
Algo: algo,
PubKey: pubKeyData,
}
if err = s.transport.writePacket(Marshal(&okMsg)); err != nil {
return nil, err
}
continue userAuthLoop
}

candidate.result == nil needs to be candidate.result == nil || candidate.result == ErrPartialSuccess.

@gopherbot
Copy link
Contributor

This PR (HEAD: 6d02a9b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/227781 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

@drakkan
Copy link
Member Author

drakkan commented May 24, 2020

There's a pretty significant security issue in the current implementation.

If PublicKeyCallback returns ErrPartialSuccess, a client will be able to authenticate by presenting any public key without proving that it possess the corresponding private key.

This is because the client can first query if a key is acceptable before it signs anything.

crypto/ssh/server.go

Lines 577 to 586 in 2aafde1

if candidate.result == nil {
okMsg := userAuthPubKeyOkMsg{
Algo: algo,
PubKey: pubKeyData,
}
if err = s.transport.writePacket(Marshal(&okMsg)); err != nil {
return nil, err
}
continue userAuthLoop
}

candidate.result == nil needs to be candidate.result == nil || candidate.result == ErrPartialSuccess.

this should be fixed now, thanks for reporting

drakkan added a commit to drakkan/sftpgo that referenced this pull request May 24, 2020
@gopherbot
Copy link
Contributor

Message from Dan Peterson:

Patch Set 4: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=9080eac4


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

@samiponkanen
Copy link
Contributor

Are there plans to accept this PR, or alternatively add server side multi auth via another PR?

I am very eagerly waiting for this feature, and can offer my help in reviewing this PR, if that will speed up the acceptance process!

@gopherbot
Copy link
Contributor

Message from Sami Pönkänen:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=9080eac4


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 4: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/227781.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants