pkcs11 aes decrypt - #665
Open
mlohvynenko wants to merge 10 commits into
Open
mlohvynenko wants to merge 10 commits into
mlohvynenko wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate issues affect plaintext safety, crash recovery, key handling, configuration, and resource accounting.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Adds AES-256-GCM encrypted OCI layer decryption using PKCS#11-backed keys.
Changes:
- Adds PKCS#11 AES-GCM key lookup and decryption.
- Integrates staged blob decryption into ImageManager.
- Adds tests, documentation, interfaces, and build updates.
| File | Summary | Review notes |
|---|---|---|
src/core/sm/imagemanager/tests/mocks/blobdecryptormock.hpp |
Blob decryptor mock | — |
src/core/sm/imagemanager/tests/keyprovider.cpp |
Key provider tests | — |
src/core/sm/imagemanager/tests/imagemanager.cpp |
Encrypted-layer integration tests | — |
src/core/sm/imagemanager/tests/CMakeLists.txt |
Test configuration | — |
src/core/sm/imagemanager/tests/blobdecryptor.cpp |
Decryption and PKCS#11 tests | — |
src/core/sm/imagemanager/README.md |
Encrypted-layer documentation | Nit: document decrypt usage provisioning. |
src/core/sm/imagemanager/keyprovider.hpp |
Key provider declaration | — |
src/core/sm/imagemanager/keyprovider.cpp |
Key loading and caching | — |
src/core/sm/imagemanager/itf/keyprovider.hpp |
Key provider interface | — |
src/core/sm/imagemanager/itf/blobdecryptor.hpp |
Blob decryptor interface | — |
src/core/sm/imagemanager/imagemanager.hpp |
ImageManager decryption integration | — |
src/core/sm/imagemanager/imagemanager.cpp |
Decryption workflow | Moderate: account for decrypted staging space. |
src/core/sm/imagemanager/CMakeLists.txt |
Module build updates | — |
src/core/sm/imagemanager/blobdecryptor.hpp |
Blob decryptor declaration | — |
src/core/sm/imagemanager/blobdecryptor.cpp |
File staging and decryption | Critical: remove stale plaintext output on failure. Moderate: make staging crash-safe. |
src/core/iam/certhandler/certmodule.cpp |
Certificate URL handling | Moderate: use configured certificate storage instead of hard-coded values. |
src/core/common/tests/mocks/cryptomock.hpp |
Private-key mock | — |
src/core/common/pkcs11/privatekey.hpp |
AES PKCS#11 declarations | — |
src/core/common/pkcs11/privatekey.cpp |
AES-GCM PKCS#11 operations | — |
src/core/common/pkcs11/pkcs11.hpp |
Secret-key lookup documentation | — |
src/core/common/pkcs11/pkcs11.cpp |
Secret-key discovery and wrapping | Moderate: validate 32-byte AES keys and handle symmetric-key removal correctly. |
src/core/common/ocispec/itf/imagespec.hpp |
Encrypted media type | — |
src/core/common/crypto/tests/certloader.cpp |
Secret-key loading tests | — |
src/core/common/crypto/openssl/cryptoprovider.cpp |
RSA GCM handling | — |
src/core/common/crypto/mbedtls/cryptoprovider.cpp |
RSA GCM handling | — |
src/core/common/crypto/itf/privkey.hpp |
GCM decryption options | Nit: update public API documentation. |
CMakeLists.txt |
Coverage configuration | — |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+108
to
+112
| auto cleanUp = DeferRelease(&stagedPath, [](const auto* path) { (void)fs::Remove(*path); }); | ||
|
|
||
| fs::File output; | ||
|
|
||
| // owner-only: it briefly holds plaintext that is already authenticated by this point, but stays private |
| {}, AOS_ERROR_WRAP(Error(ErrorEnum::eInvalidArgument, "id/label matches more than one secret key"))}; | ||
| } | ||
|
|
||
| return ExportPrivateKey(secretKeys[0], 0, keyTypeAES); |
Comment on lines
+102
to
+108
| StaticString<cFilePathLen> stagedPath; | ||
|
|
||
| if (auto err = stagedPath.Format("%s.gcmtmp", decryptedPath.CStr()); !err.IsNone()) { | ||
| return AOS_ERROR_WRAP(err); | ||
| } | ||
|
|
||
| auto cleanUp = DeferRelease(&stagedPath, [](const auto* path) { (void)fs::Remove(*path); }); |
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 25, 2026 14:29
8cc5cf2 to
166d3a7
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical PKCS#11 compatibility and secret-handling issues, plus unresolved correctness and resource-management findings, block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (8)
Comment on lines
+214
to
+217
| mGCMParams.pIv = const_cast<uint8_t*>(options.mIV.Get()); | ||
| mGCMParams.ulIvLen = static_cast<CK_ULONG>(options.mIV.Size()); | ||
| mGCMParams.ulIvBits = static_cast<CK_ULONG>(options.mIV.Size() * 8); | ||
| mGCMParams.ulTagBits = crypto::AESCipherItf::cGCMTagSize * 8; |
| std::filesystem::remove_all(cTestDir); | ||
| std::filesystem::create_directories(cTestDir); | ||
|
|
||
| ASSERT_TRUE(fs::WriteStringToFile(mPINSource.c_str(), mPIN, 0664).IsNone()); |
Comment on lines
+1180
to
+1183
| (void)secretKeyTempl.PushBack({CKA_CLASS, ConvertToAttributeValue(secretKeyClass)}); | ||
| (void)secretKeyTempl.PushBack({CKA_KEY_TYPE, ConvertToAttributeValue(keyTypeAES)}); | ||
| (void)secretKeyTempl.PushBack({CKA_ID, id}); | ||
| (void)secretKeyTempl.PushBack({CKA_LABEL, ConvertToAttributeValue(label)}); |
| ``` | ||
|
|
||
| Publish `layer.tar.gz.enc` as the layer blob with | ||
| `mediaType: application/vnd.aos.image.layer.enc.v1.aes256gcm+tar+gz`; its digest (SHA-256 of the `.enc` |
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 25, 2026 15:41
166d3a7 to
f0ca135
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved AES-GCM, key validation, deletion, configuration, staging, blob handling, provisioning, and API documentation issues remain.
Review effort: Lite
Findings: 3
Open (8)
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 25, 2026 17:08
f0ca135 to
6802f85
Compare
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 28, 2026 10:15
6802f85 to
34d1435
Compare
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 28, 2026 10:20
34d1435 to
015098e
Compare
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
To be removed once cloud side issue is resolved. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 28, 2026 12:23
015098e to
e94f7b2
Compare
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
mlohvynenko
force-pushed
the
feature_pkcs11_aes_decrypt
branch
from
September 28, 2026 20:59
e94f7b2 to
dd5f94b
Compare
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical decryption and cleanup issues, along with additional unresolved findings, block approval.
Review effort: Lite
Findings: 5
Open (10)
Comment on lines
+810
to
+814
| // C_DecryptUpdate, not at C_DecryptInit. Nothing has been consumed from chunkProvider | ||
| // except this one chunk, and nothing has been handed to chunkReceiver, so if it's the very | ||
| // first, the caller can safely retry via a fresh, single-shot decrypt instead. | ||
| if (!anyChunkDecrypted) { | ||
| return AOS_ERROR_WRAP(ErrorEnum::eNotSupported); |
| sized for the whole file regardless: empirically (against SoftHSM2) and per most PKCS11 modules, AEAD | ||
| decryption only releases data once the authentication tag has been verified, all at once, at | ||
| `C_DecryptFinal` — chunking only bounds the ciphertext side, not the plaintext side. | ||
| - `cDecryptChunkSize` (`AOS_CONFIG_IMAGEMANAGER_DECRYPT_CHUNK_SIZE`, default 16 KiB) is deliberately |
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved issues remain in PKCS#11 key deletion, staging-file safety, credential permissions, and related documentation/contracts.
Review effort: Lite
Findings: 4
Open (10)
Resolved since last review (1)
Comment on lines
+25
to
+27
| * Decrypts a blob file using the device-local symmetric key. The encrypted file's first 12 bytes are | ||
| * read as the IV; the remainder is the ciphertext followed by the authentication tag. Fails, leaving no | ||
| * output, if the data doesn't authenticate (wrong key or modified blob). |
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate issues affect decryption correctness, concurrent staging, key cleanup, error handling, and test validity.
Review effort: Lite
Findings: 6
Open (12)
Comment on lines
+295
to
+306
| // C_DecryptInit rejects the parameters before any data is consumed, so retrying is safe here. | ||
| if (mech.mechanism == CKM_AES_CTR && err.Errno() == static_cast<int32_t>(CKR_MECHANISM_PARAM_INVALID)) { | ||
| LOG_DBG() << "Token rejected CTR counter bits, retrying" << Log::Field("counterBits", cOPTEECTRCounterBits); | ||
|
|
||
| PKCS11AESMechConverter fallbackVisitor(cOPTEECTRCounterBits); | ||
|
|
||
| Tie(mech, err) = options.ApplyVisitor(fallbackVisitor); | ||
| if (!err.IsNone()) { | ||
| return AOS_ERROR_WRAP(err); | ||
| } | ||
|
|
||
| err = op(mech); |
Comment on lines
+321
to
+325
| if (auto err = stagedPath.Format("%s.gcmtmp", decryptedPath.CStr()); !err.IsNone()) { | ||
| return AOS_ERROR_WRAP(err); | ||
| } | ||
|
|
||
| auto cleanUp = DeferRelease(&stagedPath, [](const auto* path) { (void)fs::Remove(*path); }); |
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate findings remain in configuration, PKCS#11 lifecycle handling, and staging-file and storage management.
Review effort: Lite
Findings: 6
Open (13)
| * more (or needs less). | ||
| */ | ||
| #ifndef AOS_CONFIG_IMAGEMANAGER_DECRYPT_CHUNK_SIZE | ||
| #define AOS_CONFIG_IMAGEMANAGER_DECRYPT_CHUNK_SIZE 64 * 1024 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.