Skip to content

Fix verification errors caused by 'contains' function#61

Merged
AlexHearnNI merged 3 commits intoni:masterfrom
eli-engelhardt:fix_ssh_client_timeout_verify
Mar 13, 2025
Merged

Fix verification errors caused by 'contains' function#61
AlexHearnNI merged 3 commits intoni:masterfrom
eli-engelhardt:fix_ssh_client_timeout_verify

Conversation

@eli-engelhardt
Copy link
Copy Markdown
Contributor

Summary of Changes

  • Added a contains_exact function to address uncaught errors in verification caused by only checking for substrings in the original contains function.
  • Reviewed and updated all instances of the contains function in the nilrt-snac package to ensure proper usage.

Justification

As stated in the bug, the 'contains' function in _config_file.py only checks for substring which would allow for improper verification. For example, the nilrt-snac configuration puts in the following entry: ClientAliveCountMax 4

However if the user changes it, nilrt-snac verify wouldn't throw an error for something like this: ClientAliveCountMax 40, which is not the behavior we want.

Testing

Tested whether exact and non-exact string matches have acceptable behaviors for each modified configuration file.

Procedure

  • This PR: changes user-visible behavior, fixes a bug, or impacts the project's security profile; and so it includes a CHANGELOG note.
  • I certify that the contents of this pull request complies with the Developer Certificate of Origin.

- Added a contains_exact function to address uncaught errors in verifications caused by only checking for substrings in the contains function.
- Reviewed and updated all instances of the contains function in the nilrt-snac package to ensure proper usage.

Signed-off-by: Eli Engelhardt <eli.engelhardt@emerson.com>
@eli-engelhardt eli-engelhardt requested review from a team, AlexHearnNI and amstewart as code owners March 4, 2025 19:46
Signed-off-by: Eli Engelhardt <eli.engelhardt@emerson.com>
Signed-off-by: Eli Engelhardt <eli.engelhardt@emerson.com>
@eli-engelhardt eli-engelhardt requested a review from dmondrik March 11, 2025 00:10
@AlexHearnNI
Copy link
Copy Markdown
Collaborator

For what's it's worth, my opinion is that we shouldn't make changes like this. We're adding quite a bit more logic and complexity, and it's still relatively easy to bypass the verification. For example, for some of these settings the user could override the configuration value later in the file or in a different file, and this logic will be fooled. Or, perhaps the user has set an even more strict value than ours, and now verification fails when it arguably shouldn't, and when they run configure we will re-instate our less secure settings. If it were up to me, I would have closed the bug with "As Designed."

@AlexHearnNI AlexHearnNI merged commit 61450d8 into ni:master Mar 13, 2025
5 checks passed
@eli-engelhardt
Copy link
Copy Markdown
Contributor Author

@AlexHearnNI
I appreciate the feedback. My understanding is that the "verify" operation for SNAC allows users to check if their configuration is in the "default" state (i.e., no config values have been modified). If a user sets a stricter value than ours, they should be aware of this change when running "verify."

However, if we want the verification process to be less strict and only ensure the bare minimum for compliance (i.e., only critical config values are checked), that could also work. I agree that this change feels somewhat wrong, but I think the higher-level issue is deciding what kind of behavior we want for verification. I'd probably opt for the bare minimum for compliance to avoid confusing or pestering users over config changes that do not affect compliance.

@dmondrik
Copy link
Copy Markdown
Contributor

However, if we want the verification process to be less strict and only ensure the bare minimum for compliance (i.e., only critical config values are checked), that could also work. I agree that this change feels somewhat wrong, but I think the higher-level issue is deciding what kind of behavior we want for verification. I'd probably opt for the bare minimum for compliance to avoid confusing or pestering users over config changes that do not affect compliance.

Yes, Alex and I had a hallway conversation about this yesterday. Ensuring the right packages are installed (or not), and ensuring the settings exist, those are the most important part.

For some things the value is critical (e.g., is the serial console enabled or disabled) but not everywhere. This is likely something we'll want to tinker with over time to provide the best user value.

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.

4 participants