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

Refactor yacat example #403

Merged
merged 20 commits into from
Jun 1, 2021
Merged

Refactor yacat example #403

merged 20 commits into from
Jun 1, 2021

Conversation

kmazurek
Copy link
Contributor

Resolves #381

Key changes:

  1. A single Golem instance is now used for executing the two phases of computation
  2. Updated the CLI arguments for yacat
  • added a default log location to examples utils so that the path doesn't need to be specified explicitly
  • changed hash and mask to be named, required arguments (instead of positional ones)
  • replaced --number-of-providers with --chunk-size
  • added optional --max-workers argument to allow for manual override of the default dynamic value
  • added optional --hash-type argument
  1. Slimmed down the Dockerfile used for building the VM image by using a pre-built hashcat image
  • updated the volume paths that are used in this image
  1. Reduced the number of files generated on the requestor to temporary files used for storing the results from providers
  2. Refactored both worker functions

@kmazurek kmazurek requested review from azawlocki and shadeofblue May 26, 2021 08:23
@kmazurek kmazurek self-assigned this May 26, 2021
@kmazurek kmazurek requested a review from a team May 26, 2021 08:24
with open("in.hash", "w") as f:
f.write(hash)
# Ideally, this value should depend on the chunk size
ATTACK_TIMEOUT: timedelta = timedelta(minutes=30)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe DICTIONARY_ATTACK_TIMEOUT ? ... just "attack" seems more serious than it is to a casual (non-security-terminology-aware) user ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, though using the word dictionary in this case may be confusing (i.e. in hashcat dictionary and mask are two different types of attacks). How about MASK_ATTACK_TIMEOUT and renaming the worker funcion to perform_mask_attack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in daad9bc

Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

lgtm

just one minor remark

arg_parser = build_parser("Run a hashcat attack (mask mode) on Golem network")
arg_parser.add_argument("--hash", type=str, help="Target hash to be cracked", required=True)
arg_parser.add_argument(
"--mask", type=str, help="Hashcat mask to be used for the attack", required=True
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd provide an example value for --mask somewhere, preferably in help strings in the parser, to make this example runnable more easily without referring to external docs (apart from general golem/yapapi docs)

Copy link
Contributor

@azawlocki azawlocki May 27, 2021

Choose a reason for hiding this comment

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

Or maybe provide an example invocation, with --mask and --hash, in the string passed to build_parser()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one! Actually I might just do both. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in a0b544c

# we take the last item since it's the last command that was executed on the provider
cmd_result: CommandExecuted = result[-1]

if cmd_result.success:
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to check if cmd_result.success == True here, if it weren't then the line future_result = yield context.commit(timeout=KEYSPACE_TIMEOUT) would raise yapapi.executor.CommandExecutionError.

Copy link
Contributor

@azawlocki azawlocki May 27, 2021

Choose a reason for hiding this comment

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

I'd rewrite the body of compute_keyspace() as follows:

    async for task in tasks:
        cmd = f"xhashcat --keyspace " f"-a {HASHCAT_ATTACK_MODE} -m {args.hash_type} {args.mask}"
        context.run("/bin/bash", "-c", cmd)

        try:
            future_result = yield context.commit(timeout=KEYSPACE_TIMEOUT)
            # each item is the result of a single command on the provider (including setup commands)
            result: List[CommandExecuted] = await future_result
            # we take the last item since it's the last command that was executed on the provider
            cmd_result: CommandExecuted = result[-1]
            keyspace = int(cmd_result.stdout)
            task.accept_result(result=keyspace)
        except CommandExecutionError as e:
            raise RuntimeError(f"Failed to compute attack keyspace: {e}")

Copy link
Contributor

Choose a reason for hiding this comment

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

That would print for example:

[2021-05-27 13:07:04,963 WARNING yapapi.summary] Worker for provider 'provider-2' failed; reason: Failed to compute attack keyspace: Command '{'run': {'entry_point': '/bin/bash', 'args': ('-c', 'xhashcat --keyspace -a 3 -m 400 ?a?a?a'), 'capture': {'stdout': {'stream': {}}, 'stderr': {'stream': {}}}}}' failed on provider; message: 'ExeScript command exited with code 127'; stderr: 'bash: xhashcat: command not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll switch to a try-except in compute_keyspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2ab9620

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

It's a much nicer read than the original!

I'd consider changing the body of compute_keyspace() to handle CommandExecutionError instead of checking the status of the returned CommandExecuted event. Or we can just let the exception bubble up without wrapping it in a RuntimeError.

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

LGTM!

@kmazurek kmazurek merged commit 5288a26 into master Jun 1, 2021
@kmazurek kmazurek deleted the km/yacat-executor-port branch June 1, 2021 13:29
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.

port yacat example
3 participants