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.

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

Fix OWCA detection for compliance login #2401

Merged
merged 7 commits into from
Dec 22, 2017
Merged

Fix OWCA detection for compliance login #2401

merged 7 commits into from
Dec 22, 2017

Conversation

jerryaldrichiii
Copy link
Contributor

@jerryaldrichiii jerryaldrichiii commented Dec 15, 2017 •
edited by adamleff
Loading

This adds detection logic for logging into OpsWorks Chef Automate via inspec compliance login.

Currently, the OWCA /compliance/version endpoint returns a 200 code instead of a 401. This is due to a redirect when doing a GET request without authentication headers. This may change in the future, but I recommend leaving this in to support older versions.

Would welcome thoughts on the matter though.

Fixes #2375

@jerryaldrichiii jerryaldrichiii requested a review from a team as a code owner December 15, 2017 18:58
@jerryaldrichiii jerryaldrichiii force-pushed the ja/fix-owca-detection branch 4 times, most recently from 44e1622 to b4352f3 Compare December 15, 2017 22:41
@jerryaldrichiii
Add handling for OWCA login via compliance login
60ba103 
OpsWorks Chef Automate currently returns a 200 for the
`/compliance/version` endpoint and redirects to the Chef Manage page.

This adds support to `inspec compliance login` to accept this as valid
behavior and continue with the login.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
adamleff
adamleff suggested changes Dec 19, 2017
View reviewed changes
test/unit/bundles/inspec-compliance/api_test.rb
# Chef Automate currently returns 401 for `/compliance/version` but some
# versions of OpsWorks Chef Automate return 200 and a Chef Manage page when
# unauthenticated requests are received.
it 'returns `:automate` when a 200 is received from `https://URL/compliance/version`' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test that checks that it returns nil if we hit the server, get a 200, but do not get the Chef Manage page? That's the other edge case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do! Great suggestion.

@jerryaldrichiii
Add test case for 200 response but no Chef Manage
ef5d42f 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@jerryaldrichiii
Add debug info and split determine_server_type
432b615 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@jerryaldrichiii
Appease RuboCop
441663c 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@jerryaldrichiii
Copy link
Contributor Author

@adamleff I added some debug messages for determine_server_type, but it made the method a bit wordy in my opinion so I split out the logic. Let me know if you want me to drop those commits and put the debug messages with determine_server_type.

adamleff
adamleff suggested changes Dec 19, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Just a few cleanup items and this should be good to go.

lib/bundles/inspec-compliance/api.rb Outdated
:automate
elsif Compliance::HTTP.get(url + '/api/version', nil, insecure).code == '200'
:compliance
return :automate if target_is_automate_server?(url, insecure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is a pretty short method now that you've broken things out, I think it's totally fine to leave this as an if...elsif structure. No short-circuiting needed. In fact, I think it reads cleaner:

if target_is_automate_server?(url, insecure)
  :automate
elsif target_is_compliance_server?(url, insecure)
  :compliance
else
  Inspec::Log.debug('Could not determine server type using known endpoints')
  nil
end

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 agree, will fix.

lib/bundles/inspec-compliance/api.rb Outdated
'Continuing with detection attempts',
)
return false
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll return nil if we get a non-200 / non-401 status code. I think we should have a default else in the case statement that's just false

And if you do that, you can eliminate the return statements in 269, 279, and 286 and just have them be true or false accordingly. The returns are superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Great catch. I will make it so.

@jerryaldrichiii
Remove forced returns from determine_server_type
075ab6d 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@jerryaldrichiii
Add false code path for non-200/non-401 response
d79ad24 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
adamleff
adamleff suggested changes Dec 20, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

One small suggested change to a debug log message.

lib/bundles/inspec-compliance/api.rb Outdated
else
Inspec::Log.debug(
"Received neither 200 nor 401 from #{url}#{automate_endpoint}. " \
'Continuing with detection attempts',
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that whatever called this method has additional detection attempts to make :)

A better message, to keep it contained, would be "Received unexpected status code #{response.code} from #{url}#{automate_endpoint} - assuming this is not an Automate server."

... and whoever is calling can determine whether there are additional detection attempts to make, and now the user gets to see what status code they actually got.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, great catch. Fixing now.

@jerryaldrichiii
Reword debug messages
7d4066a 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
chris-rock
chris-rock approved these changes Dec 22, 2017
View reviewed changes
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 @jerryaldrichiii for working out the details!

@chris-rock chris-rock merged commit a3c993f into master Dec 22, 2017
@chris-rock chris-rock deleted the ja/fix-owca-detection branch December 22, 2017 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-rock chris-rock chris-rock approved these changes

@adamleff adamleff adamleff 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.
3 participants
@jerryaldrichiii @chris-rock @adamleff