Refactor and add support classes for KDBX 4 by angelsl · Pull Request #1179 · keepassxreboot/keepassxc

angelsl · 2017-11-12T19:41:55Z

The remaining changes are here. I will open a PR once this gets merged.

Description

Refer to commit log

Motivation and context

#148

How has this been tested?

Screenshots (if appropriate):

Types of changes

✅ New feature (non-breaking change which adds functionality)

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]
✅ My change requires a change to the documentation and I have updated it accordingly.
✅ I have added tests to cover my changes.

angelsl · 2017-11-12T19:46:42Z

Travis is failing due to too old libgcrypt.

Antidote1911 · 2017-11-12T20:53:06Z

nice work !

droidmonkey · 2017-11-12T20:59:10Z

Since you renamed the old KeePass2 format read/write to Kdbx3... would it make sense to make the new one Kdbx4....

Really we should be instantiating a single common read/write shell class and passing the specific reader/writer as a constructor parameter. This would ensure adding a new format would be painless and independent.

We can implement that more advanced usage later, but it might be smart to rename KeePass2.cpp to Kdbx4Reader/Writer.cpp

TheZ3ro · 2017-11-12T21:01:59Z

@droidmonkey the Kdbx4 classes are in the next PR this is only for the initial renaming

droidmonkey · 2017-11-12T21:02:49Z

sweet! Please disregard my post 😄

louib · 2017-11-12T21:15:10Z

src/core/Database.cpp

-            }
-        }
+    if (updateTransformSalt) {
+        m_data.kdf->randomizeSalt();


@angelsl We could name this function updateTranformSalt, or rename the option randomizeSalt, just to be consistent.

randomizeTransformSalt()

louib · 2017-11-12T21:31:25Z

src/crypto/CryptoHash.cpp

+        qWarning("Gcrypt error (ctor): %s", gcry_strerror(error));
+        qWarning("Gcrypt error (ctor): %s", gcry_strsource(error));
+    }
    Q_ASSERT(error == 0); // TODO: error handling


@angelsl not sure how we should be handling those errors, but I guess we should at some point. Right now it'll just fail silently unless the application was built in debug mode.

Yeah, but I think this PR is starting to be quite large..

Ideally the calling function would be notified that there was an error so it could display a notification to the user. Perhaps we could use a generic "cryptoError" signal that includes the error string and can be listened to and acted upon.

louib · 2017-11-12T21:33:20Z

src/crypto/CryptoHash.cpp


-    int algoGcrypt;
+    int algoGcrypt = -1;
+    unsigned int flagsGcrypt = 0;


@angelsl What about using the GCRY_MD_FLAG_SECURE flag by default? From the doc:

Allocate all buffers and the resulting digest in "secure memory". Use this is the hashed data is highly confidential.

Added that.

In preparation for multiple KDFs in KDBX 4

…nd ciphers

…Widget

… class This class will in future select Kdbx4{R,W} as appropriate.

louib · 2017-11-16T15:31:36Z

src/format/KeePass2RandomStream.cpp

-    return m_cipher.init(CryptoHash::hash(key, CryptoHash::Sha256),
-                         KeePass2::INNER_STREAM_SALSA20_IV);
+    switch (m_cipher.algorithm()) {
+        case SymmetricCipher::Salsa20:


@angelsl case and switch on same indentation levels

louib · 2017-11-16T15:32:01Z

src/format/KeePass2.cpp

+KeePass2::ProtectedStreamAlgo KeePass2::idToProtectedStreamAlgo(quint32 id)
+{
+    switch (id) {
+        case static_cast<quint32>(KeePass2::ArcFourVariant):


@angelsl case and switch on same indentation level

droidmonkey

Overall this is tremendous work. Needs to be simplified (kdf fields!) and heavily tested.

droidmonkey · 2017-11-17T22:54:48Z

src/core/Database.cpp

@@ -374,6 +323,7 @@ void Database::setEmitModified(bool value)
 void Database::copyAttributesFrom(const Database* other)
 {


This function is only used in one instance (ToDbExporter) which is not used in the code at all. I recommend in PR2 that we remove this function and the ToDbExporter code.

droidmonkey · 2017-11-17T22:57:44Z

src/core/Database.cpp

    }
 }
+
+Kdf* Database::kdf() {


Recommend rename to getKdf() and also return const Kdf*.

droidmonkey · 2017-11-17T23:02:17Z

src/core/Endian.cpp

    }
    else {
-        qToBigEndian<qint64>(num, reinterpret_cast<uchar*>(ba.data()));
+        qToBigEndian<qint16>(num, reinterpret_cast<uchar*>(ba.data()));


Wow great catch!

droidmonkey · 2017-11-17T23:09:21Z

src/crypto/Crypto.cpp

+    QByteArray sha512Test = CryptoHash::hash("abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq",
+                                             CryptoHash::Sha512);
+
+    if (sha512Test != QByteArray::fromHex("204a8fc6dda82f0a0ced7beb8e08a41657c16ef468b228a8279be331a703c33596fd15c13b1b07f9aa1d3bea57789ca031ad85c7a71dd70354ec631238ca3445")) {


Please post the reference that confirms this is the correct sequence. Other crypto tests pulled known test strings from RFC standards docs. https://tools.ietf.org/html/rfc4231#section-4.1

droidmonkey · 2017-11-17T23:27:00Z

src/crypto/Crypto.cpp

 }
+
+bool Crypto::testChaCha20() {
+    QByteArray chacha20Key = QByteArray::fromHex("0000000000000000000000000000000000000000000000000000000000000000");


Same, please add comment that points to the reference that confirms the test/result strings. https://tools.ietf.org/html/rfc7539#appendix-A.2

droidmonkey · 2017-11-19T12:55:06Z

src/format/KeePass2Reader.h

+    quint32 version() const;
+    BaseKeePass2Reader* reader() const;
+private:
+    QScopedPointer<BaseKeePass2Reader> m_reader;


I am confused why this class both inherits from BaseKeePass2Reader and implements a worker of type BaseKeePass2Reader. Either use the worker or use yourself, pick one.

droidmonkey · 2017-11-19T12:57:09Z

src/format/KeePass2Writer.h

+    virtual QString errorString() override;
+
+private:
+    QScopedPointer<BaseKeePass2Writer> m_writer;


Same comment, don't inherit and implement the same base class.

droidmonkey · 2017-11-19T12:59:09Z

src/gui/DatabaseSettingsWidget.cpp

+    }
+}
+
+void DatabaseSettingsWidget::displayKdf(const Kdf& kdf)


This function drives me bonkers. Needs to be implemented as a form and when fields get refactored... simplified immensely.

Yes, please, please make this a from. Don't convolute the CPP files with GUI boilerplate code.

droidmonkey · 2017-11-19T13:03:14Z

tests/TestKdbx3XmlReader.cpp

+
+#include "TestKeePass2XmlReader.h"
+
+QTEST_GUILESS_MAIN(TestKdbx3XmlReader)


Where did the rest of these tests go?

Look at CMakeLists.

OK... so there are two cpp files, one is basically blank. Why are the TestKdbx3XmlReader member functions defined in TestKeePass2XmlReader.cpp instead of it's own cpp file??

I guess you didn't take a look at the 4 commits that were left out of this PR.

TestKeePass2XmlReader has tests that instantiate KeePass2XmlReader directly like this one. In order to avoid having to duplicate the test functions between Kdbx3XmlReader and Kdbx4XmlReader, I pulled out the XML read and writes into virtual functions that use the correct XmlReader class. I decided to keep them in the same source file so that if these functions are ever changed, it is less likely that one half is forgotten, and so that it is easier to verify that they are identical aside from the different XmlReader class used.

The better solution, arguably, would have been to use templates, but Qt moc does not support templates.

Another solution would have been making the two XmlReader classes a subclass of a base XmlReader class like the other Reader classes, but since the XmlReaders are only ever used by the Reader classes themselves, I didn't think that was appropriate: the XmlReader classes are implementation details of the Reader classes and it does not make sense for them to be sibling classes. The only way in which they are similar is that they both take XML data (that happens to be similarly structured) and spit out a database. Arguably the XmlReader classes should be nested classes of the Reader classes, but I digress.

Finally, in case you are wondering why I didn't stick both QTEST_GUILESS_MAIN in the TestKeePass2XmlReader file, QTEST_GUILESS_MAIN is a macro that expands to a definition of int main.

droidmonkey · 2017-11-19T13:03:58Z

tests/TestKeePass2XmlReader.cpp

    QVERIFY(!historyItem->uuid().isNull());
    QCOMPARE(historyItem->uuid(), entry->uuid());
+
+    if (db) {


You can just delete db even if its nullptr (its a noop)

angelsl · 2017-11-19T13:52:22Z

The point of the KDF fields thing is to avoid having these details hardcoded in the GUI.

If you feel that is OK, then yes, we can drop the whole list-of-fields thing, but I will not personally author that.

angelsl · 2017-11-19T14:00:21Z

I will work on the rest of the comments in a few days.

It would do you good to look at the rest of the PR, @droidmonkey, so you can understand why some things were done the way they were done, so that I don't have to explain myself repeatedly. We went through some of this in March already, in case you don't remember.

Some of the inconsistencies are there because the original file had them. I'm trying to minimise code churn in this PR. Perhaps you'd like to fix the formatting and whatnot first in a separate PR, and then I, or someone else, can rebase this again.

I'm not very motivated to get this merged, especially if I keep getting comments that result from not fully understanding the codebase. Just laying it out there.

phoerious · 2017-11-19T15:14:02Z

It would do you good to look at the rest of the PR, @droidmonkey, so you can understand why some things were done the way they were done, so that I don't have to explain myself repeatedly. We went through some of this in March already, in case you don't remember.

TBH, things that don't become clear from the code itself, but only from looking at previous discussion about that code need to be documented. Although I know that you will refuse to do that, I would generally prefer doc blocks above every function. I also know that this is a requirement the rest of our code doesn't meet (and therefore I won't refuse to merge because of this), but we want to change that.

droidmonkey · 2017-11-19T15:17:03Z

I spent a solid 2.5 days diving into your contribution. If a comment is invalid or I'm being a jerk address it in that specific comment. I'm not unreasonable, this contrib is basically open heart surgery on this app. If this does not work flawlessly, the app is worthless and potentially thousands of peoples databases can be corrupted or have flawed security.

droidmonkey · 2017-11-19T16:37:24Z

FYI - I deleted the atEnd() comments since there was no action required

hifi · 2017-11-19T18:26:47Z

I suggest splitting this even more by leaving all KDBX format related changes out for now (including renames) so the things that everyone agree on can be merged in sooner making future work easier to review and comment on.

I have added a custom KDF in C style from OpenSSH in my PR and it would be really nice to refactor it into a class API that could be defined by this PR. I also used QCryptographicHash for SHA-512 and again this PR could lay the work to bring SHA-512 into the main crypto classes and I could switch to use that instead.

In my eyes this is great work towards the bigger picture and getting even half of the blood and sweat that has gone into it merged will help advance that. Keep up the good work!

phoerious

This is already a lot closer to a mergeable state. Thanks for your continued work on this.

My main complaint is also the fields concept, which just needs to be dropped altogether. Also the Kdf/AesKdf class structure needs a bit of restructuring. Apart from these major complaints and a small handful of medium problems, most requested changes are rather minor style issues.

phoerious · 2017-11-20T21:20:28Z

src/core/Database.cpp

+    }
+
+    if (m_data.kdf != nullptr) {
+        delete m_data.kdf;


Why not use a QSharedPointer instead?

phoerious · 2017-11-20T21:21:03Z

src/core/Database.h

-        QByteArray transformSeed;
-        quint64 transformRounds;
        QByteArray transformedMasterKey;
+        Kdf* kdf;


phoerious · 2017-11-20T21:23:10Z

src/core/Database.h

    Database::CompressionAlgorithm compressionAlgo() const;
-    QByteArray transformSeed() const;
-    quint64 transformRounds() const;
+    Kdf* kdf();


Method should be const (see @droidmonkey's comment for this method's implementation).

phoerious · 2017-11-20T21:28:42Z

src/core/Database.h

     * Returns a unique id that is only valid as long as the Database exists.
     */
    Uuid uuid();
+    bool changeKdf(Kdf* kdf);


The difference between setKdf and changeKdf isn't easily understandable for the user of this API without looking at the implementation. Please document. I would also move this up a few lines to group it with the setter method.

These functions probably shouldn't exist at all. The database class should create and maintain the kdf object with external entities merely telling the database which kdf to use.

phoerious · 2017-11-20T21:40:00Z

src/core/Endian.h

    QByteArray int64ToBytes(qint64 num, QSysInfo::Endian byteOrder);
+    QByteArray uint16ToBytes(quint16 num, QSysInfo::Endian byteOrder);
+    QByteArray uint32ToBytes(quint32 num, QSysInfo::Endian byteOrder);
+    QByteArray uint64ToBytes(quint64 num, QSysInfo::Endian byteOrder);


I agree. The implementation of all these methods is basically identical.
ba.resize(2|4|8); can be rewritten as ba.resize(sizeof(T));

phoerious · 2017-11-20T22:57:55Z

src/gui/DatabaseSettingsWidget.cpp

+    }
+}
+
+void DatabaseSettingsWidget::displayKdf(const Kdf& kdf)


Yes, please, please make this a from. Don't convolute the CPP files with GUI boilerplate code.

phoerious · 2017-11-20T23:00:05Z

src/gui/DatabaseSettingsWidget.cpp

+
+void DatabaseSettingsWidget::clearKdfWidgets()
+{
+    m_benchmarkField = nullptr;


Drop this function together with the one above.

phoerious · 2017-11-20T23:02:49Z

src/streams/HmacBlockStream.cpp

+    if (m_error) {
+        return -1;
+    }
+    else if (m_eof) {


Remember to not insert newlines before else.

phoerious · 2017-11-20T23:06:06Z

src/streams/HmacBlockStream.cpp

+
+        m_buffer.clear();
+    }
+    m_blockIndex++;


++m_blockIndex;

phoerious · 2017-11-20T23:13:13Z

src/crypto/kdf/AesKdf.h

+    QByteArray seed() const;
+
+    bool setRounds(quint64 rounds);
+    bool setSeed(const QByteArray& seed);


These four methods should be move to the parent class as pure virtual methods. They are generic enough to be applicable to any kind of KDF.

This would remove the need for fields.

phoerious · 2017-11-26T21:07:03Z

I merged your work into a working branch in our repository. You are very welcome to continue working on this with us, but overall, we'll take it from here. Thank you very much for your contribution!

angelsl · 2017-11-29T12:11:42Z

I haven't had time to look at this until now. Sorry about that.

@phoerious

TBH, things that don't become clear from the code itself, but only from looking at previous discussion about that code need to be documented.

My comment about some things having been discussed back in March was about certain choices I made, which (I think) would have been clear had this PR not been chopped in half.

Although I know that you will refuse to do that, I would generally prefer doc blocks above every function.

Indeed, I do not generally think there is a need to document every single internal function, but I would've agreed to add that.

@droidmonkey

I spent a solid 2.5 days diving into your contribution. If a comment is invalid or I'm being a jerk address it in that specific comment.

I did.

I'm not unreasonable,

I got pretty frustrated with these comments

This function drives me bonkers. Needs to be implemented as a form and when fields get refactored... simplified immensely.

What is the purpose of the fields if you just store it in a member variable anyway!, I am so confused. The whole kdf code needs to be completely redone and SIMPLIFIED.

because you appear to completely dismiss the fields abstraction without appearing to consider at all why I did it that way.

My original intention was to support arbitrary KDFs, if more are ever added, without having to modify the GUI code.

It is clear both you and @phoerious don't think that is necessary. So be it.

this contrib is basically open heart surgery on this app. If this does not work flawlessly, the app is worthless and potentially thousands of peoples databases can be corrupted or have flawed security.

I made an effort to make sure (almost?) every single commit in this PR compiles and passes the full test suite. So there's that.

In any case, I honestly don't feel like these changes are terribly major. A lot of it was copy and paste, or adding glue code to the crypto classes to support ciphers already supported by gcrypt, etc. A visual review plus manual testing plus the unit test suite should uncover any bugs — or perhaps I'm being too optimistic.

Perhaps it is time you reformat the entire codebase once through, so that everything is consistent and you can spend less time commenting on stylistic issues. The reason I did weird things like newline-before-else was because that was the style used by the source file, and I did not want to go in and reformat everything (too much churn), nor did I want to create a source file with a mix of different styles.

I guess it is for the better that you guys take over the PR. Be sure to pick up the rest of the KDBX 4 changes as well.

Feel free to contact me if you need any clarifications or help.

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

sebast889 · 2019-01-10T21:03:47Z

I guess it is for the better that you guys take over the PR. Be sure to pick up the rest of the KDBX 4 changes as well.

@phoerious were these changes ever picked up? If not, are they are still needed?

droidmonkey · 2019-01-10T21:39:54Z

This PR is merged.....

sebast889 · 2019-01-10T22:19:23Z

I realize that. I just thought those changes in the linked repo might have been forgotten about. If they weren't then disregard my comment.

droidmonkey · 2019-01-11T02:34:08Z

Yes we completely overhauled this implementation. It's been in since 2.3.0

droidmonkey requested a review from a team November 12, 2017 21:03

droidmonkey added high priority 🚨 new feature labels Nov 12, 2017

droidmonkey added this to the v2.3.0 milestone Nov 12, 2017

louib reviewed Nov 12, 2017

View reviewed changes

angelsl added 17 commits November 13, 2017 15:52

Pull out KDFs into their own class hierarchy

d786f14

In preparation for multiple KDFs in KDBX 4

Fix typo in Endian::int16ToBytes

2cbf17f

Add Endian::uint{16,32,64}ToBytes

d2fc0ef

Rename SymmetricCipherStream::m_isInitalized to m_isInitialized

4c0d1da

Remove unused SymmetricCipherSalsa20

8eeee57

Make HashedBlockStream::atEnd report EOF correctly

9437199

Add support for stream cipher subclasses in SymmetricCipherStream

f1a111e

Move constants in KeePass2.h to KeePass2.cpp and add a list of KDFs a…

b7ef33d

…nd ciphers

Add support for different KDFs to the database settings widget

dc97c84

Use list of ciphers instead of hardcoding ciphers in DatabaseSettings…

91d2fa5

…Widget

Add SHA512 support to CryptoHash

af7890c

Add ChaCha20 support

4a00cab

Add HMAC support

9044425

Add new HmacBlockStream, used in KDBX 4

38974d2

Add support for ChaCha20 protected stream

bcb8cf5

Use GCRY_MD_FLAG_SECURE in CryptoHash

38ef3c3

Rename KeePass2{,Xml}{R,W} to Kdbx3{,Xml}{R,W}, and add a redirection…

f3d019e

… class This class will in future select Kdbx4{R,W} as appropriate.

louib reviewed Nov 16, 2017

View reviewed changes

droidmonkey requested changes Nov 19, 2017

View reviewed changes

keepassxreboot deleted a comment from angelsl Nov 19, 2017

phoerious requested changes Nov 20, 2017

View reviewed changes

phoerious changed the base branch from develop to feature/kdbx4 November 26, 2017 21:03

phoerious merged commit 5020666 into keepassxreboot:feature/kdbx4 Nov 26, 2017

phoerious mentioned this pull request Nov 26, 2017

New Feature: KDBX4 #1230

Merged

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

		@@ -374,6 +323,7 @@ void Database::setEmitModified(bool value)
		void Database::copyAttributesFrom(const Database* other)
		{


		#include "TestKeePass2XmlReader.h"

		QTEST_GUILESS_MAIN(TestKdbx3XmlReader)

Uh oh!

Conversation

angelsl commented Nov 12, 2017

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

angelsl commented Nov 12, 2017

Uh oh!

Antidote1911 commented Nov 12, 2017

Uh oh!

droidmonkey commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro commented Nov 12, 2017

Uh oh!

droidmonkey commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droidmonkey Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angelsl commented Nov 19, 2017

Uh oh!

angelsl commented Nov 19, 2017

Uh oh!

phoerious commented Nov 19, 2017

Uh oh!

droidmonkey commented Nov 12, 2017 •

edited

Loading

droidmonkey Nov 19, 2017 •

edited

Loading

droidmonkey commented Nov 19, 2017 •

edited

Loading

droidmonkey commented Nov 19, 2017 •

edited

Loading