Skip to content

Fixes #26942: Add new settings to handle certificate trust #6395

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2025
Merged

Fixes #26942: Add new settings to handle certificate trust #6395

merged 1 commit into from
Jul 4, 2025

Conversation

@fanf
Copy link
Member

@fanf fanf commented May 23, 2025 •
edited
Loading

https://issues.rudder.io/issues/26942

Add three new system variables whose value is taken from rudder properties file.

@fanf fanf requested a review from VinceMacBuche May 23, 2025 16:48
@fanf fanf marked this pull request as draft May 23, 2025 16:49
@fanf fanf removed the request for review from VinceMacBuche May 23, 2025 16:49
@peckpeck peckpeck mentioned this pull request May 27, 2025
Merged
@peckpeck
Copy link
Member

peckpeck commented Jun 11, 2025

Looks good to me

@peckpeck
Copy link
Member

peckpeck commented Jun 12, 2025

After finding the proper option in curl, let's change how rudder.server.certificate.additionalKeyHash should be handled.

It must be a semicolon separated list of hash (not comma separated).

It must be appended to the existing key POLICY_SERVER_KEY_HASH in rudder.json, separated by a semicolon, (there must be no "empty" value in this list, ie no heading, trailing or successive semicolon)

ADDITIONAL_POLICY_SERVER_KEY_HASH system variable is not needed anymore

@peckpeck
Copy link
Member

peckpeck commented Jun 18, 2025

POLICY_SERVER_CERT_NAME_VALIDATION cannot be implemented as documented, we should replace it with : POLICY_SERVER_SECURE_VALIDATION : false/empty by default to match current state (--insecure is passed to curl)

@fanf fanf force-pushed the arch_26942/add_new_settings_to_handle_certificate_trust branch from da181bb to a3cecdd Compare June 23, 2025 14:45
@fanf
Copy link
Member Author

fanf commented Jun 23, 2025

PR updated with a new commit

@amousset amousset marked this pull request as ready for review July 3, 2025 13:03
@Normation-Quality-Assistant
Copy link
Contributor

Normation-Quality-Assistant commented Jul 3, 2025

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/6395
-- Your faithful QA
Kant merge: "Thoughts without content are empty, intuitions without concepts are blind."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/103486/console)

amousset
amousset reviewed Jul 3, 2025
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/resources/configuration.properties.sample
Comment on lines +589 to +590
# They only set system properties that can be then used for the management, but
# these parameters don't do anything more.
Copy link
Member

@amousset amousset Jul 3, 2025

Choose a reason for hiding this comment

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

it does something now!

Copy link
Member Author

@fanf fanf Jul 4, 2025

Choose a reason for hiding this comment

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

so remove these lines ? What it does now ?

amousset
amousset reviewed Jul 3, 2025
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/resources/configuration.properties.sample Outdated
#
# Value for POLICY_SERVER_CERT_CA, ie the path to the file with the CA cert that should be used
# for server certificate validation (the validation is not handled by Rudder).
# Let empty for not restricting the list of possible CA to use.
Copy link
Member

@amousset amousset Jul 3, 2025

Choose a reason for hiding this comment

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

this is unclear

Copy link
Member Author

@fanf fanf Jul 4, 2025

Choose a reason for hiding this comment

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

what would make it clearer ?

amousset
amousset reviewed Jul 3, 2025
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/resources/configuration.properties.sample

#
# Value for POLICY_SERVER_SECURE_VALIDATION, ie should the server certificate
# be checked. Default false or empty (ie `--insecure` passed to curl)
Copy link
Member

@amousset amousset Jul 3, 2025

Choose a reason for hiding this comment

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

this is confusing, and might make believe there is no validation when it is false

Copy link
Member Author

@fanf fanf Jul 4, 2025

Choose a reason for hiding this comment

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

so what would be not ?

@amousset
Copy link
Member

amousset commented Jul 3, 2025

We need a comprehensive documentation of the impact of these changes.

@fanf
Copy link
Member Author

fanf commented Jul 4, 2025

PR updated with a new commit

1 similar comment
@fanf
Copy link
Member Author

fanf commented Jul 4, 2025

PR updated with a new commit

@fanf
Copy link
Member Author

fanf commented Jul 4, 2025

OK, squash merging this PR

@fanf fanf force-pushed the arch_26942/add_new_settings_to_handle_certificate_trust branch from 3b834e8 to 9f054f9 Compare July 4, 2025 16:24
@fanf fanf merged commit 9f054f9 into Normation:branches/rudder/9.0 Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peckpeck peckpeck peckpeck left review comments

@amousset amousset amousset approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fanf @peckpeck @Normation-Quality-Assistant @amousset