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

tests: Run more TLS tests when forcing all server operations on token #453

Merged
merged 13 commits into from
Oct 24, 2024

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Oct 18, 2024

Description

This is rebase of previously closed PR #427, which adds also a test reproducer for #449.

This is trying to solve few issues:

  • The standard error is not recorded in the log files, which makes them hard to use for debugging
  • The expect script default does not catch eof, which makes some failures hidden
  • One of these failures happens in the client, when the provider is forced to be used, but it dies on verification failure. This is because the SSL/TLS code picks up errors left on stack that were not cleaned in several occasions
  • The error from reproducer from Passing CK_P11PROV_IMPORTED_HANDLE while creating mock public key #449 was also being hidden because the server closed the connection.
  • The TLS tests when the provider is forced to be used were executing only one basic use case. This again attempts to run all of them.
  • The query for ALLOWED_MECHANISM during signature was leaving errors on the stack with softokn. I added the no-allowed-mechanisms for the softokn tests and documented this undocumented quirk
  • When doing operations on imported keys, we had undefined slot id, which caused the signature verification failures in client.

Checklist

  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found

@Jakuje
Copy link
Contributor Author

Jakuje commented Oct 18, 2024

ugh. In the CI it passes now, but locally it failed for me previously. After proper clean it works for me too locally. Edited the description.

Edit: Obviously, I forgot to run make before make check. This confirms the proposed change solves the issue for me

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Largely LGTM, some comments.

That said maybe remove the last commit so we can see that CI fails, then I will merge the original PR, and then you can rebase on main and we ensure everything still works.

tests/ttls Outdated Show resolved Hide resolved
tests/ttls Show resolved Hide resolved
@Jakuje
Copy link
Contributor Author

Jakuje commented Oct 21, 2024

Pushed without the commit from #449 to demonstrate the failure.

@Jakuje
Copy link
Contributor Author

Jakuje commented Oct 22, 2024

It looks like I finally see what is going on there. The check on

rv = p11prov_check_mechanism(sigctx->provctx,
p11prov_obj_get_slotid(sigctx->key),
CKM_RSA_PKCS_PSS);

checks if the mechanism is available on the given slot the key is using, but the key is imported so it has slot -1 (CK_UNAVAILABLE_INFORMATION) which leads to conclusion the RSA-PSS mechanism is not supported by a slot. I tried to work around it, but I assume there will be a better way.

The fix in #449 changes the handle of the imported EC keys so this does not solve the client issue with imported ECDH keys so it is usable in both client and server operations.

But if I see right, it still fails with generic errors, which look like coming from OpenSSL during signature verification, which went through ok. But it looks like the OpenSSL itself is exploring the error stack and if it finds some error, it fails anyway:

80A20FED9A7F0000:error:40800012:pkcs11:p11prov_GetAttributeValue:Invalid attribute type specified in a template:../src/interface.gen.c:488:Error returned by C_GetAttributeValue^M
...
Verification error: unspecified certificate verification error^M

This leads me to the question openssl/openssl#23025 which points out that we should really clear these bogus/outdated/non-relevant-anymore messages from the error stack, as the OpenSSL (SSL/TLS API) really cares for the queue and if there is something left, it considers it as an error (or just make this error less verbose so it wont make it to the error queue -- likely not -- this is generic handling of the pkcs11 calls so I would prefer to keep it as it is).

The same issue has the C_GetOperationState() call -- it pushes the error on the openssl stack, which is than interpreted as an error during the TLS exchange and makes the exchange failed. I worked around this by using no-operation-state quirk in this PR, but we should either enable it by default for the pkcs11 tokens we know that are broken, again flush the error message after handling the error or just downgrade it to debug log.

With clearing the error stack, I can make all the tests pass (locally -- in CI it still fails for some reason I need to investigate) on both client and server.

Any comments/insights welcomed.

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Sounds like we are finally on the right path.
It should be relatively easy to address the 2 main comments (slot selection and error stack mgmt) I highlighted.

src/debug.c Outdated Show resolved Hide resolved
src/signature.c Outdated Show resolved Hide resolved
src/util.c Outdated Show resolved Hide resolved
Jakuje added 13 commits October 24, 2024 10:50
The default is not catching eof, which happens if either of the sides
dies during conversations for some reason.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje Jakuje added the covscan Triggers Coverity Scanner label Oct 24, 2024
@github-actions github-actions bot removed the covscan Triggers Coverity Scanner label Oct 24, 2024
@simo5 simo5 added the covscan-ok Coverity scan passed label Oct 24, 2024
@simo5
Copy link
Member

simo5 commented Oct 24, 2024

thanks @Jakuje, great work here!

@simo5 simo5 merged commit 7238f46 into latchset:main Oct 24, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
covscan-ok Coverity scan passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants