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

Add esp_tls_init_global_ca_store function to esp-tls #2654

Conversation

paulreimer
Copy link
Contributor

called from esp_tls_set_global_ca_store. (if global_cacert has already been initialized, re-initialize it -- same behaviour as before).

Having esp_tls_init_global_ca_store function allows allocating the mbedTLS certificate chain, but to manage it directly with mbedTLS APIs (in my use case, I only have der format certs, but I still need the cert chain initialized first, before adding them).

@igrr igrr requested a review from kedars November 23, 2018 06:32
@@ -150,13 +163,11 @@ esp_err_t esp_tls_set_global_ca_store(const unsigned char *cacert_pem_buf, const
if (global_cacert != NULL) {
mbedtls_x509_crt_free(global_cacert);
}

Choose a reason for hiding this comment

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

@paulreimer Could you move this global_cacert check and the corresponding free inside the new esp_tls_init_global_ca_store() API at the beginning? And the remove the initial NULL check from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @chiragatal -- I can do this, but then repeated calls to esp_tls_init_global_ca_store() will wipe the store if it was already configured, e.g. you can't run it multiple times. Right now repeated calls to esp_tls_set_global_ca_store will wipe the store, but init is no-op if it is already allocated. Should I change it to the new behaviour?

Choose a reason for hiding this comment

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

esp_tls_free_global_ca_store() is already present. So this looks fine. You need not change the behaviour.

Copy link
Member

@kedars kedars left a comment

Choose a reason for hiding this comment

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

looks good to me!

igrr pushed a commit that referenced this pull request Jan 24, 2019
…_tls_set_global_ca_store

Signed-off-by: Chirag Atal <chirag.atal@espressif.com>

Merges #2654
@mahavirj
Copy link
Member

@paulreimer Thanks for your contribution. Change is merged with a1204f8

@mahavirj mahavirj closed this Jan 25, 2019
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.

4 participants