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

fix: correctly compare compressed/expanded ipv6 in targets #109

Merged
merged 4 commits into from
Jul 4, 2024
Merged

fix: correctly compare compressed/expanded ipv6 in targets #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbbe4db
fix: correctly compare compressed/expanded ipv6 in targets
GGabriele Jul 1, 2024
88f11dc
apply review comments
GGabriele Jul 3, 2024
1bcb5a5
more review comments
GGabriele Jul 3, 2024
6764fd2
rename
GGabriele Jul 4, 2024
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
76 changes: 76 additions & 0 deletions pkg/file/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ import (
"context"
"errors"
"fmt"
"net/netip"
"reflect"
"regexp"
"sort"
"strings"

"github.com/blang/semver/v4"
"github.com/kong/go-database-reconciler/pkg/konnect"
Expand Down Expand Up @@ -1033,6 +1036,70 @@ func (b *stateBuilder) rbacRoles() {
}
}

var (
IPv6HasPortPattern = regexp.MustCompile(`\]\:\d+$`)
IPv6HasBracketPattern = regexp.MustCompile(`\[\S+\]$`)
)

// hasIPv6Format checks if the hostname is in ipv6 format.
// This is a best effort check, it doesn't guarantee that the hostname is a valid ipv6 address,
// but it checks if the hostname has more than 2 colons.
func hasIPv6Format(hostname string) bool {
parts := strings.Split(hostname, ":")
return len(parts) > 2
Comment on lines +1048 to +1049
Copy link
Member

Choose a reason for hiding this comment

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

This can be prone to errors or to be misused. Why not use https://pkg.go.dev/net/netip#ParseAddr and https://pkg.go.dev/net/netip#Addr.Is6 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As silly as it may look, this is enough to catch all combinations of targets in IPv6 format, including the one with port and brackets like [2600:1f16:1784:fa03:fd73::e]:1326, which the library wouldn't consider as v6

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't https://pkg.go.dev/net/netip#ParseAddrPort work for this then?

I'm just worried that this func is very easy to abuse.

isIPv6("this:is:nuts:!") will also return true. It will also return true for 1:2:3:4 which http://sqa.fyicenter.com/1000334_IPv6_Address_Validator.html#Result reports as invalid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ParseAddrPort works if the port is always there, but it would fail to parse things like ::1 (would work with [::1]:8000 though).

If you really feel strong about this we can improve this validation, but generally decK (or this library) doesn't do much input validation other than what's part of the JSON schemas from go-kong.

For the records though, those bogus entries would still fail validation at a later stage:

$ ./deck gateway sync kong.yaml
Error: building state: invalid ipv6 address 1:2:3:4

$ ./deck gateway sync kong.yaml
Error: building state: invalid ipv6 address this:is:nuts:!

Copy link
Member

Choose a reason for hiding this comment

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

Understood that this is validated on other levels.

I'm just suggesting to consider improving the reliability of this while we're at it

For instance something like this should work just fine and will work for addresses with and without ports

func IsIPv6(str string) bool {
	addr, err := netip.ParseAddrPort(str)
	if err != nil {
		addr, err := netip.ParseAddr(str)
		fmt.Printf("error parsing address: %v\n", err)
		if err != nil {
			fmt.Printf("error parsing address port: %v\n", err)
			return false
		}
		return addr.Is6()
	}

	return addr.Addr().Is6()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This still would fail to parse something like [::1], as well as detecting bogus patterns like [invalid:ipv6::address]:1326: the function would rightly return false, but this is not just false, it's invalid. At this point we would need add extra handling and validation for these cases too. I think this is going to become more complex and fragile overall.

I'm renaming this function to hasIPv6Format which better represent its purpose: 6764fd2

}

// expandIPv6 decompress an ipv6 address into its 'long' format.
// for example:
//
// from ::1 to 0000:0000:0000:0000:0000:0000:0000:0001.
func expandIPv6(address string) string {
pmalek marked this conversation as resolved.
Show resolved Hide resolved
addr, err := netip.ParseAddr(address)
if err != nil {
return ""
}
return addr.StringExpanded()
}

// normalizeIPv6 normalizes an ipv6 address to the format [address]:port.
// for example:
// from ::1 to [0000:0000:0000:0000:0000:0000:0000:0001]:8000.
func normalizeIPv6(target string) (string, error) {
pmalek marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we also use StringExpanded() here? Possibly with a port?

ip := target
port := "8000"
match := IPv6HasPortPattern.FindStringSubmatch(target)
if len(match) > 0 {
// has [address]:port pattern
ipAndPort, err := netip.ParseAddrPort(ip)
if err != nil {
return "", fmt.Errorf("invalid ipv6 address and port %s", target)
}
port = fmt.Sprint(ipAndPort.Port())
ip = ipAndPort.Addr().String()
} else {
match = IPv6HasBracketPattern.FindStringSubmatch(target)
if len(match) > 0 {
// has [address] pattern
ip = removeBrackets(match[0])
}
ipAddr, err := netip.ParseAddr(ip)
if err != nil {
return "", fmt.Errorf("invalid ipv6 address %s", target)
}
ip = ipAddr.String()
}
expandedIPv6 := expandIPv6(ip)
if expandedIPv6 == "" {
return "", fmt.Errorf("failed while expanding ipv6 address %s", target)
}
return fmt.Sprintf("[%s]:%s", expandedIPv6, port), nil
}

func removeBrackets(ip string) string {
ip = strings.ReplaceAll(ip, "[", "")
return strings.ReplaceAll(ip, "]", "")
}

func (b *stateBuilder) upstreams() {
if b.err != nil {
return
Expand Down Expand Up @@ -1075,6 +1142,15 @@ func (b *stateBuilder) upstreams() {
func (b *stateBuilder) ingestTargets(targets []kong.Target) error {
for _, t := range targets {
t := t

if t.Target != nil && hasIPv6Format(*t.Target) {
normalizedTarget, err := normalizeIPv6(*t.Target)
if err != nil {
return err
}
t.Target = kong.String(normalizedTarget)
}

if utils.Empty(t.ID) {
target, err := b.currentState.Targets.Get(*t.Upstream.ID, *t.Target)
if errors.Is(err, state.ErrNotFound) {
Expand Down
144 changes: 144 additions & 0 deletions pkg/file/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,150 @@ func Test_stateBuilder_ingestTargets(t *testing.T) {
},
},
},
{
name: "expands IPv6 address and port correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("[2001:db8:fd73::e]:1326"),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: false,
wantState: &utils.KongRawState{
Targets: []*kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("[2001:0db8:fd73:0000:0000:0000:0000:000e]:1326"),
Weight: kong.Int(100),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
},
{
name: "expands IPv6 address correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("::1"),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: false,
wantState: &utils.KongRawState{
Targets: []*kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("[0000:0000:0000:0000:0000:0000:0000:0001]:8000"),
Weight: kong.Int(100),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
},
{
name: "expands IPv6 address with square brackets correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("[::1]"),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: false,
wantState: &utils.KongRawState{
Targets: []*kong.Target{
{
ID: kong.String("d6e7f8a9-bcde-1234-5678-9abcdef01234"),
Target: kong.String("[0000:0000:0000:0000:0000:0000:0000:0001]:8000"),
Weight: kong.Int(100),
Upstream: &kong.Upstream{
ID: kong.String("a1b2c3d4-e5f6-7890-abcd-ef1234567890"),
},
},
},
},
},
{
name: "handles invalid IPv6 address correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
Target: kong.String("[invalid:ipv6::address]:1326"),
Upstream: &kong.Upstream{
ID: kong.String("b1c2d3e4-f5a6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: true,
wantState: &utils.KongRawState{},
},
{
name: "handles invalid IPv6 address correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
Target: kong.String("1:2:3:4"),
Upstream: &kong.Upstream{
ID: kong.String("b1c2d3e4-f5a6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: true,
wantState: &utils.KongRawState{},
},
{
name: "handles invalid IPv6 address correctly",
fields: fields{
currentState: emptyState(),
},
args: args{
targets: []kong.Target{
{
Target: kong.String("this:is:nuts:!"),
Upstream: &kong.Upstream{
ID: kong.String("b1c2d3e4-f5a6-7890-abcd-ef1234567890"),
},
},
},
},
wantErr: true,
wantState: &utils.KongRawState{},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
Loading