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

fix(grpcHeaders): accept values with "=" symbols #8047

Conversation

artburkart
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Before this commit, it was not possible to pass in base64-encoded
content as a header value if it contained an equals sign. This commit
changes the behavior slightly. Rather than ignore key/value pairs where
the value happens to have an equals sign, we assume the first equals
sign delimits the key from the value and pass in the rest of the value
as-is.

Also, instead of printing the header name along with its value, we only
print the name, so there is less risk of leaking information into logs
that shouldn't be there.

Which issues(s) does this PR fix?

N/A

Other notes for review

This has not been tested in the context of the full validator client.
Instead, a small example was made to demonstrate the feasibility. The
example is shown here:

package main

import (
	"log"
	"os"
	"strings"

	"github.com/davecgh/go-spew/spew"
	"github.com/urfave/cli/v2"
)

type Config struct {
	GrpcHeadersFlag string
}

func main() {
	app := &cli.App{
		Action: func(c *cli.Context) error {
			for _, hdr := range strings.Split(c.String("grpc-headers"), ",") {
				if hdr != "" {
					ss := strings.Split(hdr, "=")
					spew.Dump(ss[0])
					spew.Dump(strings.Join(ss[1:], "="))
				}
			}
			return nil
		},
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name: "grpc-headers",
				Usage: "A comma-separated list of key value pairs to pass as gRPC headers for all gRPC " +
					"calls. Example: --grpc-headers=key=value",
			},
		},
	}

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal(err)
	}
}

Example invocation:

❯ go run main.go --grpc-headers=key=value,Authorization="Basic $(echo -n hello:world | base64)"
(string) (len=3) "key"
(string) (len=5) "value"
(string) (len=13) "Authorization"
(string) (len=22) "Basic aGVsbG86d29ybGQ="

@artburkart artburkart requested a review from a team as a code owner December 4, 2020 15:39
@CLAassistant
Copy link

CLAassistant commented Dec 4, 2020

CLA assistant check
All committers have signed the CLA.

rkapka
rkapka previously requested changes Dec 4, 2020
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for the contribution!

This piece of code was never tested, but the functionality was straightforward. Now that it got more complicated, can you please add a suite of tests for various header values? It will greatly eliminate the risk of introducing bugs in case we need to add more code on top.

@artburkart artburkart changed the title fix(grpcHeaders): accept values with equal signs fix(grpcHeaders): accept values with "=" symbols Dec 4, 2020
@artburkart
Copy link
Contributor Author

Hi @rkapka,

Do you know if there's any faster way to invoke the tests than this?

$ go test github.com/prysmaticlabs/prysm/validator/client
ok  	github.com/prysmaticlabs/prysm/validator/client	44.623s

If not, is it okay if we pull the snippet of code into something that can be unit tested?

@artburkart
Copy link
Contributor Author

Ah, it looks like when I run individual tests, it goes way faster.

# What

Before this commit, it was not possible to pass in base64-encoded
content as a header value if it contained an equals sign. This commit
changes the behavior slightly. Rather than ignore key/value pairs where
the value happens to have an equals sign, we assume the first equals
sign delimits the key from the value and pass in the rest of the value
as-is.

Also, instead of printing the header name along with its value, we
print the name, so there is less risk of leaking information into logs
that shouldn't be there.

# Testing

This has not been tested in the context of the full validator client.
Instead, a small example was made to demonstrate the feasibility. The
example is shown here:

```go
package main

import (
	"log"
	"os"
	"strings"

	"github.com/davecgh/go-spew/spew"
	"github.com/urfave/cli/v2"
)

type Config struct {
	GrpcHeadersFlag string
}

func main() {
	app := &cli.App{
		Action: func(c *cli.Context) error {
			for _, hdr := range strings.Split(c.String("grpc-headers"), ",") {
				if hdr != "" {
					ss := strings.Split(hdr, "=")
					spew.Dump(ss[0])
					spew.Dump(strings.Join(ss[1:], "="))
				}
			}
			return nil
		},
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name: "grpc-headers",
				Usage: "A comma-separated list of key value pairs to pass as gRPC headers for all gRPC " +
					"calls. Example: --grpc-headers=key=value",
			},
		},
	}

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal(err)
	}
}
```

Example invocation:

```command
❯ go run main.go --grpc-headers=key=value,Authorization="Basic $(echo -n hello:world | base64)"
(string) (len=3) "key"
(string) (len=5) "value"
(string) (len=13) "Authorization"
(string) (len=22) "Basic aGVsbG86d29ybGQ="
```
@artburkart
Copy link
Contributor Author

We've added 5 new tests cases. 👍

@artburkart
Copy link
Contributor Author

Ooops, I forgot to add a failure case. One moment please.

@artburkart artburkart force-pushed the accept-headers-with-equal-signs branch from d131573 to 3fe4935 Compare December 4, 2020 17:52
Comment on lines +78 to +90
for input, output := range map[string][]string{
"should-break": []string{},
"key=value": []string{"key", "value"},
"": []string{},
",": []string{},
"key=value,Authorization=Q=": []string{
"key", "value", "Authorization", "Q=",
},
"Authorization=this is a valid value": []string{
"Authorization", "this is a valid value",
},
} {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is considered too hacky, we can take the time to improve it.

prestonvanloon
prestonvanloon previously approved these changes Dec 4, 2020
@prestonvanloon
Copy link
Member

@artburkart. Please run gazelle. I tried to push to your branch but i dont have access.

bazel run //:gazelle

@prestonvanloon prestonvanloon dismissed rkapka’s stale review December 4, 2020 21:31

Requested changes were addressed

@prylabs-bulldozer prylabs-bulldozer bot merged commit 8ad328d into prysmaticlabs:develop Dec 4, 2020
@artburkart artburkart deleted the accept-headers-with-equal-signs branch December 4, 2020 21:38
@artburkart
Copy link
Contributor Author

Thank you! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants