Skip to content

Implement CRL monitor for Windows#6437

Merged
dgarske merged 9 commits intowolfSSL:masterfrom
julek-wolfssl:windows-crl-monitor
Jul 4, 2023
Merged

Implement CRL monitor for Windows#6437
dgarske merged 9 commits intowolfSSL:masterfrom
julek-wolfssl:windows-crl-monitor

Conversation

@julek-wolfssl
Copy link
Copy Markdown
Member

@julek-wolfssl julek-wolfssl commented May 24, 2023

  • Add crl monitor unit testing
  • Add a threading and conditions interface for easier porting to new OS
  • wolfSSL_CTX_LoadCRL() now returns NOT_COMPILED_IN when monitoring is requested but it is not compiled in

@julek-wolfssl julek-wolfssl self-assigned this May 24, 2023
@julek-wolfssl julek-wolfssl force-pushed the windows-crl-monitor branch 3 times, most recently from 19062be to 35283e2 Compare May 24, 2023 13:16
@julek-wolfssl julek-wolfssl force-pushed the windows-crl-monitor branch 12 times, most recently from ff9262c to 1bc3f9c Compare June 29, 2023 09:34
@julek-wolfssl
Copy link
Copy Markdown
Member Author

retest this please

Comment thread testsuite/testsuite.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we avoid these 1 second sleep? Notice the testsuite/testsuite.test his this 6 times, so adds 6 seconds to the run...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Adding a way to shortcut this wait. Let's try to remove the file for two seconds. If we succeed then we continue with the test.

Comment thread testsuite/testsuite.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a fan of hard coded 2500 size.

I think it is to accommodate these:

ls -la certs/crl/crl.pem
-rw-r--r--  1 davidgarske  staff  2229 Feb  1 09:03 certs/crl/crl.pem
davidgarske@Davids-MBP-2 wolfssl % ls -la certs/crl/crl.revoked
-rw-r--r--  1 davidgarske  staff  2330 Feb  1 09:03 certs/crl/crl.revoked

Why not just use heap?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is no cross-platform way to check the file size (that I could find). I'll see if there is a better way to do this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to reading/writing in 100 byte chunks. That should be fine for stack size.

Comment thread wolfcrypt/src/wc_port.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, thread helpers!
Note: We have some API's for this already in async here. Feel free to pull over and leverage any of it: https://github.com/wolfSSL/wolfAsyncCrypt/blob/master/wolfcrypt/src/async.c#L865

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we make it VERY clear this is only required for CRL monitor? We don't normally need to port threading, but this will be very handy. I'd also like to see this for WOLFSSL_CMSIS_RTOSv2 eventually.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ideally I would like to use this threading interface in our unit tests. This will make them easier to port to new platforms and allow us to test TLS on more platforms.

I kept this threading interface extremely simple to make it easier to port. The async code make much more advanced usage of threading. I think it makes sense to keep them separate for now. Maybe in the future it would make sense to have an additional THREAD_OPTIONS parameter to allow for setting all of the advanced options being consumed by async. For now I want to keep it simple and port the tests to this common interface.

Comment thread wolfssl/internal.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should WOLFSSL_CRL_MONITORS_LEN be overridable? Seems like the existing CRL monitor code only support .der and .pem?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't like having the magic number 2 all around the code. There is no benefit of having this variable be overridable by the user so I didn't guard it with a #ifndef.

@dgarske
Copy link
Copy Markdown
Member

dgarske commented Jun 30, 2023

By the way @julek-wolfssl very nice work on this. It all tests out perfectly on Linux and Windows.

@dgarske dgarske assigned julek-wolfssl and unassigned dgarske Jun 30, 2023
@julek-wolfssl julek-wolfssl force-pushed the windows-crl-monitor branch 4 times, most recently from e0b4f8b to 0b974b1 Compare July 3, 2023 18:21
@julek-wolfssl julek-wolfssl force-pushed the windows-crl-monitor branch from 0b974b1 to ee64b83 Compare July 4, 2023 11:44
@julek-wolfssl julek-wolfssl force-pushed the windows-crl-monitor branch from ee64b83 to 7af1f0c Compare July 4, 2023 12:12
@julek-wolfssl julek-wolfssl requested a review from dgarske July 4, 2023 14:07
@dgarske dgarske merged commit b682c2c into wolfSSL:master Jul 4, 2023
dgarske added a commit to dgarske/wolfssl that referenced this pull request Aug 8, 2023
…ge situations. Added better signal support on MacOS. Issue created in PR wolfSSL#6437.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants