Add client_renegotiation ssl option, use more ssl options in management plugin

[Infraded]

Pull Request (PR) description

1. Adds the option for client_renegotiation to rabbitmq config
2. Copies multiple options from rabbitmq config to management plugin config similar to existing ssl settings
   • client_renegotiation
   • secure_renegotiate
   • reuse_sessions
   • honor_cipher_order
3. Wraps client_renegotiation and secure_renegotiate in a conditional to not include them when enabling TLSv1.3 as they are incompatible
4. Updates/add tests for client_renegotation setting and TLSv1.3 conditional

This Pull Request (PR) fixes the following issues

[wyardley]

LGTM. Should this be backwards-compatible for users who take the default setting?

[wyardley]

Also, thanks for the contribution!

note: I'd suggest that you may want to cherry pick this into a different branch on your fork and then do a hard reset on trunk (i.e., master in this case) -- it will make keeping up to date with the upstream repo much easier if you ever want to make other PRs in the future.

[wyardley]

@Infraded - can you revert the strings generate check after all? I'm not sure if it's the version of some gem that you have or what, but the changes here are more than what I'd expect. Typically, we do regenerate the reference file again when cutting a release.

Failing tests for Arch are, unfortunately still a thing (see #905), but other integration tests should be passing.

[Infraded]

LGTM. Should this be backwards-compatible for users who take the default setting?

From the Rabbit docs I thought this was the default, but it doesn't appear to be set by the app and falls back to the Erlang default of false so I'll be inverting that.

Also, thanks for the contribution!

note: I'd suggest that you may want to cherry pick this into a different branch on your fork and then do a hard reset on trunk (i.e., master in this case) -- it will make keeping up to date with the upstream repo much easier if you ever want to make other PRs in the future.

Agreed on the branch usage, this was a quick and dirty update to my old fork of this repo (note the puppetlabs in the name).

@Infraded - can you revert the strings generate check after all? I'm not sure if it's the version of some gem that you have or what, but the changes here are more than what I'd expect. Typically, we do regenerate the reference file again when cutting a release.

Failing tests for Arch are, unfortunately still a thing (see #905), but other integration tests should be passing.

Will do on the REFERENCE revert. Working on fix for tests now

[wyardley]

From the Rabbit docs I thought this was the default, but it doesn't appear to be set by the app and falls back to the Erlang default of false so I'll be inverting that.

I guess alternately, you could make it optional, and suppress it if it's undef?

[Infraded]

From the Rabbit docs I thought this was the default, but it doesn't appear to be set by the app and falls back to the Erlang default of false so I'll be inverting that.

I guess alternately, you could make it optional, and suppress it if it's undef?

I did consider that as well, but wasn't really sure which way Vox Pupuli wanted that to go. There are a few different cases currently in just these settings.

honor_cipher_order is defaulted to false by RabbitMQ, but is true and set in this module.
reuse_sessions and secure_negotiations match their defaults but are also explicitly set by the module.

[wyardley]

I did consider that as well, but wasn't really sure which way Vox Pupuli wanted that to go. There are a few different cases currently in just these settings.

Yeah, a good question, maybe worth checking on their IRC. I try to help out still since I've maintained this module historically, but neither use Puppet or RabbitMQ currently or in the recent past, so, while I'm happy to help get this out, may be good to get some additional feedback on whether we have a specific policy. I'd tend to say that, if this is a non-breaking change, it would be best to do this in a way that is least likely to create surprises or breaking changes for users of the module. From that standpoint, I like the approach of suppressing the directive, but I haven't used RMQ recently enough to offer an opinion on it from that side of things.

Historically, I think we have mostly tried to match defaults of the upstream module. The divergence you're seeing could be an oversight, or it could be because of a change of the parameter over time. In either case, a good thing to know is that with the default behavior (with repos_ensure set to false), most distros will get a pretty old version of RMQ, so making sure the module doesn't negatively affect users that are still on an old RMQ version is also important, while that behavior is still in place and while those versions are still supported (#845 has some discussion about changing that back to how it was a long while ago, as a breaking change).

While it's a bit wacky to get the local Docker tests working w/ Beaker these days, if you don't already have it setup, I do find it really helpful to get that setup so that you can not only run the integration tests locally, but also test various scenarios and see what the behavior is like in a relatively sandboxed environment. Regardless of how you do it, it would be ideal if you can do some functional testing of this with default settings as well as your updates.

[Infraded]

Yeah, a good question, maybe worth checking on their IRC. I try to help out still since I've maintained this module historically, but neither use Puppet or RabbitMQ currently or in the recent past, so, while I'm happy to help get this out, may be good to get some additional feedback on whether we have a specific policy. I'd tend to say that, if this is a non-breaking change, it would be best to do this in a way that is least likely to create surprises or breaking changes for users of the module. From that standpoint, I like the approach of suppressing the directive, but I haven't used RMQ recently enough to offer an opinion on it from that side of things.

I think I am gonna have to make this optional as it seems like it might not be compatible with the older RMQ/Erlang versions.

Historically, I think we have mostly tried to match defaults of the upstream module. The divergence you're seeing could be an oversight, or it could be because of a change of the parameter over time. In either case, a good thing to know is that with the default behavior (with repos_ensure set to false), most distros will get a pretty old version of RMQ, so making sure the module doesn't negatively affect users that are still on an old RMQ version is also important, while that behavior is still in place and while those versions are still supported (#845 has some discussion about changing that back to how it was a long while ago, as a breaking change).

In this case I'm thinking it might have been intentional as a more secure default for things like honor_cipher_order. I like the module choosing the more secure option for a setting that would be very rarely breaking, but I don't know the original intent there. Things do start to get tough with so many individually defined settings and supporting such a wide version range for RMQ.

While it's a bit wacky to get the local Docker tests working w/ Beaker these days, if you don't already have it setup, I do find it really helpful to get that setup so that you can not only run the integration tests locally, but also test various scenarios and see what the behavior is like in a relatively sandboxed environment. Regardless of how you do it, it would be ideal if you can do some functional testing of this with default settings as well as your updates.

I'm not sure I do enough module dev to dive into that, but it would be very nice to get into some of these test systems to get more detailed failure info. I'll have to look into it, thanks for the advice.

[wyardley]

In this case I'm thinking it might have been intentional as a more secure default for things like honor_cipher_order

And in some cases, folks will actually get feedback / review from Vox's security officer on things like this, though not sure if that's still a thing.

[wyardley]

@Infraded are you still working on this one? Seems like CentOS 7 tests are failing as well as the Arch ones (which are expected).

[Infraded]

Yeah, I am still working on this. Cent 7 tests are failing because it's still setup to test on the ancient version from EPEL and it can't handle some of those options for the management plugin. I've been contemplating/dreading best options on how to handle that without getting too deep into the can of worms :|

[wyardley]

Sounds good - good plan! Just wanted to make sure you weren’t waiting on me. Just let me know when you’ve got it where you want it.

[wyardley]

@Infraded recent PRs may have resolved some of those issues if you want to rebase / resolve conflicts.