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.

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

AEAD test with more variation #21

Closed
8 tasks done
falko-strenzke opened this issue Apr 19, 2023 · 4 comments
Closed
8 tasks done

AEAD test with more variation #21

falko-strenzke opened this issue Apr 19, 2023 · 4 comments
Assignees
Labels
test Pertaining to unit tests etc

Comments

@falko-strenzke
Copy link
Contributor

falko-strenzke commented Apr 19, 2023

  • test_ffi_encrypt_pk_with_v6_key:
    • vary
      • key length,
      • cipher,
      • AEAD-mode
  • cli-Encryption tests:
    • (v2 SEIPD is automatically chosen when encrypting with AEAD to v6 keys)
    • need cli parameter to choose also GCM (unclear if GCM support is desired in RNP) not desired by RNP, see Add GCM for v2 SEIPD? rnpgp/rnp#2218 (comment)
    • testing OCB, EAX in v2 SEIPD with v6 PQC keys in test src/tests/cli_tests.py:test_zzz_encryption_and_signing_pqc()
@TJ-91
Copy link
Collaborator

TJ-91 commented Apr 21, 2023

I added some more variation.

When explicitly setting None via rnp_op_encrypt_set_aead, we override it in init_encrypted_dst to PGP_AEAD_OCB. This is due to the fact that we choose PKESKv6 and AEAD is required. I think it's reasonable behaviour but perhaps we should ask the maintainers whether or not the explicitly requested mode should be honored. This would mean to a) either explicitly fail with an error or b) revert to PKESKv3.

It should be noted, that we are calling both rnp_op_encrypt_enable_pkesk_v6 and rnp_op_encrypt_set_aead, so setting None in the latter is contradictory in itself. Perhaps the inconsistent settings should be cought when setting either of the options and the other option is inconsistent with the currently set option.

@ni4
Copy link
Collaborator

ni4 commented Apr 24, 2023

It should be noted, that we are calling both rnp_op_encrypt_enable_pkesk_v6 and rnp_op_encrypt_set_aead, so setting None in the latter is contradictory in itself.

For me best solution in this case would be to fail with INVALID_PARAMETER error and some explanatory message via FFI_LOG(), as it is definitely something which should not happen.

@falko-strenzke falko-strenzke added the test Pertaining to unit tests etc label Feb 5, 2024
@falko-strenzke
Copy link
Contributor Author

TODO: Check whether sufficient coverage now

@falko-strenzke
Copy link
Contributor Author

I added another test for TwoFish in test_ffi_encrypt_pk_with_v6_key in the branch https://github.com/pqc-thunderbird/rnp/tree/update-draft-03. PR to follow. (see #69 for an overview of our dev branches).

Coverage is sufficient now from my point of view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Pertaining to unit tests etc
Projects
None yet
Development

No branches or pull requests

3 participants