Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be compatible to the updated botan's behaviour related to botan_cipher_update(). #2240

Merged
merged 8 commits into from
Jun 24, 2024
Merged

Be compatible to the updated botan's behaviour related to botan_cipher_update(). #2240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
73e32a2
Update workflows with new and without EOLed Centos/Fedora versions.
ni4 Jun 12, 2024
47d1a79
Fixed MaxOS 13 python update issue
maxirmx Jun 17, 2024
a708cb8
Improve coding style for aead decryption.
ni4 Jun 4, 2024
2ee24c1
Correctly handle the case when pgp_cipher_aead_update() may not proce…
ni4 May 28, 2024
98e8043
Add test for the issue #2245: Botan 3.5 compatibility on aead decrypt…
ni4 Jun 11, 2024
f7f4bbe
Fix a bunch of SonarLint warnings.
ni4 Jun 11, 2024
3224ebf
Update FindJSON-C cmake module to correctly support json-c12 and json…
ni4 Jun 18, 2024
8eae86f
CI: add RHEL 8 and 9 runners
ni4 Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 37 additions & 48 deletions .github/workflows/centos-and-fedora.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,28 +54,24 @@ jobs:
- { CC: clang, CXX: clang++, BUILD_MODE: sanitize, SHARED_LIBS: on }

# All cotainers have gpg stable and lts installed
# centos-8-amd64 has botan 2.18.2 installed
# fedora-35-amd64 has botan 3.1.1 installed
# centos-9-amd64 has botan 2.19.3 installed
# fedora-39-amd64 has botan 2.19.4 installed
# Any other version has to be built explicitly !
# Pls refer to https://github.com/rnpgp/rnp-ci-containers#readme for more image details
image:
- { name: 'CentOS 7', container: 'centos-7-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'stable' }
- { name: 'CentOS 8', container: 'centos-8-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'CentOS 8', container: 'centos-8-amd64', backend: 'Botan', botan_ver: '2.18.2', sm2: On, gpg_ver: 'lts' }
- { name: 'CentOS 8', container: 'centos-8-amd64', backend: 'Botan', botan_ver: '2.18.2', sm2: Off, gpg_ver: 'stable' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'stable' }
- { name: 'Fedora 35', container: 'fedora-35-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan', botan_ver: '3.1.1', gpg_ver: 'system' }
# Tests against gpg head fails
# - { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'head' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan', botan_ver: 'head', gpg_ver: 'system' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan', botan_ver: '3.3.0', pqc: On, gpg_ver: 'system' }
- { name: 'CentOS 8', container: 'centos-8-amd64', backend: 'OpenSSL', gpg_ver: 'lts' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'OpenSSL', idea: On, gpg_ver: 'stable' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'OpenSSL', idea: Off,gpg_ver: 'stable' }
- { name: 'Fedora 35', container: 'fedora-35-amd64', backend: 'OpenSSL', gpg_ver: 'system' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'OpenSSL', gpg_ver: 'system' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'Botan', botan_ver: 'system', sm2: Off, gpg_ver: 'lts' }
- { name: 'Fedora 39', container: 'fedora-39-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan', botan_ver: 'system', gpg_ver: 'system' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan', botan_ver: '3.1.1', gpg_ver: 'system' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan', botan_ver: 'head', gpg_ver: 'system' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan', botan_ver: '3.3.0', pqc: On, gpg_ver: 'system' }
- { name: 'CentOS 9', container: 'centos-9-amd64', backend: 'OpenSSL', gpg_ver: 'lts' }
- { name: 'Fedora 39', container: 'fedora-39-amd64', backend: 'OpenSSL', gpg_ver: 'system' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'OpenSSL', gpg_ver: 'system' }
- { name: 'RHEL 8', container: 'redhat-8-ubi', backend: 'OpenSSL', gpg_ver: 'system' }
- { name: 'RHEL 9', container: 'redhat-9-ubi', backend: 'OpenSSL', gpg_ver: 'system' }

# There is some ABI incompatibility between llvm-7, bitan shared library from ribose repo and sanitizer
# So we are enforving static lib for sanitizers on CentOS 7
Expand All @@ -86,32 +82,26 @@ jobs:
- image: { name: 'CentOS 7', container: 'centos-7-amd64', gpg_ver: stable, backend: Botan, botan_ver: 'system' }
env: { CC: clang, CXX: clang++, BUILD_MODE: sanitize, SHARED_LIBS: off }
# Coverage report for Botan 2.x backend
- image: { name: 'CentOS 8', container: 'centos-8-amd64', gpg_ver: stable, backend: Botan, botan_ver: '2.18.2' }
- image: { name: 'CentOS 9 Coverage', container: 'centos-9-amd64', gpg_ver: stable, backend: Botan, botan_ver: 'system' }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for Botan 3.x backend
- image: { name: 'Fedora 36', container: 'fedora-36-amd64', gpg_ver: stable, backend: Botan, botan_ver: '3.1.1' }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for OpenSSL 1.1.1 backend
- image: { name: 'CentOS 8', container: 'centos-8-amd64', gpg_ver: stable, backend: OpenSSL }
- image: { name: 'Fedora 40 Coverage', container: 'fedora-40-amd64', gpg_ver: stable, backend: Botan, botan_ver: '3.3.0' }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for OpenSSL 3.0 backend
- image: { name: 'Fedora 36', container: 'fedora-36-amd64', gpg_ver: stable, backend: OpenSSL }
- image: { name: 'Fedora 40 Coverage', container: 'fedora-40-amd64', gpg_ver: stable, backend: OpenSSL }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for OpenSSL 3.0 backend with disabled algos
- image: { name: 'Fedora 36', container: 'fedora-36-amd64', gpg_ver: stable, backend: OpenSSL, idea: Off, sm2: Off, two: Off, blow: Off, rmd: Off, bp: Off }
- image: { name: 'Fedora 40 Coverage', container: 'fedora-40-amd64', gpg_ver: stable, backend: OpenSSL, idea: Off, sm2: Off, two: Off, blow: Off, rmd: Off, bp: Off }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for Botan backend with disabled algos
- image: { name: 'Fedora 36', container: 'fedora-36-amd64', gpg_ver: stable, backend: Botan, idea: Off, sm2: Off, two: Off, blow: Off, rmd: Off, bp: Off }
- image: { name: 'Fedora 40 Coverage', container: 'fedora-40-amd64', gpg_ver: stable, backend: Botan, idea: Off, sm2: Off, two: Off, blow: Off, rmd: Off, bp: Off }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for PQC
- image: { name: 'Fedora 36', container: 'fedora-36-amd64', gpg_ver: stable, backend: Botan, botan_ver: '3.3.0', pqc: On }
env: { CC: clang, CXX: clang++, BUILD_MODE: sanitize, SHARED_LIBS: off }
# Fedora 38
- image: { name: 'Fedora 38', container: 'fedora-38-amd64', gpg_ver: system, backend: Botan, botan_ver: 'system' }
env: { CC: gcc, CXX: g++, BUILD_MODE: normal, SHARED_LIBS: on }
# Fedora 39
- image: { name: 'Fedora 38', container: 'fedora-39-amd64', gpg_ver: system, backend: Botan, botan_ver: 'system' }
env: { CC: gcc, CXX: g++, BUILD_MODE: normal, SHARED_LIBS: on }
# Coverage report for OpenSSL 1.1.1 backend within RHEL 8
- image: { name: 'RHEL 8 Coverage', container: 'redhat-8-ubi', gpg_ver: stable, backend: OpenSSL }
env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: on }
# Coverage report for PQC - not running yet due to very low coverage
#- image: { name: 'Fedora 40 PQC Coverage', container: 'fedora-40-amd64', gpg_ver: stable, backend: Botan, botan_ver: '3.3.0', pqc: On }
# env: { CC: gcc, CXX: g++, BUILD_MODE: coverage, SHARED_LIBS: off }

container: ghcr.io/rnpgp/ci-rnp-${{ matrix.image.container }}

Expand Down Expand Up @@ -242,10 +232,9 @@ jobs:
matrix:
image:
- { name: 'CentOS 7', container: 'centos-7-amd64' }
- { name: 'CentOS 8', container: 'centos-8-amd64' }
- { name: 'CentOS 9', container: 'centos-9-amd64' }
- { name: 'Fedora 35', container: 'fedora-35-amd64' }
- { name: 'Fedora 36', container: 'fedora-36-amd64' }
- { name: 'Fedora 39', container: 'fedora-39-amd64' }
- { name: 'Fedora 40', container: 'fedora-40-amd64' }

name: Package ${{ matrix.image.name }} SRPM

Expand Down Expand Up @@ -313,10 +302,9 @@ jobs:
matrix:
image:
- { name: 'CentOS 7', container: 'centos-7-amd64' }
- { name: 'CentOS 8', container: 'centos-8-amd64' }
- { name: 'CentOS 9', container: 'centos-9-amd64' }
- { name: 'Fedora 35', container: 'fedora-35-amd64' }
- { name: 'Fedora 36', container: 'fedora-36-amd64' }
- { name: 'Fedora 39', container: 'fedora-39-amd64' }
- { name: 'Fedora 40', container: 'fedora-40-amd64' }

name: Package ${{ matrix.image.name }} RPM
steps:
Expand Down Expand Up @@ -381,23 +369,24 @@ jobs:
matrix:
image:
- { name: 'CentOS 7', container: 'centos:7' }
- { name: 'CentOS 8', container: 'tgagor/centos:stream8' }
- { name: 'CentOS 9', container: 'quay.io/centos/centos:stream9' }
- { name: 'Fedora 35', container: 'fedora:35' }
- { name: 'Fedora 36', container: 'fedora:36' }
# Fedora 39 is disabled since it has cmake issue which prevents man pages to be packaged.
# Please see package step for error message.
#- { name: 'Fedora 39', container: 'fedora:39' }
- { name: 'Fedora 40', container: 'fedora:40' }
name: RPM test on ${{ matrix.image.name }}

steps:
- name: Install prerequisites
run: yum -y install sudo wget binutils

# CentOS 7/8 packages depend on botan.so.16 that gets installed from ribose repo
# Fedora 35/36 packages depend on botan.so.19 that comes Fedora package, that is available by default
# CentOS 7 packages depend on botan.so.16 that gets installed from ribose repo
# Fedora 39/40 packages depend on botan.so.19 that comes Fedora package, that is available by default
# CentOS 9 depend on botan.so.19 and needs EPEL9 repo that needs to be installed
# ribose repo is also a source of json-c (v12 aka json-c12) for CentOS 7

- name: Install ribose-packages
if: matrix.image.container == 'centos:7' || matrix.image.container == 'tgagor/centos:stream8'
if: matrix.image.container == 'centos:7'
run: |
sudo rpm --import https://github.com/riboseinc/yum/raw/master/ribose-packages-next.pub
sudo wget https://github.com/riboseinc/yum/raw/master/ribose.repo -O /etc/yum.repos.d/ribose.repo
Expand All @@ -410,7 +399,7 @@ jobs:
sudo dnf -y install epel-release

- name: Install xargs
if: matrix.image.container == 'fedora:35'
if: matrix.image.container == 'fedora:39'
run: sudo yum -y install findutils

- name: Download rnp rpms v3
Expand Down
6 changes: 5 additions & 1 deletion .github/workflows/macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ jobs:
echo "CRYPTO_BACKEND=openssl" >> $GITHUB_ENV

- name: Install dependencies
run: brew bundle
run: |
rm -f '/usr/local/bin/2to3' 'usr/local/bin/2to3-3.12' '/usr/local/bin/idle3' '/usr/local/bin/idle3.12' \
'/usr/local/bin/pydoc3' '/usr/local/bin/pydoc3.12' '/usr/local/bin/python3' '/usr/local/bin/python3-config' \
'/usr/local/bin/python3.12' '/usr/local/bin/python3.12-config'
brew bundle

- name: Botan2 cache
id: cache
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/time-machine.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ jobs:
# but uses just few of tehm
# Pls refer to https://github.com/rnpgp/rnp-ci-containers#readme for image details
image:
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'OpenSSL' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'OpenSSL' }

container: ghcr.io/rnpgp/ci-rnp-${{ matrix.image.container }}

Expand Down Expand Up @@ -138,8 +138,8 @@ jobs:

# Pls refer to https://github.com/rnpgp/rnp-ci-containers#readme for image details
image:
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'Botan' }
- { name: 'Fedora 36', container: 'fedora-36-amd64', backend: 'OpenSSL' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'Botan' }
- { name: 'Fedora 40', container: 'fedora-40-amd64', backend: 'OpenSSL' }

date-offset:
- '+0y'
Expand Down
12 changes: 8 additions & 4 deletions cmake/Modules/FindJSON-C.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2018 Ribose Inc.
# Copyright (c) 2018, 2024 Ribose Inc.
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -58,18 +58,23 @@ if (NOT PC_JSON-C_FOUND)
pkg_check_modules(PC_JSON-C QUIET json-c12)
endif()

# ..or even json-c13, accompanied by non-develop json-c (RHEL 8 ubi)
if (NOT PC_JSON-C_FOUND)
pkg_check_modules(PC_JSON-C QUIET json-c13)
endif()

# find the headers
find_path(JSON-C_INCLUDE_DIR
NAMES json_c_version.h
HINTS
${PC_JSON-C_INCLUDEDIR}
${PC_JSON-C_INCLUDE_DIRS}
PATH_SUFFIXES json-c
PATH_SUFFIXES json-c json-c12 json-c13
)

# find the library
find_library(JSON-C_LIBRARY
NAMES json-c libjson-c json-c12 libjson-c12
NAMES json-c libjson-c json-c12 libjson-c12 json-c13 libjson-c13
HINTS
${PC_JSON-C_LIBDIR}
${PC_JSON-C_LIBRARY_DIRS}
Expand Down Expand Up @@ -120,4 +125,3 @@ if (JSON-C_FOUND AND NOT TARGET JSON-C::JSON-C)
endif()

mark_as_advanced(JSON-C_INCLUDE_DIR JSON-C_LIBRARY)

17 changes: 9 additions & 8 deletions src/lib/crypto/symmetric.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,26 +277,27 @@
}

bool
pgp_cipher_aead_update(pgp_crypt_t *crypt, uint8_t *out, const uint8_t *in, size_t len)
pgp_cipher_aead_update(
pgp_crypt_t &crypt, uint8_t *out, const uint8_t *in, size_t len, size_t &read)
{
size_t outwr = 0;
size_t inread = 0;

if (len % crypt->aead.granularity) {
if (len % crypt.aead.granularity) {
RNP_LOG("aead wrong update len");
return false;
}

if (botan_cipher_update(crypt->aead.obj, 0, out, len, &outwr, in, len, &inread) != 0) {
size_t outwr = 0;
size_t inread = 0;
if (botan_cipher_update(crypt.aead.obj, 0, out, len, &outwr, in, len, &inread)) {
RNP_LOG("aead update failed");
return false;
}

if ((outwr != len) || (inread != len)) {
RNP_LOG("wrong aead usage");
if (outwr != inread) {
RNP_LOG("wrong aead usage: %zu vs %zu, len is %zu", outwr, inread, len);

Check warning on line 296 in src/lib/crypto/symmetric.cpp

View check run for this annotation

Codecov / codecov/patch

src/lib/crypto/symmetric.cpp#L296 

Added line #L296 was not covered by tests
return false;
}

read = inread;
return true;
}

Expand Down
4 changes: 3 additions & 1 deletion src/lib/crypto/symmetric.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,12 @@ bool pgp_cipher_aead_start(pgp_crypt_t *crypt, const uint8_t *nonce, size_t len)
* len bytes
* @param in buffer with input, cannot be NULL
* @param len number of bytes to process. Should be multiple of update granularity.
* @param read number of bytes read and processed, in rare cases could be less then len.
* @return true on success or false otherwise. On success exactly len processed bytes will be
* stored in out buffer
*/
bool pgp_cipher_aead_update(pgp_crypt_t *crypt, uint8_t *out, const uint8_t *in, size_t len);
bool pgp_cipher_aead_update(
pgp_crypt_t &crypt, uint8_t *out, const uint8_t *in, size_t len, size_t &read);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change pointer to reference here? It will be weird from user perspective if other functions use pointer but this one not.


/** @brief Do final update on the cipher. For decryption final chunk should contain at least
* authentication tag, for encryption input could be zero-size.
Expand Down
6 changes: 4 additions & 2 deletions src/lib/crypto/symmetric_ossl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,17 +268,19 @@ pgp_cipher_aead_start(pgp_crypt_t *crypt, const uint8_t *nonce, size_t len)
}

bool
pgp_cipher_aead_update(pgp_crypt_t *crypt, uint8_t *out, const uint8_t *in, size_t len)
pgp_cipher_aead_update(
pgp_crypt_t &crypt, uint8_t *out, const uint8_t *in, size_t len, size_t &read)
{
if (!len) {
return true;
}
int out_len = 0;
bool res = EVP_CipherUpdate(crypt->aead.obj, out, &out_len, in, len) == 1;
bool res = EVP_CipherUpdate(crypt.aead.obj, out, &out_len, in, len) == 1;
if (!res) {
RNP_LOG("Failed to update cipher: %lu", ERR_peek_last_error()); // LCOV_EXCL_LINE
}
assert(out_len == (int) len);
read = len;
return res;
}

Expand Down
2 changes: 1 addition & 1 deletion src/librepgp/stream-def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
#define ST_FROM ("From")

/* Preallocated cache length for AEAD encryption/decryption */
#define PGP_AEAD_CACHE_LEN (PGP_INPUT_CACHE_SIZE + PGP_AEAD_MAX_TAG_LEN)
#define PGP_AEAD_CACHE_LEN (PGP_INPUT_CACHE_SIZE + 2 * PGP_AEAD_MAX_TAG_LEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Found my answer in stream-write.cpp :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is to include the tag for the last block + the final tag after the encrypted stream.


/* Maximum OpenPGP packet nesting level */
#define MAXIMUM_NESTING_LEVEL 32
Expand Down
Loading
Loading