Skip to content

Vulnenv week1 - #1

Closed
Nayeraneru wants to merge 27 commits into
masterfrom
vulnenv-week1
Closed

Vulnenv week1#1
Nayeraneru wants to merge 27 commits into
masterfrom
vulnenv-week1

Conversation

@Nayeraneru

@Nayeraneru Nayeraneru commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Refering to #20506
GSoC 2026 — Week 1 deliverables for the Automated Vulnerable Environment Provisioning project.

Architecture docs:

  • architecture/01-command-dispatcher.md
  • architecture/02-module-metadata.md
  • architecture/03-database-schema.md
  • architecture/04-environment-schema.md
  • architecture/05-runtime-adapter.md

Additional files:

  • reference_modules.md — ActiveMQ (from PR Activemq jolokia exploit (CVE-2026-34197) rapid7/metasploit-framework#21497), Jenkins, Drupal
  • workflow.md — target user scenarios & error handling spec
  • ci_workflow.md — GitHub Actions + resource script
  • docs/test_env/README.md — master index
  • plugins/test_env.rb — loads successfully in msfconsole
  • data/vuln_envs/jenkins.yml — reference environment definition

Open question:
In 02-module-metadata.md — is it better to encapsulate mod.send(:module_info)['VulnEnv'] in a helper method, or keep the direct send call?

@h00die h00die left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall I think this looks reasonable. I added a few comments and notes

| `list` | `cmd_test_env_list(args)` | Show all tracked environments |
| `stop <ID>` | `cmd_test_env_stop(args)` | Stop a running container |
| `start <ID>` | `cmd_test_env_start(args)` | Restart a stopped container |
| `remove <ID>` | `cmd_test_env_remove(args)` | Tear down a container |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for many commands we allow a range as well. See sessions and the -k command for an example: -k, --kill <id> Terminate sessions by session ID and/or range.

I think this will apply for stop start remove. I'm not sure exec would be a good idea though.

if words.length == 2
case words[0]
when 'stop', 'start', 'remove', 'exec'
# TODO: Return IDs from registry (Week 6)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good idea marking which week in the comment


## Alternative: Encapsulate in Helper Method (Is it better to do or not?)

If It's not preferable not to use `send` directly everywhere:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think @zeroSteiner is better to answer this, its too deep in the framework weeds for my preference.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah it looks like the established pattern would be to have #vulnerable_environment method and return the key from the private module_info attribute.

https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/module/module_info.rb#L49-L60

The module class is large. I'd have a preference towards verbosity and clarity over abbreviations so #vulnerable_environment would probably have less of a chance of conflicting with anything pre-existing than say #vuln_env. In either case we should do a quick grep to make sure.

That'd make it

def vulnerable_environment
  self.module_info['VulnerableEnvironment']

https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/module/module_info.rb also has normalizations that are applied with the various #merge_* methods. These patterns are old... because framework is old. I would suggest we sort of adopt them and do some normalization, but use a Struct object to encapsulate all the info. We should also do validation on the keys and types so we know that required attributes are set, things are the correct type etc.

```ruby
'VulnEnv' => {
'definition' => String, # e.g., 'jenkins' → data/vuln_envs/jenkins.yml
'default_version' => String, # e.g., '2.361'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Keep an eye on rapid7#21583 . I'm not sure if it would necessarily help or not, but that may give the ability to run different versions on the fly if theyre in that range, or allow for checking that the docker version is in that range kind of thing. No change needed, specially since thats just an Issue/idea and not implemented yet, but figured i'd bring it up to see if you had any ideas of how that could be useful here or not.

@Nayeraneru Nayeraneru Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

For now though, since it's not implemented yet , no structural changes needed, but it's a future enhancement. however, my current design is already compatible —the EnvironmentResolver would just gain an additional validation/filtering step

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think thats reasonable

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh I see this is doing the validation I had noted in my previous comment 👍


```bash
docker run -d \
--label "msf.vulnenv.instance_id=msf-$(hostname)-$$" \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i dont know much about docker labels and how you may want to pull them later, but would it potentially be easier to do like a base64 encoded json dump of the variables? Just a thought

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

yes a base64 json blob is cleaner, however, we need individual labels for managed_by and module because Docker's --filter only does string matching on label values, we can't query inside a json blob. I'll adopt a hybrid: individual labels for the filterable identity fields, and a base64 JSON payload label for the full datastore, credentials, and exploit command. this gives us native docker ps --filter performance for discovery, plus the atomicity and schema flexibility you suggested

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it necessary to store the full datastore, credentials and exploit command in the label? If framework is running and you can associate an instance with the module, the module already has that information in the vulnerable_environment hash definition right?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

yup, I'll store only the truly dynamic data in container labels,everything else is reconstructible at runtime


| Label | Value | Purpose |
|-------|-------|---------|
| `msf.vulnenv.instance_id` | `msf-{hostname}-{pid}` | Isolate msfconsole instances |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pid may be enough, i'm not sure if hostname adds any value

@Nayeraneru Nayeraneru Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

although PID is unique among running processes on a machine. I kept hostname for three defensive reasons:
1- CI integration: multiple GitHub Actions runners may share a Docker daemon
2- PID reuse after msfconsole crash/restart
3- debugging — docker inspect shows which machine created the container
for pure single-machine use, PID alone is sufficient. If you prefer minimalism, I could switch to a random startup token instead: msf-{token}-{pid}. Would you prefer that or shall I keep hostname for the CI safety margin?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

keep it as it is

Comment thread docs/test_env/ci_workflow.md Outdated
Comment on lines +90 to +92
branches: [ main ]
pull_request:
branches: [ main ]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

spinning up an env and exploiting it is a time consuming event. Likely it'll only be run before a weekly build or something similar.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

okay, got it
I'll change the trigger from push/pull_request to a hybrid model: weekly scheduled runs to catch bit-rot, label-triggered runs for PRs that need validation (vuln-env-test label), and workflow_dispatch for manual runs

Comment thread docs/ci_workflow.md Outdated
@@ -0,0 +1,194 @@
# CI Workflow: Automated Exploit Verification

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks to be a duplicate of docs/architecture/ci_workflow.md

Comment thread docs/reference_modules.md Outdated
@@ -0,0 +1,48 @@
# Reference Modules for test_env

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks to be a duplicate of docs/architecture/reference_modules.md

Comment thread docs/workflow.md Outdated
@@ -0,0 +1,280 @@
# test_env User Workflow (Design Specification)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks to be a duplicate of docs/architecture/workflow.md

From `lib/msf/ui/console/command_dispatcher/jobs.rb`, I saw:
- One dispatcher can handle multiple commands via `commands` hash
- `cmd_jobs(*args)` uses `args.shift` to get the subcommand
- `cmd_rename_job_tabs` provides tab completion

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm pretty sure it should be cmd_jobs_tabs for a command named jobs.

Comment on lines +29 to +35
# Test: Can we add a custom key?
puts "=== Adding custom key ==="
info['VulnEnv'] = {
'definition' => 'jenkins',
'default_version' => '2.361',
'port_mapping' => { 8080 => 'RPORT' }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adding metadata to the module is perfectly fine but we'd want to do it in the #initialize method where the other module metadata exists. Otherwise, the shape looks correct. The port_mapping looked odd at first but I'm guessing the intention is to iterate over the keys and assign datastore options. With that being the case either one for the key should be fine since they should both be unique.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

okay I will clarify


## Alternative: Encapsulate in Helper Method (Is it better to do or not?)

If It's not preferable not to use `send` directly everywhere:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah it looks like the established pattern would be to have #vulnerable_environment method and return the key from the private module_info attribute.

https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/module/module_info.rb#L49-L60

The module class is large. I'd have a preference towards verbosity and clarity over abbreviations so #vulnerable_environment would probably have less of a chance of conflicting with anything pre-existing than say #vuln_env. In either case we should do a quick grep to make sure.

That'd make it

def vulnerable_environment
  self.module_info['VulnerableEnvironment']

https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/module/module_info.rb also has normalizations that are applied with the various #merge_* methods. These patterns are old... because framework is old. I would suggest we sort of adopt them and do some normalization, but use a Struct object to encapsulate all the info. We should also do validation on the keys and types so we know that required attributes are set, things are the correct type etc.

```ruby
'VulnEnv' => {
'definition' => String, # e.g., 'jenkins' → data/vuln_envs/jenkins.yml
'default_version' => String, # e.g., '2.361'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh I see this is doing the validation I had noted in my previous comment 👍


```bash
docker run -d \
--label "msf.vulnenv.instance_id=msf-$(hostname)-$$" \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it necessary to store the full datastore, credentials and exploit command in the label? If framework is running and you can associate an instance with the module, the module already has that information in the vulnerable_environment hash definition right?

image: vulnhub/jenkins:2.361
build_args:
JENKINS_VERSION: "2.361"
"2.375":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It might be a bit more flexible to refer to these as "variants" instead of versions. I could see scenarios where we have an app with one version configured two different ways such as with SQLite or postgresql but it's the same version.

In that case treating this as a list and moving the version number to either a name field or if you truly need the version, a version field.

variants:
  - name: "v2.361"
    image: vulnhub/jenkins:2.361
    build_args:
      JENKINS_VERSION: "2.361"
    ...

@Nayeraneru Nayeraneru closed this Jul 20, 2026
@Nayeraneru Nayeraneru reopened this Jul 25, 2026
@Nayeraneru Nayeraneru closed this Jul 25, 2026
Nayeraneru pushed a commit that referenced this pull request Aug 1, 2026
Nayeraneru pushed a commit that referenced this pull request Sep 2, 2026
…e-option

Fix WinRM PowerShell session resource leaks and race conditions
@Nayeraneru Nayeraneru reopened this Sep 2, 2026
@Nayeraneru Nayeraneru closed this Sep 2, 2026
@Nayeraneru

Copy link
Copy Markdown
Owner Author

Closed as already in vulnenv branch

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.

3 participants