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

Cloakify integration #342

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Conversation

heat-wave
Copy link
Contributor

Fixes #286
Summary:

  • Cloakify added as a git submodule in /extras
  • Steganographic functionality added as tomb cloak/uncloak commands rather than bury/exhume subcommands (since that was much cleaner code-wise)
  • The user is expected to have cloakify and decloakify on their path, as well as python to run them
  • Test suite updated
  • Adding noise to resulting text is not supported (this can be done but makes usage less straightforward)

Most of the code is reusing parts of existing codebase. Please let me know if something is undone or can be improved.

@heat-wave
Copy link
Contributor Author

Travis build has passed, but it's worth noting that the cloakify-related tests did not run because of unsatisifed dependencies (hadn't thought of it prior to making the PR). Should I update the config to include them?

@roddhjav
Copy link
Contributor

Yes, you should ;)

@jaromil
Copy link
Member

jaromil commented Feb 18, 2019

Thanks for the contribution! lets make sure tests run and leave some time to read your changes. I am insecure about including cloakify as a submodule or jusy statically in Tomb. Would prefer the latter

@heat-wave
Copy link
Contributor Author

I wasn't sure whether to include raw sources or git submodule either. Given the license, I believe it can be copied and modified, so that isn't a problem. And since the easiest way to make the cloakify/decloakify scripts executable is to add a shebang at the start, if we are to choose the static sources, this simplifies testing.
If you're ok with updating the source in the unlikely event that more commits happen in the cloakify repo (or if we're happy with the scripts as they are), I'll make these changes shortly.

@roddhjav
Copy link
Contributor

roddhjav commented Feb 18, 2019

in the unlikely event that more commits happen

I would say, in the very likely event more commits happen. It's not because it seems final now it will always be.

Cloakify is before all an optional dependency of Tomb, so why not simply add it to the dependency list and let the maintainer/user manage the installation separately. Furthermore, Cloakify has releases, therefore, it makes no sense to include it with Tomb.

@heat-wave
Copy link
Contributor Author

Fair enough.

My problem is that Cloakify doesn't have an installation mechanism, so even if the user downloads it, there's quite a few assumptions to be made about how it's installed and used. No matter which way the dependency will be managed (I trust you the maintainers know better), I'd like some certainty on the above.

@jaromil
Copy link
Member

jaromil commented Feb 19, 2019

I have included all optional dependencies in Tomb releases, to avoid on-line connectivity reliance. But in case of python and other more complex setups this makes no point. There is also an archival value to it in case some software disappear (steghide being left unmaintained for long...) however I see all this doesn't apply to cloakify, so perhaps lets follow @roddhjav's suggestion and include only a wrapper to facilitate calling the system wide cloakify if necessary.

@heat-wave
Copy link
Contributor Author

heat-wave commented Feb 19, 2019

Sure, I'll remove the submodule then. What kind of wrapper do you mean, though? If we assume that the user installs cloakify and therefore has the scripts on their path, it might be easier to just rewrite the calls to python2 cloakify.py/decloakify.py (and it would be consistent with the suggested usage in cloakify repo). Am I missing something?

@heat-wave
Copy link
Contributor Author

Well, after some struggle with paths, the new tests do run and pass. Apologies for messy commits.

@jaromil
Copy link
Member

jaromil commented Feb 21, 2019

Great! I'll read through the code ASAP. Meanwhile no worries for the messy commits, that's why PRs are there. Can you git rebase -i HEAD~10 and squash your commits into one, with a clear comment? then push force to your branch and it will all be clean.

@heat-wave
Copy link
Contributor Author

Done. If/when we're finished with cloakify, I could look into integration with pitchforked-sphinx as per #306, if it is worth pursuing.

@jaromil
Copy link
Member

jaromil commented Feb 22, 2019

dear heat-wave, congratulations! this looks very neat. we know shell scripting is not rocket science, but it also takes a lot of love and passion to write something that is simple, readable and trustworthy. I think you have done very well here, so be welcome to the Tomb contributors. I'm also happy to read you like to do more :^) ciao

@jaromil jaromil merged commit 59408b3 into dyne:master Feb 22, 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.

3 participants