From 8860264478e61f1e5e3ddda67d87386a76516967 Mon Sep 17 00:00:00 2001 From: Nikita Kryuchkov Date: Mon, 13 May 2019 00:26:55 +0300 Subject: [PATCH 1/5] Request a new nonce and retry HTTP request if a nonce validation error is returned --- internal/httpauth/client.go | 73 ++++++++++++++++++++++----- internal/httpauth/client_test.go | 85 ++++++++++++++++++++++++++------ 2 files changed, 131 insertions(+), 27 deletions(-) diff --git a/internal/httpauth/client.go b/internal/httpauth/client.go index 614034f99b..044b250cd0 100644 --- a/internal/httpauth/client.go +++ b/internal/httpauth/client.go @@ -13,10 +13,16 @@ import ( "strconv" "strings" "sync/atomic" + "time" "github.com/skycoin/skywire/pkg/cipher" ) +const ( + retryInterval = 100 * time.Millisecond + invalidNonceErrorMessage = "SW-Nonce does not match" +) + // NextNonceResponse represents a ServeHTTP response for json encoding type NextNonceResponse struct { Edge cipher.PubKey `json:"edge"` @@ -71,11 +77,6 @@ func NewClient(ctx context.Context, addr string, key cipher.PubKey, sec cipher.S // Do performs a new authenticated Request and returns the response. Internally, if the request was // successful nonce is incremented func (c *Client) Do(req *http.Request) (*http.Response, error) { - req.Header.Add("SW-Public", c.key.Hex()) - - // use nonce, later, if no err from req update such nonce - req.Header.Add("SW-Nonce", strconv.FormatUint(uint64(c.getCurrentNonce()), 10)) - body := make([]byte, 0) if req.ContentLength != 0 { auxBody, err := ioutil.ReadAll(req.Body) @@ -86,16 +87,42 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { req.Body = ioutil.NopCloser(bytes.NewBuffer(auxBody)) body = auxBody } - sign, err := Sign(body, c.getCurrentNonce(), c.sec) - if err != nil { - return nil, err - } - req.Header.Add("SW-Sig", sign.Hex()) + var res *http.Response + for { + nonce := c.getCurrentNonce() + sign, err := Sign(body, nonce, c.sec) + if err != nil { + return nil, err + } + + // use nonce, later, if no err from req update such nonce + req.Header.Set("SW-Nonce", strconv.FormatUint(uint64(nonce), 10)) + req.Header.Set("SW-Sig", sign.Hex()) + req.Header.Set("SW-Public", c.key.Hex()) - res, err := c.client.Do(req) - if err != nil { - return nil, err + res, err = c.client.Do(req) + if err != nil { + return nil, err + } + + isNonceValid, err := c.isNonceValid(res) + if err != nil { + return nil, err + } + + if isNonceValid { + break + } + + nonce, err = c.Nonce(context.Background(), c.key) + if err != nil { + return nil, err + } + c.SetNonce(nonce) + + res.Body.Close() + time.Sleep(retryInterval) } if res.StatusCode == http.StatusOK { @@ -144,6 +171,26 @@ func (c *Client) incrementNonce() { atomic.AddUint64(&c.nonce, 1) } +func (c *Client) isNonceValid(res *http.Response) (bool, error) { + var serverResponse HTTPResponse + + auxRespBody, err := ioutil.ReadAll(res.Body) + if err != nil { + return false, err + } + res.Body.Close() + res.Body = ioutil.NopCloser(bytes.NewBuffer(auxRespBody)) + + if err := json.Unmarshal(auxRespBody, &serverResponse); err != nil || serverResponse.Error == nil { + return true, nil + } + + isAuthorized := serverResponse.Error.Code != http.StatusUnauthorized + hasValidNonce := serverResponse.Error.Message != invalidNonceErrorMessage + + return isAuthorized && hasValidNonce, nil +} + func sanitizedAddr(addr string) string { if addr == "" { return "http://localhost" diff --git a/internal/httpauth/client_test.go b/internal/httpauth/client_test.go index 70a95eb64d..cd9f7fad31 100644 --- a/internal/httpauth/client_test.go +++ b/internal/httpauth/client_test.go @@ -1,12 +1,14 @@ package httpauth import ( + "bytes" "context" "encoding/json" "fmt" "io/ioutil" "net/http" "net/http/httptest" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -15,27 +17,49 @@ import ( "github.com/skycoin/skywire/pkg/cipher" ) +const ( + payload = "Hello, client\n" + errorMessage = `{"error":{"message":"SW-Nonce does not match","code":401}}` +) + func TestClient(t *testing.T) { pk, sk := cipher.GenerateKeyPair() - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.String() == fmt.Sprintf("/security/nonces/%s", pk) { - json.NewEncoder(w).Encode(&NextNonceResponse{pk, 1}) // nolint: errcheck - } else { - require.Equal(t, "1", r.Header.Get("Sw-Nonce")) - require.Equal(t, pk.Hex(), r.Header.Get("Sw-Public")) - sig := cipher.Sig{} - require.NoError(t, sig.UnmarshalText([]byte(r.Header.Get("Sw-Sig")))) - require.NoError(t, cipher.VerifyPubKeySignedPayload(pk, sig, PayloadWithNonce([]byte{}, 1))) - fmt.Fprintln(w, "Hello, client") - } - })) + headerCh := make(chan http.Header, 1) + ts := newTestServer(pk, headerCh) + defer ts.Close() + + c, err := NewClient(context.TODO(), ts.URL, pk, sk) + require.NoError(t, err) + + req, err := http.NewRequest("GET", ts.URL+"/foo", bytes.NewBufferString(payload)) + require.NoError(t, err) + res, err := c.Do(req) + require.NoError(t, err) + + b, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + res.Body.Close() + assert.Equal(t, []byte(payload), b) + assert.Equal(t, uint64(2), c.nonce) + + headers := <-headerCh + checkResp(t, headers, b, pk, 1) +} + +func TestBadNonce(t *testing.T) { + pk, sk := cipher.GenerateKeyPair() + + headerCh := make(chan http.Header, 1) + ts := newTestServer(pk, headerCh) defer ts.Close() c, err := NewClient(context.TODO(), ts.URL, pk, sk) require.NoError(t, err) - req, err := http.NewRequest("GET", ts.URL+"/foo", nil) + c.nonce = 999 + + req, err := http.NewRequest("GET", ts.URL+"/foo", bytes.NewBufferString(payload)) require.NoError(t, err) res, err := c.Do(req) require.NoError(t, err) @@ -43,6 +67,39 @@ func TestClient(t *testing.T) { b, err := ioutil.ReadAll(res.Body) require.NoError(t, err) res.Body.Close() - assert.Equal(t, []byte("Hello, client\n"), b) assert.Equal(t, uint64(2), c.nonce) + + headers := <-headerCh + checkResp(t, headers, b, pk, 1) +} + +func checkResp(t *testing.T, headers http.Header, body []byte, pk cipher.PubKey, nonce int) { + require.Equal(t, strconv.Itoa(nonce), headers.Get("Sw-Nonce")) + require.Equal(t, pk.Hex(), headers.Get("Sw-Public")) + sig := cipher.Sig{} + require.NoError(t, sig.UnmarshalText([]byte(headers.Get("Sw-Sig")))) + require.NoError(t, cipher.VerifyPubKeySignedPayload(pk, sig, PayloadWithNonce(body, Nonce(nonce)))) +} + +func newTestServer(pk cipher.PubKey, headerCh chan<- http.Header) *httptest.Server { + nonce := 1 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.String() == fmt.Sprintf("/security/nonces/%s", pk) { + json.NewEncoder(w).Encode(&NextNonceResponse{pk, Nonce(nonce)}) // nolint: errcheck + } else { + body, err := ioutil.ReadAll(r.Body) + if err != nil { + return + } + defer r.Body.Close() + respMessage := string(body) + if r.Header.Get("Sw-Nonce") != strconv.Itoa(nonce) { + respMessage = errorMessage + } else { + headerCh <- r.Header + nonce++ + } + fmt.Fprint(w, respMessage) + } + })) } From 4062ee2222bf25e698528b998a13e563a5b166d3 Mon Sep 17 00:00:00 2001 From: Nikita Kryuchkov Date: Mon, 13 May 2019 22:59:40 +0300 Subject: [PATCH 2/5] Do not request a new nonce anymore if retry counter exceeds threshold --- internal/httpauth/client.go | 7 ++++- internal/httpauth/client_test.go | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/internal/httpauth/client.go b/internal/httpauth/client.go index 044b250cd0..54a3b7a901 100644 --- a/internal/httpauth/client.go +++ b/internal/httpauth/client.go @@ -19,6 +19,7 @@ import ( ) const ( + maxRetries = 10 retryInterval = 100 * time.Millisecond invalidNonceErrorMessage = "SW-Nonce does not match" ) @@ -89,7 +90,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { } var res *http.Response - for { + for retriesCount := 0; retriesCount < maxRetries; retriesCount++ { nonce := c.getCurrentNonce() sign, err := Sign(body, nonce, c.sec) if err != nil { @@ -106,6 +107,10 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { return nil, err } + if retriesCount == maxRetries { + break + } + isNonceValid, err := c.isNonceValid(res) if err != nil { return nil, err diff --git a/internal/httpauth/client_test.go b/internal/httpauth/client_test.go index cd9f7fad31..5843e790ab 100644 --- a/internal/httpauth/client_test.go +++ b/internal/httpauth/client_test.go @@ -73,6 +73,35 @@ func TestBadNonce(t *testing.T) { checkResp(t, headers, b, pk, 1) } +func TestTooManyRetries(t *testing.T) { + pk, sk := cipher.GenerateKeyPair() + + respCh := make(chan string, maxRetries) + ts := newBadNonceServer(pk, respCh) + defer ts.Close() + + c, err := NewClient(context.TODO(), ts.URL, pk, sk) + require.NoError(t, err) + + req, err := http.NewRequest("GET", ts.URL+"/foo", bytes.NewBufferString(payload)) + require.NoError(t, err) + res, err := c.Do(req) + require.NoError(t, err) + + b, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + res.Body.Close() + assert.Equal(t, payload, string(b)) + + for i := 0; i < maxRetries-1; i++ { + resp := <-respCh + assert.Equal(t, errorMessage, resp) + } + + resp := <-respCh + assert.Equal(t, payload, resp) +} + func checkResp(t *testing.T, headers http.Header, body []byte, pk cipher.PubKey, nonce int) { require.Equal(t, strconv.Itoa(nonce), headers.Get("Sw-Nonce")) require.Equal(t, pk.Hex(), headers.Get("Sw-Public")) @@ -103,3 +132,25 @@ func newTestServer(pk cipher.PubKey, headerCh chan<- http.Header) *httptest.Serv } })) } + +func newBadNonceServer(pk cipher.PubKey, respCh chan<- string) *httptest.Server { + nonce := 1 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.String() == fmt.Sprintf("/security/nonces/%s", pk) { + json.NewEncoder(w).Encode(&NextNonceResponse{pk, Nonce(nonce)}) // nolint: errcheck + } else { + body, err := ioutil.ReadAll(r.Body) + if err != nil { + return + } + defer r.Body.Close() + respMessage := string(body) + if nonce < maxRetries || r.Header.Get("Sw-Nonce") != strconv.Itoa(nonce) { + respMessage = errorMessage + } + nonce++ + fmt.Fprint(w, respMessage) + respCh <- respMessage + } + })) +} From 7dd5887a7b0582e72413e8944d86dd324b5078df Mon Sep 17 00:00:00 2001 From: Nikita Kryuchkov Date: Thu, 16 May 2019 11:30:18 +0300 Subject: [PATCH 3/5] Retry only once if nonce is invalid --- internal/httpauth/client.go | 66 ++++++++++++++++---------------- internal/httpauth/client_test.go | 54 +------------------------- 2 files changed, 34 insertions(+), 86 deletions(-) diff --git a/internal/httpauth/client.go b/internal/httpauth/client.go index 54a3b7a901..cdd59e557e 100644 --- a/internal/httpauth/client.go +++ b/internal/httpauth/client.go @@ -13,14 +13,11 @@ import ( "strconv" "strings" "sync/atomic" - "time" "github.com/skycoin/skywire/pkg/cipher" ) const ( - maxRetries = 10 - retryInterval = 100 * time.Millisecond invalidNonceErrorMessage = "SW-Nonce does not match" ) @@ -89,45 +86,28 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { body = auxBody } - var res *http.Response - for retriesCount := 0; retriesCount < maxRetries; retriesCount++ { - nonce := c.getCurrentNonce() - sign, err := Sign(body, nonce, c.sec) - if err != nil { - return nil, err - } - - // use nonce, later, if no err from req update such nonce - req.Header.Set("SW-Nonce", strconv.FormatUint(uint64(nonce), 10)) - req.Header.Set("SW-Sig", sign.Hex()) - req.Header.Set("SW-Public", c.key.Hex()) - - res, err = c.client.Do(req) - if err != nil { - return nil, err - } + res, err := c.doRequest(req, body) + if err != nil { + return nil, err + } - if retriesCount == maxRetries { - break - } + isNonceValid, err := isNonceValid(res) + if err != nil { + return nil, err + } - isNonceValid, err := c.isNonceValid(res) + if !isNonceValid { + nonce, err := c.Nonce(context.Background(), c.key) if err != nil { return nil, err } + c.SetNonce(nonce) - if isNonceValid { - break - } - - nonce, err = c.Nonce(context.Background(), c.key) + res.Body.Close() + res, err = c.doRequest(req, body) if err != nil { return nil, err } - c.SetNonce(nonce) - - res.Body.Close() - time.Sleep(retryInterval) } if res.StatusCode == http.StatusOK { @@ -168,6 +148,21 @@ func (c *Client) SetNonce(n Nonce) { atomic.StoreUint64(&c.nonce, uint64(n)) } +func (c *Client) doRequest(req *http.Request, body []byte) (*http.Response, error) { + nonce := c.getCurrentNonce() + sign, err := Sign(body, nonce, c.sec) + if err != nil { + return nil, err + } + + // use nonce, later, if no err from req update such nonce + req.Header.Set("SW-Nonce", strconv.FormatUint(uint64(nonce), 10)) + req.Header.Set("SW-Sig", sign.Hex()) + req.Header.Set("SW-Public", c.key.Hex()) + + return c.client.Do(req) +} + func (c *Client) getCurrentNonce() Nonce { return Nonce(c.nonce) } @@ -176,7 +171,10 @@ func (c *Client) incrementNonce() { atomic.AddUint64(&c.nonce, 1) } -func (c *Client) isNonceValid(res *http.Response) (bool, error) { +// isNonceValid checks if `res` contains an invalid nonce error. +// The error is occurred if status code equals to `http.StatusUnauthorized` +// and body contains `invalidNonceErrorMessage`. +func isNonceValid(res *http.Response) (bool, error) { var serverResponse HTTPResponse auxRespBody, err := ioutil.ReadAll(res.Body) diff --git a/internal/httpauth/client_test.go b/internal/httpauth/client_test.go index 5843e790ab..72de5b9d23 100644 --- a/internal/httpauth/client_test.go +++ b/internal/httpauth/client_test.go @@ -47,7 +47,8 @@ func TestClient(t *testing.T) { checkResp(t, headers, b, pk, 1) } -func TestBadNonce(t *testing.T) { +// TestClient_BadNonce tests if `Client` retries the request if an invalid nonce is set. +func TestClient_BadNonce(t *testing.T) { pk, sk := cipher.GenerateKeyPair() headerCh := make(chan http.Header, 1) @@ -73,35 +74,6 @@ func TestBadNonce(t *testing.T) { checkResp(t, headers, b, pk, 1) } -func TestTooManyRetries(t *testing.T) { - pk, sk := cipher.GenerateKeyPair() - - respCh := make(chan string, maxRetries) - ts := newBadNonceServer(pk, respCh) - defer ts.Close() - - c, err := NewClient(context.TODO(), ts.URL, pk, sk) - require.NoError(t, err) - - req, err := http.NewRequest("GET", ts.URL+"/foo", bytes.NewBufferString(payload)) - require.NoError(t, err) - res, err := c.Do(req) - require.NoError(t, err) - - b, err := ioutil.ReadAll(res.Body) - require.NoError(t, err) - res.Body.Close() - assert.Equal(t, payload, string(b)) - - for i := 0; i < maxRetries-1; i++ { - resp := <-respCh - assert.Equal(t, errorMessage, resp) - } - - resp := <-respCh - assert.Equal(t, payload, resp) -} - func checkResp(t *testing.T, headers http.Header, body []byte, pk cipher.PubKey, nonce int) { require.Equal(t, strconv.Itoa(nonce), headers.Get("Sw-Nonce")) require.Equal(t, pk.Hex(), headers.Get("Sw-Public")) @@ -132,25 +104,3 @@ func newTestServer(pk cipher.PubKey, headerCh chan<- http.Header) *httptest.Serv } })) } - -func newBadNonceServer(pk cipher.PubKey, respCh chan<- string) *httptest.Server { - nonce := 1 - return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.String() == fmt.Sprintf("/security/nonces/%s", pk) { - json.NewEncoder(w).Encode(&NextNonceResponse{pk, Nonce(nonce)}) // nolint: errcheck - } else { - body, err := ioutil.ReadAll(r.Body) - if err != nil { - return - } - defer r.Body.Close() - respMessage := string(body) - if nonce < maxRetries || r.Header.Get("Sw-Nonce") != strconv.Itoa(nonce) { - respMessage = errorMessage - } - nonce++ - fmt.Fprint(w, respMessage) - respCh <- respMessage - } - })) -} From 36efc97a18567e0982d0026b4f1c819fd82378c9 Mon Sep 17 00:00:00 2001 From: Nikita Kryuchkov Date: Thu, 16 May 2019 16:08:39 +0300 Subject: [PATCH 4/5] Get current nonce atomically --- internal/httpauth/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/httpauth/client.go b/internal/httpauth/client.go index cdd59e557e..3a3dd02d3f 100644 --- a/internal/httpauth/client.go +++ b/internal/httpauth/client.go @@ -45,7 +45,7 @@ type Client struct { key cipher.PubKey sec cipher.SecKey Addr string // sanitized address of the client, which may differ from addr used in NewClient - nonce uint64 + nonce uint64 // has to be handled with the atomic package at all time } // NewClient creates a new client setting a public key to the client to be used for Auth. @@ -164,7 +164,7 @@ func (c *Client) doRequest(req *http.Request, body []byte) (*http.Response, erro } func (c *Client) getCurrentNonce() Nonce { - return Nonce(c.nonce) + return Nonce(atomic.LoadUint64(&c.nonce)) } func (c *Client) incrementNonce() { From 051119c5307742e579a7171ee498fd4999d16f89 Mon Sep 17 00:00:00 2001 From: Nikita Kryuchkov Date: Thu, 16 May 2019 16:13:50 +0300 Subject: [PATCH 5/5] Make `Addr` unexported and create its getter method --- internal/httpauth/client.go | 11 ++++++++--- pkg/transport-discovery/client/client.go | 4 ++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/httpauth/client.go b/internal/httpauth/client.go index 3a3dd02d3f..0acc851232 100644 --- a/internal/httpauth/client.go +++ b/internal/httpauth/client.go @@ -44,7 +44,7 @@ type Client struct { client http.Client key cipher.PubKey sec cipher.SecKey - Addr string // sanitized address of the client, which may differ from addr used in NewClient + addr string // sanitized address of the client, which may differ from addr used in NewClient nonce uint64 // has to be handled with the atomic package at all time } @@ -59,7 +59,7 @@ func NewClient(ctx context.Context, addr string, key cipher.PubKey, sec cipher.S client: http.Client{}, key: key, sec: sec, - Addr: sanitizedAddr(addr), + addr: sanitizedAddr(addr), } // request server for a nonce @@ -119,7 +119,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { // Nonce calls the remote API to retrieve the next expected nonce func (c *Client) Nonce(ctx context.Context, key cipher.PubKey) (Nonce, error) { - req, err := http.NewRequest(http.MethodGet, c.Addr+"/security/nonces/"+key.Hex(), nil) + req, err := http.NewRequest(http.MethodGet, c.addr+"/security/nonces/"+key.Hex(), nil) if err != nil { return 0, err } @@ -148,6 +148,11 @@ func (c *Client) SetNonce(n Nonce) { atomic.StoreUint64(&c.nonce, uint64(n)) } +// Addr returns sanitized address of the client +func (c *Client) Addr() string { + return c.addr +} + func (c *Client) doRequest(req *http.Request, body []byte) (*http.Response, error) { nonce := c.getCurrentNonce() sign, err := Sign(body, nonce, c.sec) diff --git a/pkg/transport-discovery/client/client.go b/pkg/transport-discovery/client/client.go index 507bf2ad6c..e4d7180822 100644 --- a/pkg/transport-discovery/client/client.go +++ b/pkg/transport-discovery/client/client.go @@ -53,7 +53,7 @@ func (c *apiClient) Post(ctx context.Context, path string, payload interface{}) return nil, err } - req, err := http.NewRequest("POST", c.client.Addr+path, body) + req, err := http.NewRequest("POST", c.client.Addr()+path, body) if err != nil { return nil, err } @@ -63,7 +63,7 @@ func (c *apiClient) Post(ctx context.Context, path string, payload interface{}) // Get performs a new GET request. func (c *apiClient) Get(ctx context.Context, path string) (*http.Response, error) { - req, err := http.NewRequest("GET", c.client.Addr+path, new(bytes.Buffer)) + req, err := http.NewRequest("GET", c.client.Addr()+path, new(bytes.Buffer)) if err != nil { return nil, err }