Issue 30 - implementation for columnar_transposition and advgvx

[avastmick]

This Pull Request provides a fix for Issue #30 for the columnar_transposition and advgvx ciphers. It does so, by not requiring a "padding" character in the ciphertext, however, it also adds optional support for null chars - that would provide "padding" characters, or weak obfuscation characters.

As per issue comments, support for null chars is common in the implementations of the ADFGVX and Columnar Transposition ciphers. See Wikipedia entry on CT, for example. Note, the use of the term null. This term appears to be a cryptographic convention for additional characters inserted into the ciphertext that have no meaning in plaintext.

I have increased the params for both ColumnarTransposition and ADFGVX to allow the passing of an optional null char, using the Option<> type. The null char may be anything but will error if the char exists in the keyword. Use None as a param if no null is used.

Using whitespace Some(' '), is allowed, however, it will mean the ciphers behave as per crate version 0.11.0, and the original problem of potentially trailing whitespace in the ciphertext persists. See ADFGVX test for both encrypt and decrypt using whitespace nulls:

#[test]
    fn encrypt_message_with_whitespace_nulls() {
        let a = ADFGVX::new((
            String::from("ph0qg64mea1yl2nofdxkr3cvs5zw7bj9uti8"),
            String::from("GERMAN"),
            Some(' '),
        )).unwrap();

These tests show that the ciphertext trailing space problem persists when using whitespace nulls, but can be now avoided, by using either no nulls, None, or using a different padding null char, such as Some('\u{0}')

Ciphertext trailing whitespace, as per #30 comments, is still an issue with regard to the Scytale cipher. The fix for this will come under a separate PR as the fix had too many problems to release at this time and my time is limited for the next few weeks to complete. I thought it better to give what was working now.

I have incremented the crate version to 0.12.0 as there are a fair amount of changes, including how the ADFGVX and ColumnarTransposition ciphers are initialised.

[arosspope]

Overall this looking better, but there are still some parts that could be simplified. I am more than happy to jump on and fix them up if you don't have the time/I might change some parts if you don't mind.

Also, it is immediately unclear to me if this is already the case, but do you think that if they don't specify a null_char (e.g. its None), the default behavior would be to use \u{0} character to pad uneven messages?

[avastmick]

Sorry, the docco should be clearer. The default is that if None is passed, no null_char is used at all - i.e. no padding is done.This leaves the ciphertext ragged in the columnar space, but I handle it when decrypting it.

I use a bool so that the on-going cost is lower (avoiding repeated eval), and the code is simpler - maybe that would be optimised by the compiler, not sure. Happy to look at alternatives tho...

if let Some(c) = chars.next() {
     key[i].1.push(c);
} else if self.use_nulls && i > 0 { // Only push null_char if we're using, else skip
    let null_char = Some(self.null_char).unwrap(); // Could condense this...
    key[i].1.push(null_char.unwrap());
}

@arosspope, more than happy for you to fix things up! I'm learning heaps, but still brand new at Rust, I'm still finding lots of the syntax confusing / or difficult to navigate. Your advice has really moved me on quickly - if I only had enough time right now! I should have more time in a couple of weeks' time.

Thanks!

[arosspope]

No worries. Rust has a somewhat steep learning curve (im still learning), but once you get the hang of it, it really is quite a nice language to use. How about I take a pass at this, and then we can discuss my changes?

[arosspope]

Alright @avastmick, I've pushed to a new branch called avastmick-issue-30. Check it out, and merge it into the branch of this PR once your satisfied with my changes. Two comments:

• I've decided that its probably best to reject plaintext messages to be encrypted if they contain the null_char. What do you think?
• Make sure to run cargo doc --open to make sure documentation compiles and looks good.

[avastmick]

@arosspope - had a quick look... Some really nice optimisations and cleaning up! I was not getting the Option<> usage right at all, by the looks. I'll have a better look later and merge in the PR branch.

[avastmick]

@arosspope. Yes, really nice. So condensed and readable. Makes the ADFGVX code really tight and so simple.

For me the trick is this line: if let Some(null_char) = self.null_char, now I get the usage for Option<>. The rust docco is really not that clear, given such an important syntax feature. Because of the way I was assigning the char, I always needed to .unwrap(), your usage is no much neater and clean, but not immediately obvious to a newbie. :grin:

Your idea on not allowing null_chars that are in the plaintext is sound, and I think in-line with the general usage for nulls. :+1:

I re-ran the cargo +nightly fmt and changed the variable names in the encrypt() / decrypt() functions in ADFGVX to match the comments, as I should have in the first place.