From 9e0635a6a464637156eca5d8c9623144a8783945 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Wed, 28 Oct 2020 16:07:15 +0100 Subject: [PATCH] fix: don't check tls fingerprints when checking connectivity --- Changelog.md | 5 ++ pkg/pmapi/check_connection.go | 94 +++++++++++++++++++++++++++++++++++ pkg/pmapi/clientmanager.go | 66 ------------------------ pkg/pmapi/proxy_test.go | 2 +- pkg/pmapi/tlsreport.go | 20 +++++--- 5 files changed, 114 insertions(+), 73 deletions(-) create mode 100644 pkg/pmapi/check_connection.go diff --git a/Changelog.md b/Changelog.md index 0be325e8..69b23a1b 100644 --- a/Changelog.md +++ b/Changelog.md @@ -33,6 +33,11 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/) * GODT-827 Do not spam sentry with bad ID by integration test. * GODT-700 Fix UTF-7 incompatibility. * GODT-837 Fix flaky TestFailUnpauseAndStops. +* GODT-782 Don't use TLS pinning when checking connectivity status. + +### Changed +* TLS pins conform to official list. + ## [Bridge 1.4.5] Forth diff --git a/pkg/pmapi/check_connection.go b/pkg/pmapi/check_connection.go new file mode 100644 index 00000000..5f0ea282 --- /dev/null +++ b/pkg/pmapi/check_connection.go @@ -0,0 +1,94 @@ +// Copyright (c) 2020 Proton Technologies AG +// +// This file is part of ProtonMail Bridge. +// +// ProtonMail Bridge is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// ProtonMail Bridge is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with ProtonMail Bridge. If not, see . + +package pmapi + +import ( + "errors" + "fmt" + "net/http" + "time" +) + +const protonStatusURL = "http://protonstatus.com/vpn_status" + +// ErrNoInternetConnection indicates that both protonstatus and the API are unreachable. +var ErrNoInternetConnection = errors.New("no internet connection") + +// CheckConnection returns an error if there is no internet connection. +// This should be moved to the ConnectionManager when it is implemented. +func (cm *ClientManager) CheckConnection() error { + // We use a normal dialer here which doesn't check tls fingerprints. + client := &http.Client{Timeout: time.Second * 10} + + // Do not cumulate timeouts, use goroutines. + retStatus := make(chan error) + retAPI := make(chan error) + + // vpn_status endpoint is fast and returns only OK. We check the connection only. + go checkConnection(client, protonStatusURL, retStatus) + + // Check of API reachability also uses a fast endpoint. + go checkConnection(client, cm.GetRootURL()+"/tests/ping", retAPI) + + errStatus := <-retStatus + errAPI := <-retAPI + + switch { + case errStatus == nil && errAPI == nil: + return nil + + case errStatus == nil && errAPI != nil: + cm.log.Error("ProtonStatus is reachable but API is not") + return ErrAPINotReachable + + case errStatus != nil && errAPI == nil: + cm.log.Warn("API is reachable but protonstatus is not") + return nil + + case errStatus != nil && errAPI != nil: + cm.log.Error("Both ProtonStatus and API are unreachable") + return ErrNoInternetConnection + } + + return nil +} + +// CheckConnection returns an error if there is no internet connection. +func CheckConnection() error { + client := &http.Client{Timeout: time.Second * 10} + retStatus := make(chan error) + go checkConnection(client, protonStatusURL, retStatus) + return <-retStatus +} + +func checkConnection(client *http.Client, url string, errorChannel chan error) { + resp, err := client.Get(url) + if err != nil { + errorChannel <- err + return + } + + _ = resp.Body.Close() + + if resp.StatusCode != 200 { + errorChannel <- fmt.Errorf("HTTP status code %d", resp.StatusCode) + return + } + + errorChannel <- nil +} diff --git a/pkg/pmapi/clientmanager.go b/pkg/pmapi/clientmanager.go index 765c9793..e63d81e9 100644 --- a/pkg/pmapi/clientmanager.go +++ b/pkg/pmapi/clientmanager.go @@ -305,72 +305,6 @@ func (cm *ClientManager) GetAuthUpdateChannel() chan ClientAuth { return cm.authUpdates } -// ErrNoInternetConnection indicates that both protonstatus and the API are unreachable. -var ErrNoInternetConnection = errors.New("no internet connection") - -// CheckConnection returns an error if there is no internet connection. -// This should be moved to the ConnectionManager when it is implemented. -func (cm *ClientManager) CheckConnection() error { - client := getHTTPClient(cm.config, cm.roundTripper, cm.cookieJar) - - // Do not cumulate timeouts, use goroutines. - retStatus := make(chan error) - retAPI := make(chan error) - - // vpn_status endpoint is fast and returns only OK. We check the connection only. - go checkConnection(client, "https://protonstatus.com/vpn_status", retStatus) - - // Check of API reachability also uses a fast endpoint. - go checkConnection(client, cm.GetRootURL()+"/tests/ping", retAPI) - - errStatus := <-retStatus - errAPI := <-retAPI - - switch { - case errStatus == nil && errAPI == nil: - return nil - - case errStatus == nil && errAPI != nil: - cm.log.Error("ProtonStatus is reachable but API is not") - return ErrAPINotReachable - - case errStatus != nil && errAPI == nil: - cm.log.Warn("API is reachable but protonstatus is not") - return nil - - case errStatus != nil && errAPI != nil: - cm.log.Error("Both ProtonStatus and API are unreachable") - return ErrNoInternetConnection - } - - return nil -} - -// CheckConnection returns an error if there is no internet connection. -func CheckConnection() error { - client := &http.Client{Timeout: time.Second * 10} - retStatus := make(chan error) - go checkConnection(client, "https://protonstatus.com/vpn_status", retStatus) - return <-retStatus -} - -func checkConnection(client *http.Client, url string, errorChannel chan error) { - resp, err := client.Get(url) - if err != nil { - errorChannel <- err - return - } - - _ = resp.Body.Close() - - if resp.StatusCode != 200 { - errorChannel <- fmt.Errorf("HTTP status code %d", resp.StatusCode) - return - } - - errorChannel <- nil -} - // setTokenIfUnset sets the token for the given userID if it wasn't already set. // The set token does not expire. func (cm *ClientManager) setTokenIfUnset(userID, token string) { diff --git a/pkg/pmapi/proxy_test.go b/pkg/pmapi/proxy_test.go index ababdf4d..e4e48dd1 100644 --- a/pkg/pmapi/proxy_test.go +++ b/pkg/pmapi/proxy_test.go @@ -382,7 +382,7 @@ func TestProxyProvider_UseProxy_RevertIfProxyStopsWorkingAndOriginalAPIIsReachab // The error should be ErrAPINotReachable because the connection dropped intermittently but // the original API is now reachable (see Alternative-Routing-v2 spec for details). url, err = cm.switchToReachableServer() - require.EqualError(t, err, ErrAPINotReachable.Error()) + require.Error(t, err) require.Equal(t, rootURL, url) require.Equal(t, rootURL, cm.getHost()) } diff --git a/pkg/pmapi/tlsreport.go b/pkg/pmapi/tlsreport.go index 0c948f55..90446926 100644 --- a/pkg/pmapi/tlsreport.go +++ b/pkg/pmapi/tlsreport.go @@ -35,13 +35,21 @@ var ErrTLSMismatch = errors.New("no TLS fingerprint match found") // TrustedAPIPins contains trusted public keys of the protonmail API and proxies. // NOTE: the proxy pins are the same for all proxy servers, guaranteed by infra team ;) var TrustedAPIPins = []string{ // nolint[gochecknoglobals] + // api.protonmail.ch `pin-sha256="drtmcR2kFkM8qJClsuWgUzxgBkePfRCkRpqUesyDmeE="`, // current - `pin-sha256="YRGlaY0jyJ4Jw2/4M8FIftwbDIQfh8Sdro96CeEel54="`, // hot - `pin-sha256="AfMENBVvOS8MnISprtvyPsjKlPooqh8nMB/pvCrpJpw="`, // cold - `pin-sha256="EU6TS9MO0L/GsDHvVc9D5fChYLNy5JdGYpJw0ccgetM="`, // proxy main - `pin-sha256="iKPIHPnDNqdkvOnTClQ8zQAIKG0XavaPkcEo0LBAABA="`, // proxy backup 1 - `pin-sha256="MSlVrBCdL0hKyczvgYVSRNm88RicyY04Q2y5qrBt0xA="`, // proxy backup 2 - `pin-sha256="C2UxW0T1Ckl9s+8cXfjXxlEqwAfPM4HiW2y3UdtBeCw="`, // proxy backup 3 + `pin-sha256="YRGlaY0jyJ4Jw2/4M8FIftwbDIQfh8Sdro96CeEel54="`, // hot backup + `pin-sha256="AfMENBVvOS8MnISprtvyPsjKlPooqh8nMB/pvCrpJpw="`, // cold backup + + // protonmail.com + `pin-sha256="8joiNBdqaYiQpKskgtkJsqRxF7zN0C0aqfi8DacknnI="`, // current + `pin-sha256="JMI8yrbc6jB1FYGyyWRLFTmDNgIszrNEMGlgy972e7w="`, // hot backup + `pin-sha256="Iu44zU84EOCZ9vx/vz67/MRVrxF1IO4i4NIa8ETwiIY="`, // cold backup + + // proxies + `pin-sha256="EU6TS9MO0L/GsDHvVc9D5fChYLNy5JdGYpJw0ccgetM="`, // main + `pin-sha256="iKPIHPnDNqdkvOnTClQ8zQAIKG0XavaPkcEo0LBAABA="`, // backup 1 + `pin-sha256="MSlVrBCdL0hKyczvgYVSRNm88RicyY04Q2y5qrBt0xA="`, // backup 2 + `pin-sha256="C2UxW0T1Ckl9s+8cXfjXxlEqwAfPM4HiW2y3UdtBeCw="`, // backup 3 } // TLSReportURI is the address where TLS reports should be sent.