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

[BUG]: sonic_bgp_neighbors changes encrypted password every time, even though unencrypted password is unchanged #362

Open
toreanderson opened this issue Mar 26, 2024 · 10 comments
Assignees

Comments

@toreanderson
Copy link

Bug Description

When running a task that uses sonic_bgp_neighbors to create a neighbor with an unencrypted password, each and every subsequent run of the playbook will change the encrypted password, even though the unencrypted password remains unchanged.

Furthermore, even though --diff is being specified, no diff is being reported. (This may be a systemic issue as I've seen this behaviour in #361 and #359 as well.)

Product Name

SONiC-OS-4.2.0-Enterprise_Base

Component or Module Name

sonic_bgp_neighbors

DellEMC Enterprise SONiC Ansible Collection Version

dellemc.enterprise_sonic 2.4.0

SONiC Software Version

SONiC-OS-4.2.0-Enterprise_Base

Configuration

CONFIG_FILE() = /home/debian/ansible/ansible.cfg
DEFAULT_HASH_BEHAVIOUR(/home/debian/ansible/ansible.cfg) = merge
DEFAULT_HOST_LIST(/home/debian/ansible/ansible.cfg) = ['/home/debian/ansible/hosts.yml']
DEFAULT_JINJA2_EXTENSIONS(/home/debian/ansible/ansible.cfg) = jinja2.ext.do
HOST_KEY_CHECKING(/home/debian/ansible/ansible.cfg) = False
INTERPRETER_PYTHON(/home/debian/ansible/ansible.cfg) = auto_silent
MAX_FILE_SIZE_FOR_DIFF(/home/debian/ansible/ansible.cfg) = 1048576
PERSISTENT_COMMAND_TIMEOUT(/home/debian/ansible/ansible.cfg) = 3000

Steps to Reproduce

Run a playbook with the following tasks repeatedly:

    - name: Configure BGP
      dellemc.enterprise_sonic.sonic_bgp:
        config:
          - bgp_as: 1234
            router_id: 1.2.3.4

    - name: Configure BGP neighbors
      dellemc.enterprise_sonic.sonic_bgp_neighbors:
        config:
          - bgp_as: 1234
            peer_group:
              - name: ROUTESERVERS
                remote_as:
                  peer_type: external
                ebgp_multihop:
                  enabled: true
                auth_pwd:
                  pwd: secret123
                address_family:
                  afis:
                    - afi: ipv4
                      safi: unicast
                      activate: true
                      prefix_list_in: ROUTESERVERS-IN-IPV4
                      prefix_list_out: ROUTESERVERS-OUT-IPV4
                    - afi: ipv6
                      safi: unicast
                      activate: true
                      prefix_list_in: ROUTESERVERS-IN-IPV6
                      prefix_list_out: ROUTESERVERS-OUT-IPV6
            neighbors:
              - neighbor: 192.0.2.1
                peer_group: ROUTESERVERS

Expected Behavior

a. If the unencryped password does not change between subsequent runs, no changes should be reported (but ok)
b. The module should display a diff of the changes applied when ansible-playbook is run with --diff

Actual Behavior

a. Changes are reported and applied on each run
b. The module does not display a diff of the changes applied even though ansible-playbook is run with --diff

Logs

PLAY [ent_sonic] ***************************************************************

TASK [Configure BGP] ***********************************************************
ok: [leaf1] => {"before": [{"bestpath": {"as_path": {"confed": false, "ignore": false, "multipath_relax": false, "multipath_relax_as_set": false}, "compare_routerid": false, "med": {"always_compare_med": false, "confed": false, "missing_as_worst": false}}, "bgp_as": "1234", "log_neighbor_changes": true, "router_id": "1.2.3.4", "timers": {"holdtime": 180, "keepalive_interval": 60}, "vrf_name": "default"}], "changed": false, "commands": []}

TASK [Configure BGP neighbors] *************************************************
changed: [leaf1] => {"after": [{"bgp_as": "1234", "neighbors": [{"advertisement_interval": 0, "neighbor": "192.0.2.1", "passive": false, "peer_group": "ROUTESERVERS", "timers": {"connect_retry": 30, "keepalive": 60}}], "peer_group": [{"address_family": {"afis": [{"activate": true, "afi": "ipv4", "prefix_list_in": "ROUTESERVERS-IN-IPV4", "prefix_list_out": "ROUTESERVERS-OUT-IPV4", "safi": "unicast"}, {"activate": true, "afi": "ipv6", "prefix_list_in": "ROUTESERVERS-IN-IPV6", "prefix_list_out": "ROUTESERVERS-OUT-IPV6", "safi": "unicast"}]}, "advertisement_interval": 0, "auth_pwd": {"encrypted": true, "pwd": "U2FsdGVkX1+kCWpAUut3794ANoM6yEOOb10IJKbaDd8="}, "ebgp_multihop": {"enabled": true}, "name": "ROUTESERVERS", "passive": false, "remote_as": {"peer_type": "external"}, "timers": {"connect_retry": 30}}], "vrf_name": "default"}], "before": [{"bgp_as": "1234", "neighbors": [{"advertisement_interval": 0, "neighbor": "192.0.2.1", "passive": false, "peer_group": "ROUTESERVERS", "timers": {"connect_retry": 30, "keepalive": 60}}], "peer_group": [{"address_family": {"afis": [{"activate": true, "afi": "ipv4", "prefix_list_in": "ROUTESERVERS-IN-IPV4", "prefix_list_out": "ROUTESERVERS-OUT-IPV4", "safi": "unicast"}, {"activate": true, "afi": "ipv6", "prefix_list_in": "ROUTESERVERS-IN-IPV6", "prefix_list_out": "ROUTESERVERS-OUT-IPV6", "safi": "unicast"}]}, "advertisement_interval": 0, "auth_pwd": {"encrypted": true, "pwd": "U2FsdGVkX182G0luRsxMaMbz3fRxupYoP5NZRA2QNkY="}, "ebgp_multihop": {"enabled": true}, "name": "ROUTESERVERS", "passive": false, "remote_as": {"peer_type": "external"}, "timers": {"connect_retry": 30}}], "vrf_name": "default"}], "changed": true, "commands": [{"bgp_as": "1234", "peer_group": [{"auth_pwd": {"encrypted": false, "pwd": "secret123"}, "name": "ROUTESERVERS"}], "state": "merged", "vrf_name": "default"}]}

PLAY RECAP *********************************************************************
leaf1   : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

Screenshots

Diffing the before and after blobs reported from --verbose output shows the change to the password:

$ diff -u <(jq '.before' < out.json ) <(jq .after < out.json)
--- /dev/fd/63  2024-03-26 12:05:16.285328057 +0100
+++ /dev/fd/62  2024-03-26 12:05:16.286328060 +0100
@@ -36,7 +36,7 @@
         "advertisement_interval": 0,
         "auth_pwd": {
           "encrypted": true,
-          "pwd": "U2FsdGVkX182G0luRsxMaMbz3fRxupYoP5NZRA2QNkY="
+          "pwd": "U2FsdGVkX1+kCWpAUut3794ANoM6yEOOb10IJKbaDd8="
         },
         "ebgp_multihop": {
           "enabled": true

The encryped password was indeed changed accordingly:

admin@leaf1:~$ sonic-cli -c 'show running-configuration | grep password'
  password U2FsdGVkX1+kCWpAUut3794ANoM6yEOOb10IJKbaDd8= encrypted

I suspect this happens because a randomly generated salt is generated on each run, instead of re-using the same salt every time.

A viable workaround is to specify the password as encrypted instead of unencrypted. This way, ok is reported on subsequent runs after the password was initially set. However, this workaround reveals another bug (which I do not have time to report separately now): the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

Additional Information

No response

@jeff-yin
Copy link
Collaborator

I suspect this happens because a randomly generated salt is generated on each run, instead of re-using the same salt every time.

This is accurate and intended behavior. Also, the encrypted password is what's stored in the switch's config. We should also check what's the "Ansible-correct" way of handling this type of behavior for diff

another bug: the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

We can investigate this.

@toreanderson
Copy link
Author

I suspect this happens because a randomly generated salt is generated on each run, instead of re-using the same salt every time.

This is accurate and intended behavior. Also, the encrypted password is what's stored in the switch's config. We should also check what's the "Ansible-correct" way of handling this type of behavior for diff

In my opinion, the ideal behaviour would be to check if there is already an encrypted password string stored in the switch's config. If there is, re-use the salt from that password string to encrypt the plain text passord passed to the module via the config dict.

Next, check the encrypted password string already present on the device with the one you just generated. If they are equal, no update is necessary. If they are unequal, update the switch config and report changes were performed (possibly after re-encrypting the plain-text password from config with a new randomly generated salt).

another bug: the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

We can investigate this.

Great, let me know if you want me to open a separate bug report for this.

@mingjunzhang2019 mingjunzhang2019 self-assigned this Apr 10, 2024
@mingjunzhang2019
Copy link
Collaborator

mingjunzhang2019 commented Apr 11, 2024

Expected Behavior
a. If the unencryped password does not change between subsequent runs, no changes should be reported (but ok)
b. The module should display a diff of the changes applied when ansible-playbook is run with --diff

Tested the playbook in the description.

  • The unencryped password changes every time when is applied to remote SONIC system (SONIC behavior).
  • The --diff mode for sonic_bgp_neighbors is not supported in Dell Ansible release 2.4. It will be supported in next release 2.5.

So this issue is not a bug.

@toreanderson
Copy link
Author

The unencryped password changes every time when is applied to remote SONIC system (SONIC behavior).

Would it not be possible for the module to obtain the salt from encrypted password already stored in on the switch, and re-use that when encrypting the unencrypted password passed to the module (instead of generating a new salt on every run)? If so, the encrypted password generated by the module and the one already stored on the switch should be identical, and the module would be able to detect that no change would be needed.

@mingjunzhang2019
Copy link
Collaborator

SONIC does not have API for obtaining the salt. The salt generation and encryption methods are controlled and done by SONIC internally.

@toreanderson
Copy link
Author

I see. I will leave this bug open in case you want to investigate what I mentioned earlier:

another bug: the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

@toreanderson
Copy link
Author

On second thought. As I pointed out in my initial message, when run with --verbose, Ansible reports both the old and the new encrypted passwords:

$ diff -u <(jq '.before' < out.json ) <(jq .after < out.json)
--- /dev/fd/63  2024-03-26 12:05:16.285328057 +0100
+++ /dev/fd/62  2024-03-26 12:05:16.286328060 +0100
@@ -36,7 +36,7 @@
         "advertisement_interval": 0,
         "auth_pwd": {
           "encrypted": true,
-          "pwd": "U2FsdGVkX182G0luRsxMaMbz3fRxupYoP5NZRA2QNkY="
+          "pwd": "U2FsdGVkX1+kCWpAUut3794ANoM6yEOOb10IJKbaDd8="
         },
         "ebgp_multihop": {
           "enabled": true

This encrypted password string includes the salt, does it not? Would it not be possible to extract the salt from it, use it to encrypt the plain-text password, and then check the encrypted password strings for equality before deciding to PATCH?

@mingjunzhang2019
Copy link
Collaborator

mingjunzhang2019 commented Apr 15, 2024

another bug: the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

From my test, the actual encrypted password on the switch is changed and is same as one in "after" clause in verbose logs.
How do you find out it is unchanged?

@mingjunzhang2019
Copy link
Collaborator

mingjunzhang2019 commented Apr 15, 2024

This encrypted password string includes the salt, does it not? Would it not be possible to extract the salt from it, use it to encrypt the plain-text password, and then check the encrypted password strings for equality before deciding to PATCH?

I do not think it is possible to extract the salt without salt handing method and/or key and/or key generation method and/or encrypt/decrypt methods. All those are handled by SONIC internally. Ansible, as config API, should not get involving.

@toreanderson
Copy link
Author

another bug: the module fails to change an encrypted password (i.e., if I change the value of pwd in the config dict, ansible-playbook will report changed: on each subsequent, but the actual encrypted password on the switch remains unchanged.)

From my test, the actual encrypted password on the switch is changed and is same as one in "after" clause in verbose logs. How do you find out it is unchanged?

Apologies for the late response, been extremely busy lately.

Here's how I can reproduce this.

I start out by applying a playbook as follows:

- hosts: ent_sonic
  tags: ent_sonic
  gather_facts: false
  tasks:
    - name: Configure BGP
      dellemc.enterprise_sonic.sonic_bgp:
        config:
          - bgp_as: 1234
            router_id: 1.2.3.4

    - name: Configure BGP neighbors
      dellemc.enterprise_sonic.sonic_bgp_neighbors:
        config:
          - bgp_as: 1234
            peer_group:
              - name: FOO
                remote_as:
                  peer_type: external
                ebgp_multihop:
                  enabled: true
                auth_pwd:
                  pwd: U2FsdGVkX1+WGtc2YexgKR06rLE5YPb+dyW3n2gsxuQ=
                  encrypted: true

This produces the following configuration on the switch:

$ sonic-cli -c 'show running-configuration | no-more' | grep -wA11 1234
router bgp 1234
 router-id 1.2.3.4
 log-neighbor-changes
 timers 60 180
 !
 peer-group FOO
  ebgp-multihop
  remote-as external
  timers connect 30
  advertisement-interval 0
  password U2FsdGVkX1+WGtc2YexgKR06rLE5YPb+dyW3n2gsxuQ= encrypted
!

If I run the playbook again, it reports no changes are necessary, as expected:

$ ansible-playbook -vD pwchange.yml
Using /home/debian/ansible/ansible.cfg as config file

PLAY [ent_sonic] ***************************************************************

TASK [Configure BGP] ***********************************************************
ok: [switch] => {"before": [{"bestpath": {"as_path": {"confed": false, "ignore": false, "multipath_relax": false, "multipath_relax_as_set": false}, "compare_routerid": false, "med": {"always_compare_med": false, "confed": false, "missing_as_worst": false}}, "bgp_as": "1234", "log_neighbor_changes": true, "router_id": "1.2.3.4", "timers": {"holdtime": 180, "keepalive_interval": 60}, "vrf_name": "default"}], "changed": false, "commands": []}

TASK [Configure BGP neighbors] *************************************************
ok: [switch] => {"before": [{"bgp_as": "1234", "peer_group": [{"advertisement_interval": 0, "auth_pwd": {"encrypted": true, "pwd": "U2FsdGVkX1+WGtc2YexgKR06rLE5YPb+dyW3n2gsxuQ="}, "ebgp_multihop": {"enabled": true}, "name": "FOO", "passive": false, "remote_as": {"peer_type": "external"}, "timers": {"connect_retry": 30}}], "vrf_name": "default"}], "changed": false, "commands": []}

PLAY RECAP *********************************************************************
switch   : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Next, I change the password on the switch:

$ sonic-cli -c 'configure' -c 'router bgp 1234' -c 'peer-group FOO' -c 'password blah'
$ sonic-cli -c 'show running-configuration | grep password'
  password U2FsdGVkX19ocXbDPfOYJzt8cAZ7y2q3qI6wX2htmGY= encrypted

…and then I run the playbook again, which now reports changes are necessary (again as expected):

$ ansible-playbook -vD pwchange.yml
Using /home/debian/ansible/ansible.cfg as config file

PLAY [ent_sonic] ***************************************************************

TASK [Configure BGP] ***********************************************************
ok: [switch] => {"before": [{"bestpath": {"as_path": {"confed": false, "ignore": false, "multipath_relax": false, "multipath_relax_as_set": false}, "compare_routerid": false, "med": {"always_compare_med": false, "confed": false, "missing_as_worst": false}}, "bgp_as": "1234", "log_neighbor_changes": true, "router_id": "1.2.3.4", "timers": {"holdtime": 180, "keepalive_interval": 60}, "vrf_name": "default"}], "changed": false, "commands": []}

TASK [Configure BGP neighbors] *************************************************
changed: [switch] => {"after": [{"bgp_as": "1234", "peer_group": [{"advertisement_interval": 0, "auth_pwd": {"encrypted": true, "pwd": "U2FsdGVkX19ocXbDPfOYJzt8cAZ7y2q3qI6wX2htmGY="}, "ebgp_multihop": {"enabled": true}, "name": "FOO", "passive": false, "remote_as": {"peer_type": "external"}, "timers": {"connect_retry": 30}}], "vrf_name": "default"}], "before": [{"bgp_as": "1234", "peer_group": [{"advertisement_interval": 0, "auth_pwd": {"encrypted": true, "pwd": "U2FsdGVkX19ocXbDPfOYJzt8cAZ7y2q3qI6wX2htmGY="}, "ebgp_multihop": {"enabled": true}, "name": "FOO", "passive": false, "remote_as": {"peer_type": "external"}, "timers": {"connect_retry": 30}}], "vrf_name": "default"}], "changed": true, "commands": [{"bgp_as": "1234", "peer_group": [{"auth_pwd": {"pwd": "U2FsdGVkX1+WGtc2YexgKR06rLE5YPb+dyW3n2gsxuQ="}, "name": "FOO"}], "state": "merged", "vrf_name": "default"}]}

PLAY RECAP *********************************************************************
switch   : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

However, if I check the running configuration on the switch, the password has unexpectedly not been changed to the …xuQ= string found in the playbook:

$ sonic-cli -c 'show running-configuration | grep password'
  password U2FsdGVkX19ocXbDPfOYJzt8cAZ7y2q3qI6wX2htmGY= encrypted

Repeated attempts at running the playbook yields the same result, it reports changed: and claims to have changed the password every time, something which does not get reflected in the output from sonic-cli -c 'show running-configuration'.

If I manually change the password back using sonic-cli -c 'configure' -c 'router bgp 1234' -c 'peer-group FOO' -c 'password U2FsdGVkX1+WGtc2YexgKR06rLE5YPb+dyW3n2gsxuQ= encrypted', and afterwards run the playbook, it again reports ok: (not changed:).

So as far as I can tell, sonic_bgp_neighbors is only able to set a previously unset encrypted password, but not to change a password that is already set.

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

No branches or pull requests

3 participants