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

Allow specifying allowed ssl protocol versions via attributes #152

Merged
merged 2 commits into from Apr 6, 2015

Conversation

Projects
None yet
5 participants
@JonathanTron
Copy link
Contributor

commented Oct 19, 2014

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2014

You have

Array(node['rabbitmq']['ssl_versions']).map{|n| "\'#{n}\'"}.join(',')

repeated 3 times.

@JonathanTron

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2014

Yes, this cookbook don't pass variables from the recipe to the template and I didn't want to change that.

What would you suggest to remove the duplication?

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2014

Use a function? You can even have it in the Erb template, if that's what the maintainer prefers.

@JonathanTron

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2014

Thanks for your comment, I didn't see there was already a place to put this method.

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2014

Please rebase off master, and if this is still relivant, i just need another 👍 and we'll merge it.

@jjasghar jjasghar added Rebase labels Dec 7, 2014

@JonathanTron JonathanTron force-pushed the JonathanTron:allow-ssl-version-config branch from 16a32cf to 4938e26 Dec 7, 2014

@JonathanTron

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2014

Thanks, rebase done.

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Dec 8, 2014

If i can just get another 👍 on this, i think it's good to go. I'm pretty sure a spec wouldn't help around here, but i could be wrong.

@wenchma

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

👍 LGTM, we need this patch merged.

@gekun0216

This comment has been minimized.

Copy link

commented Mar 13, 2015

@jjasghar, I think it is common requirement to specify the ssl version with regard to some security concerns, and this option is documented well in http://www.rabbitmq.com/ssl.html which is the rabbitmq official site, would you please merge this patch?

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2015

Yep, this seems like a great idea, I should have a push out for the cookbook today/tomorrow.

@@ -38,5 +38,9 @@ def format_kernel_parameters # rubocop:disable all

rendered.each { |r| r.prepend(' ') }.join(",\n")
end

def format_ssl_versions
Array(node['rabbitmq']['ssl_versions']).map { |n| "'#{n}'" }.join(',')

This comment has been minimized.

Copy link
@michaelklishin

michaelklishin Mar 13, 2015

Member

I find this less than obvious. How about we simply set default to an empty array instead of nil? nil seems illogical to me.

This comment has been minimized.

Copy link
@gekun0216

gekun0216 Mar 25, 2015

@michaelklishin, I don't think it is proper to set the default value of ['rabbitmq']['ssl_versions'] to be nil, for:

  1. In the rabbitmq document, you don't bother to add a line of
    {ssl, [{versions, ['tlsv1.2', 'tlsv1.1', tlsv1]}]},
    to the /etc/rabbitmq.config to enable the default SSL versions rabbitmq-server supported. It is needed when you
    want to customize the versions, most of time to limit the set of versions.
    From this perspective, nil just work well, means void or nothiing, right?
  2. If we merely change the default value to be a empty array in the attribute file:
    default['rabbitmq']['ssl_versions'] = []
    I am afraid some code logic will be affected:
    format_ssl_versions if node['rabbitmq']['ssl_versions']
    node['rabbitmq']['ssl_versions'] evaluates to true, thus
    format_ssl_versions returns an empty string '', but unfortunately, '' is evaluated to true as well, so
    finally, we shall end up with the following line in the /etc/rabbitmq.config file
    {ssl, [{versions, []}]},
    which I don't think is a rational result we both want to get, since it is not what the rabbitmq document tells.
  3. If we do insist on using [] as the default value, and also follow the rabbitmq document, we have to change
    something in the current patch, e.g:
    :ssl_versions => (format_ssl_versions if node['rabbitmq']['ssl_versions'].length != 0)
    But I believe you will also consider it to be less obvious than:
    :ssl_versions => (format_ssl_versions if node['rabbitmq']['ssl_versions'])

So I would like to vote that the current patch is OK, @jjasghar, could you please share some insights here as well?

@gekun0216

This comment has been minimized.

Copy link

commented Mar 30, 2015

@jjasghar, since there is no other comments, What do you think if we can merge this pull request?

jjasghar pushed a commit that referenced this pull request Apr 6, 2015

JJ Asghar
Merge pull request #152 from JonathanTron/allow-ssl-version-config
Allow specifying allowed ssl protocol versions via attributes

@jjasghar jjasghar merged commit 6ee4d77 into rabbitmq:master Apr 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.