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: ipv6 ident #11

Merged
merged 2 commits into from
Mar 31, 2024
Merged

fix: ipv6 ident #11

merged 2 commits into from
Mar 31, 2024

Conversation

MEschenbacher
Copy link
Contributor

Resolves #10

leonnicolas
leonnicolas reviewed Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@leonnicolas leonnicolas left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Can you please add a test where an IPv6 address starts with ::. I think that should be possible in theory.

And also the (d *DNSOrIP) Set(s string) in

iptables_parser/parser.go

Line 209 in 275b7c9

func (d *DNSOrIP) Set(s string) error {

is still just blindly adding a /32. I think we just resolve that with this PR to prevent an IPv6 address from being parsed with a 32 network mask. Maybe we can use https://cs.opensource.google/go/go/+/go1.22.0:src/net/ip.go;l=494 if there is no network mask and then https://cs.opensource.google/go/go/+/go1.22.0:src/net/ip.go;l=52 to find out if it is v4 or v6 and adding the suffix accordingly. wdyt?

Also a test where we parse a complete rule that contains an IPv6 address would increase my confidence a lot. Let me know if I can help you with anything!

@MEschenbacher
Copy link
Contributor Author

Yes sounds all good, will do :)

@leonnicolas
Copy link
Collaborator

leonnicolas commented Feb 12, 2024 •
edited
Loading

I think IP.IPv4 is not part of go version 1.16, but we can update it. I the only places are in go.mod and the github action file

Never mind, I totally misread the function signature of IP.IPv4.

I guess we can use net/netip's https://pkg.go.dev/net/netip#Addr.Is4

@MEschenbacher
Copy link
Contributor Author

I've added three more commits to implement/fix issues, however solving the one issue at hand, regarding extending COLON to decide whether this is a COLON or IDENT is not as easy as I thought.

leonnicolas
leonnicolas reviewed Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@leonnicolas leonnicolas left a comment

Choose a reason for hiding this comment

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

I think the COMMIT and "no counter" commits should be in separate PRs. This way we can merge the IPv6 support quickly and not block it by my slow reviews.

parser.go Outdated
@@ -64,6 +64,12 @@ func (d Policy) String() string {
return fmt.Sprintf("%s%s %s", prefix, d.Chain, d.Action)
}

type Commit struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make a separate PR for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@MEschenbacher MEschenbacher force-pushed the fix-ipv6-ident branch 2 times, most recently from d5dbd16 to 0e8c883 Compare March 29, 2024 07:20
leonnicolas
leonnicolas requested changes Mar 30, 2024
View reviewed changes
Copy link
Collaborator

@leonnicolas leonnicolas left a comment

Choose a reason for hiding this comment

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

Thanks very much. I think everything is fine with me. Can you rebase from our main branch first? I see some changes in this PR, that are already in our main branch.

.github/workflows/test.yaml Outdated
with:
go-version: '1.16'
- run: go test ./...
fmt:
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks

.github/workflows/test.yaml Outdated
go-version: '1.16'
- run: go test ./...
- uses: actions/checkout@main
- uses: actions/setup-go@v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we already touched this, we can update the actions/setup-go version

Suggested change
- uses: actions/setup-go@v1
- uses: actions/setup-go@v5

LICENSE Outdated
@@ -1,4 +1,4 @@
Copyright (c) 2021 Leon Löchner
Copyright (c) 2022 Leon Löchner
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) 2022 Leon Löchner
Copyright (c) 2024 Leon Löchner

README.md Outdated
@@ -1,5 +1,7 @@
# iptables-parser

[![Documentation](https://godoc.org/github.com/kilo-io/iptables_parser?status.svg)](http://godoc.org/github.com/kilo-io/iptables_parser)
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks

parser.go Outdated
@@ -371,7 +374,7 @@ func (p *Parser) Parse() (l Line, err error) {
case COLON:
return p.parseDefault(p.s.scanLine())
case EOF:
return nil, io.EOF //ErrEOF
return nil, io.EOF // ErrEOF
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return nil, io.EOF // ErrEOF
return nil, io.EOF

parser.go Outdated
panic("size exceeds buffer")
}
p.buf.n += n
}

func (p *Parser) unscanIgnoreWhitespace(n int) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

right, we never use that. There are some comments that mention this method. I can make a follow up to remove those confusing comments.

parser_test.go Outdated
Values: []string{"200-1000"},
},
},
},
},
err: nil,
},
{
name: "parse rule with match expression statistic",
s: "-A foo ! -s 192.168.178.2 ! --dst 1.1.1.1 -m statistic --mode random --probability 0.50000000000 --every 10 --packet 5",
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm this is a bit funny. Now, rules that have a probability of 0.5 are different from rules with 0.50.

I wonder if we can do anything about that?

parser_test.go Outdated
} else if tc.err != err && tc.err.Error() != err.Error() {
t.Errorf("%d. %s: %q error mismatch:\n\texp=%v\n\tgot=%v.", i, tc.name, tc.s, tc.err, err)
}
t.Run(tc.name, func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

targets.go Outdated
@@ -29,6 +29,8 @@ func (p *Parser) parseTarget(t *Target) (state, error) {
switch lit {
case "DNAT":
s, err = p.parseDNAT(&t.Flags)
case "SNAT":
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you need to rebase from our main branch. Your branch is behind

@leonnicolas leonnicolas merged commit a2ed1b3 into kilo-io:main Mar 31, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonnicolas leonnicolas leonnicolas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for IPv6 is missing
2 participants
@MEschenbacher @leonnicolas