-
Notifications
You must be signed in to change notification settings - Fork 137
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
Conversation
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.
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.
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 |
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.
For the title, I would use something clearer like:
=== To Deploy to AWS (Optional)
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, 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 |
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:
=== 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. |
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.
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+`: |
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 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. >> |
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.
Wrap the error in backticks (`) so that it looks like terminal output. You can remove the << and >> if you do this.
malboxes/malboxes.py
Outdated
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) |
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.
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 |
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.
no-op changes should be removed to reduce unnecessary work on the reviewer's end
@@ -22,8 +22,6 @@ | |||
] | |||
}], | |||
|
|||
{% include 'snippets/postprocessor_vagrant.json' %}, | |||
|
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.
why is this removed? This looks like a mistake.
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, 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' %}, |
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.
Confirm that this doesn't impact ESX use-case
@@ -23,7 +23,11 @@ | |||
] | |||
}], | |||
|
|||
{% include 'snippets/postprocessor_vagrant.json' %}, |
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.
same as above
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.
The feature is ready for a second review. |
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.
LGTM, I will test the changes and docs by deploying a VM in the cloud before merging.
With #123 I am now able to try this PR into our CI system. Here's the result:
|
Modified build system and now this error is failing the build with 894741f |
Smoke tests passed yesterday on this branch. I'll build a box today and merge if it's successful. |
Several failed builds yesterday and today. Latest issue is:
I think we forgot to document the |
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. |
I was able to successfully deploy to AWS today. I've pushed a few fixes but this is good to go now! |
This allows Malboxes to create and run VMs on the Amazon Elastic Compute Cloud (EC2).