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

Don't send HTTP headers that have nil values #1948

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

adamleff
Copy link
Contributor

Net::HTTP does not gracefully handle HTTP options/headers that have nil values. This updates Fetchers::Url to verify that all headers we attempt to configure have non-nil, non-empty values.

This originally surfaced via the audit cookbook with the chef-automate fetcher in use without the data_collector token being set.

Net::HTTP does not gracefully handle HTTP options/headers
that have nil values. This updates Fetchers::Url to verify
that all headers we attempt to configure have non-nil,
non-empty values.

This originally surfaced via the audit cookbook with the
chef-automate fetcher in use without the data_collector
token being set.

Signed-off-by: Adam Leff <adam@leff.co>
@adamleff adamleff requested review from chris-rock and arlimus June 21, 2017 03:59
@adamleff adamleff added the Type: Bug Feature not working as expected label Jun 21, 2017
opts = {}
opts[:ssl_verify_mode] = OpenSSL::SSL::VERIFY_NONE if @insecure

if @config['server_type'] == 'automate'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should deprecate all delivery specific handling in this class and move all the required logic into audit cookbook files/default/vendor/chef-automate/fetcher.rb. I understand that we do not want to make this in one move. But I'd like to see a follow-up on that PR to cover that. This allows us to have the chef-* specific parts in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense to me. I didn't want to tackle that in this PR but rather just add the error checking. Some better tests need to be written when we do that, and probably before, also.

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you @adamleff

@adamleff adamleff force-pushed the adamleff/validate-http-headers branch from 99a252c to 02f274a Compare June 22, 2017 00:03
@adamleff adamleff merged commit 1601b23 into master Jun 22, 2017
@adamleff adamleff deleted the adamleff/validate-http-headers branch June 22, 2017 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants