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

CRL improvements and update callback #8006

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

Conversation

ColtonWilley
Copy link
Contributor

@ColtonWilley ColtonWilley commented Sep 23, 2024

Description

CRL improvements, add parsing for CRL number, do not allow CRL duplicates, add callback for when CRL entry is updated.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

src/ssl_certman.c Show resolved Hide resolved
src/crl.c Outdated Show resolved Hide resolved
byte *nextDate;
word32 nextDateMaxLen;
byte nextDateFormat;
sword32 crlNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is/Should this really be signed 32-bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I am simply mapping to the existing types in the CRL_Entry struct. Even though it probably should be an unsigned int it is a signed int, so thats what I also used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't those int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is an int in CRL_Entry but I was trying to use wolfssl types for the public facing CrlInfo struct.

Copy link
Contributor

@SparkiDev SparkiDev left a comment

Choose a reason for hiding this comment

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

Happy with the update code.

@@ -560,12 +559,45 @@ int CheckCertCRL(WOLFSSL_CRL* crl, DecodedCert* cert)
NULL, cert->extCrlInfo, cert->extCrlInfoSz, issuerName);
}

#ifdef HAVE_CRL_UPDATE_CB
static void SetCrlInfo(CRL_Entry* entry, CrlInfo *info)
Copy link
Contributor

Choose a reason for hiding this comment

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

Only difference compared to SetCrlInfoFromDecoded is value of issuerHashLen. Combine and make length a parameter?

Looking closer, they should be the same value as same logic to choose size.
Define CRL_DIGEST_SIZE to be SIGNER_DIGEST_SIZE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the input structures are different right, CRL_entry vs DecodedCRL. I could make it void, pass a size and cast accordingly but I dont know if we have really gained much there vs the current implementation. If you strongly prefer it that way though let me know and I can make the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both functions take a CRL_Entry and CrlInfo. Doing the same thing.

CRL_DIGEST_SIZE is the issuer hash size.
SIGNER_DIGEST_SIZE is the subject/issuer hash size.

Define CERT_NAME_HASH_SIZE and have the other two use it.

/* Number of items in ASN.1 template for CrlNumber. */
#define crlNumASN_Length (sizeof(crlNumASN) / sizeof(ASNItem))

static int ParseCRL_CrlNumExt(const byte* input, int sz, DecodedCRL* dcrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use GetShortInt() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would be cleaner. I did not realize GetShortInt internals had a leg for WOLFSSL_ASN_TEMPLATE. I will rewrite to use GetShortInt instead.

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.

3 participants