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

AWS feature for Malboxes #115

Merged
merged 30 commits into from
Aug 27, 2019
Merged

AWS feature for Malboxes #115

merged 30 commits into from
Aug 27, 2019

Conversation

Bromulux
Copy link
Collaborator

This allows Malboxes to create and run VMs on the Amazon Elastic Compute Cloud (EC2).

Etienne Lacroix added 16 commits February 11, 2019 16:02
Deprecated URL for Windows10_64 has been replaced by a working one.
The build command and the end message are now adapted for the cloud feature.
The AMI description is no longer the default one and now gives information about the packer template used.
The code related to the AWS feature for Windows 10 is now documented,
The AWS feature now supports Windows 7 64x
The AMI ID is now retrieved from AWS by the library Boto 3 instead of a post-processor. Plus, an error message was added if you try to build more than once the same template.
Boto3 uses the AWS credentials from the config.js file instead of the ones stored in environmental variables.
The region can now be set in config.js
WinRM is now working even if the network profile is configured as public
Boto3 is now in the requirements.txt
The instance type and the security group can now be set by the user in the config.js
As a workaround for the public network connectivity problem, a new rule has been created with a public network type.
Windows 7 32-bit can now be run on AWS
It is now explained how to use the AWS feature of Malboxes in the README.
Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

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

Excellent work! 🤘

Some notes but nothing major. You can address most of these now I think. Tomorrow, together, we can look for the ESX use-case together since that one is badly documented and a little bit complex.

README.adoc Outdated
@@ -40,6 +41,12 @@ https://github.com/gosecure/malboxes

apt install vagrant git python3-pip

=== AWS
Copy link
Member

Choose a reason for hiding this comment

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

For the title, I would use something clearer like:

=== To Deploy to AWS (Optional)

Copy link
Member

Choose a reason for hiding this comment

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

Also, this section should be moved at the end of the Installation section.

README.adoc Outdated
@@ -136,6 +143,50 @@ For example:

malboxes spin win7_32_analyst 20160519.cryptolocker.xyz

=== AWS
Copy link
Member

Choose a reason for hiding this comment

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

also:

=== To Deploy to AWS (Optional)

README.adoc Outdated

. Your instance also requires a link:https://docs.aws.amazon.com/vpc/latest/userguide/VPC_SecurityGroups.html#CreatingSecurityGroups[security group] with at least a rule allowing inbound connections for WinRM (Type: WinRM-HTTP, Protocol: TCP, Port Range: 5985, Source: host's public IP).

. If the default config is used, change the hypervisor to aws and fill the mandatory options related. Otherwise, be sure to add all the options about AWS to your custom config.
Copy link
Member

Choose a reason for hiding this comment

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

You can link to config with the following AsciiDoc syntax: <<_configuration,The configuration section>>.

I would do:

If the <<_configuration,default config>> is used [...]

README.adoc Outdated

==== RDP

To connect to an instance on the cloud using RDP, run this command at the same location of your `+Vagrantfile+`:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the + around Vagrantfile.

README.adoc Outdated

For this to work, the instance will require a security group allowing RDP inbound connections (Type: RDP, Protocol: TCP, Port Range: 3389, Source: host's public IP).

NOTE: You can safely ignore the following error because rsync is not yet implemented: << No host IP was given to the Vagrant core NFS helper. This is an internal error that should be reported as a bug. >>
Copy link
Member

Choose a reason for hiding this comment

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

Wrap the error in backticks (`) so that it looks like terminal output. You can remove the << and >> if you do this.

def spin(parser, args):
"""
Creates a Vagrantfile meant for user-interaction in the current directory.
"""
if os.path.isfile('Vagrantfile'):
print("Vagrantfile already exists. Please move it away. Exiting...")
sys.exit(5)
sys.exit(6)
Copy link
Member

Choose a reason for hiding this comment

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

why change this? if there is no explicit reason we shouldn't change because users might rely on it.

@@ -5,4 +5,4 @@ Set-ItemProperty -Path "HKLM:\System\CurrentControlSet\Control\Terminal Server"
netsh advfirewall firewall set rule group="remote desktop" new enable=Yes

# IDA Remote Debugging
netsh advfirewall firewall add rule name="IDA Remote Debugging" dir=in action=allow protocol=TCP localport=23946
netsh advfirewall firewall add rule name="IDA Remote Debugging" dir=in action=allow protocol=TCP localport=23946
Copy link
Member

Choose a reason for hiding this comment

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

no-op changes should be removed to reduce unnecessary work on the reviewer's end

@@ -22,8 +22,6 @@
]
}],

{% include 'snippets/postprocessor_vagrant.json' %},

Copy link
Member

Choose a reason for hiding this comment

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

why is this removed? This looks like a mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it looks like it was there two times by mistake... Wouldn't removing this affect the ESX use-case?

@@ -23,7 +23,11 @@
]
}],

{% include 'snippets/postprocessor_vagrant.json' %},
{% if hypervisor == 'virtualbox' %}
{% include 'snippets/postprocessor_vagrant.json' %},
Copy link
Member

Choose a reason for hiding this comment

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

Confirm that this doesn't impact ESX use-case

@@ -23,7 +23,11 @@
]
}],

{% include 'snippets/postprocessor_vagrant.json' %},
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Etienne Lacroix added 10 commits February 27, 2019 11:04
General corrections of the README requested by obilodeau.
The imports now respect the PEP 8.
Constants were added to describe the exit status codes.
The function now follows the DRY principle and has a more precise name.
Each line of the code related to AWS has now 80 chars or less.
Details about the vagrant box were removed in the build message because the user does not need to know them.
Now there's two new lines between functions to respect the PEP 8.
The postprocessor_vagrant is now included for all the hypervisors except aws.
VAGRANT_FILE_ALREADY_EXISTS is now EXIT_VAGRANT_FILE_ALREADY_EXISTS
Finally, the postprocessor_vagrant.json was not required at all by the vsphere hypervisor.
@Bromulux
Copy link
Collaborator Author

The feature is ready for a second review.

Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

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

LGTM, I will test the changes and docs by deploying a VM in the cloud before merging.

@obilodeau
Copy link
Member

With #123 I am now able to try this PR into our CI system. Here's the result:

Fetching all templates...
Traceback (most recent call last):
  File "/var/jenkins_home/.local/bin/malboxes", line 9, in <module>
    load_entry_point('malboxes', 'console_scripts', 'malboxes')()
  File "/var/jenkins_home/workspace/malboxes-custom-branch-smoke-test/malboxes/__init__.py", line 20, in main
    from malboxes.malboxes import main
  File "/var/jenkins_home/workspace/malboxes-custom-branch-smoke-test/malboxes/malboxes.py", line 34, in <module>
    import boto3
ImportError: No module named 'boto3'

pip doesn't install boto3, I'm going to investigate as to why.

@obilodeau
Copy link
Member

Modified build system and now this error is failing the build with 894741f

@obilodeau
Copy link
Member

Smoke tests passed yesterday on this branch. I'll build a box today and merge if it's successful.

@obilodeau
Copy link
Member

Several failed builds yesterday and today. Latest issue is:

* Post-processor failed: Failed to start import from s3://malboxes-images/packer-import-1566493516.ova: InvalidParameter: The service role <vmimport> does not exist or does not have sufficient permissions for the service to continue
	status code: 400, request id: 2e1c409b-4a5e-4b27-8387-e74123c3b640

I think we forgot to document the vmimport role requirement.

@obilodeau
Copy link
Member

More tests around this yesterday and today. I think I've got our IT to get the AWS permissions sorted out.

Now there's rsync that should be disabled.

@obilodeau
Copy link
Member

I was able to successfully deploy to AWS today. I've pushed a few fixes but this is good to go now!

@obilodeau obilodeau merged commit 4ecd059 into GoSecure:master Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants