Add support for in-memory certificate root stores. #5693
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2135,20 +2135,125 @@ CxPlatTlsSecConfigCreate( | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| if ((CredConfigFlags & QUIC_CREDENTIAL_FLAG_SET_CA_CERTIFICATE_FILE) && | ||||||
| CredConfig->CaCertificateFile) { | ||||||
| X509_STORE* CertStore = SSL_CTX_get_cert_store(SecurityConfig->SSLCtx); | ||||||
|
Collaborator
There was a problem hiding this comment. What is the motivation behind that change?
Contributor
Author
There was a problem hiding this comment. I read somewhere that
Collaborator
There was a problem hiding this comment. Good catch, I could find this commit: |
||||||
| if (CertStore == NULL) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "SSL_CTX_get_cert_store failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto Exit; | ||||||
| } | ||||||
|
|
||||||
| Ret = | ||||||
| X509_STORE_load_file( | ||||||
| CertStore, | ||||||
| CredConfig->CaCertificateFile); | ||||||
| if (Ret != 1) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "X509_STORE_load_file failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto Exit; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (CredConfigFlags & QUIC_CREDENTIAL_FLAG_SET_CA_CERTIFICATE_BLOB && | ||||||
| CredConfig->CaCertificateBlob) { | ||||||
| X509_STORE* CertStore = NULL; | ||||||
| BIO* CertBio = NULL; | ||||||
| STACK_OF(X509_INFO)* CertStack = NULL; | ||||||
|
|
||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
|
|
||||||
| // BIO_new_mem_buf() takes 'int' length | ||||||
| if (CredConfig->CaCertificateBlobLength > (unsigned)INT_MAX) { | ||||||
|
Collaborator
There was a problem hiding this comment. We try to use sized integer types, and this is the type of
Suggested change
Contributor
Author
There was a problem hiding this comment. My logic was that I only cared that the right side is an unsigned type for comparison, and However, here, it doesn't matter. If I'm just showing my paranoid style that led to why it's
Collaborator
There was a problem hiding this comment. I see your reasoning, it makes sense. I was thinking the other way, casting to the type of Either works here for me. |
||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "CaCertificateBlobLength too large"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
|
|
||||||
| CertStore = SSL_CTX_get_cert_store(SecurityConfig->SSLCtx); | ||||||
| if (CertStore == NULL) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "SSL_CTX_get_cert_store failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
|
|
||||||
| CertBio = BIO_new_mem_buf(CredConfig->CaCertificateBlob, (int)CredConfig->CaCertificateBlobLength); | ||||||
| if (CertBio == NULL) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "BIO_new_mem_buf failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
|
|
||||||
| CertStack = PEM_X509_INFO_read_bio(CertBio, NULL, NULL, NULL); | ||||||
| if (!CertStack) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "PEM_X509_INFO_read_bio failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
|
|
||||||
| for (int i = 0, max = sk_X509_INFO_num(CertStack); i < max; i++) { | ||||||
| X509_INFO* CertInfo = sk_X509_INFO_value(CertStack, i); | ||||||
| if (CertInfo->x509) { | ||||||
| Ret = X509_STORE_add_cert(CertStore, CertInfo->x509); | ||||||
| if (!Ret) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "X509_STORE_add_cert failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
| } | ||||||
| if (CertInfo->crl) { | ||||||
| Ret = X509_STORE_add_crl(CertStore, CertInfo->crl); | ||||||
| if (!Ret) { | ||||||
| QuicTraceEvent( | ||||||
| LibraryErrorStatus, | ||||||
| "[ lib] ERROR, %u, %s.", | ||||||
| ERR_get_error(), | ||||||
| "X509_STORE_add_crl failed"); | ||||||
| Status = QUIC_STATUS_TLS_ERROR; | ||||||
| goto BlobExit; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Status = QUIC_STATUS_SUCCESS; | ||||||
|
|
||||||
| BlobExit: | ||||||
|
Collaborator
There was a problem hiding this comment. Could you please extract the code above in an helper function, if it needs it own cleanup section? It will be less error prone.
Contributor
Author
There was a problem hiding this comment. Yeah that sounds better; will do. |
||||||
| if (CertStack != NULL) { | ||||||
| sk_X509_INFO_pop_free(CertStack, X509_INFO_free); | ||||||
| } | ||||||
| if (CertBio != NULL) { | ||||||
| BIO_free(CertBio); | ||||||
| } | ||||||
|
|
||||||
| if (!QUIC_SUCCEEDED(Status)) { | ||||||
| goto Exit; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two new members are exclusive with
CaCertificateFile, I think it would make sense to group them in a union or to re-useCaCertificateFileas suggeseted by #5715 (sinceCaCertificateBlobis meant to hold a string IIUC)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could work. The certificate store format is traditionally a text file, though as far as I know nothing in OpenSSL stops you from passing a binary blob that happens to have a
----- BEGIN CERTIFICATE -----text block inside it. OpenSSL ignores anything outside these blocks. That's why mentally I came up withuint8_t, but it ultimately doesn't matter.I'll look into the naming/union more here.