Skip to content

Conversation

@DDvO
Copy link
Member

@DDvO DDvO commented Sep 10, 2025

  • README.md: move Configuration section to make it more visible and slightly extend it
  • doc/config/README.md: document the partly surprising protection of responses to requests with MAC-based protection

This came up while handling siemens/gencmpclient#66.

@DDvO DDvO requested a review from Copilot September 10, 2025 04:36

This comment was marked as outdated.

@DDvO DDvO force-pushed the improve_config_doc branch from 333fb65 to b6d2ad2 Compare September 10, 2025 04:43
@DDvO DDvO force-pushed the improve_config_doc branch from b6d2ad2 to 271904e Compare September 10, 2025 05:39
@DDvO DDvO requested a review from Copilot September 10, 2025 05:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the documentation by making the configuration section more prominent in the main README and documenting potentially surprising behavior related to MAC-based protection in CMP responses.

  • Moved the Configuration section earlier in the README for better visibility
  • Updated RFC references to more recent versions throughout the documentation
  • Added detailed explanation of MAC-based protection behavior for response messages

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
README.md Moved Configuration section up and enhanced its description
doc/config/README.md Updated RFC references and added MAC-based protection behavior documentation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@sonarqubecloud
Copy link

Comment on lines 419 to 420
and the respective type of the outgoing response message (e.g., `IP`)
is configured to be reprotected, any given configuration of outgoing
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this documents the behavior before the changes now being done!
This needs to be updated.

Copy link
Collaborator

@Akretsch Akretsch Oct 22, 2025

Choose a reason for hiding this comment

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

Updated the README.md

Comment on lines 418 to 419
responses to MAC-based requests at Downstream are always reprotected using the
same credentials.
Copy link
Member Author

Choose a reason for hiding this comment

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

Now the documenting text looks to me current w.r.t. the latest behavior and rather correct.

Still I propose improving the wording for more preciseness and clarity, e.g., like this:

When responding to request messages that include successfully verified MAC-based protection, the corresponding response messages are protected using the same MAC-based algorithm, credentials, and parameters — regardless of the configuration related to reprotection or output credentials.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed

@DDvO DDvO requested a review from Copilot October 28, 2025 10:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@DDvO DDvO requested a review from Akretsch October 28, 2025 10:18
@sonarqubecloud
Copy link

@DDvO DDvO merged commit 199d483 into main Oct 28, 2025
2 of 4 checks passed
@DDvO DDvO deleted the improve_config_doc branch October 28, 2025 11:48
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