Arbitrary alphabet support

[martinwaite]

What's in this PR?

This PR extends the radix range supported by the FF1 and FF3 packages from 62 to 65536.

It also adds support for alphabets containing utf-8 characters. Unicode support is basic - no attempt is made to deal with combining characters.

Backwards compatibility is maintained with the NewCipher function. In order to use an extended alphabet, use the NewCipherWithAlphabet instead.

While testing larger alphabets, the validation for radix based on the relation exp(log(radix) * minlen) >= 100 had a clause that rejected minlen < 2. That clause rejected alphabets longer than 100. In order to support longer alphabets, that clause has been removed - but perhaps we should set minlen to 2 in these cases.

TODOs

This PR needs to be reviewed.

• Incorporate enhancement into FF3 package
• Better name for NewAlphaCipher (NewCipher is kept for backwards compatibility)
• Testing on larger alphabets
• Package layout. Put utilities into sub-package.

Housekeeping Items

• Did you accept and sign the Contributor License Agreement
• Do you agree to the Open Source Code of Conduct?

[anitgandhi]

Regarding one of your TODOs, I was thinking it might be a good idea to make a subpackage fpeUtil and put the Codec and other new logic there. Then ff1 and ff3 packages can import fpeUtil

[anitgandhi]

Also, NewAlphaCipher should be NewCipherWithAlphabet (one of those Go idioms 😄 )

[martinwaite]

The sub-packages are a bit of a nuisance when you fork the code. In my fork, I have to change import statements from " github.com/capitalone/fpe/fpeUtils" to "github.com/martinwaite/fpe/fpeUtils" - and then remember to change them back in the PR. (In practice I use a symlink from github.com/capitalone to github.com/martinwaite - but still a nuisance.) Given the small size of the packages, could we fold the sub-packages into fpe ? We would need distinct `NewCipher` functions - `NewCipherFF1` and `NewCipherFF3` ? Some benefits of this are that we wouldn't need to export the utils functions, and there are some other areas of code duplication that would be easier to address in a single package. The downside is that this would be a breaking change, and client code using ff1 only would also build in ff3 code. In the meantime, I will follow your suggestion.

…

On Mon, 24 Dec 2018 at 21:24, Anit Gandhi ***@***.***> wrote: Regarding one of your TODOs, I was thinking it might be a good idea to make a subpackage fpeUtil and put the Codec and other new logic there. Then ff1 and ff3 packages can import fpeUtil — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACbnz3t45EXjgLtxduosWAfYxIfRI9AYks5u8UYMgaJpZM4ZeUBr> .

[anitgandhi]

That nuisance is present for any Go package, arguably. But I see your point - generally you should be cloning the repo into $GOPATH/src/github.com/capitalone/fpe and/or adding your fork as a git remote and pushing there.

At the moment I'm not comfortable with merging everything into one giant package because like you said, it would be a breaking change. The addition of arbitrary alphabet support is awesome, but in my opinion it doesn't quite constitute making a v2 of this package.

I don't really see an issue with exporting the utils functions, if anything, they might be useful to other folks. That said, you can make a github.com/capitalone/fpe/internal/fpeUtils sub-package and that way it can only be imported by other packages in this repo. internal is a reserved Go path keyword for limiting what can import any of the subpackages in internal to only those in the same repo.

Regarding the other points of code de-duplication, perhaps that can also be moved to fpeUtils? When I wrote the original code, I did note to myself that there are definitely places where the ff1 and ff3 code overlaps, but there were enough place where they diverged that I felt I could just keep them separate.

[anitgandhi]

btw @martinwaite I updated master branch with some new travis-ci config to do linting checks and bump the Go version, along with making a Go module. When you get a chance, please sync up your branch and if there are any linting issues, please fix. I noticed a few snake_case variables, for example.

[martinwaite]

@anitgandhi - I have integrated your Travis changes and made sure my additions pass the lint checks. I have removed 2 to-do tasks: eliminating the temporary array in DecodeNum would be difficult and not worth the effort; and using a map to translate from rune to numeral is OK.

I think this is ready to merge in now.

[anitgandhi]

Thank you @martinwaite . I'll do a full code review of this when I get some time, hopefully this week. There are a lot of changes so it may take some time. I'll do my best to at least send some feedback in so you can incorporate it into changes iteratively.

[CLAassistant]

All committers have signed the CLA.

[OjasGosar]

@anitgandhi curious to any reason this is not merged yet?

[anitgandhi]

The honest answer is because I was/am being a terrible maintainer. I sincerely apologize for that, especially to @martinwaite who put in the effort of tackling this long-standing enhancement.

Unfortunately, I myself no longer have write access to this repository since I'm no longer at Capital One. I'm glad to see there is still interest in this package, though. I'm hoping I can get some level of access back so I can merge this change and some other bug fixes 🤞