Page MenuHomeVyOS Platform

PPPoE server: wrong local usernames
Closed, ResolvedPublicBUG

Description

Does not possible to use - in usernames, because CLI script replace it to _

vyos@vyos# run show configuration commands | match local-users 
set service pppoe-server authentication local-users username test@isp-example.com password 'test'
[edit]
vyos@vyos# sudo cat /run/accel-pppd/pppoe.chap-secrets 
# username  server  password  acceptable local IP addresses   shaper
test@isp_example.com * test             *

Details

Difficulty level
Easy (less than an hour)
Version
1.4-rolling-202101171022
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

Dmitry changed the task status from Open to In progress.Jan 25 2021, 12:31 PM
Dmitry claimed this task.

On the other hand, it will not be possible to use login with "_"
https://github.com/vyos/vyos-1x/blob/current/data/templates/accel-ppp/chap-secrets.config_dict.tmpl#L6-L8

If we replace all "-"

| replace('_', '-')

@jestabro this is due to the key_mangling call for get_config_dict() - I wonder if we can exclude the mangling on the tagNode value itself and how JInja2 behaves with it.

Actually the same applies to us which is here explained for Ansible: https://blog.networktocode.com/post/Exploring-Jinja-Variable-Syntax-in-Ansible/

If we switch to [] notation we could get rid of the key mangler I guess

@c-po this is an excellent idea, and a useful reference for the bracket notation; this will need a bit of design of how best to slice the problem:

  1. keeping key_mangling but excluding on tag nodes
  2. removing key_mangling completely
  3. removing on a case by case basis with the existing argument to get_config_dict()

I'm looking into the pros/cons of each approach.

With this new information I see little to none reason to keep the key_mangling() workaround. If we manage to transform all nodes into "proper" syntax we can one day drop it.

We shall experiment with the notation on never rewrites - and on success become a design principle.

Also this solves the issue where the user specified a hyphen inside tagNode that also allows underscores - there is yet no way to determine it was a hyphen in the first place.

Yes, I very much like this, and is what I am imaging with 3., above.

As an example, consider any future conf_mode scripts that only use get_config_dict(): in that case it is easy to experiment, as key_mangling is an explicit keyword argument, of default None.

In the case of scripts using, say, get_interface_dict(), key mangling is implicit, and we will need to consider adding an explicit keyword argument, but with the default full mangling (unless we want to correct all current uses now).

Nonetheless, I agree that the first step is to test out the bracket notation; then we can make a decision on how to handle the interim period --- if the interim period involves a few extension to function definitions and use, it will not matter once the key mangling all goes away.

jestabro changed the status of subtask T3481: Exclude tag node values from key mangling from Needs testing to Backport candidate.Apr 17 2021, 12:09 AM
Dmitry changed the task status from In progress to Backport pending.May 28 2021, 9:28 AM

Properly works on the latest 1.4 rolling. Is it possible to backport changes to 1.3?

no_tag_node_value_mangle=True does not exist on VyOS 1.3, thus a backport is currently not possible. @jestabro can we backport this?

Already backported: ff7b2b0e62510ef8de28c9c4bfa34badeabec775

c-po moved this task from Need Triage to Finished on the VyOS 1.3 Equuleus board.
c-po moved this task from Need Triage to Finished on the VyOS 1.4 Sagitta board.
SrividyaA set Issue type to Bug (incorrect behavior).Aug 30 2021, 5:53 PM