Skip to content

New base32 implementation#984

Merged
droidmonkey merged 5 commits intokeepassxreboot:developfrom
adolfogc:new-base32-implementation
Sep 24, 2017
Merged

New base32 implementation#984
droidmonkey merged 5 commits intokeepassxreboot:developfrom
adolfogc:new-base32-implementation

Conversation

@adolfogc
Copy link
Copy Markdown
Contributor

@adolfogc adolfogc commented Sep 23, 2017 •
edited
Loading

This PR provides a new base32 encode/decode implementation, following RFC 4648.

Description

  • Creates a new directory, src/tools, for this type of utilities.
  • Implements missing Base32::encode.
  • Re-implements Base32::decode.
  • Changes code base to use the new Base32::decode interface, as it now returns an optional value.
  • Adds new separate tests for these functions.
  • Removes src/totp/base32.h and src/totp/base32.cpp
  • Updates licensing information.

Motivation and context

The motivation for this PR is given in #964.

How has this been tested?

  • Tested on Linux with ASAN enabled, by running build/tests/testbase32.
  • Tested on Linux using:
mkdir build && cd build
cmake -DWITH_ASAN=ON -DCMAKE_BUILD_TYPE=Debug ..
make -j4

# run tests in ./tests

Types of changes

  • ✅ New feature (non-breaking change which adds functionality)
  • ✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ I have added tests to cover my changes.

@adolfogc adolfogc mentioned this pull request Sep 23, 2017
Closed
@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Sep 23, 2017

@droidmonkey what about using src/crypto folder for Base32 encoding?

Anyway @adolfogc can you apply the following patch to /COPYING file and also delete /LICENSE.APACHE-2.0 file? (so we finally clean up that Google's base32 forever)

--- a/COPYING
+++ b/COPYING
@@ -214,10 +214,6 @@
 Copyright: none
 License: public-domain
 
-Files: src/crypto/salsa20/*
-Copyright: none
-License: public-domain
-
 Files: src/streams/qtiocompressor.*
        src/streams/QtIOCompressor
        tests/modeltest.*
@@ -242,8 +238,3 @@
            2014 Dominik Haumann <dhaumann@kde.org>
 License: LGPL-2.1
 
-Files: src/totp/base32.cpp
-       src/totp/base32.h
-Copyright: 2010 Google Inc.
-License: Apache 2.0
-

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Sep 23, 2017

Another thing, @adolfogc your tests fail on TravisCI with -DCMAKE_BUILD_TYPE=Debug

QFATAL : TestBase32::testDecode() ASSERT: "6 == nPads" in file /home/travis/build/keepassxreboot/keepassxc/src/tools/base32.cpp, line 74

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Sep 24, 2017

Concur with placing in crypto folder or core folder

@adolfogc
Copy link
Copy Markdown
Contributor Author

adolfogc commented Sep 24, 2017 •
edited
Loading

Where should tools/optional.h be placed?

Edit: Please disregard. I'll move all three files to core.

@adolfogc
Copy link
Copy Markdown
Contributor Author

adolfogc commented Sep 24, 2017 •
edited
Loading

@TheZ3ro Thanks, I reorganized the switch statement and removed that Q_ASSERT. Also, I updated the licensing information and moved all three files to src/core.

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Sep 24, 2017

Beautiful implementation

@adolfogc
Copy link
Copy Markdown
Contributor Author

adolfogc commented Sep 24, 2017

@droidmonkey Thanks

weslly
weslly approved these changes Sep 24, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@weslly weslly left a comment

Choose a reason for hiding this comment

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

👍

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Sep 24, 2017

Last thing before merging, can you rebase on top of latest develop? Thank 👍

@adolfogc
Copy link
Copy Markdown
Contributor Author

adolfogc commented Sep 24, 2017

@TheZ3ro Just rebased to the latest develop.

@droidmonkey droidmonkey merged commit 522e132 into keepassxreboot:develop Sep 24, 2017
@phoerious phoerious added this to the v2.3.0 milestone Oct 12, 2017
@adolfogc adolfogc mentioned this pull request Oct 15, 2017
Merged
@adolfogc adolfogc deleted the new-base32-implementation branch November 20, 2017 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

+1 more reviewer

@weslly weslly weslly approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@adolfogc @TheZ3ro @droidmonkey @weslly @phoerious