-
Notifications
You must be signed in to change notification settings - Fork 314
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
Feature/myst-259 validate session and signature on service provider side #126
Changes from all commits
62d422a
a647eb9
4ff31ec
6995606
57aa0ba
8b7743d
a378e18
6c5f3c0
d9b0a31
55b4fae
b7b49af
2540eda
f493f00
0b667d3
dfc74c5
117eaec
928bdc7
c5f3f7d
81f5b63
a7f794b
a9c88c1
9110836
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package session | ||
|
||
import ( | ||
"github.com/mysterium/node/identity" | ||
client_auth "github.com/mysterium/node/openvpn/middlewares/client/auth" | ||
server_auth "github.com/mysterium/node/openvpn/middlewares/server/auth" | ||
"github.com/mysterium/node/session" | ||
) | ||
|
||
const sessionSignaturePrefix = "MystVpnSessionId:" | ||
|
||
// SignatureCredentialsProvider returns session id as username and id signed with given signer as password | ||
func SignatureCredentialsProvider(id session.SessionID, signer identity.Signer) client_auth.CredentialsProvider { | ||
return func() (string, string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not fully aware, what do we gain from returning function here instead of credentials itself:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was designed with other PR. Might be one of the places for future refactoring |
||
signature, err := signer.Sign([]byte(sessionSignaturePrefix + id)) | ||
return string(id), signature.Base64(), err | ||
} | ||
} | ||
|
||
type sessionFinder func(session session.SessionID) (session.Session, bool) | ||
|
||
// NewSessionValidator provides glue code for openvpn management interface to validate incoming client login request, | ||
// it expects session id as username, and session signature signed by client as password | ||
func NewSessionValidator(findSession sessionFinder, extractor identity.Extractor) server_auth.CredentialsChecker { | ||
return func(sessionString, signatureString string) (bool, error) { | ||
sessionId := session.SessionID(sessionString) | ||
currentSession, found := findSession(sessionId) | ||
if !found { | ||
return false, nil | ||
} | ||
|
||
signature := identity.SignatureBase64(signatureString) | ||
extractedIdentity, err := extractor.Extract([]byte(sessionSignaturePrefix+sessionString), signature) | ||
if err != nil { | ||
return false, err | ||
} | ||
return currentSession.ConsumerID == extractedIdentity, nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
username, password
pair is strongly coupled - it's being passed around the system in many places and we have quite a few methods which returns(username, password string, err error)
.Maybe it's time to merge them into struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e. we wouldn't have to explain what is "auth primitives" in many places, as we do now - we could do that in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true. but since our middleware packageing is a bit different, we don't have a possibility to share credentials structure between client and server. But good point and it can be improved with separate PR.