Refactor/127 cli refactoring #151

zer0x64 · 2019-02-06T02:29:47Z

As per #127

If I did my job right, you should be able to use your old commands by simply removing the --provider switch. This is all documented in the README.md. So, to run an AWS scan with a profile, the command would be:
./Scout.py aws --profile PROFILE_NAME

This code is really error-prone and not tested that much. Especially, I haven't tested GCP at all and with a bunch of AWS switches. It would be useful to have someone on each to test out a bit seeing if something broke.

codecov-io · 2019-02-06T02:52:23Z

Codecov Report

Merging #151 into develop will decrease coverage by 0.17%.
The diff coverage is 79.33%.

@@            Coverage Diff             @@
##           develop    #151      +/-   ##
==========================================
- Coverage    30.28%   30.1%   -0.18%     
==========================================
  Files           66      66              
  Lines         4316    4298      -18     
==========================================
- Hits          1307    1294      -13     
+ Misses        3009    3004       -5

Impacted Files	Coverage Δ
ScoutSuite/providers/aws/provider.py	`10.9% <33.33%> (ø)`	⬆️
ScoutSuite/providers/azure/provider.py	`18.98% <5.55%> (ø)`	⬆️
ScoutSuite/providers/gcp/provider.py	`13.82% <50%> (ø)`	⬆️
ScoutSuite/__main__.py	`65.97% <87.87%> (-0.35%)`	⬇️
ScoutSuite/cli_parser.py	`97.33% <98.46%> (+19.07%)`	⬆️
ScoutSuite/output/utils.py	`23.68% <0%> (-15.79%)`	⬇️
ScoutSuite/output/js.py	`28.12% <0%> (-9.38%)`	⬇️
ScoutSuite/core/exceptions.py	`45.45% <0%> (-4.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 347d65b...988f8b7. Read the comment docs.

vifor2

Great job, I was indeed able to use the parameters I used previously to create a report! I've tried with these arguments : aws --force --no-browser --thread-config 1 --services iam --ruleset default.json. And they all worked as intended! 💯

A small word of notice though, it seems like the provider used must now be the first argument (which wasn't the case when --provider was used). I don't think that's an issue, simply noting that we should write it down somewhere...
...speaking of which, should I clone Scout2's wiki? 🤔

x4v13r64 · 2019-02-06T16:37:01Z

it seems like the provider used must now be the first argument (which wasn't the case when --provider was used)

@zer0x64 I'm guessing this is because you're now validating the parameters per-provider, instead of the soup of params we had before? I mean it would make sense to have the --provider as the first parameter as that helps validate what's passed to the CLI.

...speaking of which, should I clone Scout2's wiki? 🤔

Yes I think it would be a good thing to start. Maybe leave of the bits that currently don't apply/work (looking at you ListAll/RulesGenerator!!!).

zer0x64 · 2019-02-06T17:54:01Z

it seems like the provider used must now be the first argument (which wasn't the case when --provider was used)

@zer0x64 I'm guessing this is because you're now validating the parameters per-provider, instead of the soup of params we had before? I mean it would make sense to have the --provider as the first parameter as that helps validate what's passed to the CLI.

They actually now uses completely independant subparsers! It's works a little bit like git now: You cannot use git -am "commit message" commit, as the -am switch relates to the "commit" parser. The same thing happens here

README.md

Co-Authored-By: zer0x64 <17575242+zer0x64@users.noreply.github.com>

x4v13r64 · 2019-02-07T15:11:13Z

General

I've tried this for AWS, GCP and Azure with basic parameters. I really like this PR, makes the CLI much more clean. A couple comments below.

parameters vs values

Would it be possible to have --provider (or -p?) before aws,gcp,azure, and then have to specify the provider as a value (as currently implement in master)?

(venv) ~/Git/ScoutSuite (refactor/127-cli-refactoring ✔) ᐅ python Scout.py         
usage: Scout.py [-h] {ruleset,listall,aws,gcp,azure} ...
Scout.py: error: You need to input a module

I'd hate to break backwards compatibility over a detail.

Generaly parameters are preceded by -/--, and values aren't, so it would be --ruleset, --listall and --provider followed by the other parameters/values.

--help conformity

I really like the new --help command but I'm seeing a few inconsistencies:

For azure it's great:

(venv) ~/Git/ScoutSuite (refactor/127-cli-refactoring ✘)✹ ᐅ python Scout.py azure --help
[...]
Authentication modes:
  --cli                 Run Scout Suite using configured azure-cli credentials
  --msi                 Run Scout Suite with Managed Service Identity
  --service-principal   Run Scout Suite with an Azure Service Principal
  --file-auth FILE      Run Scout Suite with the specified credential file
  --user-account        Run Scout Suite with user credentials

Authentication parameters:
  --tenant TENANT_ID    Tenant ID of the service principal
  --subscription SUBSCRIPTION_ID
                        Subscription ID of the service principal
  --client-id CLIENT_ID
                        Client ID of the service principal
  --client-secret CLIENT_SECRET
                        Client of the service principal
  --username USERNAME   Username of the Scout Suite account
  --password PASSWORD   Password of the Scout Suite account

For GCP shouldn't this be split between Authentication modes, Authentication parameters (for the SA key file if we keep that parameter) and maybe Additional arguments (for project/folder/org filtering):

(venv) ~/Git/ScoutSuite (refactor/127-cli-refactoring ✘)✹ ᐅ python Scout.py gcp --help  
[...]
GCP parameters:
  --user-account        Run Scout Suite with a Google Account
  --service-account SERVICE_ACCOUNT
                        Run Scout Suite with a Google Service Account with the
                        specified Google Service Account Application
                        Credentials file
  --project-id PROJECT_ID
                        ID of the GCP Project to analyze
  --folder-id FOLDER_ID
                        ID of the GCP Folder to analyze
  --organization-id ORGANIZATION_ID
                        ID of the GCP Organization to analyze

Same for AWS, I'd put these under Authentication parameters:

  --profile PROFILE [PROFILE ...]
                        Name of the profile. Defaults to ['default'].
  --mfa-serial MFA_SERIAL
                        ARN of the user's MFA device
  --mfa-code MFA_CODE   Six-digit code displayed on the MFA device.
  --csv-credentials CSV_CREDENTIALS
                        Path to a CSV file containing the access key ID and
                        secret key

And these under Additional arguments:

  --regions REGIONS [REGIONS ...]
                        Name of regions to run the tool in, defaults to all
  --vpc VPC [VPC ...]   Name of VPC to run the tool in, defaults to all
  --ip-ranges IP_RANGES [IP_RANGES ...]
                        Config file(s) that contain your known IP ranges
  --ip-ranges-name-key IP_RANGES_NAME_KEY
                        Name of the key containing the display name of a known
                        CIDR

GCP

For service accounts, the parameter is now --service-account SERVICE_ACCOUNT. Could you either restore the --key-file parameter or change SERVICE_ACCOUNT to KEY_FILE so it's clear what's required (and update README accordingly)?

Trying to run for a user account seems to fail (I assume something isn't being passed down the code):

(venv) ~/Git/ScoutSuite (refactor/127-cli-refactoring ✘)✹ ᐅ python Scout.py gcp --user-account
Failed to authenticate to GCP - no supported account type

x4v13r64 · 2019-02-07T15:11:42Z

README.md

+Also note that the command line will try to infer the argument name if possible when receiving partial switch. For
+example, this will work and use the selected profile:
+
+    $python Scout.py aws --pro PROFILE


--profile

We can remove that part, but that was exactly what I was trying to show: The default behavior of argparse is to infer the switch if it cannot find it. So in that case, --pro, --prof, --profi, and so on would all work in place of --profile. But if this is confusing, we can remove it and let people figure it out by themselves!

Damn, didn't know that! And hadn't read the paragraph above 😅 . So yeah, keep it!

x4v13r64 · 2019-02-07T15:13:27Z

There was a bug in develop for GCP service account, fixes has just been merged so you might want to merge develop into this first.

README.md

ScoutSuite/__main__.py

ScoutSuite/cli_parser.py

zer0x64 · 2019-02-07T15:39:31Z

Would it be possible to have --provider (or -p?) before aws,gcp,azure, and then have to specify the provider as a value (as currently implement in master)?

The way the subparsers works(so we can only show the useful help options), we cannot strictly do this. However, a simple fix would be to add a dummy --provider argument before that's useless, but wouldn't break compatibility. However, I don't see how I could do it using argparse so it can be put anywhere(it would still have to be the first parameters.

Generaly parameters are preceded by -/--, and values aren't, so it would be --ruleset, --listall and --provider followed by the other parameters/values.

I "could" do the --ruleset and --listall part in a working way, but I'm not sure if we still keep them. Also, t would break compatibility for python 2.6-3.6 as they added the "required" parameter on subparsers in 3.7 and before that it was simply always required.

For GCP shouldn't this be split between Authentication modes, Authentication parameters (for the SA key file if we keep that parameter) and maybe Additional arguments (for project/folder/org filtering):
[...]
Same for AWS, I'd put these under Authentication parameters:
[...]

Will do!

For service accounts, the parameter is now --service-account SERVICE_ACCOUNT. Could you either restore the --key-file parameter or change SERVICE_ACCOUNT to KEY_FILE so it's clear what's required (and update README accordingly)?

The "KEY_FILE" is a really easy fix. If I do that, will I have to restore the --key-file argument? Just thought it would be cleaner to only need one switch in that case.

Trying to run for a user account seems to fail (I assume something isn't being passed down the code):
(venv) ~/Git/ScoutSuite (refactor/127-cli-refactoring ✘)✹ ᐅ python Scout.py gcp --user-account
Failed to authenticate to GCP - no supported account type

Is this the fix that has been pushed to develop?

Also, for single letters(like -p for --provider), I can make a bunch of them, as long as they don't conflict(if, let say, I have a -u for --username in azure, it won't conflict with another -u aws-specifc switch). Do you want me to implement a few of them?

Co-Authored-By: zer0x64 <17575242+zer0x64@users.noreply.github.com>

x4v13r64 · 2019-02-07T17:01:27Z

The way the subparsers works(so we can only show the useful help options), we cannot strictly do this.

As we've decided to deprecate listall/rulesgenerator at least for now I think that simplifies the issue. I'm ok with removing the --provider parameter and having to provide the cloud provider name as a first argument.

The "KEY_FILE" is a really easy fix. If I do that, will I have to restore the --key-file argument?

No, you won't have to.

Just thought it would be cleaner to only need one switch in that case.

I agree, as you will always have to specify the path of the key file when using an SA. So yeah just make it clear that what has to be provided is the path of the key file 😃

Is this the fix that has been pushed to develop?

No, I think something might be wrong with how the parameters are passed when using the --user-account parameter.

for single letters

I think it can be useful where it makes sense. The great think with how you've implemented this is that indeed there's less possibility for conflict 👍

ScoutSuite/cli_parser.py

zer0x64 · 2019-02-09T19:46:06Z

Everything fixed, some arguments also have single-letter switches(I did what I could without causing conflict) and to --provider switch still works withough polluting the help message. Are we ready to merge?

x4v13r64 · 2019-02-10T11:02:50Z

Are we ready to merge?

Please do, I'll try to test as many use cases as I can this week 😃

zer0x64 added 10 commits January 30, 2019 10:44

CLI refactoring: First draft

c326be0

Merged all three command line tools

bf12ad8

Azure: Made switches for credentials passing

9c1f50f

Use parent hierarchy for better CLI

0799d9a

Updated README.md

4a74723

Fixed __main__ tests to use new architecture

6dcb3a6

Made some CLI function private

3021684

Code cleanup

0ea011e

Renamed ruleset generator

73f92e0

Merged with develop

fe30017

zer0x64 added the refactoring label Feb 6, 2019

zer0x64 self-assigned this Feb 6, 2019

zer0x64 requested review from x4v13r64, Aboisier, misg, Remi05 and vifor2 February 6, 2019 02:29

zer0x64 added 2 commits February 5, 2019 21:31

Fixed bad cli argument in README.md

fbdeb54

Fixed backward compatibility

f113e58

vifor2 approved these changes Feb 6, 2019

View reviewed changes

Made --services and --skip available for every providers

60554d9

Aboisier reviewed Feb 7, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

Aboisier reviewed Feb 7, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

zer0x64 and others added 3 commits February 7, 2019 08:54

Removed switches from azure authentication method

f58918d

Update README.md

607810a

Co-Authored-By: zer0x64 <17575242+zer0x64@users.noreply.github.com>

Update README.md

00c8a3a

Co-Authored-By: zer0x64 <17575242+zer0x64@users.noreply.github.com>

x4v13r64 reviewed Feb 7, 2019

View reviewed changes

Aboisier suggested changes Feb 7, 2019

View reviewed changes

Aboisier and others added 9 commits February 7, 2019 10:41

Update README.md

8558051

Co-Authored-By: zer0x64 <17575242+zer0x64@users.noreply.github.com>

Removed comments

6b7a155

Removed TODO

a8a0229

Extracted generate_file_name function

702cf54

Changed Scout2 to Scout

8c1df55

Fixed typos in cli

fecc774

changed variable name

316ef8f

Renamed Scout Sutie to Scout in cli

c004189

Renamed Scout Sutie to Scout in cli

5068de6

This was referenced Feb 8, 2019

Feature/120 gcp sa multi project scanning #152

Merged

Feature/120 gcp svc act multi proj #162

Closed

Aboisier approved these changes Feb 8, 2019

View reviewed changes

ScoutSuite/cli_parser.py Outdated Show resolved Hide resolved

zer0x64 added 7 commits February 9, 2019 13:37

KEY_FILE name for GCP auth argument

ae894ae

Merged with develop

eb9698f

Fixed backward compatibility with --provider

82a5287

Arguments grouping for AWS and GCP

876ec01

Added a bunch of short arguments

2237943

Removed unnecessary mutually exclusive group for gcp arguments

59a0a0c

Fixed bug with GCP service-account

988f8b7

zer0x64 merged commit ef5c080 into develop Feb 10, 2019

zer0x64 deleted the refactor/127-cli-refactoring branch February 10, 2019 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/127 cli refactoring #151

Refactor/127 cli refactoring #151

zer0x64 commented Feb 6, 2019

codecov-io commented Feb 6, 2019 •

edited

vifor2 left a comment

x4v13r64 commented Feb 6, 2019

zer0x64 commented Feb 6, 2019

x4v13r64 commented Feb 7, 2019

x4v13r64 Feb 7, 2019

zer0x64 Feb 7, 2019

x4v13r64 Feb 7, 2019

x4v13r64 commented Feb 7, 2019

zer0x64 commented Feb 7, 2019

x4v13r64 commented Feb 7, 2019

zer0x64 commented Feb 9, 2019

x4v13r64 commented Feb 10, 2019

Refactor/127 cli refactoring #151

Refactor/127 cli refactoring #151

Conversation

zer0x64 commented Feb 6, 2019

codecov-io commented Feb 6, 2019 • edited

Codecov Report

vifor2 left a comment

Choose a reason for hiding this comment

x4v13r64 commented Feb 6, 2019

zer0x64 commented Feb 6, 2019

x4v13r64 commented Feb 7, 2019

General

parameters vs values

--help conformity

GCP

x4v13r64 Feb 7, 2019

Choose a reason for hiding this comment

zer0x64 Feb 7, 2019

Choose a reason for hiding this comment

x4v13r64 Feb 7, 2019

Choose a reason for hiding this comment

x4v13r64 commented Feb 7, 2019

zer0x64 commented Feb 7, 2019

x4v13r64 commented Feb 7, 2019

zer0x64 commented Feb 9, 2019

x4v13r64 commented Feb 10, 2019

codecov-io commented Feb 6, 2019 •

edited