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.

Sign up for GitHub

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

Jump to bottom

Add a header type field #4993

Merged
merged 13 commits into from
Aug 13, 2018
Merged

Add a header type field #4993

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7b86f41
add a header type field
Jul 25, 2018
fdbb32a
chris feedback changes
Jul 27, 2018
7ad2511
strip MIME conversion as Add does it
Jul 27, 2018
bc18aa0
Merge branch 'opensource-master' into add-type-headers
Jul 27, 2018
a3a7cb3
add wrapper for http header
Jul 30, 2018
a3286a9
update comment
Jul 30, 2018
ef89439
strip outdated comments
Jul 30, 2018
d4fb65f
move header to parseutil
Jul 30, 2018
f0443e0
use http.Header type
Aug 7, 2018
b190a4e
Merge branch 'opensource-master' into add-type-headers
Aug 7, 2018
b1cd520
add support for b64-encoded json headers
Aug 9, 2018
ab5a906
support headers as json strings or b64d json
Aug 10, 2018
f486653
fix comment typo
Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions logical/framework/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"regexp"
"sort"
"strings"
Expand Down Expand Up @@ -548,6 +549,8 @@ func (t FieldType) Zero() interface{} {
return []string{}
case TypeCommaIntSlice:
return []int{}
case TypeHeader:
return http.Header{}
default:
panic("unknown type: " + t.String())
}
Expand Down
7 changes: 7 additions & 0 deletions logical/framework/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"testing"
"time"

"net/http"

"github.com/hashicorp/vault/logical"
)

Expand Down Expand Up @@ -532,6 +534,11 @@ func TestFieldSchemaDefaultOrZero(t *testing.T) {
&FieldSchema{Type: TypeDurationSecond},
0,
},

"default header not set": {
&FieldSchema{Type: TypeHeader},
http.Header{},
},
}

for name, tc := range cases {
Expand Down
41 changes: 39 additions & 2 deletions logical/framework/field_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"encoding/json"
"errors"
"fmt"
"net/http"
"net/textproto"
"regexp"
"strings"

Expand Down Expand Up @@ -37,7 +39,7 @@ func (d *FieldData) Validate() error {
switch schema.Type {
case TypeBool, TypeInt, TypeMap, TypeDurationSecond, TypeString, TypeLowerCaseString,
TypeNameString, TypeSlice, TypeStringSlice, TypeCommaStringSlice,
TypeKVPairs, TypeCommaIntSlice:
TypeKVPairs, TypeCommaIntSlice, TypeHeader:
_, _, err := d.getPrimitive(field, schema)
if err != nil {
return errwrap.Wrapf(fmt.Sprintf("error converting input %v for field %q: {{err}}", value, field), err)
Expand Down Expand Up @@ -126,7 +128,7 @@ func (d *FieldData) GetOkErr(k string) (interface{}, bool, error) {
switch schema.Type {
case TypeBool, TypeInt, TypeMap, TypeDurationSecond, TypeString, TypeLowerCaseString,
TypeNameString, TypeSlice, TypeStringSlice, TypeCommaStringSlice,
TypeKVPairs, TypeCommaIntSlice:
TypeKVPairs, TypeCommaIntSlice, TypeHeader:
return d.getPrimitive(k, schema)
default:
return nil, false,
Expand Down Expand Up @@ -291,6 +293,41 @@ func (d *FieldData) getPrimitive(k string, schema *FieldSchema) (interface{}, bo
}
return result, true, nil

case TypeHeader:
// First try to parse this as a map
var mapResult map[string][]string
if err := mapstructure.WeakDecode(raw, &mapResult); err == nil {
// The http.Header Get method is case-insensitive in the key it takes,
// but searches the map for a matching canonicalized value. Most headers
// will arrive in this format, but just in case they don't, let's
// canonicalize all the keys.
result := http.Header{}
for k, slice := range mapResult {
canonicalKey := textproto.CanonicalMIMEHeaderKey(k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to do this step? It seems to me you would want to preserve formatting and any comparisons you would retrieve the header using the Get function and do any normalized text compares there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely weird.

The reason I do it is because if you send in a header of "hello:world" and then later do header.Get("hello"), it doesn't find "world". That's because under the hood, the Get method first converts "hello" to "Hello" and then looks for it in the map.

I thought this was a really weird behavior, and tried to think through how to best achieve what someone would expect. I thought about, instead of using an http.Header, maybe I should just use a straight-out map. But then, people are sending in headers so maybe they would expect that kind of behavior.

In the end, I did this because I thought from a usability perspective it might be the best option. But I'm open to other approaches because it's a tough call.

Copy link
Member

Choose a reason for hiding this comment

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

I am relatively certain that colons are illegal in header key values, which would explain this behavior.

Copy link
Contributor Author

@tyrannosaurus-becks tyrannosaurus-becks Jul 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if I explained that well LOL. In the example of "hello:world" I was thinking of "hello" as the key and "world" as the value. Probably would have been a better example if I'd done JSON or something.

I was trying to say that if you give it a lower-case key of hello, then later try to pull out a lower-case hello, it finds nothing. And it's because under the hood, it converts it to Title Case before searching for it in the map, so it's looking for "Hello" while it's sitting in the map as "hello", and thus misses it. It converts it to Title Case using that canonical method.

So by also calling that canonical method on keys on the way in, it also Title Cases the keys in the map so they'll have hits. This is definitely a trade-off because, while you'll now have case-insensitive hits using the header.Get("hello") method, if you need to pull headers out like this: header["hello"] it won't work anymore. You'd use the Get method for headers that you know only have one value, and the map-key-accessing method for headers that may have multiple values.

Ultimately, I'm honestly not super in love with the magic the http.Header type is doing because I think it's actually pretty weird. This is why I'm tempted to just turn it into a map[string][]string, and to do away with the canonical stuff, so it's way more evident and straight-forward how to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I too struggled with this type for whatever reason, it is challenging to use. Maybe you're doing a little too much sanitation and you just need to pass through what they give you and serialize it directly to disk when necessary. Here is the code I came up with dealing with headers, config, and output. https://github.com/hashicorp/vault/blob/master/vault/ui.go

Copy link
Member

Choose a reason for hiding this comment

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

Probably you shouldn't do any sanitization at all -- you should get it to the correct type and just copy it over verbatim. There's no need for you to do any textproto stuff when Header is doing it anyways.

for _, v := range slice {
result.Add(canonicalKey, v)
}
}
return result, true, nil
}

// If map parse fails, parse as a string list of : delimited pairs
var listResult []string
if err := mapstructure.WeakDecode(raw, &listResult); err != nil {
return nil, true, err
}

result := http.Header{}
for _, keyPair := range listResult {
keyPairSlice := strings.SplitN(keyPair, ":", 2)
if len(keyPairSlice) != 2 || keyPairSlice[0] == "" {
return nil, false, fmt.Errorf("invalid key pair %q", keyPair)
}
// See note above about why we're canonicalizing.
result.Add(textproto.CanonicalMIMEHeaderKey(keyPairSlice[0]), keyPairSlice[1])
}
return result, true, nil

default:
panic(fmt.Sprintf("Unknown type: %s", schema.Type))
}
Expand Down
59 changes: 59 additions & 0 deletions logical/framework/field_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package framework

import (
"net/http"
"reflect"
"testing"
)
Expand Down Expand Up @@ -467,6 +468,55 @@ func TestFieldDataGet(t *testing.T) {
},
},

"type header, keypair string array": {
map[string]*FieldSchema{
"foo": {Type: TypeHeader},
},
map[string]interface{}{
"foo": []interface{}{"key1:value1", "key2:value2", "key3:1"},
},
"foo",
http.Header{
"Key1": []string{"value1"},
"Key2": []string{"value2"},
"Key3": []string{"1"},
},
},

"type header, keypair string array with dupe key": {
map[string]*FieldSchema{
"foo": {Type: TypeHeader},
},
map[string]interface{}{
"foo": []interface{}{"key1:value1", "key2:value2", "key3:1", "key3:true"},
},
"foo",
http.Header{
"Key1": []string{"value1"},
"Key2": []string{"value2"},
"Key3": []string{"1", "true"},
},
},

"type header, map string slice": {
map[string]*FieldSchema{
"foo": {Type: TypeHeader},
},
map[string]interface{}{
"foo": map[string][]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the JSON Unmarshaller will return this as a map[string][]interface{}, can you test some non-string value and make sure you get what you expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

@tyrannosaurus-becks tyrannosaurus-becks Jul 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It was a good test!

I ran Vault in dev mode and made a little path that took TypeHeader for a field named headers. Then I used the following command:

vault write auth/somewhere/foo headers='hello:world' headers='hello:true' headers="fizz:1"

And it actually parsed both the faux bool and the faux int field with no error, and they came in as strings, which is what I would want for a header.

screenshot from 2018-07-27 10-03-35

"key1": {"value1"},
"key2": {"value2"},
"key3": {"1"},
},
},
"foo",
http.Header{
"Key1": []string{"value1"},
"Key2": []string{"value2"},
"Key3": []string{"1"},
},
},

"name string type, not supplied": {
map[string]*FieldSchema{
"foo": {Type: TypeNameString},
Expand Down Expand Up @@ -556,6 +606,15 @@ func TestFieldDataGet(t *testing.T) {
"foo",
map[string]string{},
},

"type header, not supplied": {
map[string]*FieldSchema{
"foo": {Type: TypeHeader},
},
map[string]interface{}{},
"foo",
http.Header{},
},
}

for name, tc := range cases {
Expand Down
16 changes: 16 additions & 0 deletions logical/framework/field_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,20 @@ const (
// TypeCommaIntSlice is a helper for TypeSlice that returns a sanitized
// slice of Ints
TypeCommaIntSlice

// TypeHeader is a helper for sending request headers through to Vault.
// For instance, the AWS and AliCloud credential plugins both act as a
// benevolent MITM for a request, and the headers are sent through and
// parsed.
// IMPORTANT NOTES:
// - Under the hood, http.Header is a map[string][]string and its Get
// method will only return the first value. To retrieve all values,
// you must access them like you would in a map.
// - This implementation of http.Header has CASE-INSENSITIVE keys. All
// keys are converted to Title Case on their way in so that the Get
// method will match the keys no matter what case you use when
// looking for them.
TypeHeader
)

func (t FieldType) String() string {
Expand All @@ -64,6 +78,8 @@ func (t FieldType) String() string {
return "duration (sec)"
case TypeSlice, TypeStringSlice, TypeCommaStringSlice, TypeCommaIntSlice:
return "slice"
case TypeHeader:
return "header"
default:
return "unknown type"
}
Expand Down