Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

fix keymanager: use ed25519 for to sign manifest #1608

Merged
merged 2 commits into from
May 13, 2020

Conversation

kbushgit
Copy link
Contributor

Signed-off-by: Kostiantyn Bushko kbushko@intellias.com

@kbushgit kbushgit requested review from mike-sul and pattivacek March 18, 2020 12:56
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Looks fine, but can we have a unit test that exercises this for both key algorithms?

@kbushgit
Copy link
Contributor Author

Looks fine, but can we have a unit test that exercises this for both key algorithms?

Yes sure, but it still not clear for me how the different methods are processed on the BE, so far we have only one definition for the RSA algorithm rsassa-pss , it's not related to the ed25519 but anyway...

Copy link
Collaborator

@mike-sul mike-sul left a comment

Choose a reason for hiding this comment

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

Perhaps it makes sense to add some low-level test(s) verifying if a signature method is set to an expected value and a signature actually matches the specified method (e.g. signature["method"] is set to rsassa-pss while it was actually signed by different method)

@kbushgit kbushgit force-pushed the fix/OTA-4276/keymanager-sign-tuf-method branch from 3f88cd0 to 595a6ac Compare April 27, 2020 14:51
@codecov-io
Copy link

codecov-io commented Apr 27, 2020

Codecov Report

Merging #1608 into master will increase coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1608      +/-   ##
==========================================
+ Coverage   82.68%   82.69%   +0.01%     
==========================================
  Files         190      190              
  Lines       12079    12087       +8     
==========================================
+ Hits         9987     9995       +8     
  Misses       2092     2092              
Impacted Files Coverage Δ
src/libaktualizr/crypto/keymanager.cc 87.24% <77.77%> (-0.53%) ⬇️
src/libaktualizr/package_manager/ostreemanager.cc 79.04% <0.00%> (-0.74%) ⬇️
src/libaktualizr/storage/sqlstorage.cc 80.03% <0.00%> (-0.39%) ⬇️
src/libaktualizr/crypto/crypto.cc 83.78% <0.00%> (+0.67%) ⬆️
src/aktualizr_info/main.cc 92.34% <0.00%> (+0.85%) ⬆️
src/libaktualizr/storage/sqlstorage_base.cc 76.87% <0.00%> (+1.36%) ⬆️
src/libaktualizr/storage/sql_utils.h 85.71% <0.00%> (+2.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b0ce21...fe1fec5. Read the comment docs.

@kbushgit kbushgit marked this pull request as ready for review April 29, 2020 21:13
kbushgit added 2 commits May 13, 2020 01:23
Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>
Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>
@kbushgit kbushgit force-pushed the fix/OTA-4276/keymanager-sign-tuf-method branch from 09652f8 to fe1fec5 Compare May 12, 2020 23:37
@pattivacek
Copy link
Collaborator

This looks fine. How does the BE react to the manifest?

@kbushgit
Copy link
Contributor Author

This looks fine. How does the BE react to the manifest?

Without this fix, the server response with the following error

response: {"code":"signature_not_valid","description":"The given signature is not valid","cause":null,"errorId":"5cc595df-2e8e-4ed0-83f2-9b76e3309666"}
Put manifest request failed: 0  HTTP 400

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@pattivacek pattivacek merged commit 48dcb6e into master May 13, 2020
@pattivacek pattivacek deleted the fix/OTA-4276/keymanager-sign-tuf-method branch May 13, 2020 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants