Skip to content

T8292: Fix ndp-proxy verify key mismatch for prefix rules#5004

Open
jd82k wants to merge 1 commit intovyos:currentfrom
jd82k:ndproxy
Open

T8292: Fix ndp-proxy verify key mismatch for prefix rules#5004
jd82k wants to merge 1 commit intovyos:currentfrom
jd82k:ndproxy

Conversation

@jd82k
Copy link
Copy Markdown
Contributor

@jd82k jd82k commented Feb 21, 2026

Change summary

Switch verify() to validate the real config tree (interface -> prefix) instead of non-existent rule nodes.
This enforces:

  • mode interface requires prefix ... interface
  • non-interface modes must not set prefix ... interface
  • referenced interface exists when mode is interface

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T8292

Related PR(s)

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

Smoketest

vyos@vyos# /usr/libexec/vyos/tests/smoke/cli/test_service_ndp-proxy.py
test_basic (__main__.TestServiceNDPProxy.test_basic) ... ok
test_disabled_interface_skips_validation (__main__.TestServiceNDPProxy.test_disabled_interface_skips_validation) ... ok
test_disabled_prefix_skips_validation (__main__.TestServiceNDPProxy.test_disabled_prefix_skips_validation) ... ok
test_prefix_mode_auto_rejects_interface (__main__.TestServiceNDPProxy.test_prefix_mode_auto_rejects_interface) ... ok
test_prefix_mode_interface_requires_interface (__main__.TestServiceNDPProxy.test_prefix_mode_interface_requires_interface) ... ok
test_prefix_mode_interface_with_interface (__main__.TestServiceNDPProxy.test_prefix_mode_interface_with_interface) ... ok

----------------------------------------------------------------------
Ran 6 tests in 13.570s

OK

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 21, 2026

👍
No issues in PR Title / Commit Title

@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Feb 22, 2026

I have read the CLA Document and I hereby sign the CLA

@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Feb 23, 2026

@c-po @sarthurdev @sever-sever Please review this PR.

Copy link
Copy Markdown
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

This PR looks like something inspired by an agent which is not wrong in general, but probably should somehow be marked as such?

Despite some minor changes in help strings or missing completion helpers it has one fundamental issue - described in our CONTRIBUTING file.

Every change set must be consistent (self containing)! Do not fix multiple bugs in a single commit. If you already worked on multiple fixes in the same file use git add –patch to only add the parts related to the one issue into your upcoming commit.

So here you have changes in radvd and also ndppd which should both go even into their own discrete task filed with out public issue tracker.

@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Feb 24, 2026

This PR looks like something inspired by an agent which is not wrong in general, but probably should somehow be marked as such?

Despite some minor changes in help strings or missing completion helpers it has one fundamental issue - described in our CONTRIBUTING file.

Every change set must be consistent (self containing)! Do not fix multiple bugs in a single commit. If you already worked on multiple fixes in the same file use git add –patch to only add the parts related to the one issue into your upcoming commit.

So here you have changes in radvd and also ndppd which should both go even into their own discrete task filed with out public issue tracker.

Thanks for the review.

English is not my first language, so sorry if any wording was unclear.

I understand and respect the CONTRIBUTING rule that each change set must be self-contained and focused on one issue.
In this PR, I grouped ndppd and radvd changes together because I was trying to deliver one end-to-end IPv6 passthrough behavior. I understand this is still too broad for a single task/commit in this project.

I will split this into separate discrete tasks/PRs:

  1. ndppd-related changes only
  2. radvd-related changes only

Each PR will be independently testable and scoped to one issue.

Thanks again for the guidance.

@jd82k jd82k changed the title T8292: Add dynamic-prefix support for IPv6 passthrough/NDP proxy while keeping the implementation minimal and backward-compatible. T8292: Add dynamic-prefix support for IPv6 passthrough/NDP proxy Feb 24, 2026
@jd82k jd82k changed the title T8292: Add dynamic-prefix support for IPv6 passthrough/NDP proxy T8292: Add dynamic-prefix support for NDP proxy Feb 24, 2026
@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Feb 25, 2026

Please review this PR. Thanks! @sever-sever @dmbaturin @sarthurdev @c-po

Copy link
Copy Markdown
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

Why is it required to introduce a new dynamic CLI node?

Using VyOS stream 2026.02 shows:

set service ndp-proxy interface eth0 prefix ::/0
commit
$ cat /run/ndppd/ndppd.conf
# autogenerated by service_ndp-proxy.py

# This tells 'ndppd' how often to reload the route file /proc/net/ipv6_route
route-ttl 30000

# This sets up a listener, that will listen for any Neighbor Solicitation
# messages, and respond to them according to a set of rules
proxy eth0 {
    # Turn on or off the router flag for Neighbor Advertisements
    router no
    # Control how long to wait for a Neighbor Advertisment message before invalidating the entry (milliseconds)
    timeout 500
    # Control how long a valid or invalid entry remains in the cache (milliseconds)
    ttl 30000

    # This is a rule that the target address is to match against. If no netmask
    # is provided, /128 is assumed. You may have several rule sections, and the
    # addresses may or may not overlap.
    rule ::/0 {
        static
    }
}

So then generated rule looks like the same as what you wanted to do using dynamic keyword.

@jd82k jd82k changed the title T8292: Add dynamic-prefix support for NDP proxy T8292: Fix ndp-proxy verify key mismatch for prefix rules Mar 6, 2026
@github-actions github-actions bot added the rebase label Mar 6, 2026
@jd82k jd82k requested a review from c-po March 6, 2026 20:23
@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Mar 6, 2026

Why is it required to introduce a new dynamic CLI node?

Using VyOS stream 2026.02 shows:

set service ndp-proxy interface eth0 prefix ::/0
commit
$ cat /run/ndppd/ndppd.conf
# autogenerated by service_ndp-proxy.py

# This tells 'ndppd' how often to reload the route file /proc/net/ipv6_route
route-ttl 30000

# This sets up a listener, that will listen for any Neighbor Solicitation
# messages, and respond to them according to a set of rules
proxy eth0 {
    # Turn on or off the router flag for Neighbor Advertisements
    router no
    # Control how long to wait for a Neighbor Advertisment message before invalidating the entry (milliseconds)
    timeout 500
    # Control how long a valid or invalid entry remains in the cache (milliseconds)
    ttl 30000

    # This is a rule that the target address is to match against. If no netmask
    # is provided, /128 is assumed. You may have several rule sections, and the
    # addresses may or may not overlap.
    rule ::/0 {
        static
    }
}

So then generated rule looks like the same as what you wanted to do using dynamic keyword.

Thanks for pointing this out.

My original intent was to add a more semantic dynamic CLI keyword and fix the verify bug at the same time. You are right that this made the change set mixed and not clean enough.

I have updated the PR so it now contains only the verify bug fix (rule/prefix key mismatch) and nothing else.

Sorry for the scope noise in the previous version, and thank you for the guidance.

@c-po c-po added the bp/circinus Create automatic backport for circinus label Mar 6, 2026
@c-po
Copy link
Copy Markdown
Member

c-po commented Mar 7, 2026

As this feels to be LLM generated lets try my luck. The changed code works on parts which will raise ConfigErrors if certain CLI settings are honored/dishonored. The code path has no corresponding test in test_service_ndp-proxy.py - please extend!

@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Mar 7, 2026

As this feels to be LLM generated lets try my luck. The changed code works on parts which will raise ConfigErrors if certain CLI settings are honored/dishonored. The code path has no corresponding test in test_service_ndp-proxy.py - please extend!

Done. Please review. Thanks

@jd82k jd82k requested a review from c-po March 7, 2026 08:25
@jd82k
Copy link
Copy Markdown
Contributor Author

jd82k commented Mar 8, 2026

Now everything is ok.

@sever-sever sever-sever marked this pull request as draft March 9, 2026 13:53
@jd82k jd82k force-pushed the ndproxy branch 4 times, most recently from 7ba5f94 to 39eb612 Compare March 9, 2026 17:57
…orrectly`

Switch `verify()` to validate the real config tree (`interface -> prefix`) instead of non-existent `rule` nodes.
This enforces:
- `mode interface` requires `prefix ... interface`
- non-`interface` modes must not set `prefix ... interface`
- referenced interface exists when `mode` is `interface`
@jd82k jd82k marked this pull request as ready for review March 9, 2026 18:26
@jd82k jd82k requested a review from sever-sever March 9, 2026 18:26
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • CLI Smoketests (interfaces only) ❌ failed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • CLI Smoketests VPP 👍 passed
  • Config tests VPP 👍 passed
  • TPM tests 👍 passed


if mode == 'interface':
if not prefix_interface:
raise ConfigError(f'Prefix "{prefix}" uses interface mode but no interface defined!')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interface is optional https://github.com/DanielAdolfsson/ndppd/blob/master/ndppd.conf-dist#L83C1-L86C43

but here you make it required which also breaks existing configurations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original code already required interface when mode == interface.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can change it but it will not follow the original code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there is a misunderstanding here: in ndppd, choosing a rule method is optional (it defaults to static for compatibility), but if the selected method is iface/interface, the interface argument is not optional. The syntax in ndppd.conf-dist is "iface ", and the description says forwarding is done through the specified interface.

@jd82k jd82k requested a review from c-po April 1, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bp/circinus Create automatic backport for circinus current rebase

Development

Successfully merging this pull request may close these issues.

3 participants