[Radius] Add new option "dont_send_msg_auth" to control the addition of "message_authenticator". #263
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…of "message_authenticator". Signed-off-by: Julian Chang - TW <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@asha-behera @a-barboza Please review this PR, Thanks! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new configuration option "dont_send_msg_auth" to control whether the "Message-Authenticator" is added to the Access-Request packet, supporting backward compatibility.
- Added the "dont_send_msg_auth" option in test vectors and configuration constants.
- Updated the processing logic and Jinja template to account for the new option.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/hostcfgd/test_radius_vectors.py | Added key in test payload to reflect new behavior. |
| tests/hostcfgd/sample_output/RADIUS/common-auth-sonic | Updated sample output to include the new flag. |
| scripts/hostcfgd | Introduced a new constant and conversion for dont_send_msg_auth. |
| data/templates/common-auth-sonic.j2 | Enhanced template rendering to optionally include the dont_send_msg_auth flag. |
Comments suppressed due to low confidence (3)
tests/hostcfgd/test_radius_vectors.py:94
- Ensure that test cases explicitly cover both 'skip_msg_auth' and 'dont_send_msg_auth' scenarios to validate their distinct behaviors.
"dont_send_msg_auth": "true",
tests/hostcfgd/sample_output/RADIUS/common-auth-sonic:19
- Update the related configuration documentation to include details on the 'dont_send_msg_auth' flag for clarity.
auth [success=2 new_authtok_reqd=done default=ignore auth_err=die] pam_radius_auth.so conf=/etc/pam_radius_auth.d/10.10.10.5_1645.conf privilege_level protocol=pap retry=1 nas_ip_address=10.10.10.10 debug try_first_pass dont_send_msg_auth
scripts/hostcfgd:537
- Verify that the conversion using 'is_true' for the 'dont_send_msg_auth' flag handles any unexpected input formats consistently with the handling of other flags.
data['dont_send_msg_auth'] = is_true(self.radius_servers[key]['dont_send_msg_auth'])
| @@ -87,6 +87,7 @@ RADIUS_SERVER_TIMEOUT_DEFAULT = "5" | |||
| RADIUS_SERVER_AUTH_TYPE_DEFAULT = "pap" | |||
| RADIUS_PAM_AUTH_CONF_DIR = "/etc/pam_radius_auth.d/" | |||
| RADIUS_SERVER_SKIP_MSG_AUTH = False | |||
| RADIUS_SERVER_DONT_SEND_MSG_AUTH = False | |||
There was a problem hiding this comment.
Ensure the new constant follows the naming conventions used for similar options and that its purpose is well-documented in the configuration guide.
Suggested change
| RADIUS_SERVER_DONT_SEND_MSG_AUTH = False | |
| # Default value indicating whether RADIUS server should avoid sending message authenticator attributes. | |
| RADIUS_SERVER_SKIP_MSG_AUTH_DEFAULT = False |
Copilot uses AI. Check for mistakes.
| @@ -32,7 +32,7 @@ auth [success=1 default=ignore] pam_unix.so nullok try_first_pass | |||
| auth [success=done new_authtok_reqd=done default=ignore{{ ' auth_err=die maxtries=die' if not auth['failthrough'] }}] pam_unix.so nullok try_first_pass | |||
| # For the RADIUS servers, on success jump to the cacheing the MPL(Privilege) | |||
| {% for server in servers %} | |||
| auth [success={{ (servers | count) - loop.index0 }} new_authtok_reqd=done default=ignore{{ ' auth_err=die' if not auth['failthrough'] }}] pam_radius_auth.so conf=/etc/pam_radius_auth.d/{{ server.ip }}_{{ server.auth_port }}.conf privilege_level protocol={{ server.auth_type }} retry={{ server.retransmit }}{% if server.nas_ip is defined %} nas_ip_address={{ server.nas_ip }}{% endif %}{% if server.nas_id is defined %} client_id={{ server.nas_id }}{% endif %}{% if debug %} debug{% endif %}{% if trace %} trace{% endif %}{% if server.statistics %} statistics={{ server.ip }}{% endif %} try_first_pass {% if not server.skip_msg_auth %}require_message_authenticator{% endif %} | |||
| auth [success={{ (servers | count) - loop.index0 }} new_authtok_reqd=done default=ignore{{ ' auth_err=die' if not auth['failthrough'] }}] pam_radius_auth.so conf=/etc/pam_radius_auth.d/{{ server.ip }}_{{ server.auth_port }}.conf privilege_level protocol={{ server.auth_type }} retry={{ server.retransmit }}{% if server.nas_ip is defined %} nas_ip_address={{ server.nas_ip }}{% endif %}{% if server.nas_id is defined %} client_id={{ server.nas_id }}{% endif %}{% if debug %} debug{% endif %}{% if trace %} trace{% endif %}{% if server.statistics %} statistics={{ server.ip }}{% endif %} try_first_pass {% if not server.skip_msg_auth %}require_message_authenticator{% endif %} {% if server.dont_send_msg_auth %}dont_send_msg_auth{% endif %} | |||
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment in the template to clarify the purpose of 'dont_send_msg_auth' for future maintainers.
Suggested change
| auth [success={{ (servers | count) - loop.index0 }} new_authtok_reqd=done default=ignore{{ ' auth_err=die' if not auth['failthrough'] }}] pam_radius_auth.so conf=/etc/pam_radius_auth.d/{{ server.ip }}_{{ server.auth_port }}.conf privilege_level protocol={{ server.auth_type }} retry={{ server.retransmit }}{% if server.nas_ip is defined %} nas_ip_address={{ server.nas_ip }}{% endif %}{% if server.nas_id is defined %} client_id={{ server.nas_id }}{% endif %}{% if debug %} debug{% endif %}{% if trace %} trace{% endif %}{% if server.statistics %} statistics={{ server.ip }}{% endif %} try_first_pass {% if not server.skip_msg_auth %}require_message_authenticator{% endif %} {% if server.dont_send_msg_auth %}dont_send_msg_auth{% endif %} | |
| auth [success={{ (servers | count) - loop.index0 }} new_authtok_reqd=done default=ignore{{ ' auth_err=die' if not auth['failthrough'] }}] pam_radius_auth.so conf=/etc/pam_radius_auth.d/{{ server.ip }}_{{ server.auth_port }}.conf privilege_level protocol={{ server.auth_type }} retry={{ server.retransmit }}{% if server.nas_ip is defined %} nas_ip_address={{ server.nas_ip }}{% endif %}{% if server.nas_id is defined %} client_id={{ server.nas_id }}{% endif %}{% if debug %} debug{% endif %}{% if trace %} trace{% endif %}{% if server.statistics %} statistics={{ server.ip }}{% endif %} try_first_pass {% if not server.skip_msg_auth %}require_message_authenticator{% endif %} {% if server.dont_send_msg_auth %}dont_send_msg_auth{% endif %} # Prevents sending the message authenticator to the RADIUS server if not required |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To address backward compatibility concerns, add a new option "dont_send_msg_auth" to control the addition of "Message-Authenticator" in the Access-Request packet from pam_radius.