From 9354391b5d38350a622cfcf0658a4656036f3da5 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Tue, 25 Jun 2024 12:20:51 +0200 Subject: [PATCH 01/14] wip --- internal/cmd/miniooni/main.go | 8 +++++ internal/cmd/miniooni/oonirun.go | 1 + internal/oonirun/link.go | 4 +++ internal/oonirun/v2.go | 57 +++++++++++++++++++++++++++++--- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/cmd/miniooni/main.go b/internal/cmd/miniooni/main.go index db9e71da9b..590bde732f 100644 --- a/internal/cmd/miniooni/main.go +++ b/internal/cmd/miniooni/main.go @@ -26,6 +26,7 @@ import ( // Options contains the options you can set from the CLI. type Options struct { Annotations []string + AuthFile string Emoji bool ExtraOptions []string HomeDir string @@ -228,6 +229,13 @@ func registerOONIRun(rootCmd *cobra.Command, globalOptions *Options) { []string{}, "Path to the OONI Run v2 descriptor to run (may be specified multiple times)", ) + flags.StringVarP( + &globalOptions.AuthFile, + "auth-file", + "a", + "", + "Path to a file with authentication headers (for fetching a remote OONI Run v2 descriptor)", + ) } // registerAllExperiments registers a subcommand for each experiment diff --git a/internal/cmd/miniooni/oonirun.go b/internal/cmd/miniooni/oonirun.go index 366839badb..d4c2a2690f 100644 --- a/internal/cmd/miniooni/oonirun.go +++ b/internal/cmd/miniooni/oonirun.go @@ -21,6 +21,7 @@ func ooniRunMain(ctx context.Context, logger := sess.Logger() cfg := &oonirun.LinkConfig{ AcceptChanges: currentOptions.Yes, + AuthFile: currentOptions.AuthFile, Annotations: annotations, KVStore: sess.KeyValueStore(), MaxRuntime: currentOptions.MaxRuntime, diff --git a/internal/oonirun/link.go b/internal/oonirun/link.go index d5ceba2652..dbae82d0b4 100644 --- a/internal/oonirun/link.go +++ b/internal/oonirun/link.go @@ -19,6 +19,10 @@ type LinkConfig struct { // reviewing what it contains or what has changed. AcceptChanges bool + // AuthFile is OPTIONAL and will add any authentication headers to the request + // for fetching this OONI Run link. + AuthFile string + // Annotations contains OPTIONAL Annotations for the experiment. Annotations map[string]string diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 6552ad582e..fb02d4544f 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -5,11 +5,15 @@ package oonirun // import ( + "bufio" "context" "encoding/json" "errors" "fmt" + "os" + "strings" "sync/atomic" + "unicode" "github.com/hexops/gotextdiff" "github.com/hexops/gotextdiff/myers" @@ -63,12 +67,16 @@ type V2Nettest struct { // getV2DescriptorFromHTTPSURL GETs a v2Descriptor instance from // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, - logger model.Logger, URL string) (*V2Descriptor, error) { + logger model.Logger, URL, auth string) (*V2Descriptor, error) { + if auth != "" { + // we assume a bearer token + auth = fmt.Sprintf("Bearer %s", auth) + } return httpclientx.GetJSON[*V2Descriptor]( ctx, httpclientx.NewEndpoint(URL), &httpclientx.Config{ - Authorization: "", // not needed + Authorization: auth, Client: client, Logger: logger, UserAgent: model.HTTPHeaderUserAgent, @@ -140,9 +148,9 @@ func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, err // - err is the error that occurred, or nil in case of success. func (cache *v2DescriptorCache) PullChangesWithoutSideEffects( ctx context.Context, client model.HTTPClient, logger model.Logger, - URL string) (oldValue, newValue *V2Descriptor, err error) { + URL, auth string) (oldValue, newValue *V2Descriptor, err error) { oldValue = cache.Entries[URL] - newValue, err = getV2DescriptorFromHTTPSURL(ctx, client, logger, URL) + newValue, err = getV2DescriptorFromHTTPSURL(ctx, client, logger, URL, auth) return } @@ -263,7 +271,11 @@ func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { // pull a possibly new descriptor without updating the old descriptor clnt := config.Session.DefaultHTTPClient() - oldValue, newValue, err := cache.PullChangesWithoutSideEffects(ctx, clnt, logger, URL) + auth, err := maybeGetAuthenticationTokenFromFile(config.AuthFile) + if err != nil { + logger.Warnf("oonirun: failed to retrieve auth token: %v", err) + } + oldValue, newValue, err := cache.PullChangesWithoutSideEffects(ctx, clnt, logger, URL, auth) if err != nil { return err } @@ -290,3 +302,38 @@ func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { // note: this function gracefully handles nil values return V2MeasureDescriptor(ctx, config, newValue) } + +func maybeGetAuthenticationTokenFromFile(path string) (string, error) { + if path != "" { + return readFirstLineFromFile(path) + } + return "", nil +} + +// readFirstLineFromFile reads the first line of the passed text file and trims any non-printable characters. +func readFirstLineFromFile(filePath string) (string, error) { + f, err := os.Open(filePath) + if err != nil { + return "", err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + + // Scan the first line + if scanner.Scan() { + line := scanner.Text() + trimmed := strings.TrimFunc(line, func(r rune) bool { + return !unicode.IsPrint(r) + }) + return trimmed, nil + } + + // Check for any scanning error + if err := scanner.Err(); err != nil { + return "", err + } + + // Return empty string if file is empty + return "", nil +} From fe40b60568643118aece0315a6e0a60340e305bf Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Tue, 25 Jun 2024 17:19:50 +0200 Subject: [PATCH 02/14] add tests --- internal/cmd/miniooni/auth.txt | 1 + internal/oonirun/v2_test.go | 127 ++++++++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 internal/cmd/miniooni/auth.txt diff --git a/internal/cmd/miniooni/auth.txt b/internal/cmd/miniooni/auth.txt new file mode 100644 index 0000000000..d97c5eada5 --- /dev/null +++ b/internal/cmd/miniooni/auth.txt @@ -0,0 +1 @@ +secret diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index b77b9b7bf7..249bef9b25 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "net/http/httptest" + "os" "testing" "github.com/ooni/probe-cli/v3/internal/httpclientx" @@ -262,6 +263,131 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { } } +func TestOONIRunV2LinkWithAuthentication(t *testing.T) { + + t.Run("authentication raises error if no token is passed", func(t *testing.T) { + token := "secret-token" + bearerToken := "Bearer " + token + + // make a local server that returns a reasonable descriptor for the example experiment + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authHeader := r.Header.Get("Authorization") + if authHeader != bearerToken { + // If the header is not what expected, return a 401 Unauthorized status + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + descriptor := &V2Descriptor{ + Name: "", + Description: "", + Author: "", + Nettests: []V2Nettest{{ + Inputs: []string{}, + Options: map[string]any{ + "SleepTime": int64(10 * time.Millisecond), + }, + TestName: "example", + }}, + } + data, err := json.Marshal(descriptor) + runtimex.PanicOnError(err, "json.Marshal failed") + w.Write(data) + })) + + defer server.Close() + ctx := context.Background() + + // create a minimal link configuration + config := &LinkConfig{ + AcceptChanges: true, // avoid "oonirun: need to accept changes" error + Annotations: map[string]string{ + "platform": "linux", + }, + KVStore: &kvstore.Memory{}, + MaxRuntime: 0, + NoCollector: true, + NoJSON: true, + Random: false, + ReportFile: "", + Session: newMinimalFakeSession(), + } + + // construct a link runner relative to the local server URL + r := NewLinkRunner(config, server.URL) + + if err := r.Run(ctx); err != nil { + if err.Error() != "httpx: request failed" { + t.Fatal("expected error") + } + } + }) + + t.Run("authentication does not fail the auth token is passed", func(t *testing.T) { + token := "secret-token" + bearerToken := "Bearer " + token + + // make a local server that returns a reasonable descriptor for the example experiment + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authHeader := r.Header.Get("Authorization") + if authHeader != bearerToken { + // If the header is not what expected, return a 401 Unauthorized status + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + descriptor := &V2Descriptor{ + Name: "", + Description: "", + Author: "", + Nettests: []V2Nettest{{ + Inputs: []string{}, + Options: map[string]any{ + "SleepTime": int64(10 * time.Millisecond), + }, + TestName: "example", + }}, + } + data, err := json.Marshal(descriptor) + runtimex.PanicOnError(err, "json.Marshal failed") + w.Write(data) + })) + + defer server.Close() + ctx := context.Background() + + authFile, err := os.CreateTemp(t.TempDir(), "token-") + if err != nil { + t.Fatal(err) + } + defer authFile.Close() + defer os.Remove(authFile.Name()) + + authFile.Write([]byte(token)) + + // create a minimal link configuration + config := &LinkConfig{ + AcceptChanges: true, // avoid "oonirun: need to accept changes" error + Annotations: map[string]string{ + "platform": "linux", + }, + AuthFile: authFile.Name(), + KVStore: &kvstore.Memory{}, + MaxRuntime: 0, + NoCollector: true, + NoJSON: true, + Random: false, + ReportFile: "", + Session: newMinimalFakeSession(), + } + + // construct a link runner relative to the local server URL + r := NewLinkRunner(config, server.URL) + + if err := r.Run(ctx); err != nil { + t.Fatal(err) + } + }) +} + func TestOONIRunV2LinkConnectionResetByPeer(t *testing.T) { // create a local server that will reset the connection immediately. // actually contains an empty test name, which is what we want to test @@ -509,7 +635,6 @@ func TestV2MeasureHTTPS(t *testing.T) { t.Fatal("unexpected err", err) } }) - } func TestV2DescriptorCacheLoad(t *testing.T) { From 81c5d2ce89b20c0f9fc2439e109df782ac0b9e46 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Tue, 25 Jun 2024 17:32:45 +0200 Subject: [PATCH 03/14] add tests --- internal/oonirun/v2_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index 249bef9b25..583169dcac 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "testing" "github.com/ooni/probe-cli/v3/internal/httpclientx" @@ -660,3 +661,35 @@ func TestV2DescriptorCacheLoad(t *testing.T) { } }) } + +func Test_readFirstLineFromFile(t *testing.T) { + + t.Run("return empty string if file is empty", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + f.Write([]byte("")) + defer f.Close() + defer os.Remove(f.Name()) + + line, err := readFirstLineFromFile(f.Name()) + if line != "" { + t.Fatal("expected empty string") + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + + t.Run("return error if file does not exist", func(t *testing.T) { + line, err := readFirstLineFromFile(filepath.Join(t.TempDir(), "non-existent")) + if line != "" { + t.Fatal("expected empty string") + } + if !errors.Is(err, os.ErrNotExist) { + t.Fatal("expected ErrNotExist") + } + }) + +} From 98312748cd8208a1950d6d6c60a8b0e7a0d3ddab Mon Sep 17 00:00:00 2001 From: Ain Ghazal <99027643+ainghazal@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:25:50 +0200 Subject: [PATCH 04/14] Update internal/cmd/miniooni/main.go Co-authored-by: Simone Basso --- internal/cmd/miniooni/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cmd/miniooni/main.go b/internal/cmd/miniooni/main.go index 590bde732f..26d86ef3fb 100644 --- a/internal/cmd/miniooni/main.go +++ b/internal/cmd/miniooni/main.go @@ -234,7 +234,7 @@ func registerOONIRun(rootCmd *cobra.Command, globalOptions *Options) { "auth-file", "a", "", - "Path to a file with authentication headers (for fetching a remote OONI Run v2 descriptor)", + "Path to a file containing a bearer token for fetching a remote OONI Run v2 descriptor", ) } From 006929dbcd81da6b1878ec3df001f1b0a3fbe076 Mon Sep 17 00:00:00 2001 From: Ain Ghazal <99027643+ainghazal@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:26:07 +0200 Subject: [PATCH 05/14] Update internal/oonirun/link.go Co-authored-by: Simone Basso --- internal/oonirun/link.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oonirun/link.go b/internal/oonirun/link.go index dbae82d0b4..3321db03b0 100644 --- a/internal/oonirun/link.go +++ b/internal/oonirun/link.go @@ -19,8 +19,8 @@ type LinkConfig struct { // reviewing what it contains or what has changed. AcceptChanges bool - // AuthFile is OPTIONAL and will add any authentication headers to the request - // for fetching this OONI Run link. + // AuthFile is OPTIONAL and will add an authentication header to the + // request used for fetching this OONI Run link. AuthFile string // Annotations contains OPTIONAL Annotations for the experiment. From 3177b9c25dc5ce62c8ed6597c38766fde805a2cf Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 14:27:43 +0200 Subject: [PATCH 06/14] remove test file --- internal/cmd/miniooni/auth.txt | 1 - 1 file changed, 1 deletion(-) delete mode 100644 internal/cmd/miniooni/auth.txt diff --git a/internal/cmd/miniooni/auth.txt b/internal/cmd/miniooni/auth.txt deleted file mode 100644 index d97c5eada5..0000000000 --- a/internal/cmd/miniooni/auth.txt +++ /dev/null @@ -1 +0,0 @@ -secret From aa98295b5456a10c8855d69b57d09215bb32633a Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 14:31:32 +0200 Subject: [PATCH 07/14] use fsx.OpenFile --- internal/oonirun/v2.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index fb02d4544f..3ed915a026 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -10,7 +10,6 @@ import ( "encoding/json" "errors" "fmt" - "os" "strings" "sync/atomic" "unicode" @@ -18,6 +17,7 @@ import ( "github.com/hexops/gotextdiff" "github.com/hexops/gotextdiff/myers" "github.com/hexops/gotextdiff/span" + "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" @@ -311,8 +311,8 @@ func maybeGetAuthenticationTokenFromFile(path string) (string, error) { } // readFirstLineFromFile reads the first line of the passed text file and trims any non-printable characters. -func readFirstLineFromFile(filePath string) (string, error) { - f, err := os.Open(filePath) +func readFirstLineFromFile(filep string) (string, error) { + f, err := fsx.OpenFile(filep) if err != nil { return "", err } From 9ac9fe54866f7075297479b5719475ebc3cbb7ba Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 14:57:23 +0200 Subject: [PATCH 08/14] add tests --- internal/oonirun/v2_test.go | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index 583169dcac..a9e6f1573e 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -692,4 +692,46 @@ func Test_readFirstLineFromFile(t *testing.T) { } }) + t.Run("return first line with a file of one line", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + + token := "asecret" + f.Write([]byte(token)) + defer f.Close() + defer os.Remove(f.Name()) + + line, err := readFirstLineFromFile(f.Name()) + if line != token { + t.Fatalf("expected %s, got %s", token, line) + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + + t.Run("return first line with a file of >1 line", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + + token := "asecret" + f.Write([]byte(token)) + f.Write([]byte("\n")) + f.Write([]byte("something\nelse\nand\nsomething\nmore")) + defer f.Close() + defer os.Remove(f.Name()) + + line, err := readFirstLineFromFile(f.Name()) + if line != token { + t.Fatalf("expected %s, got %s", token, line) + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + } From c8867b1614d7da7871a1309e60c9ac5b20f93709 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 15:05:24 +0200 Subject: [PATCH 09/14] fix after rebase --- internal/oonirun/v2_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index a9e6f1573e..f54ed827d2 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -284,9 +284,9 @@ func TestOONIRunV2LinkWithAuthentication(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -341,9 +341,9 @@ func TestOONIRunV2LinkWithAuthentication(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } From a4e364674ec1feb817cfad289ab66e532213973d Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 15:56:03 +0200 Subject: [PATCH 10/14] refactor tokenizing function to pick just base64 --- internal/oonirun/v2.go | 31 ++++++++++++-- internal/oonirun/v2_test.go | 80 +++++++++++++++++++++++++++++++++---- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 3ed915a026..974be9790d 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -7,6 +7,7 @@ package oonirun import ( "bufio" "context" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -305,13 +306,16 @@ func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { func maybeGetAuthenticationTokenFromFile(path string) (string, error) { if path != "" { - return readFirstLineFromFile(path) + return readBearerTokenFromFile(path) } return "", nil } -// readFirstLineFromFile reads the first line of the passed text file and trims any non-printable characters. -func readFirstLineFromFile(filep string) (string, error) { +// readBearerTokenFromFile tries to extract a valid (base64) bearer token from +// the first line of the passed text file. +// If there is an error while reading from the file, the error will be returned. +// If we can read from the file but there's no valid token found, an empty string will be returned. +func readBearerTokenFromFile(filep string) (string, error) { f, err := fsx.OpenFile(filep) if err != nil { return "", err @@ -323,10 +327,29 @@ func readFirstLineFromFile(filep string) (string, error) { // Scan the first line if scanner.Scan() { line := scanner.Text() + + // trim any non printable characters (like control chars) trimmed := strings.TrimFunc(line, func(r rune) bool { return !unicode.IsPrint(r) }) - return trimmed, nil + + // tokenize by whitespace + tokens := strings.Fields(trimmed) + + // return empty string if tokens is empty + if len(tokens) <= 0 { + return "", nil + } + + // ignore all tokens after the first + token := tokens[0] + + // if this is not a valid base64 token, return empty string + if _, err := base64.StdEncoding.DecodeString(token); err != nil { + return "", nil + } + + return token, nil } // Check for any scanning error diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index f54ed827d2..4cd58d0140 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -267,7 +267,7 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { func TestOONIRunV2LinkWithAuthentication(t *testing.T) { t.Run("authentication raises error if no token is passed", func(t *testing.T) { - token := "secret-token" + token := "c2VjcmV0" bearerToken := "Bearer " + token // make a local server that returns a reasonable descriptor for the example experiment @@ -324,7 +324,7 @@ func TestOONIRunV2LinkWithAuthentication(t *testing.T) { }) t.Run("authentication does not fail the auth token is passed", func(t *testing.T) { - token := "secret-token" + token := "c2VjcmV0" bearerToken := "Bearer " + token // make a local server that returns a reasonable descriptor for the example experiment @@ -673,7 +673,25 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readFirstLineFromFile(f.Name()) + line, err := readBearerTokenFromFile(f.Name()) + if line != "" { + t.Fatal("expected empty string") + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + + t.Run("return empty string if first line is just whitespace", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + f.Write([]byte(" \n")) + defer f.Close() + defer os.Remove(f.Name()) + + line, err := readBearerTokenFromFile(f.Name()) if line != "" { t.Fatal("expected empty string") } @@ -683,7 +701,7 @@ func Test_readFirstLineFromFile(t *testing.T) { }) t.Run("return error if file does not exist", func(t *testing.T) { - line, err := readFirstLineFromFile(filepath.Join(t.TempDir(), "non-existent")) + line, err := readBearerTokenFromFile(filepath.Join(t.TempDir(), "non-existent")) if line != "" { t.Fatal("expected empty string") } @@ -698,12 +716,12 @@ func Test_readFirstLineFromFile(t *testing.T) { t.Fatal(err) } - token := "asecret" + token := "c2VjcmV0" // b64("secret") f.Write([]byte(token)) defer f.Close() defer os.Remove(f.Name()) - line, err := readFirstLineFromFile(f.Name()) + line, err := readBearerTokenFromFile(f.Name()) if line != token { t.Fatalf("expected %s, got %s", token, line) } @@ -718,14 +736,36 @@ func Test_readFirstLineFromFile(t *testing.T) { t.Fatal(err) } - token := "asecret" + token := "c2VjcmV0" // b64("secret") f.Write([]byte(token)) f.Write([]byte("\n")) f.Write([]byte("something\nelse\nand\nsomething\nmore")) defer f.Close() defer os.Remove(f.Name()) - line, err := readFirstLineFromFile(f.Name()) + line, err := readBearerTokenFromFile(f.Name()) + if line != token { + t.Fatalf("expected %s, got %s", token, line) + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + + t.Run("return only first token if >1 is found", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + + token := "c2VjcmV0" // b64("secret") + f.Write([]byte(token)) + f.Write([]byte("\n")) + f.Write([]byte(" antani\n")) + defer f.Close() + defer os.Remove(f.Name()) + + line, err := readBearerTokenFromFile(f.Name()) if line != token { t.Fatalf("expected %s, got %s", token, line) } @@ -734,4 +774,28 @@ func Test_readFirstLineFromFile(t *testing.T) { } }) + t.Run("return empty string if not a valid b64 token", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "auth-") + if err != nil { + t.Fatal(err) + } + + token := "secret!" + f.Write([]byte(token)) + f.Write([]byte("\n")) + f.Write([]byte(" antani\n")) + defer f.Close() + defer os.Remove(f.Name()) + + expected := "" + + line, err := readBearerTokenFromFile(f.Name()) + if line != expected { + t.Fatalf("expected empty string, got %s", line) + } + if err != nil { + t.Fatal("expected err==nil") + } + }) + } From da1e5b485eada85153f366c59a8d09eaa58c9f66 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 18:31:29 +0400 Subject: [PATCH 11/14] chore: minimal coding style changes No functional change. --- internal/oonirun/v2.go | 16 ++++++++-------- internal/oonirun/v2_test.go | 14 +++++++------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 974be9790d..625e44e4b1 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -272,7 +272,7 @@ func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { // pull a possibly new descriptor without updating the old descriptor clnt := config.Session.DefaultHTTPClient() - auth, err := maybeGetAuthenticationTokenFromFile(config.AuthFile) + auth, err := v2MaybeGetAuthenticationTokenFromFile(config.AuthFile) if err != nil { logger.Warnf("oonirun: failed to retrieve auth token: %v", err) } @@ -304,25 +304,25 @@ func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { return V2MeasureDescriptor(ctx, config, newValue) } -func maybeGetAuthenticationTokenFromFile(path string) (string, error) { +func v2MaybeGetAuthenticationTokenFromFile(path string) (string, error) { if path != "" { - return readBearerTokenFromFile(path) + return v2ReadBearerTokenFromFile(path) } return "", nil } -// readBearerTokenFromFile tries to extract a valid (base64) bearer token from +// v2ReadBearerTokenFromFile tries to extract a valid (base64) bearer token from // the first line of the passed text file. // If there is an error while reading from the file, the error will be returned. // If we can read from the file but there's no valid token found, an empty string will be returned. -func readBearerTokenFromFile(filep string) (string, error) { - f, err := fsx.OpenFile(filep) +func v2ReadBearerTokenFromFile(fileName string) (string, error) { + filep, err := fsx.OpenFile(fileName) if err != nil { return "", err } - defer f.Close() + defer filep.Close() - scanner := bufio.NewScanner(f) + scanner := bufio.NewScanner(filep) // Scan the first line if scanner.Scan() { diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index 4cd58d0140..d75761c4d7 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -673,7 +673,7 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != "" { t.Fatal("expected empty string") } @@ -691,7 +691,7 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != "" { t.Fatal("expected empty string") } @@ -701,7 +701,7 @@ func Test_readFirstLineFromFile(t *testing.T) { }) t.Run("return error if file does not exist", func(t *testing.T) { - line, err := readBearerTokenFromFile(filepath.Join(t.TempDir(), "non-existent")) + line, err := v2ReadBearerTokenFromFile(filepath.Join(t.TempDir(), "non-existent")) if line != "" { t.Fatal("expected empty string") } @@ -721,7 +721,7 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != token { t.Fatalf("expected %s, got %s", token, line) } @@ -743,7 +743,7 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != token { t.Fatalf("expected %s, got %s", token, line) } @@ -765,7 +765,7 @@ func Test_readFirstLineFromFile(t *testing.T) { defer f.Close() defer os.Remove(f.Name()) - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != token { t.Fatalf("expected %s, got %s", token, line) } @@ -789,7 +789,7 @@ func Test_readFirstLineFromFile(t *testing.T) { expected := "" - line, err := readBearerTokenFromFile(f.Name()) + line, err := v2ReadBearerTokenFromFile(f.Name()) if line != expected { t.Fatalf("expected empty string, got %s", line) } From 274961fb96dd5c71ec72a7e4819b1df8484ebbc7 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Thu, 27 Jun 2024 16:42:57 +0200 Subject: [PATCH 12/14] remove unneeded complexity --- internal/oonirun/v2.go | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 625e44e4b1..c2a3ca065d 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -13,7 +13,6 @@ import ( "fmt" "strings" "sync/atomic" - "unicode" "github.com/hexops/gotextdiff" "github.com/hexops/gotextdiff/myers" @@ -328,21 +327,7 @@ func v2ReadBearerTokenFromFile(fileName string) (string, error) { if scanner.Scan() { line := scanner.Text() - // trim any non printable characters (like control chars) - trimmed := strings.TrimFunc(line, func(r rune) bool { - return !unicode.IsPrint(r) - }) - - // tokenize by whitespace - tokens := strings.Fields(trimmed) - - // return empty string if tokens is empty - if len(tokens) <= 0 { - return "", nil - } - - // ignore all tokens after the first - token := tokens[0] + token := strings.TrimSpace(line) // if this is not a valid base64 token, return empty string if _, err := base64.StdEncoding.DecodeString(token); err != nil { From 9b010490aea83308846779e395e4fc6d6fbe5fc7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 18:53:51 +0400 Subject: [PATCH 13/14] minor rework of test cases - we don't need to care about tokens - we need to care about existing/nonexisting newline at EOL when there's just a single line in the file --- internal/oonirun/v2_test.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index d75761c4d7..63d3075af0 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -710,7 +710,7 @@ func Test_readFirstLineFromFile(t *testing.T) { } }) - t.Run("return first line with a file of one line", func(t *testing.T) { + t.Run("return first line with a file of one line without EOL", func(t *testing.T) { f, err := os.CreateTemp(t.TempDir(), "auth-") if err != nil { t.Fatal(err) @@ -730,16 +730,14 @@ func Test_readFirstLineFromFile(t *testing.T) { } }) - t.Run("return first line with a file of >1 line", func(t *testing.T) { + t.Run("return first line with a file of one line with EOL", func(t *testing.T) { f, err := os.CreateTemp(t.TempDir(), "auth-") if err != nil { t.Fatal(err) } token := "c2VjcmV0" // b64("secret") - f.Write([]byte(token)) - f.Write([]byte("\n")) - f.Write([]byte("something\nelse\nand\nsomething\nmore")) + f.Write(append([]byte(token), '\n')) defer f.Close() defer os.Remove(f.Name()) @@ -752,7 +750,7 @@ func Test_readFirstLineFromFile(t *testing.T) { } }) - t.Run("return only first token if >1 is found", func(t *testing.T) { + t.Run("return first line with a file of >1 line", func(t *testing.T) { f, err := os.CreateTemp(t.TempDir(), "auth-") if err != nil { t.Fatal(err) @@ -761,7 +759,7 @@ func Test_readFirstLineFromFile(t *testing.T) { token := "c2VjcmV0" // b64("secret") f.Write([]byte(token)) f.Write([]byte("\n")) - f.Write([]byte(" antani\n")) + f.Write([]byte("something\nelse\nand\nsomething\nmore")) defer f.Close() defer os.Remove(f.Name()) @@ -797,5 +795,4 @@ func Test_readFirstLineFromFile(t *testing.T) { t.Fatal("expected err==nil") } }) - } From bf1cacd7244d83f9a7e49b06f457d25b35d5114d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 19:06:25 +0400 Subject: [PATCH 14/14] change the name of the flag --- internal/cmd/miniooni/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cmd/miniooni/main.go b/internal/cmd/miniooni/main.go index 26d86ef3fb..11add9c7e4 100644 --- a/internal/cmd/miniooni/main.go +++ b/internal/cmd/miniooni/main.go @@ -231,8 +231,8 @@ func registerOONIRun(rootCmd *cobra.Command, globalOptions *Options) { ) flags.StringVarP( &globalOptions.AuthFile, - "auth-file", - "a", + "bearer-token-file", + "", "", "Path to a file containing a bearer token for fetching a remote OONI Run v2 descriptor", )