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

Feature/myst-114-sign-mysterium-apimessages #88

Merged
merged 14 commits into from Jan 10, 2018

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Jan 8, 2018

No description provided.

var noSignature = identity.SignatureBytes(nil)

type HttpClient interface {
DoGet(path string, values url.Values) (*http.Response, error)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Jan 8, 2018

Member

Is it worth prefixing the methods with "Do"?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Well obj.Get usually says get something from object. I see a better intent expression here. as GET POST etc. are actions and DoAction provides better readability. Ofcourse if it's confusing it can be renamed

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Agree with @ignasbernotas - Get is a verb itself and since this is not Java, getters like GetAmounts shouldn't be very common to confuse with.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Will rename it to Get Post and SignedPost

@@ -0,0 +1,122 @@
package server

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Jan 8, 2018

Member

I know this is probably the best naming we can come up with, but maybe worth naming it discovery_api? mysterium_api is pretty generic:)

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Well from node/client perspective this api provides 3 distinct functionalities:

  1. Register identities
  2. Register/provide proposals (actual discovery)
  3. Collect statistics
    We call remote service "discovery service" as we don't have a better naming yet. But from client/node perspective - its not discovery but something more generic - i.e. mysterium_centralized_api
var noSignature = identity.SignatureBytes(nil)

type HttpClient interface {
DoGet(path string, values url.Values) (*http.Response, error)

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Agree with @ignasbernotas - Get is a verb itself and since this is not Java, getters like GetAmounts shouldn't be very common to confuse with.

}

func (signature *Signature) Bytes() []byte {
return signature.raw
}

func (signature Signature) EqualsTo(other Signature) bool {
return bytes.Equal(signature.raw, other.raw)

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Is it safe to ignore cases when signature or other is nil?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Since receiver and other are values but not pointers they cannot be nil. If you try to pass nil here you will get compiler error

log.Info(mysteriumApiLogPrefix, "Node stats sent: ", nodeKey)
}

return nil

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Shouldn't this be return, instead return nil?
It seems that we are always returning no error, even when error happens.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Yeah, I dont se case then error is returned

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Same applies for some of other endpoints.
This isn't related to this PR, so adding a TODO or Jira ticket seems enough.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Will fix this just as example for other endpoints.

func ConfigureVpnClientFactory(mysteriumApiClient server.Client, vpnClientRuntimeDirectory string) VpnClientFactory {
return func(vpnSession session.SessionDto) (openvpn.Client, error) {
func ConfigureVpnClientFactory(mysteriumApiClient server.Client, vpnClientRuntimeDirectory string, signerFactory SignerFactory) VpnClientFactory {
return func(vpnSession session.SessionDto, identity identity.Identity) (openvpn.Client, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

I would be passing Signer here, and factoring it else-there.
Manager could signerFactory injected.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Another idea could be to inject already factoried middlewares...:

  • it would decrease number of passed dependencies
  • would make clientFactory mocking in tests simpler

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

I also thought about first idea, but then it became that connection manager only creates a signer and passes it to vpn client factory, also signer is too low level and communication specific (sign outogoing requests). So i decided that it has to be open vpn connection factory dependency on configuration phase.
As for option two - middlewares are very openvpn specific and should stay that way - inside openvpnfactory and on openvpn client construction, too much low level info moved outside.


var noSignature = identity.SignatureBytes(nil)

type HttpClient interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

What is idea of personal HttpClient with DoPost, DoGet etc.?
Then perfect native http.Client with Do, Post

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

It could be event simpler interface: httpClient.Do(request) which is compatible with http.Client

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

native httpClient.Post get etc hides constructed request objects so there is no way insert custom headers, access bodies etc. httpClient.Do is too low level - operates on req created elsewere. The point of this interface was to provide more flexible http actions to uper level component (mysterium_api_client) and to have reusable bit of functionality - http client with request bodies serialized to json and auto signed if needed. It could disappear after we fully migrate mysterium api to signed requests.

if err != nil {
return nil, err
}
signature, err := signer.Sign(payloadJson)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

jsonHttpClient looks having coupled 2 interfaces:

  • httpClient.Post() something that does requests
  • httpSignerClient.Post() something that signs + does requests

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

we cannot have a signedHttpClient as per our discussion - in that case signer will become a state and new httpClient will be needed to construct for each signer

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Signing actually has much simpler flow:

request := client.createPOSTRequest("v1/node_register", requestDto{})
client.signRequest(&request, signer)

response, err := client.httpClient.Do(request)

err = parseResponseJson(response, &dto.ProposalsResponse{})

I think most complication comes, because we mixed request creating + request transporting:

  • doGetRequest()
  • doPostRequest()

versus:

  • request := createGetRequest()
  • signRequest(request)
  • doRequest(request) wich already exist in http.Client.Do()

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

In that case JsonClient becomes requestFactory?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

I did not get idea of JsonClient much. I am just advocating for function with single responsibility

req.Header.Set("Content-Type", "application/json")
req.Header.Set("Accept", "application/json")

if !signature.EqualsTo(noSignature) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Decoupling | composition would allow to avoid conditional logic

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

In this case it's not - currently signature is optional for posts and exists only if signer returns real signature instead of no signature constant. It will disappear after all posts become signed

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Anyway this function has no single responsibility, it does: requestCreation + requestAuthentification + requestTransport.
I like new refactoring better

BytesSent: bytesSent,
BytesReceived: bytesReceived,
},
signer)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Formatting

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Fixed


func NewClient() Client {
return &mysteriumApiClient{
http: NewJsonClient(

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Looks quite complicated http, transport and httpTransport

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

A better naming might help. Will fix that.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

it became mysteriumApi {
json : NewJsonClient()
}

}

func (jhc *jsonHttpClient) DoGet(path string, values url.Values) (*http.Response, error) {
pathWithQuery := fmt.Sprintf("%v?%v", path, values.Encode())

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Looks like not building "/path", then dont have query params

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

Please explain. The idea is path or endpoint concated it with params. I.e. find proposals:
client.DoGet("proposals" , {map with params to find proposal with node id})

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

I am talking about case, then query params are empty.
DoGet('v1/path', url.Values{})
IMHO should be asserted to become:
/path

log.Info(mysteriumApiLogPrefix, "Node stats sent: ", nodeKey)
}

return nil

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Yeah, I dont se case then error is returned

return nil
}

func parseResponseJson(response *http.Response, dto interface{}) error {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Marshar|Unmarshar now come in different classes, should not they come together?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Author Member

It must unless jsonClient can get a respose struct factory somehow to know which types of concrete structures to unmarshal and return as response.

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@Waldz Refactored mysterium api client internals (requests are created independently)


resp, err := mApi.http.Do(req)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

This block could be DRYied:

resp, err := mApi.http.Do(req)
if err == nil {
    defer resp.Body.Close()
}

Similar places was refactored in ipify.doRequest(reqest, responseDto)

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 10, 2018

Author Member

I will check it

}

func newSignedPostRequest(path string, requestBody interface{}, signer identity.Signer) (*http.Request, error) {
bodyBytes, err := encodeToJson(requestBody)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

I would just reuse newPostRequest() + add header

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 10, 2018

Author Member

That's the problem - newPostRequest serializes body inside and inserts into request - no clean way to access body bytes later and create signature.

type VpnClientFactory func(vpnSession session.SessionDto) (openvpn.Client, error)
type VpnClientFactory func(vpnSession session.SessionDto, identity identity.Identity) (openvpn.Client, error)

type SignerFactory func(identity identity.Identity) identity.Signer

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

identity is package name, lets not use it for variables.
Use id, providerId, customerId, myId, signerId

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 10, 2018

Author Member

Will fix that.

@@ -66,7 +68,7 @@ func (manager *connectionManager) Connect(identity identity.Identity, nodeKey st
return err
}

manager.vpnClient, err = manager.vpnClientFactory(*vpnSession)
manager.vpnClient, err = manager.vpnClientFactory(*vpnSession, identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

identity is package name

@@ -121,7 +123,7 @@ func ConfigureVpnClientFactory(mysteriumApiClient server.Client, vpnClientRuntim
return nil, err
}

statsSender := bytescount_client.NewSessionStatsSender(mysteriumApiClient, vpnSession.Id)
statsSender := bytescount_client.NewSessionStatsSender(mysteriumApiClient, vpnSession.Id, signerFactory(identity))

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

identity is package name

@@ -36,7 +36,11 @@ func NewCommandWith(
return nats_dialog.NewDialogEstablisher(identity)
}

vpnClientFactory := client_connection.ConfigureVpnClientFactory(mysteriumClient, options.DirectoryRuntime)
signerFactory := func(id identity.Identity) identity.Signer {

This comment has been minimized.

Copy link
@Waldz
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Accept", "application/json")

if !signature.EqualsTo(noSignature) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Anyway this function has no single responsibility, it does: requestCreation + requestAuthentification + requestTransport.
I like new refactoring better

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

@Waldz all your mentiond issues addressed :)

@Waldz

Waldz approved these changes Jan 10, 2018

@donce donce merged commit a520877 into master Jan 10, 2018

@donce donce deleted the feature/MYST-114-sign-mysterium-api-messages branch Jan 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.