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 parsing of IPv6 addresses #53

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix parsing of IPv6 addresses #53

wants to merge 3 commits into from

Conversation

bbannier
Copy link
Member

@bbannier bbannier commented Feb 27, 2025

Closes #34.

@bbannier
Copy link
Member Author

See zeek/zeekscript#106 for the related change there.

@bbannier bbannier marked this pull request as ready for review February 27, 2025 17:38
@bbannier bbannier requested a review from ckreibich February 27, 2025 17:39
bbannier added 3 commits March 1, 2025 11:40
While we are not able to parse dual IPv6 addresses (zeek/zeekscript#34)
we apparently were not even able to parse Zeek IPv6 address literals
since we did not include the surrounding `[..]` in the grammar. This
patch takes care of that, but leaves zeek/zeekscript#34 unaddressed.
Our previous regex for IPv6 addresses was very complicated but still did
not accommodate all possible formats. This patch radically simplifies
it so it should be able to match all possible addresses at the cost of
performing less validation during parsing which seems like a reasonable
trade-off.

Closes #34.
@bbannier bbannier force-pushed the topic/bbannier/ipv6 branch from 5c48f00 to 152a814 Compare March 1, 2025 10:47
Copy link
Member

@ckreibich ckreibich left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! This was a total construction site when I left off, per the comment about wanting to revisit Zeek's own regex for v6.

The one you put in accepts things like [:::1] or [abcdef], but I think you're okay with that, right? I agree that that's obviously better than rejecting valid Zeek v6 strings, as long as it doesn't cause conflicts with other [...] constructs.

@bbannier
Copy link
Member Author

bbannier commented Mar 4, 2025

The one you put in accepts things like [:::1] or [abcdef], but I think you're okay with that, right? I agree that that's obviously better than rejecting valid Zeek v6 strings, as long as it doesn't cause conflicts with other [...] constructs.

Yes, this accepts even invalid ipv6 addresses, mainly so we do not turn "parsing" into "full-fledged validation". I deliberately left ipv4 more strict so it does not conflict with parsing of real values, but do not see where we'd conflict with captures or sets. Should we run into issues we can still make it more restrictive, but at least now we are at all able to parse ipv6 addresses.

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.

2 participants