-
Notifications
You must be signed in to change notification settings - Fork 683
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
enable inspec compliance cli support automate #1297
Conversation
284a9ac
to
e57036a
Compare
e57036a
to
9038be7
Compare
@alexpop could you test this out please? |
fa6aeef
to
dbf37eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @vjeffrey
@@ -56,6 +57,32 @@ def login(server) # rubocop:disable Metrics/AbcSize | |||
puts '', msg | |||
end | |||
|
|||
desc "automate SERVER --user='USER' --dctoken='DATA_COLLECTOR_TOKEN' or --usertoken='AUTOMATE_TOKEN' --ent='ENT'", 'Log in to an Automate SERVER' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we call it login-automate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure on this. What would be a better way to name it? Another option could be to just stick with inspec compliance login
and just offer more options there. On the other hand this may become more complex for a user to understand which config is for which server? What are you thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had started out with just making it an extension of inspec compliance login, but then i couldn't display the desc well with the different options...which is why I went this route. I'm up for a rename to login-automate, i think that makes sense.
# iterate over tests and add compliance scheme | ||
tests = tests.map { |t| 'compliance://' + t } | ||
|
||
config['automate'][0] ? tests = tests.map { |t| 'automate://' + t } : tests = tests.map { |t| 'compliance://' + t } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we could stick to compliance://
no matter if the profiles are stored in Chef Compliance or Chef Automate
uri = if target.is_a?(String) && URI(target).scheme == 'compliance' | ||
URI(target) | ||
elsif target.is_a?(String) && URI(target).scheme == 'automate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not need the automate scheme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ya, you're right - i'll change that
conversation with hannah: inspec compliance
|
@vjeffrey Does this make sense to you? |
thanks hannah!!!!! |
i'll update this change to address comments later today/wednesday morning at latest |
Updated the color pallet here as well: https://github.com/chef/inspec/pull/1313/files |
updated to name it login_automate and removed the unnecessary automate scheme logic |
2d6e29e
to
1d5530b
Compare
625c206
to
bc48d59
Compare
config = Compliance::Configuration.new | ||
url = "#{config['server']}/logout" | ||
Compliance::API.post(url, config['token'], config['insecure'], !config.supported?(:oidc)) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should clear the token information of the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should remove the the server information from the config. This was missing even before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -198,6 +228,29 @@ def logout | |||
|
|||
private | |||
|
|||
def login_automate_config(url, user, dctoken, usertoken, ent) | |||
config = Compliance::Configuration.new | |||
config['server'] = url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have a config['server_type']
to indicate the type of server. At the moment that is compliance
and automate
def login_automate_config(url, user, dctoken, usertoken, ent) | ||
config = Compliance::Configuration.new | ||
config['server'] = url | ||
config['ent'] = ent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should store all automate specific config under the section config['automate']
# return all compliance profiles available for the user | ||
def self.profiles(config) | ||
url = "#{config['server']}/user/compliance" | ||
config['automate'][0] ? url = "#{config['server']}/#{config['user']}" : url = "#{config['server']}/user/compliance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fan of config['automate'][0]
. We should ask directly for the information we need: config['automate']['ent']
. For this case I propose config['server_type']
@@ -56,6 +57,33 @@ def login(server) # rubocop:disable Metrics/AbcSize | |||
puts '', msg | |||
end | |||
|
|||
desc "login_automate SERVER --user='USER' --ent='ENT' --dctoken or --usertoken='TOKEN'", 'Log in to an Automate SERVER' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the login_automate
approach
@@ -26,14 +25,21 @@ def self.resolve(target) | |||
# check if we have a compliance token | |||
config = Compliance::Configuration.new | |||
if config['token'].nil? | |||
if config['automate'][0] | |||
server = 'automate' | |||
msg = 'inspec compliance automate https://your_automate_server --user USER --ent ENT --dctoken DCTOKEN or --usertoken USERTOKEN' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be inspec compliance login_automate ...
@@ -26,14 +25,21 @@ def self.resolve(target) | |||
# check if we have a compliance token | |||
config = Compliance::Configuration.new | |||
if config['token'].nil? | |||
if config['automate'][0] | |||
server = 'automate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use this in config['server_type']
@@ -126,7 +126,19 @@ def download_archive_to_temp | |||
Inspec::Log.debug("Fetching URL: #{@target}") | |||
http_opts = {} | |||
http_opts['ssl_verify_mode'.to_sym] = OpenSSL::SSL::VERIFY_NONE if @insecure | |||
http_opts['Authorization'] = "Bearer #{@token}" if @token | |||
if @config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have the server_type
, lets do a simple if then clause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bc48d59
to
8c69a97
Compare
777ef1a
to
7e0c7c8
Compare
7e0c7c8
to
e2d5b0e
Compare
Signed-off-by: Victoria Jeffrey <vjeffrey@chef.io>
Signed-off-by: Victoria Jeffrey <vjeffrey@chef.io>
e2d5b0e
to
60009b2
Compare
{ org: owner, name: name } | ||
end | ||
end.flatten | ||
if config['server_type'] == 'automate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -9,13 +9,21 @@ module Compliance | |||
# implements a simple http abstraction on top of Net::HTTP | |||
class HTTP | |||
# generic get requires | |||
def self.get(url, token, insecure, basic_auth = false) | |||
def self.get(url, token, insecure, user, basic_auth = false, automate = nil, server_type) # rubocop:disable Metrics/ParameterLists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with @vjeffrey we think we can abstract that even further:
def self.get(url, headers, insecure)
. This would allow us to pass in the required headers from outside:
headers = {
'x-data-collector-token': token
}
headers = {
'chef-delivery-user': user,
'chef-delivery-token': token,
}
headers = {
'Authorization', "Bearer #{token}",
}
c6e725e
to
6aeba49
Compare
@chris-rock pushed up a commit with updated headers stuffs |
6aeba49
to
544784b
Compare
Signed-off-by: Victoria Jeffrey <vjeffrey@chef.io>
544784b
to
d8b512e
Compare
Awesome. Thank you @vjeffrey |
fixes #1295
b inspec compliance login_automate http://delivery --user admin --dctoken $DC_TOKEN --ent $AUTOMATE_ENT
b inspec compliance login_automate http://delivery --user $AUTOMATE_USER --usertoken $AUTOMATE_TOKEN --ent $AUTOMATE_ENT
b inspec compliance profiles
b inspec compliance upload ~/linux.tar.gz
b inspec compliance logout
<-- just destroys config when automateb inspec compliance version
<-- returns msg saying that's not available with automateb inspec compliance exec admin/linux