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

Basic certificate support #96

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

bigbrett
Copy link
Contributor

@bigbrett bigbrett commented Mar 24, 2025

Adds basic support for certificate management using wolfSSL Cert Manager.

Client API has add/delete/get functionality for trusted certificate (root), and then a verify function that verifies an (ordered) certificate chain against a given trusted root.

Limitations:

  • CM is initialized at verification time to prevent issues managing state between CM and NVM. This may result in slightly more latency for verification operations
  • Chain must be ordered
  • No support for CRLs (yet)

Future work:

  • Feature flag for "minimal" public-key-only verification that doesn't check anything else about certificate validity. Would be useful on space constrained platforms where you really only care about trust and are using X509 to simply interop with existing PKI
  • Introduce the concept of a "session" with a live CM instance and directly expose CM via client API so client apps can use it in a more granular way, essentially providing direct (remote) access to the cert manager API from the client
  • Better handling of intermediates?

@bigbrett bigbrett marked this pull request as draft March 24, 2025 23:49
@bigbrett bigbrett marked this pull request as ready for review March 25, 2025 16:18
@bigbrett bigbrett requested a review from billphipps March 25, 2025 16:18
Copy link
Contributor

@billphipps billphipps left a comment

Choose a reason for hiding this comment

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

Looks very reasonable. A few API names and inout questions, but certainly is a good start. I have some concerns about the RAM necessary on the server to parse the certification chain. We may need to preallocate or use some of the existing cache memory to better handle this.

@@ -0,0 +1,58 @@
/*
* Copyright (C) 2024 wolfSSL Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (C) 2024 wolfSSL Inc.
* Copyright (C) 2025 wolfSSL Inc.

Comment on lines +45 to +46
int wh_Server_CertGetTrusted(whServerContext* server, whNvmId id, uint8_t* cert,
uint32_t* cert_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int wh_Server_CertGetTrusted(whServerContext* server, whNvmId id, uint8_t* cert,
uint32_t* cert_len);
int wh_Server_CertGetTrusted(whServerContext* server, whNvmId id, uint8_t* cert,
uint32_t* inout_cert_len);

I assume cert_len is set to the size of the cert buffer on invocation and set to the actual length on return, even if cert is NULL or not large enough? Starting with API first.

Naming: shouldn't this be CertReadTrusted, since we are reading the NVM object behind it?

const uint8_t* cert, uint32_t cert_len);

/* Delete a trusted certificate from NVM storage */
int wh_Server_CertDeleteTrusted(whServerContext* server, whNvmId id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming: should this be CertEraseTrusted to match other API's?

Comment on lines +38 to +39
int wh_Server_CertAddTrusted(whServerContext* server, whNvmId id,
const uint8_t* cert, uint32_t cert_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a path where the id would be inout? Like we pass in an INVALID id and it returns with a guaranteed unique id?

Comment on lines +40 to +42
#if defined(WOLFHSM_CFG_CERTIFICATE_MANAGER) && !defined(WOLFHSM_CFG_NO_CRYPTO)

#include "wh_test_cert.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(WOLFHSM_CFG_CERTIFICATE_MANAGER) && !defined(WOLFHSM_CFG_NO_CRYPTO)
#include "wh_test_cert.h"
#if defined(WOLFHSM_CFG_CERTIFICATE_MANAGER) && !defined(WOLFHSM_CFG_NO_CRYPTO)
#include "wh_test_cert.h"

@@ -0,0 +1,1503 @@
/*
* Certificate arrays for embedded SSL/TLS applications
* Generated by OpenSSL dual certificate chain script
Copy link
Contributor

Choose a reason for hiding this comment

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

These are new certs or are these already in wolfssl test data?

Yuck that we still are using OpenSSL rather than wolfCLU for this.

@@ -40,14 +40,19 @@ extern "C" {
*/

/** wolfHSM required settings for wolfCrypt */
#define WOLFCRYPT_ONLY
/* #define WOLFCRYPT_ONLY */
Copy link
Contributor

Choose a reason for hiding this comment

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

Grumble that cert manager is outside of wolfcrypt.

}

/* Check if the provided buffer is large enough */
if (meta.len > *cert_len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

recommend to set *cert_len to the actual len first. Maybe return a different error, like buffer len? CFG_MAX_CERT_SIZE may be set differently on the client and server, so this config should be added to the info response as well.

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