Skip to content

Commit

Permalink
partial fix for broken automated tests
Browse files Browse the repository at this point in the history 
will use different validation method for hardware stored certificates
and pure software certificates emited by the nextcloud server

Signed-off-by: Matthieu Gallien <[email protected]>
  • Loading branch information
@mgallien
mgallien committed Jan 29, 2025
1 parent 8e10a2c commit fb7a5d1
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 19 deletions.
44 changes: 36 additions & 8 deletions src/libsync/clientsideencryption.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,9 @@ const QString &ClientSideEncryption::getMnemonic() const

void ClientSideEncryption::setCertificate(const QSslCertificate &certificate)
{
_encryptionCertificate = CertificateInformation{_encryptionCertificate.getPrivateKeyData(), QSslCertificate{certificate}};
_encryptionCertificate = CertificateInformation{useTokenBasedEncryption() ? CertificateInformation::CertificateType::HardwareCertificate : CertificateInformation::CertificateType::SoftwareNextcloudCertificate,
_encryptionCertificate.getPrivateKeyData(),
QSslCertificate{certificate}};
}

const QSslCertificate& ClientSideEncryption::getCertificate() const
Expand Down Expand Up @@ -1414,7 +1416,9 @@ void ClientSideEncryption::publicCertificateFetched(Job *incoming)
return;
}

_encryptionCertificate = CertificateInformation{_encryptionCertificate.getPrivateKeyData(), QSslCertificate{readJob->binaryData(), QSsl::Pem}};
_encryptionCertificate = CertificateInformation{useTokenBasedEncryption() ? CertificateInformation::CertificateType::HardwareCertificate : CertificateInformation::CertificateType::SoftwareNextcloudCertificate,
_encryptionCertificate.getPrivateKeyData(),
QSslCertificate{readJob->binaryData(), QSsl::Pem}};

if (_encryptionCertificate.getCertificate().isNull()) {
fetchPublicKeyFromKeyChain(account);
Expand Down Expand Up @@ -2045,7 +2049,9 @@ void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account,
connect(job, &SignPublicKeyApiJob::jsonReceived, job, [this, account, keyPair = std::move(keyPair)](const QJsonDocument& json, const int retCode) {
if (retCode == 200) {
const auto cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString();
_encryptionCertificate = CertificateInformation{_encryptionCertificate.getPrivateKeyData(), QSslCertificate{cert.toLocal8Bit(), QSsl::Pem}};
_encryptionCertificate = CertificateInformation{useTokenBasedEncryption() ? CertificateInformation::CertificateType::HardwareCertificate : CertificateInformation::CertificateType::SoftwareNextcloudCertificate,
_encryptionCertificate.getPrivateKeyData(),
QSslCertificate{cert.toLocal8Bit(), QSsl::Pem}};
Bio certificateBio;
const auto certificatePem = _encryptionCertificate.getCertificate().toPem();
BIO_write(certificateBio, certificatePem.constData(), certificatePem.size());
Expand Down Expand Up @@ -2331,7 +2337,9 @@ void ClientSideEncryption::getPublicKeyFromServer(const AccountPtr &account)
connect(job, &JsonApiJob::jsonReceived, [this, account](const QJsonDocument& doc, int retCode) {
if (retCode == 200) {
QString publicKey = doc.object()["ocs"].toObject()["data"].toObject()["public-keys"].toObject()[account->davUser()].toString();
_encryptionCertificate = CertificateInformation{_encryptionCertificate.getPrivateKeyData(), QSslCertificate{publicKey.toLocal8Bit(), QSsl::Pem}};
_encryptionCertificate = CertificateInformation{useTokenBasedEncryption() ? CertificateInformation::CertificateType::HardwareCertificate : CertificateInformation::CertificateType::SoftwareNextcloudCertificate,
_encryptionCertificate.getPrivateKeyData(),
QSslCertificate{publicKey.toLocal8Bit(), QSsl::Pem}};
fetchAndValidatePublicKeyFromServer(account);
} else if (retCode == 404) {
qCDebug(lcCse()) << "No public key on the server";
Expand Down Expand Up @@ -3005,22 +3013,34 @@ CertificateInformation::CertificateInformation()

CertificateInformation::CertificateInformation(PKCS11_KEY *hardwarePrivateKey,

Check warning on line 3014 in src/libsync/clientsideencryption.cpp

View workflow job for this annotation

GitHub Actions / build

src/libsync/clientsideencryption.cpp:3014:1 [cppcoreguidelines-pro-type-member-init] 

constructor does not initialize these fields: _privateKeyData, _certificate
QSslCertificate &&certificate)
: _hardwarePrivateKey(hardwarePrivateKey)
, _certificate(std::move(certificate))
: _hardwarePrivateKey{hardwarePrivateKey}
, _certificate{std::move(certificate)}
, _certificateType{CertificateType::HardwareCertificate}
{
checkEncryptionCertificate();
}

CertificateInformation::CertificateInformation(const QByteArray &privateKey, QSslCertificate &&certificate)
CertificateInformation::CertificateInformation(CertificateType certificateType,

Check warning on line 3023 in src/libsync/clientsideencryption.cpp

View workflow job for this annotation

GitHub Actions / build

src/libsync/clientsideencryption.cpp:3023:1 [cppcoreguidelines-pro-type-member-init] 

constructor does not initialize these fields: _privateKeyData, _certificate
const QByteArray &privateKey,
QSslCertificate &&certificate)
: _hardwarePrivateKey()
, _privateKeyData()
, _certificate(std::move(certificate))
, _certificateType{certificateType}
{
if (!privateKey.isEmpty()) {
setPrivateKeyData(privateKey);
}

checkEncryptionCertificate();
switch (_certificateType)
{
case CertificateType::HardwareCertificate:
checkEncryptionCertificate();
break;
case CertificateType::SoftwareNextcloudCertificate:
doNotCheckEncryptionCertificate();
break;
}
}

bool CertificateInformation::operator==(const CertificateInformation &other) const
Expand Down Expand Up @@ -3211,4 +3231,12 @@ void CertificateInformation::checkEncryptionCertificate()
}
}

void CertificateInformation::doNotCheckEncryptionCertificate()
{
_certificateExpired = false;
_certificateNotYetValid = false;
_certificateRevoked = false;
_certificateInvalid = false;
}

}
12 changes: 11 additions & 1 deletion src/libsync/clientsideencryption.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,18 @@ class ClientSideEncryption;

class CertificateInformation {
public:
enum class CertificateType {
SoftwareNextcloudCertificate,
HardwareCertificate,
};

CertificateInformation();

explicit CertificateInformation(PKCS11_KEY *hardwarePrivateKey,
QSslCertificate &&certificate);

explicit CertificateInformation(const QByteArray& privateKey,
explicit CertificateInformation(CertificateType certificateType,
const QByteArray& privateKey,
QSslCertificate &&certificate);

[[nodiscard]] bool operator==(const CertificateInformation &other) const;
Expand Down Expand Up @@ -104,12 +110,16 @@ class CertificateInformation {
private:
void checkEncryptionCertificate();

void doNotCheckEncryptionCertificate();

PKCS11_KEY* _hardwarePrivateKey = nullptr;

QByteArray _privateKeyData;

QSslCertificate _certificate;

CertificateType _certificateType = CertificateType::SoftwareNextcloudCertificate;

bool _certificateExpired = true;

bool _certificateNotYetValid = true;
Expand Down
40 changes: 34 additions & 6 deletions src/libsync/foldermetadata.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,16 @@ QByteArray FolderMetadata::encryptDataWithPublicKey(const QByteArray &binaryData
const CertificateInformation &shareUserCertificate) const
{
const auto encryptBase64Result = EncryptionHelper::encryptStringAsymmetric(shareUserCertificate, _account->e2e()->paddingMode(), *_account->e2e(), binaryData);
qCDebug(lcCseMetadata()) << "encryptDataWithPublicKey" << binaryData.toBase64() << *encryptBase64Result;
return *encryptBase64Result;

if (encryptBase64Result) {
qCDebug(lcCseMetadata()) << "encryptDataWithPublicKey" << binaryData.toBase64() << *encryptBase64Result;
return *encryptBase64Result;
} else {
qCWarning(lcCseMetadata()) << "fail to encryptDataWithPublicKey" << binaryData.toBase64();
_account->reportClientStatus(OCC::ClientStatusReportingStatus::E2EeError_GeneralError);
return {};
}
return {};
}

QByteArray FolderMetadata::decryptDataWithPrivateKey(const QByteArray &base64Data,
Expand Down Expand Up @@ -551,8 +559,12 @@ void FolderMetadata::initEmptyMetadata()
return initEmptyMetadataLegacy();
}
qCDebug(lcCseMetadata()) << "Setting up empty metadata v2";

const auto certificateType = _account->e2e()->useTokenBasedEncryption() ?
FolderMetadata::CertificateType::HardwareCertificate : FolderMetadata::CertificateType::SoftwareNextcloudCertificate;

if (_isRootEncryptedFolder) {
if (!addUser(_account->davUser(), _account->e2e()->getCertificate())) {
if (!addUser(_account->davUser(), _account->e2e()->getCertificate(), certificateType)) {
qCDebug(lcCseMetadata) << "Empty metadata setup failed. Could not add first user.";
_account->reportClientStatus(OCC::ClientStatusReportingStatus::E2EeError_GeneralError);
return;
Expand Down Expand Up @@ -1037,7 +1049,9 @@ void FolderMetadata::slotRootE2eeFolderMetadataReceived(int statusCode, const QS
initMetadata();
}

bool FolderMetadata::addUser(const QString &userId, const QSslCertificate &certificate)
bool FolderMetadata::addUser(const QString &userId,
const QSslCertificate &certificate,
CertificateType certificateType)
{
Q_ASSERT(_isRootEncryptedFolder);
Q_ASSERT(!certificate.isNull());
Expand All @@ -1046,9 +1060,23 @@ bool FolderMetadata::addUser(const QString &userId, const QSslCertificate &certi
return false;
}

const auto shareUserCertificate = CertificateInformation{{}, QSslCertificate{certificate}};
auto convertedCertificateType = CertificateInformation::CertificateType::HardwareCertificate;
switch (certificateType)
{
case CertificateType::HardwareCertificate:
convertedCertificateType = CertificateInformation::CertificateType::HardwareCertificate;
break;
case CertificateType::SoftwareNextcloudCertificate:
convertedCertificateType = CertificateInformation::CertificateType::SoftwareNextcloudCertificate;
break;
}

const auto shareUserCertificate = CertificateInformation{convertedCertificateType, {}, QSslCertificate{certificate}};
if (userId.isEmpty() || certificate.isNull() || !shareUserCertificate.canEncrypt()) {
qCWarning(lcCseMetadata()) << "Could not add a folder user. Invalid userId or certificate.";
qCWarning(lcCseMetadata()) << "Could not add a folder user. Invalid userId or certificate."
<< userId
<< (certificate.isNull() ? "user certificate is invalid" : "user certificate is valid")
<< (shareUserCertificate.canEncrypt() ? "certificate of share receiver user can encrypt" : "certificate of share receiver user cannot encrypt");
return false;
}

Expand Down
8 changes: 7 additions & 1 deletion src/libsync/foldermetadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ class OWNCLOUDSYNC_EXPORT FolderMetadata : public QObject
};
Q_ENUM(FolderType)

enum class CertificateType {
SoftwareNextcloudCertificate,
HardwareCertificate,
};
Q_ENUM(CertificateType)

FolderMetadata(AccountPtr account, const QString &remoteFolderRoot, FolderType folderType = FolderType::Nested);
/*
* construct metadata based on RootEncryptedFolderInfo
Expand Down Expand Up @@ -121,7 +127,7 @@ class OWNCLOUDSYNC_EXPORT FolderMetadata : public QObject
[[nodiscard]] bool moveFromFileDropToFiles();

// adds a user to have access to this folder (always generates new metadata key)
[[nodiscard]] bool addUser(const QString &userId, const QSslCertificate &certificate);
[[nodiscard]] bool addUser(const QString &userId, const QSslCertificate &certificate, CertificateType certificateType);
// removes a user from this folder and removes and generates a new metadata key
[[nodiscard]] bool removeUser(const QString &userId);

Expand Down
5 changes: 4 additions & 1 deletion src/libsync/updatee2eefolderusersmetadatajob.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,11 @@ void UpdateE2eeFolderUsersMetadataJob::startUpdate()
return;
}

const auto certificateType = _account->e2e()->useTokenBasedEncryption() ?
FolderMetadata::CertificateType::HardwareCertificate : FolderMetadata::CertificateType::SoftwareNextcloudCertificate;

const auto result = _operation == Operation::Add
? _encryptedFolderMetadataHandler->folderMetadata()->addUser(_folderUserId, _folderUserCertificate)
? _encryptedFolderMetadataHandler->folderMetadata()->addUser(_folderUserId, _folderUserCertificate, certificateType)
: _encryptedFolderMetadataHandler->folderMetadata()->removeUser(_folderUserId);

if (!result) {
Expand Down
4 changes: 2 additions & 2 deletions test/testclientsideencryptionv2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,11 @@ private slots:
encryptedFile.initializationVector = EncryptionHelper::generateRandom(16);
metadata->addEncryptedFile(encryptedFile);

QVERIFY(metadata->addUser(_secondAccount->davUser(), _secondAccount->e2e()->getCertificate()));
QVERIFY(metadata->addUser(_secondAccount->davUser(), _secondAccount->e2e()->getCertificate(), FolderMetadata::CertificateType::SoftwareNextcloudCertificate));

QVERIFY(metadata->removeUser(_secondAccount->davUser()));

QVERIFY(metadata->addUser(_secondAccount->davUser(), _secondAccount->e2e()->getCertificate()));
QVERIFY(metadata->addUser(_secondAccount->davUser(), _secondAccount->e2e()->getCertificate(), FolderMetadata::CertificateType::SoftwareNextcloudCertificate));

const auto encryptedMetadata = metadata->encryptedMetadata();
QVERIFY(!encryptedMetadata.isEmpty());
Expand Down

0 comments on commit fb7a5d1

Please sign in to comment.