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

Make Wazuh Agent able to register and report to different IPs #136

Merged
merged 6 commits into from Jul 25, 2019

Conversation

jm404
Copy link
Contributor

@jm404 jm404 commented Jul 23, 2019

Hi team,

This PR fixes #110 , there are two new parameters $ossec_registration_ip which is used by authd to register the Agent and $ossec_reporting_ip which will establish to which IP will the agent report.

The parameters $ossec_ip and $ossec_hostname have been deprecated in favor of the new ones.

Best regards,

Jose

@jm404 jm404 mentioned this pull request Jul 23, 2019
Copy link
Contributor

@JPLachance JPLachance left a comment

Choose a reason for hiding this comment

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

Good job! I think there is a little glitch with parameters validation. Fix it then ship it! 🎉

@@ -53,8 +53,8 @@

# Server configuration

$ossec_ip = $wazuh::params_agent::ossec_ip,
$ossec_hostname = $wazuh::params_agent::ossec_hostname,
$ossec_registration_ip = $wazuh::params_agent::ossec_registration_ip,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we postfixing the parameter name with_ip? Wazuh supports both an IP or a domain name.

We could name those parameters wazuh_master_endpoint and wazuh_workers_enpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! Domains can also be specified and this naming could confuse users.

I like the naming you propose for cluster but I think it could be confusing for non-cluster configurations, as you would have to declare master and worker endpoint for same IP/domain and user that are not familiar with cluster could be confused in my opinion.

Something like: wazuh_register_endpoint and wazuh_reporting_endpoint comes to my mind in a quick approach

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to declare both wazuh_master_endpoint and wazuh_workers_enpoint if we bring back double <address> in the ossec.conf file:

  <%- if @wazuh_register_endpoint then -%>	
    <address><%= @wazuh_master_endpoint %></address>	
  <%- end -%>
  <%- if @wazuh_reporting_endpoint then -%>	
    <address><%= @wazuh_workers_enpoint %></address>	
  <%- end -%>

The Wazuh agent takes the last one as I wrote in #110 (comment).

But it conflicts with my suggestion of setting 127.0.0.1 as a default value for both 😄 If a user of the Puppet modules sets a value for wazuh_master_endpoint, but not for wazuh_workers_endpoint, he would end up with his agents reporting to 127.0.0.1.

I agree with your naming suggestion and I think setting 127.0.0.1 as a default value might cause issues, so lets keep undef! 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly what I tried to point out, I think this way users have more flexibility and they could even register agents without configuring them to do it later or in with other tool.

Anyway, we are really happy about this kind of discussion since it helps us to understand the user's needs and make the code better.

Feel free to comment anytime, feedback is always welcome!

if ( ( $ossec_ip == undef ) and ( $ossec_hostname == undef ) and ( $ossec_address == undef ) ) {
fail('must pass either $ossec_ip or $ossec_hostname or $ossec_address to Class[\'wazuh::agent\'].')
if (($manage_client_keys == 'yes')){
if ( ( $ossec_registration_ip == undef ) or ( $ossec_reporting_ip == undef ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we have a or, but if we look below, this class cannot work without a value for ossec_registration_ip.

We should change the condition or adapt the code below.

Copy link
Contributor Author

@jm404 jm404 Jul 24, 2019

Choose a reason for hiding this comment

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

Oh, you are completely right, I will fix it.

Thanks for reporting!

$ossec_ip = 'YOUR_MANAGER_IP'
$ossec_hostname = undef
$ossec_address = undef
$ossec_registration_ip = undef
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we set '127.0.0.0' as a default value?

When building an AWS IAM that will be used in multiple environments, you want to install the Wazuh agent without registering it. In the AMI build, you cannot set the final master/worker nodes endpoints because they change from one environment to another.

Setting a default value like '127.0.0.0' will simplify the code that installs the Wazuh agent without registering it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I declared them to be undef to make sure users consciously set them to avoid misconfigurations, but as you stated maybe configure it to 127.0.0.0 would be a good idea, I will think about it.

I take the chance to mention that I implemented a variable to configure if the Agent will atempt to auto-register or not: $manage_client_keys

You can check it at:

$manage_client_keys = 'yes' # Enable/Disable agent registration

Thanks again for contributing!

@jm404
Copy link
Contributor Author

jm404 commented Jul 24, 2019

Hi team,

After reviewing the recommendations of @JPLachance , I modified the PR:

Changes

  • Renamed $ossec_registration_ip -> $wazuh_register_endpoint
  • Renamed $ossec_reporting_ip -> $wazuh_reporting_endpoint
  • Fixed conditional to check if $wazuh_register_endpoint is correctly set
  • The $wazuh_reporting_endpoint is no longer a requirement as users may want to register agents but don't configure them.
  • If $wazuh_reporting_endpoint is not set, the Agent won't attempt to start. In the opposite case, it will normally start.

Best regards!

Jose

@jm404 jm404 requested a review from manuasir July 24, 2019 08:34
@manuasir manuasir added this to To Review in v3.9.x via automation Jul 25, 2019
@manuasir manuasir changed the base branch from master to 3.9.4_7.2.0 July 25, 2019 22:32
@manuasir manuasir merged commit 11dd4e9 into 3.9.4_7.2.0 Jul 25, 2019
v3.9.x automation moved this from To Review to Done Jul 25, 2019
@manuasir manuasir deleted the 3.9.3_7.2.0-fix-110 branch July 25, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v3.9.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants