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

Expose additional WinRM options for transport, basic auth, and SSPI #3669

Merged
merged 5 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@frezbo
Copy link
Contributor

frezbo commented Dec 12, 2018

Signed-off-by: Noel Georgi git@frezbo.com

Fixes: inspec/train#391

@frezbo frezbo changed the title Expose additonal WInRM opts are per https://github.com/inspec/train/pull/132 Expose additonal WInRM opts are per https://github.com/inspec/train/pull/392 Dec 13, 2018

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 18, 2018

I'm all for making it easier to use transports in the real world, with the settings needed to do so. However, I think we need to think carefully about how we go about this: we already have a proliferation of options.

#3661 proposes using the configuration file to store credential sets - allowing transports to have extensive options and arguments. With #3661, you could have multiple credentials for different winrm targets, with each configured as it needs. Adding new options to that would be a non-event; InSpec would simply pass all information along to Train.

However, that's not convenient for more casual use. Command line args are really what's wanted. We face a few challenges:

  1. Organizing CLI opts effectively
  2. Allowing Train plugins to dynamically configure InSpec's CLI option parser (Thor)

The second point is a long-term problem - winrm is part of core Train for today, and we can bear some overtight coupling on that.

For the first issue, I propose that we prefix all winrm options with --winrm- - so this PR would add --winrm-transport, --winrm-disable-sspi, and --winrm-basic-auth. We'd then also alias existing WinRM options to include that prefix, and at some (major version) point deprecate the "prefix-less" versions of the options. We could also use the Thor group facility to improve help output.

Obviously, a similar approach could be taken for other transports that expose options (most egregiously SSH).

@frezbo

This comment has been minimized.

Copy link
Contributor

frezbo commented Dec 18, 2018

@clintoncwolfe Would just prefixing the three options I added with winrm would be enough to get this going forward? Do you want me to update the train PR too, inspec/train#392?

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 18, 2018

Yep, that'll do for this PR. I'll put in a review.

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

As discussed, just looking for the prefix change; we'll also need to change the version pin on Train in the gemspec.

@@ -69,6 +69,12 @@ def self.target_options # rubocop:disable MethodLength
desc: 'Use SSL for transport layer encryption (WinRM).'
option :self_signed, type: :boolean,
desc: 'Allow remote scans with self-signed certificates (WinRM).'
option :transport, type: :string,

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 18, 2018

Contributor

As discussed, please prefix these with --winrm-

This comment has been minimized.

@frezbo

frezbo Dec 19, 2018

Contributor

resolved.

@@ -69,6 +69,12 @@ def self.target_options # rubocop:disable MethodLength
desc: 'Use SSL for transport layer encryption (WinRM).'
option :self_signed, type: :boolean,
desc: 'Allow remote scans with self-signed certificates (WinRM).'
option :transport, type: :string,
desc: 'Specify which transport to use, defaults to negotiate (WinRM).'

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 18, 2018

Contributor

I suggest we add a list of valid transport names. Also, the help text says it defaults to :negotiate, but the Thor option doesn't have a default (perhaps omission causes the WinRM library to negotiate - but if so, a comment would be good)

This comment has been minimized.

@frezbo

frezbo Dec 19, 2018

Contributor

resolved.

frezbo added some commits Dec 12, 2018

Expose additonal WInRM opts are per inspec/train#392
Signed-off-by: Noel Georgi <git@frezbo.com>
Fix typo
Signed-off-by: Noel Georgi <git@frezbo.com>
Update the cli option names
Signed-off-by: Noel Georgi <git@frezbo.com>

@frezbo frezbo force-pushed the frezbo:feature/expose-additional-winrm-opts branch from ea924b1 to a9b4e6e Dec 19, 2018

@frezbo

This comment has been minimized.

Copy link
Contributor

frezbo commented Dec 19, 2018

@clintoncwolfe PR updated, I will update the gemspec once a new version of train is released.

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 19, 2018

Train 1.6.3 has been released, with the new options included.

@clintoncwolfe clintoncwolfe changed the title Expose additonal WInRM opts are per https://github.com/inspec/train/pull/392 Expose additional WinRM options for transport, basic auth, and SSPI Dec 19, 2018

frezbo added some commits Dec 20, 2018

Update train constraint
Signed-off-by: Noel Georgi <git@frezbo.com>
Add a functional test
Signed-off-by: Noel Georgi <git@frezbo.com>
@frezbo

This comment has been minimized.

Copy link
Contributor

frezbo commented Dec 20, 2018

@clintoncwolfe The tests fail due to the newer ruby versions packaging bundler along with it. I have also seen this locally. The functional tests pass locally.

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks so much, @frezbo !

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

Great work @frezbo!

@clintoncwolfe clintoncwolfe merged commit f1fbd51 into inspec:master Dec 20, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
expeditor/config-validation Validated your Expeditor config file
Details

@frezbo frezbo deleted the frezbo:feature/expose-additional-winrm-opts branch Dec 20, 2018

@frezbo

This comment has been minimized.

Copy link
Contributor

frezbo commented Dec 20, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment